Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Aug 18, 2016

Conversation

Riateche
Copy link
Contributor

No description provided.

@brson
Copy link
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
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
Contributor

brson commented Aug 11, 2016

Looks great!

@brson
Copy link
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
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
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
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
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
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
Contributor

brson commented Aug 18, 2016

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

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