-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Use exit code 2 to indicate error #954
Use exit code 2 to indicate error #954
Conversation
Exit code 1 was shared to indicate both "no results" and "error." Use status code 2 to indicate errors, similar to grep's behavior.
@sirreal What do you think about adding few tests for this? We should be able to capture your manual tests in ripgrep's integration tests. The test helpers themselves might need some additions to test this. For example, you might consider adding something similar to |
I was wondering about testing. I'm happy to look into that, I'll investigate soon 👍 |
tests/tests.rs
Outdated
// See: https://github.com/BurntSushi/ripgrep/issues/948 | ||
sherlock!( | ||
feature_948_match, | ||
".", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should match any non-empty file
tests/tests.rs
Outdated
// See: https://github.com/BurntSushi/ripgrep/issues/948 | ||
sherlock!( | ||
feature_948_no_match, | ||
"6d28e48b5224a42b167e{10}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Highly unlikely to match any file. 10 random hex digits repeated 10 times.
tests/tests.rs
Outdated
// See: https://github.com/BurntSushi/ripgrep/issues/948 | ||
sherlock!( | ||
feature_948_error, | ||
"*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid regex.
I've added some integration tests as suggested, I'd appreciate feedback. |
tests/workdir.rs
Outdated
/// Runs the given command and asserts that its exit code matches expected exit code. | ||
pub fn assert_exit_code(&self, expected_code: i32, cmd: &mut process::Command) { | ||
let code = cmd.status() | ||
.expect("no status") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These expect
s can just become unwrap
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! This is perfect as-is, thank you so much. :-)
A pleasure! 🙌 |
Exit code
1
was shared to indicate both "no results" and "error." Use status code2
to indicate errors, similar to grep's behavior.Fixes #948
Adds integration tests for exit codes.
Adds test helper
WorkDir::assert_exit_code(&self, expected_code: i32, cmd: &mut process::Command)
to match arbitrary exit codes in tests.Testing
cargo test exit_code
Manual testing
Verify that the exit code is now
2
for error:And remains
0
when matches are found:And
1
for no matches: