Fix CI regression where most Windows failures passed CI #1559
Conversation
This file contains 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 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
|
Although the new CI job in #1429 after rebasing that PR's branch onto this has not finished yet as of this writing, the expected failures can already be observed, so I've marked this PR as ready for review. Update: The job there has failed, as it should, confirming that the change here is effective. |
|
I didn't think to mention this in the commit message (which I don't plan to amend unless that has to be done for some other reason), but another reason for doing it this way is that CI output is more readable with these two test commands in separate steps, as well as it being immediately obvious whether the |
Byron
approved these changes
Aug 26, 2024
This has also led to a refinement and explanation for #1359, detailed in #1359 (comment), which I now expect to be able to fix soon. |
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.)
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 runs
cargo nextest ...andcargo test --docin separate steps in thetest-fastjob, so that the job fails whencargo nextest ...fails. Otherwise, withpwshon Windows, test failures (other than doctests) are masked.Background: Since 89a0567 (#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
bashshell is used by default, with-epassed. But on Windows, GitHub Actions usespwshas the default shell.pwshis not run in a way that causes it to stop at the first failing command.So, on Windows, when the
cargo nextestcommand failed but thecargo test --doccommand that followed it in the same script step passed, the step passed, thus allowing the job and workflow to pass. This was observed in #1429 after a rebase (see comments).Note that this is not related to the increased use of
nextest. While that was also done in #1556, it did not affect thetest-fastjob where the bug was introduced, which was already usingnextest.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
bashas 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 #1359 (and shows that the claim I made is overly strong, since CI on Windows with
pwshnot itself started from a Unix-style shell is not "Git Bash or a similar environment").This is a draft because I'm going to rebase #1359 onto this or otherwise test that this really does catch failures. That should not take long.