-
Notifications
You must be signed in to change notification settings - Fork 201
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -268,19 +268,29 @@ 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); | ||
|
||
function on404() { | ||
num404s++; | ||
if (num404s === 2) { | ||
window.removeEventListener('error', on404); | ||
// Need a tick to ensure the loader error handlers have fired. | ||
setTimeout(() => { | ||
assert.equal(numCallbackCalls, 1); | ||
done(); | ||
}); | ||
} | ||
} | ||
|
||
define(['require'], (require: any) => { | ||
require( | ||
['./not-found-a.js', './not-found-b.js'], | ||
() => assert.fail(), | ||
() => numErrors++); | ||
() => numCallbackCalls++); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
}); | ||
}); | ||
|
||
|
@@ -407,16 +417,16 @@ suite('html imports', () => { | |
} | ||
|
||
function testImport(href: string, expectedOrder: string[], done: () => void) { | ||
// Each time an amd module in the chain is executed, it registers itself. | ||
// If we've reached the length of modules we are expecing to be loaded, | ||
// we check if the right modules were loaded in the expected order | ||
window.addExecutedForImport = (name: string) => { | ||
window.executionOrder.push(name); | ||
if (window.executionOrder.length === expectedOrder.length) { | ||
assert.deepEqual(window.executionOrder, expectedOrder); | ||
done(); | ||
} | ||
}; | ||
// Each time an amd module in the chain is executed, it registers itself. | ||
// If we've reached the length of modules we are expecing to be loaded, | ||
// we check if the right modules were loaded in the expected order | ||
window.addExecutedForImport = (name: string) => { | ||
window.executionOrder.push(name); | ||
if (window.executionOrder.length === expectedOrder.length) { | ||
assert.deepEqual(window.executionOrder, expectedOrder); | ||
done(); | ||
} | ||
}; | ||
|
||
importHref(href); | ||
} | ||
|
@@ -426,11 +436,17 @@ suite('html imports', () => { | |
}); | ||
|
||
test('modules inside deeper level html import', (done) => { | ||
testImport('../html-import/y/deep-import.html', ['x', 'z', 'y', 'deep-import'], done); | ||
testImport( | ||
'../html-import/y/deep-import.html', | ||
['x', 'z', 'y', 'deep-import'], | ||
done); | ||
}); | ||
|
||
test('imports with child imports', (done) => { | ||
testImport('../html-import/parent-import.html', ['z', 'y', 'child-import', 'x', 'parent-import'], done); | ||
testImport( | ||
'../html-import/parent-import.html', | ||
['z', 'y', 'child-import', 'x', 'parent-import'], | ||
done); | ||
}); | ||
|
||
test('import with meta', (done) => { | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).