search

GitoxideLabs / gitoxide

An idiomatic, lean, fast & safe pure Rust implementation of Git
Apache License 2.0
8.91k stars 303 forks source link

Fix CI regression where most Windows failures passed CI #1559

Closed EliahKagan closed 1 month ago

EliahKagan commented 1 month ago

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 (#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 #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 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 #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 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.

EliahKagan commented 1 month ago

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.

EliahKagan commented 1 month ago

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 nextest or doctest run is what failed.

EliahKagan commented 1 month ago

Thanks so much, I am very glad we caught this!

This has also led to a refinement and explanation for #1359, detailed in https://github.com/Byron/gitoxide/issues/1359#issuecomment-2316614616, which I now expect to be able to fix soon.