Regroup CI doctests and adjust unit-tests recipe
#1996
Merged
+40
−40
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This seeks to make improvements in four overlapping areas: - The CI `test-fast` job for `windows-latest` had been taking the longest, and integrating PRs would be more efficient if it could be sped up. If it didn't have to build and run doctests, then it would run markedly faster. `test-fast` runs doctests because... - The `unit-tests` recipe in the `justfile`, which is one of the recipes the "full" CI `test` job runs via the `ci-test` recipe, runs a number of `nextest` commands on individual crates (with `-p`, except for testing the top-level `gitoxide` crate, and not with `--workspace`). It also ran doctests, but only on the `gitoxide` top-level crate. But the `gitoxide` crate currently has no doctests, while some `gix-*` crates do. - On CI, we usually prefer `--no-fail-fast`. But it is not always used, because the commands in the `unit-tests` justfile recipe do not use it. `--no-fail-fast` is not always preferred when running tests locally, but... - Both locally and on CI, in most cases that a test fails in a commmand in the `unit-tests` justfile recipe, the effect is that tests have run and results have been reported for multiple `nextest` commands, yet not all the tests specified in the most recent `nextest` command to run. Thus, omitting `--no-fail-fast` may not have the most intuitive effect in `just unit-tests`, even when run locally (even if the user would omit `--no-fail-fast` in individual `cargo nextest` runs carried out manually). This commit makes the following changes: 1. Add `--no-fail-fast` to each of the commands in the `unit-tests` recipe in the `justfile`: the numerous `cargo nextest` commands, as well as the `cargo test` command used to run doctests. 2. Add `--workspace` to the `cargo test` command used to run doctests in the `unit-tests` recipe in the `justfile`, and move it to the end of the recipe. 3. Not to be confused with that `cargo test` command, move the other `cargo test` command used to run doctests in the `ci.yml` workflow (which alredy passed `--workspace`, as its purpose was to run all doctests in all crates) from the `tests-fast` job definition into the `test-32bit-windows-size` job, and rename that latter job `test-32bit-windows-size-doc` accordingly. The rationale for (3) may not be obvious. The idea is: - Running the doctests on only one Unix-like system should be enough, so long as they are run for all crates in the workspace. So the change in the `unit-tests` recipe in the `justfile` makes it so the CI `test` job (which includes a `unit-tests` run) covers doctests sufficiently, *except* for Windows. - Although we should probably keep running doctests regularly on Windows, removing it from `test-fast`, including on Windows, is the simplest way to make the Windows `test-fast` job run faster. (It also makes the job definition clearer, since some of the other steps relate to each other more closely than they do to the step that ran the doctests.) - It should be sufficient to run the doctests in any Windows environment. And it is best to avoid adding a new Windows job just for this, since various other Windows jobs might be added sometime soon (such as for ARM64, native Windows containers, the Git for Windows SDK, MinGit, BusyBox MinGit, and possibly others; some of these may be possible to combine, but likely a few more Windows jobs may be introduced for these, so avoiding adding extra Windows jobs now may make it easier to avoid having too many Windows jobs, in terms of queuing, GHA cache usage, energy usage, and other resources). So if this can be added to another Windows job without causing problems, that is preferable. - The Windows job that took the least amount of time, usually by several minutes, was the `test-32bit-windows-size` job. It is hope that this keeps the benefits of GitoxideLabs#1556, GitoxideLabs#1559, and GitoxideLabs#1654, while improving CI testing performance most of the time.
|
It's hard to predict what effect caching dependencies (with |
|
Thanks so much! The CI run took 17 minutes, which is less than the 30 minutes I thought it would take by now. Overall wallclock time is 1h27m, which seems close to what I remember though. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This distributes work across CI jobs more evenly. In particular, it speeds up
test-fastonwindows-latest(which had become the slowest of all CI jobs, including other Windows jobs). To do so, it leverages the idea that the doctests are important enough that we should probably continue automatically running them on Windows, but not so important that it particularly matters which of reasonably normal Windows environments and targets they run for.Those changes entail modifying the
justfile, so I've included the further overlapping change of adding--no-fail-fastto the commands in theunit-testsrecipe there. This benefits CI, and I think does not hurt local runs either.This also makes it so that the doctest-running command in
just unit-testsruns more than zero doctests, which I think is the more intuitive effect. (That is, it runs all doctests in the workspace, rather than just all the doctests in thegitoxidecrate, of which there currently aren't any.)Substantial further details, including about what this is trying to improve, and rationale, are in the commit message.
The diff, as shown on GitHub, is somewhat confusing in
justfile. Except for one line that is moved and changed, all changed lines are actually changed only by having--no-fail-fastadded to the end.