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

[v6.x] test: investigate test-fs-read-buffer-tostring-fail #14430

Closed
refack opened this issue Jul 22, 2017 · 12 comments
Closed

[v6.x] test: investigate test-fs-read-buffer-tostring-fail #14430

refack opened this issue Jul 22, 2017 · 12 comments
Labels
buffer Issues and PRs related to the buffer subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.

Comments

@refack
Copy link
Contributor

refack commented Jul 22, 2017

  • Version: v6.x-staging
  • Platform: macOS
  • Subsystem: test
not ok 386 parallel/test-fs-read-buffer-tostring-fail
  ---
  duration_ms: 61.861
  severity: fail
  stack: |-
    timeout
  ...

https://ci.nodejs.org/job/node-test-commit-osx/11278/nodes=osx1010/

@refack refack added buffer Issues and PRs related to the buffer subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests. labels Jul 22, 2017
@refack
Copy link
Contributor Author

refack commented Jul 22, 2017

Test file - https://github.com/nodejs/node/blob/v6.x-staging/test/parallel/test-fs-read-buffer-tostring-fail.js
/cc @nodejs/platform-macos @nodejs/streams @nodejs/buffer

@mcollina
Copy link
Member

@refack I don't think it's possible to not have this test as flaky. We are generating a very big file on memory, then we try to save that extremely big file on disk, and then load it. Everything can happen in all this I/O and GC() activity.

However, one thing we can do: convert https://github.com/nodejs/node/blob/v6.x-staging/test/parallel/test-fs-read-buffer-tostring-fail.js#L29-L33 into the pattern as the end of the https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback (the one using the 'drain' event). That would reduce memory allocation on the Node.js process during the setup phase.

I'm happy to review a PR that addresses this issue, and I think it would be good for someone that wants to improve their streams knowledge.

@refack
Copy link
Contributor Author

refack commented Jul 24, 2017

Also in the worse case, could create a PR adding

test-fs-read-buffer-tostring-fail      : PASS,FLAKY

to https://github.com/nodejs/node/blob/v6.x-staging/test/parallel/parallel.status#L7

@refack refack added the good first issue Issues that are suitable for first-time contributors. label Jul 24, 2017
@Jeyanthinath
Copy link
Contributor

Can I take and contribute to this ?

@bnoordhuis
Copy link
Member

@Jeyanthinath Go for it.

@Trott Trott removed the good first issue Issues that are suitable for first-time contributors. label Jul 25, 2017
@refack refack added the wip Issues and PRs that are still a work in progress. label Jul 25, 2017
@Jeyanthinath
Copy link
Contributor

Jeyanthinath commented Jul 25, 2017

PR #14472 ( deleted )

@Jeyanthinath
Copy link
Contributor

New PR #14495

@Trott
Copy link
Member

Trott commented Jul 26, 2017

I don't think it's possible to not have this test as flaky.

If the test is necessarily flaky on 6.x, then I'm not sure it makes sense to mark it as flaky on 6.x. By marking it as flaky, the results will always be ignored. For a test where there's a chance of fixing the issue, that's fine.

For a test where there's no possible fix and/or no intention of fixing, it may be better to remove the test rather than have a test whose results are ignored.

Or if the test is minimally flaky, then leave the status alone. You'll get red once in a month or whatever, but if it starts going red 100% of the time, you'll know something is (probably) broken.

Not a blocking objection on the "mark as flaky" approach. Just suggestions. Feel free to proceed with the current plan if you disagree.

@Trott
Copy link
Member

Trott commented Jul 26, 2017

Oh, not sure it's reasonable if the test is resource-intensive, but another option would be to program in a retry into the test before deciding the test has failed. Not great, but probably better than not having a test or having a test whose failures are discounted.

@refack
Copy link
Contributor Author

refack commented Jul 27, 2017

If the test is necessarily flaky on 6.x, then I'm not sure it makes sense to mark it as flaky on 6.x. By marking it as flaky, the results will always be ignored. For a test where there's a chance of fixing the issue, that's fine.

IMHO yellow ⚠️ is better than ❌. It communicates that something it sort of wrong, and if it becomes consistent, it will get investigated. Red just makes people dig, find it's unrelated, and lose faith in the CI.

I do agree that it the test could be rewritten to stabilize it, that is the better option in the longer term.

@mcollina
Copy link
Member

mcollina commented Aug 1, 2017

Considering the fact that the test is gone from master as it is targeting a deprecated API, I'm definitely 👍 in flagging it as flaky.

refack pushed a commit to Jeyanthinath/node that referenced this issue Aug 1, 2017
PR-URL: nodejs#14495
Fixes: nodejs#14430
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 14, 2017
PR-URL: #14495
Fixes: #14430
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 14, 2017
PR-URL: #14495
Fixes: #14430
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
PR-URL: #14495
Fixes: #14430
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@maclover7 maclover7 added v6.x and removed wip Issues and PRs that are still a work in progress. labels Dec 24, 2017
@apapirovski
Copy link
Member

As far as I can tell this has been resolved by marking it as flaky. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

7 participants