Correct testlaunch python script to discover individual unit tests#2332
Conversation
- do not break after discovering suite, still check for individual tests - fix for issue microsoft#2241
|
Transient timeout during unit tests on VSTS for Unit Tests - argsService :: pytest? |
Codecov Report
@@ Coverage Diff @@
## master #2332 +/- ##
==========================================
+ Coverage 75.69% 75.98% +0.29%
==========================================
Files 309 309
Lines 14341 14341
Branches 2540 2540
==========================================
+ Hits 10855 10897 +42
+ Misses 3476 3434 -42
Partials 10 10
Continue to review full report at Codecov.
|
DonJayamanne
left a comment
There was a problem hiding this comment.
Can we create some tests for this. We already have some system tests for unit tests, at least need one more with a different layout (folder structure) of a workspaces to cover this scenario.
- reduce skipped-test noise in our CI
|
Added some further system (not unit!) tests to ensure we've tested for this issue in the future. Note: I also removed the use of |
| return; | ||
| } | ||
| await settings.update(setting, value, configTarget); | ||
| await sleep(2000); |
There was a problem hiding this comment.
Eric and I discussed this and we couldn't understand why was it there. When I took it out, the build machines as well as my local machine succeed when running the tests.
I will certainly put it back, but I think a comment would be helpful. What would that comment be?
Edit: Remove pre-caffeine abruptness from my original comment (apologies!)...
There was a problem hiding this comment.
This was added to ensure config changes took place and VSC had sufficient time to read to the changes. VSC broke there times (official regressions) in this space.
I.e. previously, even though we call VSC API to update settings, using the get methods we could see the changes were still stale!!!
There was a problem hiding this comment.
I should have added comments.
Please add comments something along the lines of:
sometimes VSC returns stake data even after invoking the update method. This issue was reported and resoled numerous times in VSC. This delay is merely a backup to ensure it extension doesn't break the tests due to similar regressions in VSC
There was a problem hiding this comment.
I'll put it back the way it was and add the comment then, thanks for the clarification.
I'll open an issue about this and we can discuss as a team there. #2356.
| expect(testDirs[0]).to.equal(dir); | ||
| expect(testDirs[1]).to.equal(dir2); | ||
| }); | ||
| if (product === Product.unittest) { |
There was a problem hiding this comment.
Sometimes, having a skip inside the function is more readable, as in the way the code was earlier. Right now we have too many top level if blocks (outside the functions ) that reduces readability.
I.e. earlier code was better, as all functions were at one level and easy to read.
There was a problem hiding this comment.
Hmmm... I'm not entirely sure I agree on that being more readable, that is, I think we can make it more readable in other ways such as separating out the various high-level cases into their own test suites.
What I really didn't agree with was polluting the output of the CI automation with "Skipped Test" warnings.
Before my change:
After my change:
...so I reduced the count of skipped tests, but we still have work to do.
It's good to have Skipped Test warnings for tests that have been actually skipped for a legit test-related reason. I think a legitimate reason to skip a test is while we are waiting for a fix for some upstream bug, for instance.
I don't feel that skipping a test just to suit the logic of the test setup is a good use for this, which is why I changed it.
In this particular test class, there is not many tests that overlap - 5 common, 6 pytest, 4 unittest, and 2 for nosetest.
There was a problem hiding this comment.
We chatted about this in the standup today and came to the conclusion that the readability issue isn't improved by my change. However, we can improve on it further (and make our unit test suites even more succinct) by splitting out the test suites for each Python unit test type. I'll do this today.
@DonJayamanne thanks for the review, we'll get this nailed down!
- improve readability of our unit tests - improve intent of our unit tests - remove use of `.skip()` to suite test execution logic - remove unnecessary tslint warning suppressions
Do not break after discovering suite, still check for individual tests
Fix for issue #2241