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: move test-worker-eventlooputil to sequential #35996

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 6, 2020

The test is not nearly as unreliable as it used to be but we're still
seeing failures around the timing checks that will definitely be
affected by other tests running in other processes. So move it to
sequential.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

The test is not nearly as unreliable as it used to be but we're still
seeing failures around the timing checks that will definitely be
affected by other tests running in other processes. So move it to
sequential.

Refs: nodejs#35961 (comment)
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 6, 2020
@Trott
Copy link
Member Author

Trott commented Nov 6, 2020

Refs: #35961 (comment)

@Trott
Copy link
Member Author

Trott commented Nov 6, 2020

Here's what the very occasional failure looks like today:

22:31:18 not ok 2552 parallel/test-worker-eventlooputil # TODO : Fix flaky test
22:31:18   ---
22:31:18   duration_ms: 0.990
22:31:18   severity: flaky
22:31:18   exitcode: 1
22:31:18   stack: |-
22:31:18     node:assert:385
22:31:18         throw err;
22:31:18         ^
22:31:18     
22:31:18     AssertionError [ERR_ASSERTION]: 0 < 25
22:31:18         at Timeout.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1015/test/parallel/test-worker-eventlooputil.js:90:12)
22:31:18         at Timeout._onTimeout (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1015/test/common/index.js:377:15)
22:31:18         at listOnTimeout (node:internal/timers:555:17)
22:31:18         at processTimers (node:internal/timers:498:7) {
22:31:18       generatedMessage: false,
22:31:18       code: 'ERR_ASSERTION',
22:31:18       actual: false,
22:31:18       expected: true,
22:31:18       operator: '=='
22:31:18     }
22:31:18   ...

@Trott Trott added flaky-test Issues and PRs related to the tests with unstable failures on the CI. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 6, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
@nodejs-github-bot

This comment has been minimized.

@Flarna
Copy link
Member

Flarna commented Nov 6, 2020

I think the test could be fixed by using a larger timeout, or maybe by adding some more logic to ensure that worker already reached the waitForNext event place in libuv after posting the return message.

Do you think it's worth to invest some time in this or is it fine to keep the test in sequential forever?

@Trott
Copy link
Member Author

Trott commented Nov 7, 2020

Do you think it's worth to invest some time in this or is it fine to keep the test in sequential forever?

Using a larger timeout? I'd rather just move to sequential because the magic numbers in parallel come back to bite us in surprising ways sooner or later.

Adding some more logic to ensure that worker already reached the waitForNext event place in libuv after posting the return message? That sounds more robust/correct to me. Whether or not it's worth it depends on how satisfied you will feel to have done that. 😄

@Trott
Copy link
Member Author

Trott commented Nov 7, 2020

FWIW, here's what I had to do to see the test fail more than once or twice in a stress test:

https://ci.nodejs.org/job/node-stress-single-test/200/

12 failures in 1000 runs. The key was -j 8 --repeat=100 test/parallel/test-worker-eventlooputil.js. EDIT: This was on an osx1015 host in CI.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@Flarna Flarna added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 12, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 12, 2020
@github-actions
Copy link
Contributor

Landed in f5a86b5...660265e

@github-actions github-actions bot closed this Nov 12, 2020
nodejs-github-bot pushed a commit that referenced this pull request Nov 12, 2020
The test is not nearly as unreliable as it used to be but we're still
seeing failures around the timing checks that will definitely be
affected by other tests running in other processes. So move it to
sequential.

Refs: #35961 (comment)

PR-URL: #35996
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
The test is not nearly as unreliable as it used to be but we're still
seeing failures around the timing checks that will definitely be
affected by other tests running in other processes. So move it to
sequential.

Refs: #35961 (comment)

PR-URL: #35996
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
@Trott Trott deleted the eventlooputil branch April 14, 2022 11:29
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants