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: improve test-async-hooks-http-parser-destroy #28253

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Jun 16, 2019

Improve asserts to distinguish between reequest and response parsers.

Change the assert sequence to first assert on the number of ids to easier identify if some operation is missing/incomplete.

Destroy HTTP agent once expected number of events have been seen to avoid waiting on socket timeouts.

Refs: #28112

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

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

@Flarna
Copy link
Member Author

Flarna commented Jul 3, 2019

I don't think all these test fails are caused by my changes...

@nodejs-github-bot
Copy link
Collaborator

@Flarna
Copy link
Member Author

Flarna commented Jul 4, 2019

Seems the test I modified fails now in node-test-commit-custom-suites-freestyle (test-worker). But there are other, unrelated fails there. Is this something I should look into or is this build known to have some issues?

Other fails look unrelated to me.

@Trott
Copy link
Member

Trott commented Jul 4, 2019

Seems the test I modified fails now in node-test-commit-custom-suites-freestyle (test-worker). But there are other, unrelated fails there. Is this something I should look into or is this build known to have some issues?

Other fails look unrelated to me.

Ignore unrelated fails. To test the worker thing locally: tools/test.py --worker test/parallel/test-async-hooks-http-parser-destroy.js.

@nodejs-github-bot
Copy link
Collaborator

@Flarna
Copy link
Member Author

Flarna commented Jul 4, 2019

According to docs the use of process.on('SIGTERM', <listener>) is wrong in such tests as signals are not delivered in workers.
I'm not sure about process.on('exit', <listener>) as docs explicit mention signals are not delivered. But from pure naming point of view it looks not correct.
Are there any limitations/races/known issues regarding exit event and workers?

@Flarna
Copy link
Member Author

Flarna commented Jul 4, 2019

I was able to reproduce a similar fail locally and was able to fix it by closing server later. I haven't fully understood why this matters but it seems server.close() behaves different in workers for some reason.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Improve asserts to distinguish between reequest and response parsers.

Change the assert sequence to first assert on the number of ids to
easier identify if some operation is missing/incomplete.

Destroy HTTP agent once expected number of events have been seen to
avoid waiting on socket timeouts.

Refs: nodejs#28112
@Flarna Flarna force-pushed the test-async-hooks-http-parser-destroy branch from e8b11c4 to 5ebef48 Compare July 11, 2019 16:00
@Flarna
Copy link
Member Author

Flarna commented Jul 11, 2019

Rebased as indicated at #28610 (comment) - maybe this helps CI. If not I'm out of ideas and will abandon this one.

@nodejs-github-bot
Copy link
Collaborator

@Flarna
Copy link
Member Author

Flarna commented Jul 16, 2019

If I understand CI correct again an unrelated fail from not ok 2503 sequential/test-perf-hooks.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jul 30, 2019

Landed in 8007134.

@Trott Trott closed this Jul 30, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jul 30, 2019
Improve asserts to distinguish between reequest and response parsers.

Change the assert sequence to first assert on the number of ids to
easier identify if some operation is missing/incomplete.

Destroy HTTP agent once expected number of events have been seen to
avoid waiting on socket timeouts.

Refs: nodejs#28112

PR-URL: nodejs#28253
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@Flarna Flarna deleted the test-async-hooks-http-parser-destroy branch July 30, 2019 05:18
targos pushed a commit that referenced this pull request Aug 2, 2019
Improve asserts to distinguish between reequest and response parsers.

Change the assert sequence to first assert on the number of ids to
easier identify if some operation is missing/incomplete.

Destroy HTTP agent once expected number of events have been seen to
avoid waiting on socket timeouts.

Refs: #28112

PR-URL: #28253
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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.

7 participants