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-runner-cli-concurrency.js #50108

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Oct 10, 2023

This test was flaky on Windows when trying to clean up the tmp directory between tests. This commit updates the test to not delete anything between tests.

Fixes: #50101

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Oct 10, 2023
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented Oct 10, 2023

FWIW it's also flaky on non-Windows, e.g.: https://ci.nodejs.org/job/node-test-commit-linux-containered/39780/

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@StefanStojanovic
Copy link
Contributor

I've taken these changes and tested them locally. Although the drop in the number of failures is drastic I still saw a few errors like this:

not ok 31 parallel/test-runner-cli-concurrency
  ---
  duration_ms: 2415.70200
  severity: fail
  exitcode: 1
  stack: |-
    TAP version 13
    # Subtest: concurrency of one
    ok 1 - concurrency of one
      ---
      duration_ms: 1028.2008
      ...
    # Subtest: concurrency of two
    ok 2 - concurrency of two
      ---
      duration_ms: 1027.8476
      ...
    1..2
    # tests 2
    # suites 0
    # pass 2
    # fail 0
    # cancelled 0
    # skipped 0
    # todo 0
    # duration_ms 2071.5305
    Can't clean tmpdir: E:\work\node\test\.tmp.31
    Files blocking: [ '1', 'test-1.js', 'test-2.js' ]
    
    E:\work\node\test\common\tmpdir.js:69
        throw e;
        ^
    
    Error: EBUSY: resource busy or locked, rmdir '\\?\E:\work\node\test\.tmp.31\1'
        at rmdirSync (node:fs:1232:10)
        at _rmdirSync (node:internal/fs/rimraf:235:5)
        at rimrafSync (node:internal/fs/rimraf:193:7)
        at node:internal/fs/rimraf:253:9
        at Array.forEach (<anonymous>)
        at _rmdirSync (node:internal/fs/rimraf:250:7)
        at rimrafSync (node:internal/fs/rimraf:193:7)
        at Object.rmSync (node:fs:1281:10)
        at rmSync (E:\work\node\test\common\tmpdir.js:20:8)
        at onexit (E:\work\node\test\common\tmpdir.js:54:5) {
      errno: -4082,
      syscall: 'rmdir',
      code: 'EBUSY',
      path: '\\\\?\\E:\\work\\node\\test\\.tmp.31\\1'
    }
    
    Node.js v21.0.0-pre
  ...

To be honest, this is not a new failure as I saw it happening in the old implementation as well (the call stack is a bit different because of the code changes, but it's the same error). The other error (from #50101) where the second test would fail after the first one passed I was not able to reproduce in 10k+ runs.

This test was flaky on Windows when trying to clean up the
tmp directory, probably because it relied on child processes
timing out and being killed.

This commit updates the test to check for debug output
from the test runner. This should be adequate because the
original change was effectively:

let concurrency = getOptionValue('--test-concurrency') || true;

The test runner now logs the value of the concurrency variable.

Fixes: nodejs#50101
@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 10, 2023

I have completely rewritten the test. The original change that this test was written for is effectively only this:

let concurrency = getOptionValue('--test-concurrency') || true;

So all we really care about is the value of concurrency. I have updated the test runner to use the NODE_DEBUG variable to log its settings, including that variable.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 11, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 12, 2023
@nodejs-github-bot nodejs-github-bot merged commit 0d8faf2 into nodejs:main Oct 12, 2023
50 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 0d8faf2

@cjihrig cjihrig deleted the test-flake branch October 12, 2023 13:15
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
This test was flaky on Windows when trying to clean up the
tmp directory, probably because it relied on child processes
timing out and being killed.

This commit updates the test to check for debug output
from the test runner. This should be adequate because the
original change was effectively:

let concurrency = getOptionValue('--test-concurrency') || true;

The test runner now logs the value of the concurrency variable.

Fixes: nodejs#50101
PR-URL: nodejs#50108
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
targos pushed a commit that referenced this pull request Nov 11, 2023
This test was flaky on Windows when trying to clean up the
tmp directory, probably because it relied on child processes
timing out and being killed.

This commit updates the test to check for debug output
from the test runner. This should be adequate because the
original change was effectively:

let concurrency = getOptionValue('--test-concurrency') || true;

The test runner now logs the value of the concurrency variable.

Fixes: #50101
PR-URL: #50108
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
targos pushed a commit that referenced this pull request Nov 27, 2023
This test was flaky on Windows when trying to clean up the
tmp directory, probably because it relied on child processes
timing out and being killed.

This commit updates the test to check for debug output
from the test runner. This should be adequate because the
original change was effectively:

let concurrency = getOptionValue('--test-concurrency') || true;

The test runner now logs the value of the concurrency variable.

Fixes: #50101
PR-URL: #50108
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
This test was flaky on Windows when trying to clean up the
tmp directory, probably because it relied on child processes
timing out and being killed.

This commit updates the test to check for debug output
from the test runner. This should be adequate because the
original change was effectively:

let concurrency = getOptionValue('--test-concurrency') || true;

The test runner now logs the value of the concurrency variable.

Fixes: nodejs#50101
PR-URL: nodejs#50108
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This test was flaky on Windows when trying to clean up the
tmp directory, probably because it relied on child processes
timing out and being killed.

This commit updates the test to check for debug output
from the test runner. This should be adequate because the
original change was effectively:

let concurrency = getOptionValue('--test-concurrency') || true;

The test runner now logs the value of the concurrency variable.

Fixes: nodejs/node#50101
PR-URL: nodejs/node#50108
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This test was flaky on Windows when trying to clean up the
tmp directory, probably because it relied on child processes
timing out and being killed.

This commit updates the test to check for debug output
from the test runner. This should be adequate because the
original change was effectively:

let concurrency = getOptionValue('--test-concurrency') || true;

The test runner now logs the value of the concurrency variable.

Fixes: nodejs/node#50101
PR-URL: nodejs/node#50108
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parallel/test-runner-cli-concurrency fails with EBUSY: resource busy or locked
8 participants