Skip to content

test: move test-http-dump-req-when-res-end to sequential#19819

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:cornflakes
Closed

test: move test-http-dump-req-when-res-end to sequential#19819
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:cornflakes

Conversation

@Trott

@Trott Trott commented Apr 5, 2018

Copy link
Copy Markdown
Member

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

Trott commented Apr 5, 2018

Copy link
Copy Markdown
Member Author

@lpinca

lpinca commented Apr 5, 2018

Copy link
Copy Markdown
Member

Moving it to sequential != fix

@Trott

Trott commented Apr 5, 2018

Copy link
Copy Markdown
Member Author

Moving it to sequential != fix

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

@lpinca

lpinca commented Apr 5, 2018

Copy link
Copy Markdown
Member

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

@Trott

Trott commented Apr 5, 2018

Copy link
Copy Markdown
Member Author

"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

lpinca commented Apr 5, 2018

Copy link
Copy Markdown
Member

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

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

@Trott

Trott commented Apr 5, 2018

Copy link
Copy Markdown
Member Author

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

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

@Trott

Trott commented Apr 5, 2018

Copy link
Copy Markdown
Member Author

@lpinca

lpinca commented Apr 5, 2018

Copy link
Copy Markdown
Member

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

@mcollina

mcollina commented Apr 5, 2018

Copy link
Copy Markdown
Member

CI seems ok.

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

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

@lpinca

lpinca commented Apr 5, 2018

Copy link
Copy Markdown
Member

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

@lpinca

lpinca commented Apr 5, 2018

Copy link
Copy Markdown
Member

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

mcollina commented Apr 5, 2018

Copy link
Copy Markdown
Member

Proper fix in #19823.

@Trott Trott mentioned this pull request Apr 7, 2018
3 tasks
@Trott

Trott commented Apr 7, 2018

Copy link
Copy Markdown
Member Author

@Trott

Trott commented Apr 7, 2018

Copy link
Copy Markdown
Member Author

CI re-runs are green. Finally landing.

@Trott

Trott commented Apr 7, 2018

Copy link
Copy Markdown
Member Author

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

mcollina commented Apr 7, 2018

Copy link
Copy Markdown
Member

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

Trott commented Apr 7, 2018

Copy link
Copy Markdown
Member Author

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