Speedup ci #1556
Merged
Speedup ci #1556
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
|
@Byron it shows that this PR makes ci.yml 5m faster |
Byron
approved these changes
Aug 25, 2024
There was a problem hiding this comment.
This is a very impressive outcome, thanks so much!
I am also surprised that cargo nextest manages to do the same faster, but maybe it's just the removal of clear-target (which exists there for reasons lost to time) which so much.
In any case, I prefer the output of cargo nextest and wasn't aware it can be used as cargo test stand-in - it's very neat, indeed!
It could be caching, but nextest is indeed faster if you have lots of tests and more than 1 cores, I checked the log it does run tests for some crates faster by running them in parallel by spawning one process for each tests. It might be more robust if your tests involve global variables or potential UBs, running in separate processes protects against that. |
It's not a full drop-in, it can't run doc-test right now, you'd need However I think you don't have the habit of using doc-test so often as most of the valuable tests is probably in |
|
P.S. nextest also integrates with llvm-cov, miri, criterion and cargo-mutants |
|
https://github.com/nextest-rs/reuse-build-partition-example/blob/main/.github/workflows/ci.yml And it can:
|
|
Thanks again for the heads-up! |
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Aug 26, 2024
This runs `cargo nextest ...` and `cargo test --doc` in separate steps in the `test-fast` job, so that the job fails when `cargo nextest ...` fails. Otherwise, with `pwsh` on Windows, test failures (other than doctests) are masked. Background: Since 89a0567 (GitoxideLabs#1556), doctests are run on all three major platforms, and not only on the full test job done on Ubuntu. But the way this was done relied on a script failing as soon as (or, at least, whenever) any command in the script failed. That works on Ubuntu and macOS, where a `bash` shell is used by default, with `-e` passed. But on Windows, GitHub Actions uses `pwsh` as the default shell. `pwsh` is not run in a way that causes it to stop at the first failing command. So, on Windows, when the `cargo nextest` command failed but the `cargo test --doc` command that followed it in the same script step passed, the step passed, thus allowing the job and workflow to pass. This was observed in GitoxideLabs#1429 after a rebase (see comments). Note that this is not related to the increased use of `nextest`. While that was also done in GitoxideLabs#1556, it did not affect the `test-fast` job where the bug was introduced, which was already using `nextest`. This fixes the problem by putting the two commands in separate steps. This is simpler than doing anything in PowerShell to make the script stop, such as using `&&` or attempting to produce `-e`-like behavior. Another option could be to use `bash` as the shell, which is a Git Bash environment suitable for running the tests. The reason I didn't do that is that I think it is valuable to see the results when the tests are run from a PowerShell environment. In particular, continuing to use PowerShell here should help in investigating GitoxideLabs#1359 (and shows that the claim I made is overly strong, since CI on Windows with `pwsh` not itself started from a Unix-style shell is not "Git Bash or a similar environment").
This was referenced Aug 26, 2024
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 6, 2024
The `test-fast` job has been intended to run doctests since 89a0567 (GitoxideLabs#1556). But because there are no doctests in the top-level project and neither `--workspace` nor its `--all` alias were passed, the effect has been: Compiling ... Finished `test` profile [unoptimized + debuginfo] target(s) in 28.55s Doc-tests gitoxide running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s (Where ... stands for details in various "Compiling lines. That output is copied from https://github.com/GitoxideLabs/gitoxide/actions/runs/11690609999/job/32555922512#step:9:70 though that log will eventually become available, and only the build time changes.) Note that zero tests are run and the status reports zero of each possible kind of result. There are (at least currently) no doctests in the top-level package, and `--workspace` is not implied. This adds `--workspace` to the command that runs the doctests, so it will collect and run doctests throughout the entire workspace. For now, this is not done on the corresponding command in the `unit-tests` rule in `justfile`; it may make sense to do that, but if it is done, then this step should probably be omitted on the `ubuntu-latest` run of `test-fast` since the CI job that runs `just unit-tests` is `test` which itself runs on `ubuntu-latest`. (The changes in GitoxideLabs#1556 were revised in GitoxideLabs#1559, but that only fixed a problem with reporting results from non-doctest tests. I had not noticed the problem of not running any doctests at that time.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Nov 6, 2024
The `test-fast` job has been intended to run doctests since 89a0567 (GitoxideLabs#1556). But because there are no doctests in the top-level project and neither `--workspace` nor its `--all` alias were passed, the effect has been: Compiling ... Finished `test` profile [unoptimized + debuginfo] target(s) in 28.55s Doc-tests gitoxide running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s (Where ... stands for details in various "Compiling lines. That output is copied from https://github.com/GitoxideLabs/gitoxide/actions/runs/11690609999/job/32555922512#step:9:70 though that log will eventually become unavailable, and only the build time changes.) Note that zero tests are run and the status reports zero of each possible kind of result. There are (at least currently) no doctests in the top-level package, and `--workspace` is not implied. This adds `--workspace` to the command that runs the doctests, so it will collect and run doctests throughout the entire workspace. For now, this is not done on the corresponding command in the `unit-tests` rule in `justfile`; it may make sense to do that, but if it is done, then this step should probably be omitted on the `ubuntu-latest` run of `test-fast` since the CI job that runs `just unit-tests` is `test` which itself runs on `ubuntu-latest`. (The changes in GitoxideLabs#1556 were revised in GitoxideLabs#1559, but that only fixed a problem with reporting results from non-doctest tests. I had not noticed the problem of not running any doctests at that time.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
May 6, 2025
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, 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 (and even if the user would omit `--workspace` in individual `cargo nextest` runs if 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 `just 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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
May 6, 2025
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 (and even if the user would omit `--workspace` in individual `cargo nextest` runs if 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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
May 6, 2025
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.
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.
target