Run doctests across the workspace on CI #1654
Conversation
Most `cargo` commands that support `--all` to affect the entire workspace rather than a single package have deprecated it in favor of `--workspace` and treat them as synonyms. The major exception (at least as relevant to this repository) is `cargo fmt`, which recognizes `--all` and not `--workspace`. This replaces `--all` with `--workspace` in `cargo check`, `cargo doc`, and `cargo nextest` commands in a CI workflow and the `justfile`. All these subcommands have deprecated `--all` in favor of `--workspace`, and document this in relevant `--help` output. (Other major subcommands that recommend `--workspace` over its deprecated `--all` alias include `cargo build` and `cargo test`, but there are currently no occurrences of such commands with `--all` in this repository.)
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.)
They are not Rust code (they are text with conflict markers and a shell command, respectively) and they are not intended as doctests, but the absence of anything on the opening line caused them to be taken as doctests, so `cargo test --workspace --doc` would fail with parsing errors. (Doctests for all crates have not always been run automatically on CI, so this was not caught when these documentation comments were introduced in GitoxideLabs#1585.)
Instead of stopping running doctests even in other packages. This change only applies to the `test-fast` job on CI. It does not affect any default configuration for doctests, nor the `justfile`. The reason for making this change before fixing the remaining doctest failure (which is in `gix-pack/src/index/mod.rs`) is to confirm that it takes effect on CI.
That macro is not exported, and adjusting the doctest so that it can run successfully (or even compile) might be tricky without a significant change (or adding dependencies). This fixes a failure when running doctests for the whole workspace (as CI now includes). This change is not "user-facing", as the macro is not part of the public interface of `gix_pack`. Its documentation comment doesn't appear https://docs.rs/gix-pack/0.54.0/gix_pack/index/index.html. Nonetheless, this adds a note to clarify the change, to avoid attributing it to the original code from which this was vendored.
There was a problem hiding this comment.
That's a great catch, thanks a lot!
For a moment I was wondering if it makes sense to spend more than a minute on compiling the entire workspace again just to run the very few doctests that there are, instead of selectively running only the known ones.
But then again, I thought it's probably not worth it to change that at the cost of having to remember to add a new crate to CI if doc-tests would be added to a crate that wasn't previously included.
Also I hope that one day cargo nextest will support doctests out of the box, maybe it could run faster then ever then.
This is a good point. Although the build times are not new, their impact was likely underestimated since it was not combined with the time to actually run the doctests.
I think we already have something like that in the |
|
Thinking again, I am now even more in fear that one day such a selection could backfire for the sake of saving CPU that won't be noticeable (except for cents on the bill of GitHub). The unit-tests are explicitly run with different features because running all permutations of all features isn't always feasible or possible, so I think it's a different intent. I certainly wouldn't object to such a change, but I'd be much more interested in a way to either solve the 15 failing tests, or have a safeguard. And even that, the safeguard, is a nice to have and only if it's simple enough, all in all. |
Another option, if it's not important for the doctests to run on all platforms, could be to change Line 155 in c5955fc to pass
That makes sense, and yes, that does not apply to the doctest runs.
For now, I've opened #1657 for that. |
This changes the deprecated `--all` option for `cargo nextest` to the recommended (and equivelent) `--workspace` option, in quoted default argument values for recipes in `justfile`. Other occurrences of `--all` to `cargo nextest`, and other commands for which `--all` is a deprecated alias of `--workspace`, were changed to `--workspace` in 14d472d (GitoxideLabs#1654), but I had missed these.
The attempt to run doctests, added to the CI
test-fastjobs in 89a0567 (#1556), actually runs zero doctests, because there are none in the top-level project and neither--workspacenor its deprecated--allalias is passed.This was the case both before and after #1559 (which revised #1556 in other ways). I am actually not sure doctests across all creates had ever been run automatically. The doctest command in the
justfile, which can be run manually but is also in the CItestjob, is the same as the the command added totest-fastin 89a0567. Furthermore, there are two situations with failing doctests, one of which is long-standing:gix-merge), or in one case a suggested shell command. Since the opening```was not followed by text to indicate the language, however, they were taken to be Rust doctests.zip!was vendored fromitertools, including the originalzip!macro's doctest. This fails because it tries touseit fromitertools, and the failure is not--as far as I can tell--straightforwardly repairable by making the doctest use the vendored macro, due to the rules about macro visibility, the way scoping interacts with doctests, and the status of the vendoredzip!as something that should not be part of the interface ofgix-pack. That doctest could of course be converted into a regular unit test with#[test]. But the goal doesn't seem to be to actually test that, so much as to keep its origin initertoolsclear.This PR adds
--workspaceto the command to run doctests in all projects/crates, as well as--no-fail-fastso the status of all doctests will be seen in any fail. After observing those failures, I fixed them, though forzip!the "fix" was toignorethe doctest. It also replaces--allwith--workspaceforcargosubcommands where it is a deprecated alias of--workspace, which is not specifically related to doctests. There is a bit more detailed information in the commit messages.