Repository navigation
Expanding tests run in coverage job #1676
Description
I changed the configuration for https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-daily/ to also run pummel and benchmark jobs.
Under "Execute Shell", I changed the last line from this:
NODE_TEST_DIR=${HOME}/node-tmp PYTHON=python COVTESTS="test-ci test-internet" make coverage -j $(getconf _NPROCESSORS_ONLN)...to this:
NODE_TEST_DIR=${HOME}/node-tmp PYTHON=python COVTESTS="test-ci test-internet test-pummel test-benchmark" make coverage -j $(getconf _NPROCESSORS_ONLN)Once nodejs/node#25799 lands, we can replace the list of suites with test-all-suites and all tests under the test directory will run no matter where they are.
Activity
[-]Add pummel and benchmark tests to coverage job[/-][+]Expanding tests run in coverage job[/+]Trott commentedon Jan 31, 2019
Guess I'll leave this open until we can change it to
test-all-suites.Trott commentedon Jan 31, 2019
Hmmm...my change somehow added code in the
benchmarkdirectory to the measurements, which is definitely not what I wanted to do. Any idea how to exclude it or why it got included? @bcoeTrott commentedon Jan 31, 2019
Oh, I see, I need to add it as an
--excludein the call toc8in the Makefile.Trott commentedon Jan 31, 2019
(In the meantime, I'll remove the benchmark tests from the coverage job.)
mhdawson commentedon Jan 31, 2019
@Trott, how much run time does adding those 2 make?
Working on #1669 and just want to understand how that will affect the run time there as well. We will also have to sync that job.
Trott commentedon Jan 31, 2019
The whole job, start-to-finish, takes around 20 minutes give-or-take 5 minutes. My run took 19 minutes and the previous run (without these additions) was 21 minutes, so whatever additional time there is, it is negligible. On the other hand, that is odd because I believe two pummel tests timed out, so I would expect that to add 4 minutes right there. (Maybe there's some Jenkins magic there? Does this job run things in parallel even though they're not parallel-safe somehow?)
mhdawson commentedon Jan 31, 2019
@Trott key thanks, if its not significantly longer that's good.
I doubt there is "magic".
mhdawson commentedon Feb 1, 2019
Something strange is going on. I'm seeing failures in the sanity coverage job when trying to add pummel. Looks like the different types are running in parallel which seems wrong so I'm not sure adding these until we figure that out is a good idea as it seems to be causing tests to fail that previously passed because they are running in parallel.
The coverage job does not intentionally try to run anything in parallel that is not normally run that way.
refack commentedon Feb 1, 2019
The change in nodejs/node#25799 will fix that. Parallelism will only be done by
test.pynot bymake.(We should make sure all
make test*run with-janyway)mhdawson commentedon Feb 1, 2019
I don't see how 25799 is going to fix that for the coverage jobs. @refack is it a known problem that the suites are running in parallel?
Trott commentedon Feb 1, 2019
@mhdawson I'd guess it will fix it because the coverage job will run only one make target (
test-all-suites) whereas currently, make runs multiple targets (test-pummelandtest-internetand so on) and ends up running multipletest.pyinstances.Trott commentedon Feb 1, 2019
By the way, the only thing keeping nodejs/node#25799 from landing at this point is that it needs one more approval. @refack? @mhdawson?
11 remaining items
Trott commentedon Feb 2, 2019
The problem would seem to be that
test-all-suiteshastest-js-native-apias a pre-requisite. We really just needtest-buildI think. Testing that locally with amake clean && make test-all-suites. If it works out, I'll open a PR, run a modified coverage job against it as part of the test, and hopefully that will finally get this done.Trott commentedon Feb 2, 2019
nodejs/node#25892
Trott commentedon Feb 2, 2019
Changed it back to
test-all-suitesand running a job against nodejs/node#25892 rather than master and with the publish checkbox unchecked: https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-daily/28/Will now change things back to
test-ci test-internet test-pummel test-benchmark.Trott commentedon Feb 2, 2019
Coverage stats with that PR look reasonable:
Trott commentedon Feb 2, 2019
(And it took 20 minutes to run, so that seems reasonable too.)
Trott commentedon Feb 3, 2019
nodejs/node#25892 landed. Updating job again:
Running coverage job: https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-daily/29/
Trott commentedon Feb 3, 2019
Hmmm...
gittiming out in that run it seems...Trying again: https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-daily/30/
Trott commentedon Feb 3, 2019
@refack reported that a Jenkins workspace needed attention but that things should be fine now. So, trying again: https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-daily/31/
Trott commentedon Feb 3, 2019
That run looks good. Optimistically closing this.
tools: exclude benchmark code from coverage report
mhdawson commentedon Feb 4, 2019
The coverage test job where we are trying to only fail if something other than test failing seems to be having problems with the updates.
In particular, this seems to be timing out after 5 mins: test-dnsBuild timed out. See https://ci.nodejs.org/job/node-test-commit-coverage/nodes=ubuntu1604_sharedlibs_x64/59/console
mhdawson commentedon Feb 4, 2019
Not sure we don't see the same failure in the main coverage run. IN any case I guess we'll investigate that separately.