cargo insta test cli: add --disable-nextest-doctest option

[ilyagr]

This adds an option to cargo insta test that prevents running doctests with cargo test when Nextest is the test runner, even if the given targets would ortherwise include doctests.

I usually use cargo insta test with --test-runner nextest --workspace options. This always runs cargo test --doc after nextest is done and this takes a bit of time. This is an annoyance for me. I think the project I work on might have a doctest or two, but they are not very relevant, so I'm happy to rely on CI to catch any doctest errors.

I only have a vague understanding of cargo test options, but I have not found an existing set of test specifiers that's equivalent to --workspace --no-doctests-pretty-please.

---

This is a bit of a quick hack, but I'll be using it, and I think others might appreciate it. If you have any thoughts about a better approach, please let me know.

[mitsuhiko]

Kinda wish we had a general --no-doc option but not sure how to make this work for regular cargo test. I will dive into it.

[max-sixty]

One plausible alternative to this is running --tests --examples — would that suffice?

[ilyagr]

One plausible alternative to this is running --tests --examples — would that suffice?

These options, for me at least, are a bit confusing, which contributes to the problem in my eyes (even though insta is probably not responsible for the confusion). Will --tests run all tests, just the library tests, or both the library tests and the tests in the tests/ dir? It's also a bit surprising for me to hear that --tests does not run doctests; that makes some sense but wouldn't be my first guess.

But also, thank you, I will try it out, and it is possible that this is also a solution.

(Personally, I'm still using this PR as awkward as it is, though I see I need to rebase and fix some merge conflicts Update: done, also added another commit that I find convenient)

[max-sixty]

Yes, tbc insta is just passing those through to cargo, so while they might be a bit confusing, they're only confusing once!

I would probably vote to either mimic nextest and not attempt to run doctests, or suggest passing the two options (assuming those work). Augmenting nextest and then adding an option to disable augmenting seems over-done on options...

[ilyagr]

If we were considering breaking changes, another option would be to not run doctests by default with nextest, but have an option to do so, and perhaps a warning message if cargo test with the same option would have run doctests.

[ilyagr]

I don't understand all the details, but it seems that Rust is significantly reworking how doctests are run. The main goal seems to be to link them into one binary; I don't know how that affects nextest and insta --test-runner nextest.

https://rust-lang.github.io/rust-project-goals/2024h2/merged-doctests.html
rust-lang/rust#126245

[max-sixty]

If we were considering breaking changes, another option would be to not run doctests by default with nextest, but have an option to do so, and perhaps a warning message if cargo test with the same option would have run doctests.

On reflection, I would support this — I think we could start this now; standard approach:

• Merge something like this PR, but raise a warning if --disable-doctest / --enable-doctest isn't passed when --test-runner=nextest, saying that default behavior will change to not running doctests
• Later, change the default to not running doctests, still raise a warning if nothing is passed saying behavior recently changed
• Later, turn off the warning
• Eventually deprecate the option, never supplement nextest test runner with additional doctests

This would add an option but only temporarily, such that we'd eventually be in a place that we're just delegating test running to a test runner, which is a simple & legible place to be...

[mitsuhiko]

That is probably the right way. However I do still wonder if nextest will ever fix the doctest situation: nextest-rs/nextest#16

[max-sixty]

FWIW given #460, I think if we don't get to the full deprecation cycle of #438 (comment), I would support just disabling the supplementary doctest run.

I don't think there's a great reason for us to augment nextest's behavior; if they don't run doctests then that's a decision they make...

[max-sixty]

@ilyagr I notice you updated this. I would support merging something that either started a full deprecation cycle or just disabled the docs run:

FWIW given #460, I think if we don't get to the full deprecation cycle of #438 (comment), I would support just disabling the supplementary doctest run.

[ilyagr]

Yeah, I keep updating and using this PR. I might again spend some time thinking about how to make it more than a hack, but probably not right now. You can unilaterally do it if you feel like it.

BTW, nextest-rs/nextest#16 just got a somewhat hopeful update, though it's unclear whether it will turn into a solution (let alone when).