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

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
@lpinca
lpinca approved these changes Jun 28, 2019
@Trott
Trott approved these changes Jun 30, 2019

@Trott Trott added the author ready label Jun 30, 2019

@nodejs-github-bot

This comment has been minimized.

@Flarna

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

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

@nodejs-github-bot

This comment has been minimized.

@Flarna

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

@Flarna

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

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

@Flarna Flarna force-pushed the dynatrace-oss-contrib:test-async-hooks-http-parser-destroy branch from e8b11c4 to 5ebef48 Jul 11, 2019

@Flarna

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

@Flarna

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

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

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Landed in 8007134.

@Trott Trott closed this Jul 30, 2019

Trott added a commit to Trott/io.js that referenced this pull request Jul 30, 2019
test: improve test-async-hooks-http-parser-destroy
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 dynatrace-oss-contrib:test-async-hooks-http-parser-destroy branch Jul 30, 2019

targos added a commit that referenced this pull request Aug 2, 2019
test: improve test-async-hooks-http-parser-destroy
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>
@BridgeAR BridgeAR referenced this pull request Aug 6, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
test: improve test-async-hooks-http-parser-destroy
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>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
test: improve test-async-hooks-http-parser-destroy
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.