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: make test-fs-access stricter #11087

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 31, 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.

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

test fs

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 Trott added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels Jan 31, 2017
@Trott
Copy link
Member Author

Trott commented Jan 31, 2017

@Trott
Copy link
Member Author

Trott commented Feb 2, 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/

@Trott
Copy link
Member Author

Trott commented Feb 2, 2017

AIX failure due to stalled processes, unrelated, but hey, let's try CI again: https://ci.nodejs.org/job/node-test-pull-request/6159/

@Trott
Copy link
Member Author

Trott commented Feb 2, 2017

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/

@jasnell
Copy link
Member

jasnell commented Feb 2, 2017

Looks like the CI stalled out again

@gibfahn
Copy link
Member

gibfahn commented Feb 2, 2017

I looked at the AIX machines and there weren't an obvious number of processes lying around. I cleaned up 3 from test-osuosl-aix61-ppc64_be-2, but there weren't any on test-osuosl-aix61-ppc64_be-1, and the job failed on both.

    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

CI 3: https://ci.nodejs.org/job/node-test-commit/7623/

@jasnell
Copy link
Member

jasnell commented Feb 2, 2017

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>
@jasnell
Copy link
Member

jasnell commented Feb 2, 2017

Landed in dfd3046

@jasnell jasnell closed this Feb 2, 2017
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>
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

Will need a backport PR to land on v6 or v4

@Trott Trott deleted the fs-access branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants