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

In process test #2367

Merged
merged 6 commits into from
Jun 13, 2020
Merged

In process test #2367

merged 6 commits into from
Jun 13, 2020

Conversation

rbtcollins
Copy link
Contributor

@rbtcollins rbtcollins commented Jun 6, 2020

Run rustup tests in-process

Using the in-process test support feature, we can now change the test runner to run select CLI operations in-process.

More can be done, but this is the first major step: running rustup-mode in-process much of the time.

@kinnison
Copy link
Contributor

kinnison commented Jun 7, 2020

In its current state, this PR blows my ~/.cargo and ~/.rustup away

@rbtcollins
Copy link
Contributor Author

success!

@rbtcollins rbtcollins marked this pull request as ready for review June 12, 2020 01:27
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

Two minor niggles and then I think this is mergeable. What sort of test time improvement do we see?

Comment on lines +58 to +67
let matches = match cli().get_matches_from_safe(process().args_os()) {
Ok(matches) => Ok(matches),
Err(e)
if e.kind == clap::ErrorKind::HelpDisplayed
|| e.kind == clap::ErrorKind::VersionDisplayed =>
{
return Ok(utils::ExitCode(0))
}
Err(e) => Err(e),
}?;
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice to have this factored into a function given it's also used in setup_mode.rs and someone fixing something in one might miss the other.

@rbtcollins rbtcollins force-pushed the in-process-test branch 2 times, most recently from 3083faf to 8ec378b Compare June 13, 2020 10:40
The home crate also accesses global process state, so we need to
abstract over it as well.
There's a certain amount of hackiness in making this work, because a
number of our tests are intrinsically hostile to in-process testing, but
generally speaking it works well.
The better to test with.
In-process tests have a tty and clap doesn't honour our process()
abstraction; fortunately it does consult TERM, which is good enough.
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

Okay, I guess you didn't think the refactor worthwhile, fair enough.

@rbtcollins rbtcollins merged commit a93284b into rust-lang:master Jun 13, 2020
@rbtcollins rbtcollins deleted the in-process-test branch June 13, 2020 11:13
@rbtcollins
Copy link
Contributor Author

Okay, I guess you didn't think the refactor worthwhile, fair enough.

I think its marginal - there's really only one possible spelling here, and it's dictated by clap; and we have full test coverage. I'm happy to swing back and add it in when I get some more time.

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