Expanding tests run in coverage job #1676
Comments
Trott
changed the title
|
Guess I'll leave this open until we can change it to |
|
Hmmm...my change somehow added code in the |
|
Oh, I see, I need to add it as an |
|
(In the meantime, I'll remove the benchmark tests from the coverage job.) |
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?) |
|
@Trott key thanks, if its not significantly longer that's good. I doubt there is "magic". |
|
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. |
The change in nodejs/node#25799 will fix that. Parallelism will only be done by |
|
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? |
|
@mhdawson I'd guess it will fix it because the coverage job will run only one make target ( |
|
By the way, the only thing keeping nodejs/node#25799 from landing at this point is that it needs one more approval. @refack? @mhdawson? |
|
Hmm, making we think a bit more about the coverage sanity job. We want it to run as part of the regular PR validation so that tests don't get introduced that will cause the main coverage job to fail. But if we've chosen not to run tests already it should probably not run them either, unless running a single instance (ubuntu) is ok for every PR run. |
|
The goal is to avoid things going in that then later require cleanup by people watching the main coverage job. |
Yes. It's nodejs/node#22006 and nodejs/node#24966. Patched for regular testing with nodejs/node#23733 |
|
This patch is currently being applied in respect to 24966 but I guess it does not completely address the issue? #1671 |
If we get to a point where the coverage jobs works with all tests passing, we could fail it on a test fail (i.e. nodejs/node#25432) and that way we get daily sanity (even trigger it as part of the general daily master sanity)
It solves the one one aspect of a race while building. It does not solve the fragility of parallel |
|
OK, switched the node-test-commit-linux-coverage-daily job up to use test-all-suites and running it with the publish-to-coverage.nodejs.org box unchecked: https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-daily/24/ |
Refs: nodejs/build#1676 (comment) PR-URL: nodejs#25841 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
Shoot, it's still running suites in parallel, resulting in failures/missed coverage. Making this change now: Before: After: Running to see if it gets correct stats that way and also to see how long it takes to run: |
|
Well, that helped a little but not much. Putting the job back to this until we can figure out why it doesn't work with And kicking off a job to restore coverage stats: https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-daily/27/ |
|
OK, so even though I was running make[2]: Leaving directory `/home/iojs/build/workspace/node-test-commit-linux-coverage-daily/nodes/benchmark/test/node-api/test_uv_loop/build'
/usr/bin/python2.7 tools/test.py -j 8 --mode=release js-native-api |
|
The problem would seem to be that |
|
Changed it back to Will now change things back to |
|
Coverage stats with that PR look reasonable: 06:30:40 Javascript coverage %: 95.47%
06:30:40 C++ coverage %: 90.3% |
|
(And it took 20 minutes to run, so that seems reasonable too.) |
|
nodejs/node#25892 landed. Updating job again: Running coverage job: https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-daily/29/ |
|
Hmmm... Trying again: https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-daily/30/ |
|
@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/ |
|
That run looks good. Optimistically closing this. |
Refs: nodejs/build#1676 (comment) PR-URL: #25841 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
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 |
|
Not sure we don't see the same failure in the main coverage run. IN any case I guess we'll investigate that separately. |
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:
...to this:
Once nodejs/node#25799 lands, we can replace the list of suites with
test-all-suitesand all tests under thetestdirectory will run no matter where they are.The text was updated successfully, but these errors were encountered: