GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: fix flaky test-runner-cli-concurrency.js by cjihrig · Pull Request #50108 · nodejs/node

Conversation

@cjihrig

@cjihrig cjihrig commented Oct 10, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with eyes emoji
All reactions
  • 👀 1 reaction
@nodejs-github-bot

nodejs-github-bot commented Oct 10, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure. labels Oct 10, 2023
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 10, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 10, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 10, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cjihrig
cjihrig force-pushed the test-flake branch from e4a1b3d to cb1688a Compare October 10, 2023 14:43
@nodejs-github-bot

nodejs-github-bot commented Oct 10, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95

aduh95 commented Oct 10, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aduh95
aduh95 approved these changes Oct 10, 2023
View reviewed changes
GeoffreyBooth
GeoffreyBooth approved these changes Oct 10, 2023
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Oct 10, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca
lpinca approved these changes Oct 10, 2023
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Oct 10, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@StefanStojanovic

StefanStojanovic commented Oct 10, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cjihrig
test_runner,test: fix flaky test-runner-cli-concurrency.js
e8f7146
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
cjihrig force-pushed the test-flake branch from cb1688a to e8f7146 Compare October 10, 2023 22:23
@cjihrig

cjihrig commented Oct 10, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

StefanStojanovic reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Oct 10, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Oct 11, 2023
CI Reliability 2023-10-11 nodejs/reliability#690
Open
30 tasks
@nodejs-github-bot

nodejs-github-bot commented Oct 11, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Oct 11, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Oct 11, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

anonrig
anonrig approved these changes Oct 11, 2023
View reviewed changes
@anonrig anonrig added the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 11, 2023
@richardlau richardlau mentioned this pull request Oct 11, 2023
test: add EOVERFLOW as an allowed error #50128
Merged
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 12, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 0d8faf2 into nodejs:main Oct 12, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 12, 2023

Copy link
Copy Markdown
Collaborator

Landed in 0d8faf2

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@cjihrig
cjihrig deleted the test-flake branch October 12, 2023 13:15
@RafaelGSS RafaelGSS mentioned this pull request Oct 15, 2023
2023-10-17, Version 21.0.0 (Current) #49870
Merged
@richardlau richardlau mentioned this pull request Oct 17, 2023
fs: add flush option to appendFile() functions #50095
Merged
targos pushed a commit that referenced this pull request Nov 11, 2023
@cjihrig @targos
test_runner,test: fix flaky test-runner-cli-concurrency.js
64ef108
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 targos mentioned this pull request Nov 12, 2023
v20.10.0 release proposal #50682
Merged
targos pushed a commit that referenced this pull request Nov 27, 2023
@cjihrig @targos
test_runner,test: fix flaky test-runner-cli-concurrency.js
4b2e258
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 targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged
@juanarbol juanarbol mentioned this pull request Dec 6, 2023
2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41
Merged
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
@cjihrig @debadree25
test_runner,test: fix flaky test-runner-cli-concurrency.js
128aae4
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
@sercher
test_runner,test: fix flaky test-runner-cli-concurrency.js
df81730
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
@sercher
test_runner,test: fix flaky test-runner-cli-concurrency.js
55212e2
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

Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes
@lpinca lpinca lpinca approved these changes
@anonrig anonrig anonrig approved these changes
@aduh95 aduh95 aduh95 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@cjihrig @nodejs-github-bot @aduh95 @StefanStojanovic @GeoffreyBooth @lpinca @anonrig @richardlau
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.