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

libtest: Replace panics with error messages #47986

Merged
merged 1 commit into from
Feb 6, 2018

Conversation

Gilnaa
Copy link

@Gilnaa Gilnaa commented Feb 3, 2018

This replaces explicit panics on failures in libtest with prints to stderr.
Where "failures" == CLI argument parsing and such

Before:

$ ./foo-stable --not-an-option
thread 'main' panicked at '"Unrecognized option: \'not-an-option\'"', libtest/lib.rs:251:27
note: Run with `RUST_BACKTRACE=1` for a backtrace.

After:

$ ./foo-nightly --not-an-option
error: Unrecognized option: 'not-an-option'

@rust-highfive
Copy link
Collaborator

r? @Kimundi

(rust_highfive has picked a reviewer for you, use r? to override)

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

I'm not sure if the exit code is correct, but I'm fine with approving this with the small not fixed. We can iterate further later.

Err(e) => panic!("io error when running tests: {:?}", e),
Err(e) => {
eprintln!("error: io error when listing tests: {:?}", e);
::std::process::exit(101);
Copy link
Member

Choose a reason for hiding this comment

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

Let's import std::process to make these a little cleaner.

@Gilnaa
Copy link
Author

Gilnaa commented Feb 4, 2018

Refactored as you suggested.

The error code is the same as what have been used elsewhere in the file; is there some sort of standard?
AFAIK it's just nonzero=error

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 4, 2018
@Mark-Simulacrum
Copy link
Member

Not really. @bors r+

@bors
Copy link
Contributor

bors commented Feb 4, 2018

📌 Commit 61ff3ba has been approved by Mark-Simulacrum

kennytm added a commit to kennytm/rust that referenced this pull request Feb 6, 2018
…lacrum

libtest: Replace panics with error messages

This replaces explicit panics on failures in libtest with prints to stderr.
Where "failures" == CLI argument parsing and such

Before:
```
$ ./foo-stable --not-an-option
thread 'main' panicked at '"Unrecognized option: \'not-an-option\'"', libtest/lib.rs:251:27
note: Run with `RUST_BACKTRACE=1` for a backtrace.
```

After:
```
$ ./foo-nightly --not-an-option
error: Unrecognized option: 'not-an-option'
```
bors added a commit that referenced this pull request Feb 6, 2018
Rollup of 7 pull requests

- Successful merges: #46962, #47986, #48012, #48013, #48026, #48031, #48036
- Failed merges:
@bors bors merged commit 61ff3ba into rust-lang:master Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants