taiki-e / cargo-llvm-cov

Cargo subcommand to easily use LLVM source-based code coverage (-C instrument-coverage).
Apache License 2.0
898 stars 57 forks source link

Compatibility with `cargo-nextest` options #271

Open TriplEight opened 1 year ago

TriplEight commented 1 year ago

https://github.com/taiki-e/cargo-llvm-cov/pull/270#issuecomment-1528794104

It would be great if someone could exhaustively test for compatibility with cargo-nextest options, document and issue warnings about options that don't work, but that would require a lot of work.

It's worth a separate tracking issue. I think the first step would be to populate cargo llvm-cov nextest --help with the supported ones.

TriplEight commented 1 year ago

And the first input would be error: doctest is not supported for nextest while running cargo llvm-cov nextest ... --doctests. Cc https://github.com/taiki-e/cargo-llvm-cov/issues/2

taiki-e commented 1 year ago

Thanks for starting this!

And the first input would be error: doctest is not supported for nextest while running cargo llvm-cov nextest ... --doctests. Cc #2

Lack of support for doctest is a problem/limitation on nextest side (https://github.com/nextest-rs/nextest/issues/16), not #2.

TriplEight commented 1 year ago

As far as I understood from https://github.com/taiki-e/cargo-llvm-cov/pull/269/files#diff-b2812f19576dd53d0c35b107a322f58a00fce6977f4b5976e1961853982af3ccR357-R362 it's not handling the case when cargo llvm-cov nextest is used with flags --config-file <nextest config path> --profile <profile from that config>. This is rather specific case, when nextest config path sits in a non-default location (default: workspace-root/.config/nextest.toml). But when nextest config sits in the default location, --config-file is not required, but --profile will look for a nextest config's profile. Nextest only handles cargo's --profile with --cargo-profile and nextest --profile looks for the config. Maybe it makes sense to introduce a --nextest-profile flag and pass it through.

taiki-e commented 1 year ago

https://github.com/taiki-e/cargo-llvm-cov/pull/269/files#diff-b2812f19576dd53d0c35b107a322f58a00fce6977f4b5976e1961853982af3ccR357-R362

Flags not caught in those lines are also passed to nextest; what is specified in the nextest profile does not affect the behavior of the cargo-llvm-cov itself, so it should already work fine.

TriplEight commented 1 year ago

It wouldn't work in the case if a non-default profile from nextest is used in i.e. CI. That way, without catching --profile nextest will go with a default profile. And that might be a local thing that ignores a lot of heavy/int tests/benches etc or uses some special rules of repeating the flaky tests.

Example: in https://github.com/taiki-e/cargo-llvm-cov/issues/265 the author uses -P ci option (which I assume would pass, but not --profile)

taiki-e commented 1 year ago

Oh, do nextest seem to change the directory to which build artifacts are written? If so, surely the current cargo-llvm-cov cannot handle that.

taiki-e commented 1 year ago

-P ci option (which I assume would pass, but not --profile

Hmm, assuming nextest handles both the same way, if -P ci works, then --profile ci should also work. The current cargo-llvm-cov handles both the same way.

TriplEight commented 1 year ago

you're right, tested both:

root@526ea96919f8:/builds/# cargo llvm-cov --verbose --no-report --include-build-script nextest --color always --no-fail-fast --locked --workspace --profile ci                  
     Running `/usr/local/rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/cargo nextest run --manifest-path /builds/Cargo.toml --target-dir /builds/target/llvm-cov-target --color always --no-fail-fast --locked --workspace --profile ci`
error: profile `ci not found (known profiles: default, default-miri)`
error: process didn't exit successfully: `/usr/local/rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/cargo nextest run --manifest-path /builds/Cargo.toml --target-dir /builds/target/llvm-cov-target --color always --no-fail-fast --locked --workspace --profile ci` (exit status: 96)
root@526ea96919f8:/builds/# cargo llvm-cov --verbose --no-report --include-build-script nextest --color always --no-fail-fast --locked --workspace -P ci
     Running `/usr/local/rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/cargo nextest run --manifest-path /builds/Cargo.toml --target-dir /builds/target/llvm-cov-target --color always --no-fail-fast --locked --workspace -P ci`
error: profile `ci not found (known profiles: default, default-miri)`
error: process didn't exit successfully: `/usr/local/rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/cargo nextest run --manifest-path /builds/Cargo.toml --target-dir /builds/target/llvm-cov-target --color always --no-fail-fast --locked --workspace -P ci` (exit status: 96)

And both are passed to nextest.

TriplEight commented 1 year ago

Oh, do nextest seem to change the directory to which build artifacts are written? If so, surely the current cargo-llvm-cov cannot handle that.

no, by default it writes to target/

taiki-e commented 1 year ago

by default it writes to target/

If the (non-default) Nextest profile (not the Cargo profile) is set, will it still be written to target/? or subdirectory of target/? If the latter, the current cargo-llvm-cov cannot handle that.

TriplEight commented 1 year ago

As per https://nexte.st/book/configuration.html:

The default configuration shipped with cargo-nextest is:
[store]
# The directory under the workspace root at which nextest-related files are
# written. Profile-specific storage is currently written to dir/<profile-name>.
dir = "target/nextest"
taiki-e commented 1 year ago

It does not seem to be the directory to which cargo build artifacts are written when the --target-dir option is passed.

cargo llvm-cov nextest --text --profile ci runs without problems (https://github.com/taiki-e/cargo-llvm-cov/commit/474a8a26ee376a973d15d9c0241491315d95ad5f), and build artifacts are written to target/llvm-cov-target that cargo-llvm-cov specified.

$ cd target
$ fd | rg 'nextest|test-'                   
llvm-cov-target/debug/deps/test-f1d69101d739b095
llvm-cov-target/debug/deps/test-f1d69101d739b095.d
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.0.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.1.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.10.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.11.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.12.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.13.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.14.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.15.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.2.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.3.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.4.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.5.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.6.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.7.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.8.rcgu.o
llvm-cov-target/debug/deps/test-f1d69101d739b095.test.71bd97801f479f48-cgu.9.rcgu.o
nextest/
nextest/ci/
TriplEight commented 1 year ago

Another one: (context: https://github.com/taiki-e/cargo-llvm-cov/issues/265#issuecomment-1527508505 and https://github.com/taiki-e/cargo-llvm-cov/issues/265#issuecomment-1532042501) --partition https://nexte.st/book/partitioning.html. The idea is that the tests are divided (randomly with count and deterministically with hash) to run on the separate machines. This can be used for different platform-specific tests or just to scale horizontally. The issue is that this command succeeds on each of the partitions, but certainly gives only the partial coverage.

There needs to be a way to collect only the llvm-cov-related instrumentation artifacts to merge them together in the second job and generate the coverage report. nextest's --archive-file does not entirely fit for the job (in https://github.com/taiki-e/cargo-llvm-cov/issues/265 author receives the artifact from the previous job and runs tests and coverage from it.

  1. It doesn't assume that llvm-cov artifacts are getting into this archive because nextest generates it possibly without running the tests
  2. --archive-file is designed to run the tests from it, so it should exist before the tests are executed, and nextest does not support a lot of the test-running options for this flag too.

This is both a bug and a feature request, it would be nice to have an archive flag and the ability to generate a report from it for llvm-cov too.

TriplEight commented 1 year ago

It does not seem to be the directory to which cargo build artifacts are written when the --target-dir option is passed.

Altered your test a bit to see what's in target/ and added that default nextest config [store], but it seems fine yeah. Does it mean that llvm-cov puts all the relevant artifacts to target/llvm-cov-target?

taiki-e commented 1 year ago

Does it mean that llvm-cov puts all the relevant artifacts to target/llvm-cov-target?

Changing rustflags triggers rebuild of the entire dependencies, so the default is target/llvm-cov-target not target. However, show-env (target) and https://github.com/taiki-e/cargo-llvm-cov/pull/266#issuecomment-1527785686 (target/llvm-cov-target/target) are different.

taiki-e commented 1 year ago

As for --partition, could you open another issue? And if possible, please provide a reproducible example that we can confirm cargo nextest works and cargo llvm-cov nextest doesn't. It helps us understand what the actual problem is that you are encountering.