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.

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

test/pummel/test-timers.js is flaky #55092

Open
avivkeller opened this issue Sep 23, 2024 · 4 comments · Fixed by #55098
Open

test/pummel/test-timers.js is flaky #55092

avivkeller opened this issue Sep 23, 2024 · 4 comments · Fixed by #55098
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. linux Issues and PRs related to the Linux platform.

Comments

@avivkeller
Copy link
Member

Test

test/pummel/test-timers.js

Platform

Linux x64

Console output

=== release test-timers ===
Path: pummel/test-timers
--- stderr ---
diff: 999
node:internal/assert/utils:281
    throw err;
    ^

AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

  assert.ok(1000 <= diff && diff < 1000 + WINDOW)

    at Timeout.<anonymous> (/home/runner/work/_temp/node-v23.0.0-nightly2024-09-2371eb7381f9/test/pummel/test-timers.js:39:12)
    at Timeout._onTimeout (/home/runner/work/_temp/node-v23.0.0-nightly2024-09-2371eb7381f9/test/common/index.js:493:15)
    at listOnTimeout (node:internal/timers:614:17)
    at process.processTimers (node:internal/timers:549:7) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

Node.js v23.0.0-pre
Command: out/Release/node --test-reporter=spec /home/runner/work/_temp/node-v23.0.0-nightly2024-09-2371eb7381f9/test/pummel/test-timers.js

Build links

Additional information

No response

@avivkeller avivkeller added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Sep 23, 2024
@github-actions github-actions bot added the linux Issues and PRs related to the Linux platform. label Sep 23, 2024
targos pushed a commit that referenced this issue Oct 4, 2024
PR-URL: #55098
Fixes: #55092
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Oct 4, 2024
PR-URL: #55098
Fixes: #55092
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@lpinca
Copy link
Member

lpinca commented Oct 15, 2024

It is still flaky.

https://ci.nodejs.org/job/node-test-commit-arm/55372/nodes=ubuntu2204-armv7l/testReport/junit/(root)/pummel/test_timers/

---
duration_ms: 1913.169
exitcode: 1
severity: fail
stack: |-
  diff: 1003
  diff: 1682
  node:internal/assert/utils:281
      throw err;
      ^

  AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

    assert.ok(Math.abs(diff - t) < WINDOW * interval_count)

      at Timeout.<anonymous> (/home/iojs/build/workspace/node-test-commit-arm/test/pummel/test-timers.js:64:12)
      at Timeout._onTimeout (/home/iojs/build/workspace/node-test-commit-arm/test/common/index.js:491:15)
      at listOnTimeout (node:internal/timers:614:17)
      at process.processTimers (node:internal/timers:549:7) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: false,
    expected: true,
    operator: '=='
  }

  Node.js v23.0.0-pre
...

@lpinca lpinca reopened this Oct 15, 2024
@jakecastelli
Copy link
Member

Thanks for reporting 🙏 I will take a look soon

@jakecastelli
Copy link
Member

I took some time to investigate this test failure more thoroughly. Looks like it used to fail prior to fixing the other test, so it seems we've uncovered another flaky test by resolving the previous one 😄

Joke aside. Looks like when the machine is heavily overloaded, it would have a very significant impact on scheduling / firing timers over and over. Especially this failure - here

duration_ms: 3147.927
exitcode: 1
severity: fail
stack: |-
  diff: 1002 <------- first
  diff: 1003 <------- second
  diff: 2426

The actual interval between second interval and the first interval is only 1ms difference which seems pretty wild to me.

I believe the original intent of the test was to check if setInterval allows for a tolerance of 200ms between each timer when under pressure, but the results are proving to be unpredictable.

I would consider to dig into timers a bit more and at the moment I would purpose to mark the test as flaky on the appeared platforms.

Happy for any suggestions! @nodejs/timers

@jakecastelli
Copy link
Member

I've been monitoring the reliability report for nearly a week, and I've noticed that flakiness frequently occurs on these three platforms, but not on others (so far):

  • ubi81_container-x64-1
  • ubuntu2204_sharedlibs_container-x64-7
  • ubuntu2204_container-armv7l-1

It could be a coincidence, but I believe we can label them as flaky. I'm open to alternative suggestions.

louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
PR-URL: nodejs#55098
Fixes: nodejs#55092
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito pushed a commit that referenced this issue Nov 16, 2024
PR-URL: #55098
Fixes: #55092
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
tpoisseau pushed a commit to tpoisseau/node that referenced this issue Nov 21, 2024
PR-URL: nodejs#55098
Fixes: nodejs#55092
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@lpinca lpinca reopened this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. linux Issues and PRs related to the Linux platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants