Skip to content

test: deflake test-config-file #58799

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

Closed
wants to merge 2 commits into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Jun 23, 2025

test: deflake test-config-file

Port 9229 may already be used by another process. Use a random
available one.

test: save the config file in a temporary directory

Allow the test to be run in parallel.

Refs: #58799 (comment)

Port 9229 may already be used by another process. Use a random
available one.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/config

@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 Jun 23, 2025
@lpinca
Copy link
Member Author

lpinca commented Jun 23, 2025

This fixes

not ok 228 parallel/test-config-file
  ---
  duration_ms: 4215.14300
  severity: fail
  exitcode: 1
  stack: |-
    Test failure: '--inspect=true should be parsed correctly'
    Location: test\parallel\test-config-file.js:237:1
    AssertionError [ERR_ASSERTION]: The input did not match the regular expression /^Debugger listening on (ws:\/\/[^\s]+)/. Input:
    
    'Starting inspector on 127.0.0.1:9229 failed: address already in use\r\n'
    
        at TestContext.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-config-file.js:244:3)
        at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
        at async Test.run (node:internal/test_runner/test:1054:7)
        at async Test.processPendingSubtests (node:internal/test_runner/test:744:7) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 'Starting inspector on 127.0.0.1:9229 failed: address already in use\r\n',
      expected: /^Debugger listening on (ws:\/\/[^\s]+)/,
 ...

@lpinca
Copy link
Member Author

lpinca commented Jun 23, 2025

I also think that this test

// Skip on windows because it doesn't support chmod changing read permissions
// Also skip if user is root because it would have read permissions anyway
test('should throw an error when the file is non readable', {
skip: common.isWindows || process.getuid() === 0,
}, async () => {
chmodSync(fixtures.path('rc/non-readable/node.config.json'), constants.O_RDONLY);
const result = await spawnPromisified(process.execPath, [
'--no-warnings',
'--experimental-default-config-file',
'-p', 'http.maxHeaderSize',
], {
cwd: fixtures.path('rc/non-readable'),
});
match(result.stderr, /Cannot read configuration from node\.config\.json: permission denied/);
strictEqual(result.stdout, '');
strictEqual(result.code, 9);
chmodSync(fixtures.path('rc/non-readable/node.config.json'),
constants.S_IRWXU | constants.S_IRWXG | constants.S_IRWXO);
});

Should be moved to sequential or use a temporary directory as it cannot be run in parallel (./tools/tests.py --repeat 1000).

lpinca added a commit to lpinca/node that referenced this pull request Jun 23, 2025
@lpinca
Copy link
Member Author

lpinca commented Jun 23, 2025

PTAL, I've added a new a commit to address my previous comment.

@lpinca lpinca force-pushed the deflake/test-config-file branch from 6e1d0c0 to 8bfd35b Compare June 23, 2025 10:04
@lpinca lpinca added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Jun 23, 2025
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 23, 2025
@marco-ippolito marco-ippolito added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 23, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 23, 2025
@nodejs-github-bot
Copy link
Collaborator

Copy link

codecov bot commented Jun 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.08%. Comparing base (910a8af) to head (8bfd35b).
Report is 23 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58799      +/-   ##
==========================================
+ Coverage   90.07%   90.08%   +0.01%     
==========================================
  Files         640      640              
  Lines      188287   188262      -25     
  Branches    36909    36912       +3     
==========================================
- Hits       169599   169598       -1     
+ Misses      11410    11397      -13     
+ Partials     7278     7267      -11     

see 44 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 23, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 25, 2025
@nodejs-github-bot
Copy link
Collaborator

Landed in ac540c0...100c6da

nodejs-github-bot pushed a commit that referenced this pull request Jun 25, 2025
Port 9229 may already be used by another process. Use a random
available one.

PR-URL: #58799
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Pietro Marchini <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Jun 25, 2025
Allow the test to be run in parallel.

Refs: #58799 (comment)
PR-URL: #58799
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Pietro Marchini <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Edy Silva <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. 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.

8 participants