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

test case to check opt_present() panic for undefined options #51. #68

Merged
merged 2 commits into from May 27, 2018
Merged

test case to check opt_present() panic for undefined options #51. #68

merged 2 commits into from May 27, 2018

Conversation

prataprc
Copy link
Contributor

I am looking at issues to get a hang of rust lang and its ecosystem. If this commit does not make sense, please feel free to abandon. Thanks,

At later point, if opt_present() decide to return gracefully with
false, instead of panic, we can just remove #[should_panic] attribute
on the test case.
@KodrAus
Copy link
Contributor

KodrAus commented May 23, 2018

Thanks for the pull request @prataprc! This looks like a great start, I think we could refactor the test just a bit so it only hits the panic we expect when calling matches.opt_present:

match opts.parse(args) {
    Ok(matches) => assert!(!matches.opt_present("undefined")),
    Err(e) => println!("failed to parse opts: {}", e),
}

So that way if we fail to parse the args (even though we won't, and if we do then other tests should fail) we won't pass this one because there won't be any panic.

@prataprc
Copy link
Contributor Author

Suggestion looks neat ! Amended the PR.

@KodrAus
Copy link
Contributor

KodrAus commented May 27, 2018

This looks good to me! Thanks @prataprc

@KodrAus KodrAus merged commit 08af2e5 into rust-lang:master May 27, 2018
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.

None yet

2 participants