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 heapdump tests to pummel #25181

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@Trott
Copy link
Member

Trott commented Dec 22, 2018

The heapdump tests take a lot more time to run than our other tests in
parallel. They are also a bit of an internal test that perhaps does not
need to be run on every commit on every platform. This change moves them
to the pummel directory where they will be run on a single platform once
a day in CI.

This shaves more than 20 seconds off make test on my laptop, FWIW.

@addaleax @Fishrock123

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

This comment has been minimized.

Copy link
Member Author

Trott commented Dec 22, 2018

pummel tests are not (yet) run in CI, so lite CI seems sufficient.

@lpinca

lpinca approved these changes Dec 22, 2018

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Dec 26, 2018

Hm … just for context, the original motivation for these tests was increasing native code coverage. They do take 20 seconds for me as well, but only when not run in parallel, so things could be worse…

Does it make sense to maybe create a directory for tests that should be run locally only on request, and in CI always? We have a lot of tests that would qualify for that, imo, and that might be a good path to speeding up make test in general?

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Dec 26, 2018

Does it make sense to maybe create a directory for tests that should be run locally only on request, and in CI always?

The temptation there would be to put all the slow tests in the CI-only directory.

My sense is that it would be better to split out unit tests from end-to-end tests. The unit tests will run fast and you can run them very frequently. The end-to-end tests will definitely run on CI but optionally locally as well. (People can choose whether they are that kind of completist or not.)

Then we can collect coverage stats for unit tests only and coverage stats for integration tests. The goal would be to get good coverage with both types of tests.

If we split out the tests based on speed only, then it's far more subjective. (What is the cut-off and on what type of machine? What of tests that are slow only on, say, Windows?) There's subjectivity with unit-test vs. integration test too, but I would expect it to be less frequent.

All that said, as resistant as I've been to the idea (e.g., @BridgeAR proposed something similar at some point, I'm pretty sure, many months ago), separating the slow tests may be a decent first step. I'm just concerned it will also be the last step...

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Dec 26, 2018

Also: Arguably, we already have a place for slow tests: pummel. We just need to get it running on CI. I had said once a day, but we can have it run on every CI.

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Dec 26, 2018

Lastly: I don't intend for this to be merged until pummel is running in CI (whether once a day or on every pull request). So I'll put a blocked label on this for now.

@Trott Trott added the blocked label Dec 26, 2018

test: move heapdump tests to pummel
The heapdump tests take a lot more time to run than our other tests in
parallel. They are also a bit of an internal test that perhaps does not
need to be run on every commit on every platform. This change moves them
to the pummel directory where they will be run on a single platform once
a day in CI.

This shaves more than 20 seconds off `make test` on my laptop, FWIW.

@Trott Trott force-pushed the Trott:redmond-2 branch from 5525647 to ae3d382 Jan 18, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 18, 2019

The pummel tests will now run nightly in node-daily-master, so removing blocked label here.

@Trott Trott removed the blocked label Jan 18, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 18, 2019

@Trott Trott added the author ready label Jan 18, 2019

Trott added a commit to Trott/io.js that referenced this pull request Jan 29, 2019

test: move heapdump tests to pummel
The heapdump tests take a lot more time to run than our other tests in
parallel. They are also a bit of an internal test that perhaps does not
need to be run on every commit on every platform. This change moves them
to the pummel directory where they will be run on a single platform once
a day in CI.

This shaves more than 20 seconds off `make test` on my laptop, FWIW.

PR-URL: nodejs#25181
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 29, 2019

Landed in 87f6804.

I'll take a try at creating a make test-all or something.

@Trott Trott closed this Jan 29, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 29, 2019

Oh, there already is a test-all... guess I ought to see what that does....

@Trott Trott deleted the Trott:redmond-2 branch Jan 29, 2019

targos added a commit that referenced this pull request Jan 29, 2019

test: move heapdump tests to pummel
The heapdump tests take a lot more time to run than our other tests in
parallel. They are also a bit of an internal test that perhaps does not
need to be run on every commit on every platform. This change moves them
to the pummel directory where they will be run on a single platform once
a day in CI.

This shaves more than 20 seconds off `make test` on my laptop, FWIW.

PR-URL: #25181
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

@targos targos referenced this pull request Jan 29, 2019

Merged

v11.9.0 proposal #25802

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 30, 2019

@Trott C++ coverage on https://coverage.nodejs.org/ dropped a full percent after this landed … do you think you could add pummel to the test suites we run for coverage?

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 30, 2019

@Trott C++ coverage on https://coverage.nodejs.org/ dropped a full percent after this landed … do you think you could add pummel to the test suites we run for coverage?

Yeah, very much something I want done. (internet and even benchmark tests too.) I'll take it up with @nodejs/build.

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 31, 2019

@Trott C++ coverage on https://coverage.nodejs.org/ dropped a full percent after this landed … do you think you could add pummel to the test suites we run for coverage?

Yeah, very much something I want done. (internet and even benchmark tests too.) I'll take it up with @nodejs/build.

OK, I added pummel and benchmark tests. Once #25799 lands, we can change it to run test-all-suites and it will always run all tests no matter where they are under the test directory.

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 31, 2019

@Trott

This comment has been minimized.

Copy link
Member Author

Trott commented Jan 31, 2019

Adding pummel and benchmark tests increased C++ coverage back to 91%. Oddly, JS coverage is down to 92.75% which seems wrong unless some big chunk of largely-untested code landed today, which I don't think happened.... Have to look into that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment