test: make test-fs-access stricter #11087
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change regular expression matching in `assert.throws()` to match the entire error message. In `assert.throws()` that uses a function for matching rather than a regular expression, add checks for the `message` property and the error's constructor. Also, refactored to remove unnecessary temp file handling. No need to remove temp files after the test. Each test is responsible for clearing the temp directory if it needs to use it.
Trott
added
fs
jasnell
approved these changes
Jan 31, 2017
3 tasks
edsadr
approved these changes
Jan 31, 2017
cjihrig
approved these changes
Jan 31, 2017
danbev
approved these changes
Feb 1, 2017
lpinca
approved these changes
Feb 1, 2017
hiroppy
approved these changes
Feb 1, 2017
|
CI failures on FreeBSD were due to stalled processes and are unrelated to this change. AIX failure is a known flaky. Re-running CI although it looks like CI might be having issues right now...: https://ci.nodejs.org/job/node-test-pull-request/6157/ |
|
AIX failure due to stalled processes, unrelated, but hey, let's try CI again: https://ci.nodejs.org/job/node-test-pull-request/6159/ |
|
We got a different box with stalled processes this last time. Fixed it too. Once more with feeling... CI: https://ci.nodejs.org/job/node-test-pull-request/6160/ |
|
Looks like the CI stalled out again |
|
I looked at the AIX machines and there weren't an obvious number of processes lying around. I cleaned up 3 from iojs 5767336 14155960 0 11:10:39 - 0:00 /home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/out/Release/node /home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/fixtures/clustered-server/app.js /home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/fixtures/clustered-server/app.js
iojs 9044052 14155960 0 11:10:39 - 0:00 /home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/out/Release/node /home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/fixtures/clustered-server/app.js /home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/fixtures/clustered-server/app.js
iojs 14155960 1 0 11:10:39 - 0:00 /home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/out/Release/node /home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/fixtures/clustered-server/app.js /home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/fixtures/clustered-server/app.js |
|
Looks like CI was successful on that last run |
jasnell
pushed a commit
that referenced
this pull request
Feb 2, 2017
Change regular expression matching in `assert.throws()` to match the entire error message. In `assert.throws()` that uses a function for matching rather than a regular expression, add checks for the `message` property and the error's constructor. Also, refactored to remove unnecessary temp file handling. No need to remove temp files after the test. Each test is responsible for clearing the temp directory if it needs to use it. PR-URL: #11087 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
|
Landed in dfd3046 |
italoacasas
pushed a commit
that referenced
this pull request
Feb 3, 2017
Change regular expression matching in `assert.throws()` to match the entire error message. In `assert.throws()` that uses a function for matching rather than a regular expression, add checks for the `message` property and the error's constructor. Also, refactored to remove unnecessary temp file handling. No need to remove temp files after the test. Each test is responsible for clearing the temp directory if it needs to use it. PR-URL: #11087 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
italoacasas
pushed a commit
to italoacasas/node
that referenced
this pull request
Feb 14, 2017
Change regular expression matching in `assert.throws()` to match the entire error message. In `assert.throws()` that uses a function for matching rather than a regular expression, add checks for the `message` property and the error's constructor. Also, refactored to remove unnecessary temp file handling. No need to remove temp files after the test. Each test is responsible for clearing the temp directory if it needs to use it. PR-URL: nodejs#11087 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
krydos
pushed a commit
to krydos/node
that referenced
this pull request
Feb 25, 2017
Change regular expression matching in `assert.throws()` to match the entire error message. In `assert.throws()` that uses a function for matching rather than a regular expression, add checks for the `message` property and the error's constructor. Also, refactored to remove unnecessary temp file handling. No need to remove temp files after the test. Each test is responsible for clearing the temp directory if it needs to use it. PR-URL: nodejs#11087 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Adrian Estrada <edsadr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
|
Will need a backport PR to land on v6 or v4 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Change regular expression matching in
assert.throws()to match theentire error message. In
assert.throws()that uses a function formatching rather than a regular expression, add checks for the
messageproperty and the error's constructor.
Also, refactored to remove unnecessary temp file handling. No need to
remove temp files after the test. Each test is responsible for clearing
the temp directory if it needs to use it.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test fs