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

Conversation

Projects
None yet
8 participants
@Trott
Member

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

test: make test-fs-access stricter
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 test labels Jan 31, 2017

@edsadr edsadr referenced this pull request Jan 31, 2017

Closed

test: improve test-fs-access error validation #10988

3 of 3 tasks complete
@edsadr

edsadr approved these changes Jan 31, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev approved these changes Feb 1, 2017

@lpinca

lpinca approved these changes Feb 1, 2017

@hiroppy

hiroppy approved these changes Feb 1, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 2, 2017

Member

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/

Member

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

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 2, 2017

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 2, 2017

Member

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/

Member

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 2, 2017

Member

Looks like the CI stalled out again

Member

jasnell commented Feb 2, 2017

Looks like the CI stalled out again

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Feb 2, 2017

Member

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/

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 2, 2017

Member

Looks like CI was successful on that last run

Member

jasnell commented Feb 2, 2017

Looks like CI was successful on that last run

jasnell added a commit that referenced this pull request Feb 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 2, 2017

Member

Landed in dfd3046

Member

jasnell commented Feb 2, 2017

Landed in dfd3046

@jasnell jasnell closed this Feb 2, 2017

italoacasas added a commit that referenced this pull request Feb 3, 2017

test: make test-fs-access stricter
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 added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: make test-fs-access stricter
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 added a commit to KryDos/node that referenced this pull request Feb 25, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

Will need a backport PR to land on v6 or v4

Member

jasnell commented Mar 7, 2017

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