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 too optimistic guess in setproctitle #12792

Closed
wants to merge 1 commit into from
Closed

test: fix too optimistic guess in setproctitle #12792

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

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

test, process

test/parallel/test-setproctitle.js assumes that process.title can't be test before the test changes it. However, if this test is launched as a child several times, process.title is saved in the parent on Windows and is shared with childs.

I've found this issue running tests with some script with different options. A reduced example (edit the path to reproduce):

const { execSync } = require('child_process');
const cmd = 'node j:/temp/_git/node-fork/test/parallel/test-setproctitle.js';

console.log('run 1');
try { execSync(cmd); } catch (err) { console.log('run 1 error'); }
console.log('run 2');
try { execSync(cmd); } catch (err) { console.log('run 2 error'); }

Before this PR:

run 1
run 2

assert.js:86
  throw new assert.AssertionError({
  ^
AssertionError: 'test' !== 'test'
    at Object.<anonymous> (j:\temp\_git\node-fork\test\parallel\test-setproctitle.js:19:8)
    at Module._compile (module.js:582:30)
    at Object.Module._extensions..js (module.js:593:10)
    at Module.load (module.js:516:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:618:10)
    at startup (bootstrap_node.js:144:16)
    at bootstrap_node.js:548:3
run 2 error

After this PR:

run 1
run 2

It seems this better be fixed to prevent such issues with various test approaches to test base.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 2, 2017
@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt vsemozhetbyt added process Issues and PRs related to the process subsystem. s390 Issues and PRs related to the s390 architecture. and removed s390 Issues and PRs related to the s390 architecture. labels May 2, 2017
@Fishrock123
Copy link
Contributor

AssertionError: 'test' !== 'test' ... err what is going there? Different encoding?

@vsemozhetbyt
Copy link
Contributor Author

@Fishrock123 Actually, the 'test' === 'test' here, what is against assert's expectation.

@Fishrock123
Copy link
Contributor

Ah sorry, my bad.

@jasnell
Copy link
Member

jasnell commented May 2, 2017

That's confusing. Perhaps it's worth changing the message?

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented May 2, 2017

@jasnell Do you mean commit message? What would you advise?

@jasnell
Copy link
Member

jasnell commented May 2, 2017

No, the message on the assert error

@vsemozhetbyt
Copy link
Contributor Author

What message would you add?

@Trott
Copy link
Member

Trott commented May 3, 2017

That's the default message from assert.notStrictEqual(). I recommend leaving it exactly as is.

@vsemozhetbyt
Copy link
Contributor Author

Landed in f971916

@vsemozhetbyt vsemozhetbyt deleted the test-fs-chmod.js branch May 4, 2017 18:16
vsemozhetbyt added a commit that referenced this pull request May 4, 2017
PR-URL: #12792
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
PR-URL: nodejs#12792
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

Is this okay to land on v6.x?

Should land with #13072

DavidCai1111 pushed a commit to DavidCai1111/node that referenced this pull request Jun 19, 2017
PR-URL: nodejs#12792
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 17, 2017
PR-URL: #12792
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants