Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make esm-amd-loader dynamic require 404 test not flaky. #511

Merged
merged 4 commits into from
Jun 13, 2018
Merged

Make esm-amd-loader dynamic require 404 test not flaky. #511

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c7f1f04
Format esm-amd-loader.
aomarks Jun 12, 2018
e779ee0
Make esm-amd-loader dynamic require 404 test not flaky.
aomarks Jun 12, 2018
8e0a160
Inert conditional.
aomarks Jun 12, 2018
11152c3
Add a tick.
aomarks Jun 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions packages/esm-amd-loader/test/src/suite.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,19 +268,25 @@ suite('dynamic require', () => {
});

test('calls error callback only once on multiple 404s', (done) => {
let numErrors = 0;
let num404s = 0;
let numCallbackCalls = 0;

window.addEventListener('error', on404, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, is this the browser doing this? Emitting a window error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you can capture the browser network errors by adding the capture phase listener (the true).


function on404() {
if (++num404s === 2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer inert conditionals.

num404s++;
if (num404s === 2) {
  ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

window.removeEventListener('error', on404);
assert.equal(numCallbackCalls, 1);
done();
}
}

define(['require'], (require: any) => {
require(
['./not-found-a.js', './not-found-b.js'],
() => assert.fail(),
() => numErrors++);
() => numCallbackCalls++);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});

setTimeout(() => {
assert.equal(numErrors, 1);
done();
}, 1000);
});
});

Expand Down