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: move test-http-dump-req-when-res-end to sequential #19819

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 5, 2018

The implementataion is sensitive to system resource availability, so
move it to sequential instead of running out of parallel directory.

Fixes: #19139

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 5, 2018
@Trott
Copy link
Member Author

Trott commented Apr 5, 2018

@lpinca
Copy link
Member

lpinca commented Apr 5, 2018

Moving it to sequential != fix

@Trott
Copy link
Member Author

Trott commented Apr 5, 2018

Moving it to sequential != fix

I'm open to other verbs. "mitigate"? "work around"? Something else?

@lpinca
Copy link
Member

lpinca commented Apr 5, 2018

"test: move http-dump-req-when-res-ends to sequential" ?

@Trott
Copy link
Member Author

Trott commented Apr 5, 2018

"test: move http-dump-req-when-res-ends to sequential"

Works for me!

@Trott Trott changed the title test: fix flaky test-http-dump-req-when-res-end test: move test-http-dump-req-when-res-end to sequential Apr 5, 2018
The implementataion is sensitive to system resource availability, so
move it to sequential instead of running out of parallel directory.

Fixes: nodejs#19139
@lpinca
Copy link
Member

lpinca commented Apr 5, 2018

FWIW the test is using common.mustNotCall instead of common.mustNotCall().

req.on('end', common.mustNotCall);

@Trott
Copy link
Member Author

Trott commented Apr 5, 2018

FWIW the test is using common.mustNotCall instead of common.mustNotCall().

Ugh, and fixing that causes the test to fail...

@Trott
Copy link
Member Author

Trott commented Apr 5, 2018

@lpinca
Copy link
Member

lpinca commented Apr 5, 2018

cc: @mcollina as the test was added in 29be1e5

@mcollina
Copy link
Member

mcollina commented Apr 5, 2018

CI seems ok.

@lpinca what should I do? CI is passing with this change.

Should I send a PR to fix #19819 (comment)?

@lpinca
Copy link
Member

lpinca commented Apr 5, 2018

@mcollina yes but by doing that the test fails so I'm not sure if it is working as it was originally intended.

@lpinca
Copy link
Member

lpinca commented Apr 5, 2018

The following part of the test

// end is not called as we are just exhausting
// the in-memory buffer
req.on('end', common.mustNotCall);
// this 'data' handler will be removed when dumped
req.on('data', common.mustNotCall);

is not testing anything right now.

@mcollina
Copy link
Member

mcollina commented Apr 5, 2018

Proper fix in #19823.

@Trott Trott mentioned this pull request Apr 7, 2018
3 tasks
@Trott
Copy link
Member Author

Trott commented Apr 7, 2018

@Trott
Copy link
Member Author

Trott commented Apr 7, 2018

CI re-runs are green. Finally landing.

@Trott
Copy link
Member Author

Trott commented Apr 7, 2018

Landed in a639ec4

@Trott Trott closed this Apr 7, 2018
Trott added a commit to Trott/io.js that referenced this pull request Apr 7, 2018
The implementataion is sensitive to system resource availability, so
move it to sequential instead of running out of parallel directory.

Fixes: nodejs#19139

PR-URL: nodejs#19819
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mcollina
Copy link
Member

mcollina commented Apr 7, 2018

I don’t think this was needed at with the other PR. The provided command was running fine on my machine. I’m sorry this was landed without a discussion and/or verifying that the other PR solved the problem first.

@Trott
Copy link
Member Author

Trott commented Apr 7, 2018

I don’t think this was needed at with the other PR. The provided command was running fine on my machine. I’m sorry this was landed without a discussion and/or verifying that the other PR solved the problem first.

I confirmed that the other PR did not fix the problem when multiple tests were running at once.

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

Successfully merging this pull request may close these issues.

Investigate flaky parallel/test-http-dump-req-when-res-ends.js
7 participants