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.

Sign up for GitHub

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

Jump to bottom

test: isolate individual assert test to improve test hygiene #20861

Closed
wants to merge 2 commits into from
Closed

test: isolate individual assert test to improve test hygiene #20861

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 21, 2018

First commit:

test: isolate unusual assert test in its own file

test-assert.js contains a test that writes to the source tree, requires
an internal module, and depends on modules not needed by the plethora of
other test cases in the file. Move it to its own file so that there are
not side effects in test-assert.js and so that it can be refactored to
not write to the source tree.

Second commit:

test: improve assert test hygiene

Do not pollute the source tree for the test. Instead of writing to the
source tree, spawn a process with the temp dir as cwd and write the file
there.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label May 21, 2018
@Trott
Copy link
Member Author

Trott commented May 21, 2018

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

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 21, 2018
@Trott
Copy link
Member Author

Trott commented May 21, 2018

Windows is the only platform that wasn't green/yellow, but it was an unrelated (and known) failure.

Windows re-run: https://ci.nodejs.org/job/node-test-commit-windows-fanned/18166/

@BridgeAR
Copy link
Member

This needs a rebase.

Trott added 2 commits May 24, 2018 10:55
@Trott
test: isolate unusual assert test in its own file
029747a 
test-assert.js contains a test that writes to the source tree, requires
an internal module, and depends on modules not needed by the plethora of
other test cases in the file. Move it to its own file so that there are
not side effects in test-assert.js and so that it can be refactored to
not write to the source tree.
@Trott
test: improve assert test hygiene
564ccac 
Do not pollute the source tree for the test. Instead of writing to the
source tree, spawn a process with the temp dir as cwd and write the file
there.
@Trott
Copy link
Member Author

Trott commented May 24, 2018

Rebased.

@Trott
Copy link
Member Author

Trott commented May 26, 2018

Windows CI re-run: https://ci.nodejs.org/job/node-test-commit-windows-fanned/18300/

@Trott
Copy link
Member Author

Trott commented May 28, 2018

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

Trott added a commit to Trott/io.js that referenced this pull request May 28, 2018
@Trott
test: isolate unusual assert test in its own file
6e1324e 
test-assert.js contains a test that writes to the source tree, requires
an internal module, and depends on modules not needed by the plethora of
other test cases in the file. Move it to its own file so that there are
not side effects in test-assert.js and so that it can be refactored to
not write to the source tree.

PR-URL: nodejs#20861
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request May 28, 2018
@Trott
test: improve assert test hygiene
5624a6f 
Do not pollute the source tree for the test. Instead of writing to the
source tree, spawn a process with the temp dir as cwd and write the file
there.

PR-URL: nodejs#20861
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@Trott
Copy link
Member Author

Trott commented May 28, 2018

Landed in cfc3866...5624a6f

@Trott Trott closed this May 28, 2018
MylesBorins pushed a commit that referenced this pull request May 29, 2018
@Trott @MylesBorins
test: isolate unusual assert test in its own file
88f9a39 
test-assert.js contains a test that writes to the source tree, requires
an internal module, and depends on modules not needed by the plethora of
other test cases in the file. Move it to its own file so that there are
not side effects in test-assert.js and so that it can be refactored to
not write to the source tree.

PR-URL: #20861
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 29, 2018
@Trott @MylesBorins
test: improve assert test hygiene
0112357 
Do not pollute the source tree for the test. Instead of writing to the
source tree, spawn a process with the temp dir as cwd and write the file
there.

PR-URL: #20861
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 29, 2018
Merged
@Trott Trott deleted the write-in-tmp branch January 13, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Trott @BridgeAR @jasnell @thefourtheye @cjihrig @trivikr @nodejs-github-bot