Make it easier to compose a subset of nextest in code (as opposed to through CLI)

[TobiasvdVen]

See feature request for details: #2619

[codecov]

Codecov Report

❌ Patch coverage is 42.75362% with 79 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.37%. Comparing base (e0336da) to head (6e50ae2).
⚠️ Report is 37 commits behind head on main.

Files with missing lines	Patch %	Lines
nextest-runner/src/errors.rs	0.00%	36 Missing ⚠️
cargo-nextest/src/errors.rs	16.21%	31 Missing ⚠️
nextest-runner/src/cargo_cli.rs	81.53%	12 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@ ## main #2622 +/- ## ========================================== - Coverage 78.40% 78.37% -0.03%  ========================================== Files 107 107 Lines 24861 24969 +108 ========================================== + Hits 19492 19570 +78  - Misses 5369 5399 +30 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TobiasvdVen]

I see the CargoOptions::new() function is causing a drop in coverage, do you have a recommended approach here? I could add a test of some form, but it would be rather trivial for what is merely a constructor.

Not sure why the CI/ Lint (ubuntu-latest) step is failing, it seems to also be the case on other PRs, so perhaps not related to the changes?

[sunshowers]

Ahh, unfortunately this isn't a correct approach, because the public API for cargo-nextest is defined as just the CLI. We do patch releases for nextest version upgrades based on that, and making functions from cargo-nextest public would necessitate a breaking version upgrade each time the APIs changed.

I would be fine moving bits into nextest-runner as needed.

The big new function is also not a good idea. I don't know why that needs to be a builder -- can the fields just be pub?

[TobiasvdVen]

Ah, I see, I'll move these required pieces out of cargo-nextest and into nextest-runner instead.

Regarding the new() function, I will remove it and make the fields pub, as suggested. My personal intuition was that a new() function would be a less invasive change. And I was too lazy to write a whole builder pattern without relying on a proc-macro :)

[TobiasvdVen]

Let me know if this update matches your expectations. A few notes:

I started by trying to pull OutputContext to nextest-runner, but after that lead to a few more things needing to move, it seemed the better route might be to remove the dependency on OutputContext in CargoOptions::compute_binary_list and acquire_graph_data. I did this under the presumption that the OutputContext was only used to set the --color argument, and these functions seemed to capture the stdout of the command, thus it would be irrelevant. It's possible I missed something and that the OutputContext does need to be included.

The next largest change was the need to introduce new error types for compute_binary_list and acquire_graph_data to return: CreateBinaryListError (modeled after CreateTestListError) and CargoMetadataError, respectively. The latter includes a TargetTripleError as a kind, which is why the error handling in cargo-nextest/src/errors.rs now has a function display_target_triple_error_to_stderr. Maybe there's a better way to avoid duplicating the TargetTripleError handling that I didn't see.

It can be hard to tell in the diff of moved code, so I'll add some comments where the OutputContext was removed.

[TobiasvdVen]

Removal of OutputContext, was passed into CargoCli::new, where its only usage was in CargoCli::to_expression to add the --color argument.

[sunshowers]

Where is the --color argument passed in now?

[TobiasvdVen]

Well, it isn't 😄

The way I went about this is noticing that the --color argument seemed unnecessary, as all usages of to_expression (the only place --color was added) are in places that don't display their output - it is captured and only used internally (specifically in compute_binary_list and acquire_graph_data).

There is this comment in CargoCli, which may or may not be relevant: // --color is handled by runner.

I'm not entirely sure what it's intended to communicate, but I think it's explaining why there is no color field on the CargoCli struct. Maybe if we're no longer "internally" adding the --color argument it should become a field of CargoCli, so that users can set it? I'm not sure if CargoCli is intended for use beyond what it is currently used for, though, in which case it could be superfluous.

My concern would be that I missed some way in which it was relevant. Presumably it was there for a reason, at least at one point, but I was unable to identify one.

[sunshowers]

Well -- when the user runs cargo nextest run, we run cargo test --no-run to build all the tests -- we want to forward the --color argument to that underlying cargo test execution. Totally fine with it being factored out to be easier to do, but we need to maintain this property.

[TobiasvdVen]

Ah, I went and had another look, and I misinterpreted things before.

I thought the output of cargo test --no-run (in compute_binary_list) was being captured, in the sense that it was only used for parsing to a list of binaries. But its output is also sent to stdout, so --color is relevant.

My bad, I'll have to take a different approach.

[sunshowers]

Thanks -- stdout (machine-readable output) is captured, but stderr (human-readable output) is passed through.

[TobiasvdVen]

The original printing of TargetTripleError to stderr.

[sunshowers]

Thanks, hoping to look at this soon (currently dealing with a hard deadline at my workplace)

[sunshowers]

Looks pretty reasonable! Just a few comments.

[sunshowers]

Where is the --color argument passed in now?