search

rust-lang / rustup

The Rust toolchain installer
https://rust-lang.github.io/rustup/
Apache License 2.0
6.18k stars 891 forks source link

Additional tests for `rust-toolchain` desirable #2575

Open kinnison opened 3 years ago

kinnison commented 3 years ago

We could do with some additional testing of the rust-toolchain functionality. Particularly around healing damaged toolchain installations, ensuring that the right components/targets are installed into it, etc.

This is related to #2574

chansuke commented 3 years ago

I'd like to work on this.

kinnison commented 3 years ago

Wonderful. Perhaps a good start would be for you to look at all the tests which already work on rust-toolchain, and then come up with some proposed tests as comments on this issue. Once we have a good idea of what tests you should write, it'll be worth putting hand-to-keyboard to produce some test code.

chansuke commented 3 years ago

@kinnison I appreciate your comment and suggestion. I will start working at this weekend.

chansuke commented 3 years ago

@kinnison I read through the test codes... My idea is that creating the function that generates a damaged toolchain in tests/mock/clitools.rs. After that, I will start adding some test codes. Is that the right approach...? Thanks.

kinnison commented 3 years ago

I'm not sure you need to create broken toolchains in clitools.rs.

Let's first consider the 'checking components and targets work' part of the above:

A possible testing scenario might be:

  1. a project has a rust-toolchain stating that it wants nightly with the rust-src component installed.
  2. the user already has nightly but without rust-src
  3. we should cause rust-src to be installed when the user interacts with rustup (e.g. rustup show active-toolchain) in that project

You could extend the above to include, for example, the user deliberately uninstalling a needed component or target, and verifying that on interacting with rustup show active-toolchain in that directory, the uninstalled bits are reinstalled.

Now for the 'healing' aspect, specifically the linked PR deals with the fact that if the already installed toolchain looked like a dist toolchain but somehow was missing its manifest file then we would have failed to "heal" it. To understand what's going on there we need to think through the code which leads up to that point:

  1. The override file talks about a toolchain which ought to be a dist toolchain (e.g. nightly)
  2. The toolchain is already installed
  3. The installed toolchain is damaged (missing its manifest file)
  4. The user interacts with rustup in a way which causes it to need to use that override (e.g. a rust-toolchain file is present with nightly in it and the user runs rustup show active-toolchain)

To achieve 3 there, all we need do in a test is:

  1. cause nightly to be installed (e.g. rustup default nightly - there's lots of examples of this in the test suite)
  2. remove the manifest file from the installed rustup (this will be more tricky, but since this is rustup's test suite you're permitted to "know" how to find and remove that file, it'll be lib/rustlib/multirust-channel-manifest.toml inside the relevant installed toolchain dir

I hope that's enough for you to proceed, good luck. I really want these tests, so it's wonderful that you're going to give it a go 👍🏻

rbtcollins commented 3 years ago

Does show active-toolchain install the toolchain today? I have cognitive friction around the idea that 'show' installs things. Other than that, this all sounds good.

kinnison commented 3 years ago

@rbtcollins Yep if there's a rust-toolchain file then rustup show active-toolchain will install that in order to be able to show it.