Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatibility with cargo-nextest options #271

Open
TriplEight opened this issue May 2, 2023 · 16 comments
Open

Compatibility with cargo-nextest options #271

TriplEight opened this issue May 2, 2023 · 16 comments
Labels
A-nextest Area: nextest integration https://github.com/nextest-rs/nextest

Comments

@TriplEight
Copy link

#270 (comment)

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
Copy link
Author

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

@taiki-e
Copy link
Owner

taiki-e commented May 2, 2023

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 (nextest-rs/nextest#16), not #2.

@taiki-e taiki-e added the A-nextest Area: nextest integration https://github.com/nextest-rs/nextest label May 2, 2023
@TriplEight
Copy link
Author

TriplEight commented May 3, 2023

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
Copy link
Owner

taiki-e commented May 3, 2023

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
Copy link
Author

TriplEight commented May 3, 2023

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 #265 the author uses -P ci option (which I assume would pass, but not --profile)

@taiki-e
Copy link
Owner

taiki-e commented May 3, 2023

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
Copy link
Owner

taiki-e commented May 3, 2023

-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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

taiki-e commented May 3, 2023

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
Copy link
Author

TriplEight commented May 3, 2023

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
Copy link
Owner

taiki-e commented May 3, 2023

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 (474a8a2), 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
Copy link
Author

Another one: (context: #265 (comment) and #265 (comment))
--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 #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
Copy link
Author

TriplEight commented May 3, 2023

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
Copy link
Owner

taiki-e commented May 3, 2023

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 #266 (comment) (target/llvm-cov-target/target) are different.

@taiki-e
Copy link
Owner

taiki-e commented May 3, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-nextest Area: nextest integration https://github.com/nextest-rs/nextest
Projects
None yet
Development

No branches or pull requests

2 participants