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: add gc tracking API to common, refactor pummel net test + move it to parallel #21794

Closed
wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

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

@addaleax addaleax requested a review from Trott July 13, 2018 16:39
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 13, 2018
@addaleax addaleax added the net Issues and PRs related to the net subsystem. label Jul 13, 2018
@Trott
Copy link
Member

Trott commented Jul 13, 2018

Any chance these changes also apply to the other two memleak tests in pummel? (test/pummel/test-vm-memleak.js and/or test/pummel/test-tls-connect-memleak.js)

Also, we should probably run a stress test just to make sure we're not introducing unreliable tests into the parallel directory. I'll do it once this gets another review or two and the above question is resolved. Or if you want to do it, cool. Just include -j96 --repeat 192 or something like that in the RUN_TESTS parameter before you list out the test or tests to be stressed. Probably best to keep RUN_TIMES to 100 in that case. Running the test 19200 should be plenty. :-D

This enables the test to run as part of the regular test suite.
@addaleax
Copy link
Member Author

addaleax commented Jul 13, 2018

Any chance these changes also apply to the other two memleak tests in pummel? (test/pummel/test-vm-memleak.js and/or test/pummel/test-tls-connect-memleak.js)

Yes – not sure about the VM test but the TLS one is definitely of the same kind. Added a similar commit for the TLS test :)

Also, we should probably run a stress test just to make sure we're not introducing unreliable tests into the parallel directory. I'll do it once this gets another review or two and the above question is resolved. Or if you want to do it, cool. Just include -j96 --repeat 192 or something like that in the RUN_TESTS parameter before you list out the test or tests to be stressed. Probably best to keep RUN_TIMES to 100 in that case. Running the test 19200 should be plenty. :-D

Locally, all 3 tests happily pass -j96 --repeat 192.

Regular CI: https://ci.nodejs.org/job/node-test-pull-request/15859/
Stress test CI: https://ci.nodejs.org/job/node-stress-single-test/1960/ (edit: ✔️ )
Regular CI again: https://ci.nodejs.org/job/node-test-pull-request/15868/

@@ -326,6 +326,21 @@ See `common.expectWarning()` for usage.

Indicates whether 'opensslCli' is supported.

### onGC(target, listener)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this section go before the opensslCli(), ABC-wise?

### onGC(target, listener)
* `target` [<Object>]
* `listener` [<Object>]
* `listener.ongc` [<Function>]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`listener.ongc` -> `ongc` for consistency with other doc cases?

a full `setImmediate()` invocation passes.

`listener` is an object to make it easier to use a closure; the target object
should not be in scope when `listener.ongc` is created.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

`listener.ongc` -> `listener.ongc()`?

@addaleax
Copy link
Member Author

@vsemozhetbyt Thanks, done in 24bd33b :)

@addaleax
Copy link
Member Author

Landed in d94950e...9a34c5b

@addaleax addaleax closed this Jul 16, 2018
@addaleax addaleax deleted the pummel-net-connect-memleak branch July 16, 2018 18:52
addaleax added a commit that referenced this pull request Jul 16, 2018
PR-URL: #21794
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Jul 16, 2018
This enables the test to run as part of the regular test suite.

PR-URL: #21794
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Jul 16, 2018
This enables the test to run as part of the regular test suite.

PR-URL: #21794
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 16, 2018
PR-URL: #21794
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 16, 2018
This enables the test to run as part of the regular test suite.

PR-URL: #21794
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 16, 2018
This enables the test to run as part of the regular test suite.

PR-URL: #21794
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants