Make esm-amd-loader dynamic require 404 test not flaky.#511
Conversation
Previously this test used a 1 second timeout to wait to see how many times the error callback was called, which was flaky on CI. Now we capture 404 errors and check callback calls after two 404s. Fixes #507
aomarks
changed the title
| window.addEventListener('error', on404, true); | ||
|
|
||
| function on404() { | ||
| if (++num404s === 2) { |
There was a problem hiding this comment.
Prefer inert conditionals.
num404s++;
if (num404s === 2) {
...| let num404s = 0; | ||
| let numCallbackCalls = 0; | ||
|
|
||
| window.addEventListener('error', on404, true); |
There was a problem hiding this comment.
Hm, I don't think this is the expected behavior. require should not throw a global error, it should only call the failure callback shouldn't it?
There was a problem hiding this comment.
Oh, is this the browser doing this? Emitting a window error?
There was a problem hiding this comment.
Yeah, you can capture the browser network errors by adding the capture phase listener (the true).
| ['./not-found-a.js', './not-found-b.js'], | ||
| () => assert.fail(), | ||
| () => numErrors++); | ||
| () => numCallbackCalls++); |
There was a problem hiding this comment.
Hm, what we really want to do is wait for the first error callback, and consider the test a success if the error callback isn't called again within the next little while (i.e. using setTimeout).
That should be non-flaky.
There was a problem hiding this comment.
IIUC that would be more error prone (since network latency between the two fetches could cause the test to incorrectly pass) and slower (since we have to wait for the timeout).
There was a problem hiding this comment.
I did need to add a tick to let the error handlers get called though. Seems reliably green on sauce now, will see what CI says.
4 participants
Previously this test used a 1 second timeout to wait to see how many times the error callback was called, which was flaky on CI. Now we capture 404 errors and check callback calls after two 404s.
Fixes #507
(Note first commit is just clang formatting).