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: fix flaky test-process-fatal-execption-tick.js #18461

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 30, 2018

test-process-fatal-execption-tick.js is sensitive to load. On a loaded
machine, it will fail. Move it to sequential so that it does not compete
for resources.

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

test

@Trott Trott added test Issues and PRs related to the tests. flaky-test Issues and PRs related to the tests with unstable failures on the CI. fast-track PRs that do not need to wait for 48 hours to land. labels Jan 30, 2018
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 30, 2018
@Trott
Copy link
Member Author

Trott commented Jan 30, 2018

Sample CI failure:

https://ci.nodejs.org/job/node-test-commit-linux/15941/nodes=fedora24/console

not ok 1242 parallel/test-process-fatal-exception-tick
  ---
  duration_ms: 0.294
  severity: fail
  stack: |-
    assert.js:72
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: 1 strictEqual 2
        at process.nextTick (/home/iojs/build/workspace/node-test-commit-linux/nodes/fedora24/test/parallel/test-process-fatal-exception-tick.js:19:33)
        at process._tickCallback (internal/process/next_tick.js:89:11)
  ...

@Trott
Copy link
Member Author

Trott commented Jan 30, 2018

How to replicate locally:

$ tools/test.py -j 96 --repeat 192 test/parallel/test-process-fatal-exception-tick.js 
=== release test-process-fatal-exception-tick ===                    
Path: parallel/test-process-fatal-exception-tick
assert.js:72
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: 1 strictEqual 2
    at process.nextTick (/Users/trott/io.js/test/parallel/test-process-fatal-exception-tick.js:19:33)
    at process._tickCallback (internal/process/next_tick.js:89:11)
Command: out/Release/node /Users/trott/io.js/test/parallel/test-process-fatal-exception-tick.js
[00:07|% 100|+ 191|-   1]: Done    
$ 

@Trott
Copy link
Member Author

Trott commented Jan 30, 2018

@apapirovski
Copy link
Member

apapirovski commented Jan 30, 2018

@Trott could you instead remove line 21 and replace line 23 with common.busyLoop(50);? I tested it locally and that didn't fail a single time in 960 tries.

(Would prefer as few sequential tests as possible.)

@Trott
Copy link
Member Author

Trott commented Jan 30, 2018

@Trott could you instead remove line 21 and replace line 23 with common.busyLoop(50);? I tested it locally and that didn't fail a single time in 960 tries.

I can't make it fail even with common.busyLoop(1). Can you? There's something else going on there. (Maybe function-calling overhead?)

I'm fine with using common.busyLoop() (although I'd want to know why it doesn't fail with even a value of 1 first), but I'd still recommend keeping it in sequential because (IMO) anything with an arbitrary duration should be there and not in parallel.

@apapirovski
Copy link
Member

I can't make it fail even with common.busyLoop(1). Can you? There's something else going on there. (Maybe function-calling overhead?)

It calls into C++ and libuv so it's definitely different semantics from using Date.now(). I would still prefer to keep it in parallel because any extra tests in sequential extend the overall running time of tests but I'm fine with moving it if you prefer.

Use common.busyLoop() which happens to make the test robust.
@Trott
Copy link
Member Author

Trott commented Jan 30, 2018

OK, we'll try it that way. Used common.busyLoop(10) and put it back in parallel.

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

@Trott
Copy link
Member Author

Trott commented Jan 30, 2018

OMG! First yellow CI in dozens of runs! Let's land this thing! :-D

@nodejs/testing

Trott added a commit to Trott/io.js that referenced this pull request Jan 31, 2018
Use common.busyLoop() which happens to make the test robust.

PR-URL: nodejs#18461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@Trott
Copy link
Member Author

Trott commented Jan 31, 2018

Landed in a025723

@MylesBorins
Copy link
Contributor

Test doesn't exist on v9.x, setting as don't land

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Use common.busyLoop() which happens to make the test robust.

PR-URL: nodejs#18461
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@Trott Trott deleted the fix-flaky-process-test branch January 13, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. 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.

7 participants