test: deflake long timer WPTs

[marcopiraccini]

type-long-settimeout.any.js and type-long-setinterval.any.js in wpt/test-timers are flaky. Both arm setTimeout(assert_unreached, 100) as a guard and call done() from a timer whose delay overflows to 1 ms. Under load the guard can still fire after done(). testharness.js ignores exceptions once a test has a result, but in Node.js the throw becomes an uncaughtException, and the resulting Worker 'error' event can reach the runner before the completion message and fail the spec.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.29%. Comparing base (357ba2f) to head (e5f48b4).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #63106   +/-   ##
=======================================
  Coverage   90.29%   90.29%           
=======================================
  Files         790      790           
  Lines      271882   271882           
  Branches    51899    51906    +7     
=======================================
+ Hits       245485   245502   +17     
+ Misses      16888    16881    -7     
+ Partials     9509     9499   -10     

see 23 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[panva]

You might want to update test/wpt/status/web-locks.json which has an expected uncaught rejection marked as flaky.

[panva]

matching browser semantics

Can you elaborate? Is this rooted in the WPT runner browsers run, generally more context for review would be great.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/73662/

[marcopiraccini]

matching browser semantics

Can you elaborate? Is this rooted in the WPT runner browsers run, generally more context for review would be great.

It's actually WPT semantic. In resources/testharness.js. The global error handler ignores exceptions once a test has a result (phase >= HAS_RESULT).
Our worker instead forwards a post-done() uncaughtException as a Worker 'error' event.

I think WPT is mirroring the browser's beaviour where the uncaugth exceptions after done are non-fatal

Editing the description and the comment.

[marcopiraccini]

You might want to update test/wpt/status/web-locks.json which has an expected uncaught rejection marked as flaky.

Maybe I am missing something, but I don't think this change covers it. It seems held.https.any.js sets allow_uncaught_exception: true and rejects during the test, not after done().
The handler here is installed in add_completion_callback, so it only covers post-completion exceptions and won't reach that rejection.

[github-actions]

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

[panva]

I'd prefer to scope this workaround to test/wpt/test-timers.js. This changes error handling for every WPT suite, including swallowing unhandled rejections reported after completion, and removeAllListeners('uncaughtException') also removes unrelated handlers.

Can we use setScriptModifier() like in test-user-timing.js, limited to the two affected fixtures? For example:

'use strict';

const assert = require('assert');
const { basename } = require('path');
const { WPTRunner } = require('../common/wpt');

const runner = new WPTRunner('html/webappapis/timers', { concurrency: 1 });

runner.setScriptModifier((script) => {
  if (!['type-long-settimeout.any.js', 'type-long-setinterval.any.js']
    .includes(basename(script.filename))) return;

  // Cancel the failure timer when the test completes so it cannot throw
  // while the runner is still processing the completion message.
  const failureTimer = 'setTimeout(assert_unreached, 100);';
  assert(script.code.includes(failureTimer), `Unexpected contents of ${script.filename}`);
  script.code = script.code.replace(failureTimer,
                                    'const failureTimer = setTimeout(assert_unreached, 100);\n' +
                                    'add_completion_callback(() => clearTimeout(failureTimer));');
});

runner.runJsTests();

The failure timer would still catch missing completion, but couldn't throw after the test has finished.

[panva]

@marcopiraccini please squash, rebase, and update the first commit to reflect the changes and the current PR title.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/77647/

[nodejs-github-bot]

Landed in c90c3dd