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: log errors in test-fs-readfile-tostring-fail #27058

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented Apr 2, 2019

The test writes out a large file via fs.createWriteStream() but was
not listening for the error event, which the fs docs describe as the
reliable way to detect write errors.

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 2, 2019
@nodejs-github-bot
Copy link
Collaborator

test/pummel/test-fs-readfile-tostring-fail.js Outdated Show resolved Hide resolved
@richardlau
Copy link
Member Author

For context: #16601 (comment)

The test is failing on that host because it has less than 1Gb of free disk space, so the file gets truncated and the error does not occur when the file is read. I think moving to pummel is the right answer after all.

I'd expect the test to detect that -- Is it ignoring errors when writing the file out?

@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 2, 2019
@nodejs-github-bot
Copy link
Collaborator

@refack
Copy link
Contributor

refack commented Apr 2, 2019

We're getting the same thing:

11:48:25 not ok 7 pummel/test-fs-readfile-tostring-fail
11:48:25   ---
11:48:25   duration_ms: 24.415
11:48:25   severity: fail
11:48:25   exitcode: 7
11:48:25   stack: |-
11:48:25     /mnt/iojs/node-test-commit-custom-suites-freestyle/test/pummel/test-fs-readfile-tostring-fail.js:69
11:48:25       throw err;
11:48:25       ^
11:48:25     
11:48:25     AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
11:48:25     
11:48:25       assert.ok(err instanceof Error)
11:48:25     
11:48:25         at /mnt/iojs/node-test-commit-custom-suites-freestyle/test/pummel/test-fs-readfile-tostring-fail.js:36:12
11:48:25         at /mnt/iojs/node-test-commit-custom-suites-freestyle/test/common/index.js:369:15
11:48:25         at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:54:3)
11:48:25   ...

https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/5902/console

@refack
Copy link
Contributor

refack commented Apr 2, 2019

Added 33947f7 to try to see other errors
https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/5905/

Got 🍩 :

12:34:42 not ok 7 pummel/test-fs-readfile-tostring-fail
12:34:42   ---
12:34:42   duration_ms: 23.579
12:34:42   severity: fail
12:34:42   exitcode: 1
12:34:43   stack: |-
12:34:43   ...

@BridgeAR
Copy link
Member

What's the status here?

@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member Author

What's the status here?

Rebased and pushed out 33947f7 which broke the linter. Even if this doesn't reveal the original error that is causing the tests to flake on the CI it is no worse and is the correct way to detect write errors according to our documentation so I think we should land this.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

The test writes out a large file via `fs.createWriteStream()` but was
not listening for the `error` event, which the `fs` docs describe as the
reliable way to detect write errors.

PR-URL: nodejs#27058
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@richardlau
Copy link
Member Author

Landed in f85ef97.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants