test: improve reliability on firefox for tests that inject stylesheets#4938
test: improve reliability on firefox for tests that inject stylesheets#4938
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves the reliability of Firefox tests by fixing flaky behavior in stylesheet injection. The main issue was that Firefox has a delay between adding a <style> element and it appearing in document.styleSheets, which was causing test failures on resource-constrained runners.
Key Changes:
- Replaced fixed 100ms
setTimeoutwith an active polling mechanism that checks when stylesheets actually load - Improved error messages in both test utilities and integration tests to make failures more actionable
- Added timeout handling with clear error messages for debugging
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/testutils.js | Replaced fixed 100ms delay with polling mechanism that actively checks document.styleSheets until the injected stylesheet appears (or times out after 500ms) |
| test/integration/full/preload-cssom/preload-cssom.js | Refactored test to use predefined stylesheet constant and enhanced error message to show actual resolution value when rejection is expected |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const interval = setInterval(function () { | ||
| const isLoaded = Array.from(doc.styleSheets).some( | ||
| sheet => sheet.ownerNode === style | ||
| ); | ||
| if (isLoaded) { | ||
| clearInterval(interval); | ||
| resolve(); | ||
| } else if (Date.now() > timeoutAt) { | ||
| clearInterval(interval); | ||
| reject( | ||
| new Error( | ||
| 'Added <style> element was not reflected in doc.styleSheets within timeout' | ||
| ) | ||
| ); | ||
| } | ||
| }, 5); |
There was a problem hiding this comment.
The polling logic should be wrapped in a try-catch block to handle potential exceptions when accessing doc.styleSheets or sheet.ownerNode. If an error occurs (e.g., due to security restrictions or unexpected browser behavior), the interval will never be cleared, causing a memory leak and the promise will never resolve or reject.
Consider wrapping the logic in a try-catch:
const interval = setInterval(function () {
try {
const isLoaded = Array.from(doc.styleSheets).some(
sheet => sheet.ownerNode === style
);
if (isLoaded) {
clearInterval(interval);
resolve();
} else if (Date.now() > timeoutAt) {
clearInterval(interval);
reject(
new Error(
'Added <style> element was not reflected in doc.styleSheets within timeout'
)
);
}
} catch (err) {
clearInterval(interval);
reject(
new Error(
`Error while polling for stylesheet: ${err.message}`
)
);
}
}, 5);
In #4919, we found that the
test_firefoxjob was unacceptably flaky in thepreload-cssomtests. I believe this is because the GHA runners are slightly more resource-constrained than the circleci ones, which broke an assumption inaxe.testUtils.addStyleSheetthat adding a<style>element would result indocument.styleSheetsbeing updated accordingly within 100ms (in Chrome this happens instantly, but in Firefox there is a delay). I updated the test util to poll until it actually seesdocument.styleSheetsupdate as expected, and to reject with a more actionable error if a timeout occurs (rather than letting a test run in an unexpected state). This also has a minor side-benefit of making the tests marginally faster on Chrome, since they no longer wait 100ms-per-injected-sheet unnecessarily.You can see the results of this in #4936 - the preload-cssom test_firefox case only passed 6/10 runs without this fix and passed 10/10 runs with this fix.
The updates in
preload-cssom.jsturned out to be unnecessary to fix the issue, but they make errors in the test more actionable and make the test more consistent with others, so I left them in.No QA required.