Skip to content

adds --path and --nonexistent options to "rustup override unset"; adds hint to "rustup override list output" (as discussed in #642)#650

Merged
brson merged 7 commits intorust-lang:masterfrom
Riateche:unset-override-options
Aug 18, 2016
Merged

adds --path and --nonexistent options to "rustup override unset"; adds hint to "rustup override list output" (as discussed in #642)#650
brson merged 7 commits intorust-lang:masterfrom
Riateche:unset-override-options

Conversation

@Riateche
Copy link
Copy Markdown
Contributor

No description provided.

@brson
Copy link
Copy Markdown
Contributor

brson commented Aug 11, 2016

Awesome. Thanks @Riateche ! Reviewing now.

.after_help("If \"--path\" argument is present, removes the override toolchain for \
specified directory. If \"--nonexistent\" argument is present, removes the override \
toolchain for all nonexistent directories. Otherwise, removes the override toolchain \
for current directory.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this text to help.rs.

"toolchain for specified directory" -> "toolchain for the specified directory"

"toolchain for current directory" -> "toolchain for the current directory"

@brson
Copy link
Copy Markdown
Contributor

brson commented Aug 11, 2016

Looks great!

@brson
Copy link
Copy Markdown
Contributor

brson commented Aug 11, 2016

This calls for several tests.

Can you add tests for rustup override list with a nonexistant directory, rustup override unset --path with both existing and non-existing dirs, rustup override unset --nonexistent, and similar cases for rustup override remove? They can go in tests/cli-exact.rs and crib off the existing tests there.

@Riateche
Copy link
Copy Markdown
Contributor Author

I'm a bit stuck with tests. I don't want to write fully duplicated tests for "remove" and "unset" because they should be complete aliases. But when I attempt to move test implementation to a function, it suddenly stops working, although nothing else is changed:

fn remove_override_test_helper(keyword: &'static str) {
    setup(&|config| {
        let cwd = env::current_dir().unwrap();
        expect_ok(config, &["rustup", "override", "add", "nightly"]);
        expect_ok_ex(config, &["rustup", "override", keyword],
            r"",
            &format!(r"info: override toolchain for '{}' removed
            ", cwd.display()));
    });

}

#[test]
fn remove_override() {
    remove_override_test_helper("remove");
}

Output:

failures:

---- remove_override stdout ----
    out.ok: true
out.stdout:


  nightly-x86_64-unknown-linux-gnu installed - 1.3.0 (hash-n-2)


out.stderr:

info: syncing channel updates for 'nightly-x86_64-unknown-linux-gnu'
info: downloading component 'rust-std'
info: downloading component 'rustc'
info: downloading component 'cargo'
info: downloading component 'rust-docs'
info: installing component 'rust-std'
info: installing component 'rustc'
info: installing component 'cargo'
info: installing component 'rust-docs'
info: override toolchain for '/home/ri/rust/forks/rustup.rs' set to 'nightly-x86_64-unknown-linux-gnu'

expected: 
out.ok: true
out.stdout:


out.stderr:

info: override toolchain for '/home/ri/rust/forks/rustup.rs' removed

expected.stdout: 


expected.stderr: 

info: override toolchain for '/home/ri/rust/forks/rustup.rs' removed

thread 'remove_override' panicked at 'err ["rustup", "override", "remove"]', src/rustup-mock/src/clitools.rs:206
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    remove_override

Next, I will need to create temporary directories to perform some tests. Is it ok to create them in the current directory or should I create them somewhere else?

@brson
Copy link
Copy Markdown
Contributor

brson commented Aug 12, 2016

For temporary directories please use the tempdir crate. It's already a dependency of rustup, but needs to be added to dev-dependencies.

The error with the test is frustrating - the output looks write. The rustup test harness is fairly primitive and can be hard to interpret. If you push the non-working commits I can take a look.

@Riateche
Copy link
Copy Markdown
Contributor Author

Looks like it was just white space mismatch caused by code formatting. It seems to be working now. I'll try to write new tests.

@Riateche
Copy link
Copy Markdown
Contributor Author

I've created all needed tests. I had to remove "canonicalize" call when removing nonexistent path. otherwise it produced a warning. Now Travis says there is an error on Mac OS. It may be related to "canonicalize" call because paths do not match. I don't have Mac OS so I can't fix it quickly. Maybe you can take a look.

@Riateche
Copy link
Copy Markdown
Contributor Author

Looks like I got it working. Please review.

@brson brson merged commit d0c3063 into rust-lang:master Aug 18, 2016
@brson
Copy link
Copy Markdown
Contributor

brson commented Aug 18, 2016

Woo! Thanks @Riateche ! Thank you so much for sticking it out and writing those tests. I <3 tests.

@Riateche Riateche deleted the unset-override-options branch August 18, 2016 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants