🏗🐛 Prevent most async throwing of errors during tests#16916
🏗🐛 Prevent most async throwing of errors during tests#16916rsimha merged 3 commits intoampproject:masterfrom rsimha:2018-07-19-AsyncThrow
Conversation
lannka
left a comment
There was a problem hiding this comment.
I'm surprised (in a good way) that only 19 tests start to fail.
You can skip them for now and assign owners to fix.
|
No, it's 19 failures out of 400 tests. Most of the 8000 tests didn't run. This isn't ready yet. |
src/log.js
Outdated
| * @visibleForTesting | ||
| * @param {...*} var_args | ||
| */ | ||
| export function rethrowAsyncForTests(var_args) { |
There was a problem hiding this comment.
we allow rest/spread in the code base right? lets just use that instead of var_args
There was a problem hiding this comment.
We use var_args in several other spots in log.js, so I wanted to be consistent here. Also, there's a lint rule against using the spread operator.
Line 74 in db751cb
If it's possible to use rest/spread here, I think it makes sense to switch this and all other instances in the file in a separate PR.
src/log.js
Outdated
| * @visibleForTesting | ||
| * @param {...*} var_args | ||
| */ | ||
| export function rethrowAsyncForTests(var_args) { |
There was a problem hiding this comment.
can we move this method to init_tests? or any where under test folders.
There was a problem hiding this comment.
I meant to do this once I got everything to work. Done.
|
|
||
| allowConsoleError(() => { expect(() => { | ||
| clock.tick(1); | ||
| validateData({ |
There was a problem hiding this comment.
i guess you can remove all the clock ticks above in this test.
src/log.js
Outdated
| * @visibleForTesting | ||
| * @param {...*} var_args | ||
| */ | ||
| export function rethrowAsyncForTests(var_args) { |
There was a problem hiding this comment.
I meant to do this once I got everything to work. Done.
|
|
||
| allowConsoleError(() => { expect(() => { | ||
| clock.tick(1); | ||
| validateData({ |
Codecov Report
@@ Coverage Diff @@
## master #16916 +/- ##
==========================================
- Coverage 77.89% 77.88% -0.02%
==========================================
Files 562 562
Lines 41142 41142
==========================================
- Hits 32048 32042 -6
- Misses 9094 9100 +6
Continue to review full report at Codecov.
|
|
|
||
| it('should do simple JSON fetch', () => { | ||
| // TODO(#16916): Make this test work with synchronous throws. | ||
| it.skip('should do simple JSON fetch', () => { |
There was a problem hiding this comment.
I have no explanation why but after this, it silently fails rest of the tests in the file.
What should have been an execution of 100 tests altogether, are now just 9 tests but passing
results via locally running gulp test --files=test/functional/test-xhr.js --watch
There was a problem hiding this comment.
If you hit the debug button in the karma window, do you see any errors when the tests are re run?
There was a problem hiding this comment.
@prateekbh I was able to repro your problem. One of your tests is throwing an error in beforeEach(). Mocha considers this a catastrophic error and bails. Let me know if you need help reproing it.
There was a problem hiding this comment.
well i can use some help on how to find whats the issue, I am creating a PR which just touched xhr-impl.js and hence I reproduced this error.
Not really sure about the setup and test cases here in functional
This PR does the following:
rethrowAsyncinsrc/log.jsduring tests withrethrowAsyncForTeststest-log.js)With this. the vast majority of async throw problems we're seeing should be eliminated.
Partial fix for #14406
Partial fix for #14360
Fixes #15658