-
Notifications
You must be signed in to change notification settings - Fork 58
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
Use nextest in CI #243
Use nextest in CI #243
Conversation
I'm all for this in theory, but what I'm seeing on CI is that the current iteration increases rather than decreases test time. Before: mac=17m22s; arm=14m31s; linux=22m55s After (this PR): mac=19m29s; arm=15m46s; linux=23m27s Am I missing something? If not, can we iterate on this and wait until it's demonstrably an improvement before making the switch? |
I'm guessing the difference is attributable to the new doc test job. |
I don't see the linux doc test job in the 'after' log, though. Do you know what's going on there? |
Okay, so perf is not expected to improve yet. Got it (I think). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that performance total test time is expected to worsen with this PR (as noted in comments), and it is an incremental step toward improvement, I'm fine with merging it.
Doc tests have not been added as a separate job, but as an additional step to the existing jobs. They are running and show up in the logs. The concept for this organization that we could conceivably have a doc test that behaves differently according to the platform on which it is run. However, reducing the amount of tests that need to run on every platform would help. Would you want to run doc tests a separate job on a single platform? |
dc19b4b
to
459d179
Compare
Well, if you want to compare apple to apples, you want to compare specifically the test step (run under cargo test before and under cargo nextest after). The difference is launching doctests, but before this PR we didn't have any of those. Looking at the same CircleCI log you do. Note linux is unchanged to nextest (in the links you shared: I fixed that later in the PR). Also note linux-release runs with a pretty stringent test-threads=1 in both configs. Before: mac=15m6s, arm64=12m28s. So I suspect what we're measuring is the overhead of installation of the tool (nextest) and that of running doctests. Since the cache has been made per-arch in #211, I am hopeful we can save up significantly on those steps, but have not addressed this in the current PR. |
OK, so I have:
End-to-end: Test-only: Given the threading limitations of the WA runtime, I can't see how the "linux" numbers would be better with nextest. The gap is pretty big, but unfortunately I'm not allowed to re-run that specific job to see if it's within the measure error for CircleCI. |
708eac2
to
aa61e7a
Compare
Sorry, missed this. Looks good to me, although I don't believe the Wasm build uses nextest given that Wasm is incompatible with testing in general. It looks like the Linux job instead runs nextest on the default target, which is great. Also a big fan of dropping the linux-release job's test thread limitation. My guess is the above caching issues will at least bring the other targets back to par, and then #228 will solve slow CI once and for all! |
This introduces the use of nextest in CI. This is expected to bring performance improvements for some jobs, due to a better parallelization model - except for the
linux-release
job, which is completely sequential anyway. (Edit: the linux-release job is made non-sequential in this PR. The linux job is a wasm job)Since nextest does not support documentation tests, this introduces a perfunctory doctest ahead of the CI change, and runs a separate
cargo test --doc
task.The goal of this CI is to introduce feature parity with the existing, and it does not aim to change behavior (yet).