Replace `test-invocation-variants.sh` with `cargo test` #215

Enselic · 2022-11-24T18:39:29Z

This makes ./scripts/run-ci-locally.sh run in ~10s instead of ~12s for me.

It also significantly simplifies the testing infrastructure.

This makes `scripts/run-ci-locally.sh` run in 10s instead of 12s for me. It also significantly simplifies the testing setup.

Enselic · 2022-11-24T19:47:23Z

I think the problem we are seeing is rust-lang/rustup#988

We might need to handle rustup concurrency ourselves somehow :(
Or simply install toolchains as a pre-cargo test step (:vomiting_face: )

An adventure for some other day...

cargo-public-api/tests/cargo-public-api-bin-tests.rs

The use of `&` is because pedantic clippy insists on it.

Emilgardis · 2022-11-25T07:14:03Z

cargo-public-api/tests/cargo-public-api-bin-tests.rs

+                let mut cmd = Command::from_std(std::process::Command::new("cargo"));
+                if let Some(toolchain) = toolchain {
+                    cmd.arg(format!("+{}", toolchain));
+                }
+                cmd.arg("public-api");


might want to do the same thing as here:

cargo-public-api/cargo-public-api/src/toolchain.rs

Lines 10 to 12 in e5324e4

let mut cmd = std::process::Command::new("rustup");

cmd.args(["run", toolchain, "cargo"]);

cmd

Hmm yes that might work, but then the test would not do what regular users do, which IMHO is quite important.

After thinking some more about how to approach this short term, I think requiring toolchains to be pre-installed before tests run is reasonable, and provide a helper script to do so. I'd really like to get this PR merge sooner rather than later, because otherwise the risk of conflicts is high, and I see it as a significant improvement.

If I just can get this to work in Windows now...

My bad, I should have made myself clear.

The code works around rust-lang/rustup#3036, which is this exact issue (however the error message seems to have changed a bit)

error: no such subcommand: `+beta` Cargo does not handle `+toolchain` directives. Did you mean to invoke `cargo` through `rustup` instead?

Thanks a ton for clarifying. I had completely forgotten about that bug. You saved me a lot of debugging time.

I've pushed a fix now, and CI is green.

I can think of many follow-up improvements to this PR, but let's get this merged before we improve on it further.

One idea:
Why don't we put the helper to install toolchains in e.g. rustdoc-json crate, and then install the necessary toolchain automatically for users (if it is missing) during regular cargo public-api use?

I would not expect rustdoc-json to have rustup stuff in it. I'm definitely myself against installing the toolchain automatically via a rustup toolchain install but I see the value 👍🏼

Interesting viewpoint! Let's break it down to a specific example. Consider a user that has stable installed but not nightly. Then that user does cargo install cargo-public-api and then cargo public-api.

In this case, I bet that say 95% of users want us to do the work to install nightly for them. Otherwise the tool that they just downloaded is useless!

The 5% that do NOT want us to install nightly for them can just Ctrl-C and stop the process.

This is a classic interaction design rule actually. Don't let the 5% ruin the experience for the other 95% :)

I am of course happy to keep discussing this. There might very well be some aspect here I am not thinking about. Meanwhile, I have created a new rustup-toolchain crate: https://crates.io/crates/rustup-toolchain, which we'll need it regardless of if we install nightly by default or allow users to opt-in. I'll wait with creating a PR for that though until we have landed this PR.

good point, I guess I'm that 5% :D

I did a quick experiment, but all tests started failing after uninstalling nightly toolchain, so I don't see me pursuing auto-install in the near future

For the record, prototype code at main...auto-install-toolchain

I mean, probably quite easy to make it work, but I don't consider this urgent or crucial, so I'll put it on hold for now

Which will install toolchains needed for tests. Long-term it will be nice to get rid of this, but since this is a one-time thing, I think we can live with it. If a dev runs `cargo test` with missing toolchains, the dev will see this error: thread 'warn_when_using_beta' panicked at ' You must run ./scripts/install-toolchains-for-tests.sh before running tests. Or simply run ./scripts/run-ci-locally.sh to both install toolchains and run tests in one command. ', cargo-public-api/tests/../../test-utils/src/lib.rs:70:5

Enselic · 2022-11-25T16:00:53Z

Hmm interesting, we get the same error now. So probably we don't need ./scripts/install-toolchains-for-tests.sh. We just need the regular code to work on Windows.

Maybe time for me to buy a Windows license so I can debug this...

…riants.sh

…nvocations

This reverts commit e1a3b4b. Keep some of the changes to the README though.

Since a single test takes a very long time if it needs to install a toolchain, I want there to be an indication of what is going on and why the test is so slow.

Enselic added the category-exclude Will be excluded from the auto-generated release notes. label Nov 24, 2022

Enselic changed the title ~~Replace test-invocation-variants.sh tests with proper cargo test tests~~ Replace test-invocation-variants.sh with cargo test Nov 24, 2022

Enselic force-pushed the rm-test-invocation-variants.sh branch from 081dd3f to 446faec Compare November 24, 2022 18:56

Enselic mentioned this pull request Nov 24, 2022

Move some tests from test-invocation-variants.sh to cargo test #216

Merged

Enselic force-pushed the rm-test-invocation-variants.sh branch from 446faec to 90218ee Compare November 24, 2022 19:30

Remove ./scripts/test-invocation-variants.sh after porting last tests

7fdb404

This makes `scripts/run-ci-locally.sh` run in 10s instead of 12s for me. It also significantly simplifies the testing setup.

Enselic force-pushed the rm-test-invocation-variants.sh branch from 90218ee to 7fdb404 Compare November 24, 2022 19:40

Enselic marked this pull request as draft November 24, 2022 19:47

Emilgardis reviewed Nov 24, 2022

View reviewed changes

cargo-public-api/tests/cargo-public-api-bin-tests.rs Outdated Show resolved Hide resolved

Enselic added 2 commits November 25, 2022 07:19

Workaround rustup limitation by preventing concurrent rustup usage

b54a809

Obviate assert with TestCmdType enum

1493100

The use of `&` is because pedantic clippy insists on it.

Emilgardis reviewed Nov 25, 2022

View reviewed changes

Enselic force-pushed the rm-test-invocation-variants.sh branch from 9302261 to e1a3b4b Compare November 25, 2022 15:46

Merge remote-tracking branch 'origin/main' into rm-test-invocation-va…

7054ed9

…riants.sh

Enselic force-pushed the rm-test-invocation-variants.sh branch from 8171531 to 7054ed9 Compare November 30, 2022 06:54

Enselic added 3 commits November 30, 2022 18:51

Workaround rustup limitation on Windows with nested cargo toolchain i…

5f83eb0

…nvocations

Revert "Add ./scripts/install-toolchains-for-tests.sh script"

fa233aa

This reverts commit e1a3b4b. Keep some of the changes to the README though.

Don't be quiet when installing toolchains in tests

3ad24b8

Since a single test takes a very long time if it needs to install a toolchain, I want there to be an indication of what is going on and why the test is so slow.

Enselic marked this pull request as ready for review November 30, 2022 18:02

Enselic merged commit e990141 into main Dec 1, 2022

Enselic deleted the rm-test-invocation-variants.sh branch December 1, 2022 04:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `test-invocation-variants.sh` with `cargo test` #215

Replace `test-invocation-variants.sh` with `cargo test` #215

Enselic commented Nov 24, 2022 •

edited

Enselic commented Nov 24, 2022 •

edited

Emilgardis Nov 25, 2022

Enselic Nov 25, 2022

Emilgardis Nov 30, 2022

Enselic Nov 30, 2022 •

edited

Emilgardis Nov 30, 2022 •

edited

Enselic Nov 30, 2022

Emilgardis Nov 30, 2022

Enselic Dec 1, 2022 •

edited

Enselic commented Nov 25, 2022

	let mut cmd = std::process::Command::new("rustup");
	cmd.args(["run", toolchain, "cargo"]);
	cmd

Replace test-invocation-variants.sh with cargo test #215

Replace test-invocation-variants.sh with cargo test #215

Conversation

Enselic commented Nov 24, 2022 • edited

Enselic commented Nov 24, 2022 • edited

Emilgardis Nov 25, 2022

Choose a reason for hiding this comment

Enselic Nov 25, 2022

Choose a reason for hiding this comment

Emilgardis Nov 30, 2022

Choose a reason for hiding this comment

Enselic Nov 30, 2022 • edited

Choose a reason for hiding this comment

Emilgardis Nov 30, 2022 • edited

Choose a reason for hiding this comment

Enselic Nov 30, 2022

Choose a reason for hiding this comment

Emilgardis Nov 30, 2022

Choose a reason for hiding this comment

Enselic Dec 1, 2022 • edited

Choose a reason for hiding this comment

Enselic commented Nov 25, 2022

Replace `test-invocation-variants.sh` with `cargo test` #215

Replace `test-invocation-variants.sh` with `cargo test` #215

Enselic commented Nov 24, 2022 •

edited

Enselic commented Nov 24, 2022 •

edited

Enselic Nov 30, 2022 •

edited

Emilgardis Nov 30, 2022 •

edited

Enselic Dec 1, 2022 •

edited