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: replace panic in favor of ParseError #4558

Merged
merged 3 commits into from
Mar 26, 2023

Conversation

leon3s
Copy link
Contributor

@leon3s leon3s commented Mar 19, 2023

Closes #4556
Closes #4555

@leon3s leon3s changed the title test: remplaced panic in favor of ParseError test: (wip) remplaced panic in favor of ParseError Mar 19, 2023
@github-actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

@cakebaker cakebaker changed the title test: (wip) remplaced panic in favor of ParseError test: (wip) replace panic in favor of ParseError Mar 20, 2023
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Nice! I think we can even take this a bit further and avoid using strings as errors altogether if ParseError implements UError.

src/uu/test/src/parser.rs Outdated Show resolved Hide resolved
src/uu/test/src/parser.rs Show resolved Hide resolved
@leon3s leon3s marked this pull request as draft March 20, 2023 16:22
@leon3s leon3s changed the title test: (wip) replace panic in favor of ParseError test: replace panic in favor of ParseError Mar 25, 2023
@leon3s leon3s marked this pull request as ready for review March 25, 2023 02:55
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

It's looking good! Just a few small suggestions.

src/uu/test/src/parser.rs Outdated Show resolved Hide resolved
src/uu/test/src/error.rs Show resolved Hide resolved
@@ -128,23 +130,18 @@ pub fn uumain(mut args: impl uucore::Args) -> UResult<()> {
}
}

let result = parse(args).and_then(|mut stack| eval(&mut stack));
let result = parse(args).map(|mut stack| eval(&mut stack))??;
Copy link
Member

Choose a reason for hiding this comment

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

Is this equivalent to eval(parse(args)?)?

Copy link
Contributor Author

@leon3s leon3s Mar 25, 2023

Choose a reason for hiding this comment

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

@tertsdiepraam Yes it is, you prefer eval(parse(args)?)? syntax ?

@leon3s leon3s force-pushed the fix-test-with-parse-error branch from 393eb2b to 11fd56c Compare March 25, 2023 13:03
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/tail-2/inotify-dir-recreate is no longer failing!

@leon3s
Copy link
Contributor Author

leon3s commented Mar 25, 2023

Look like the failling tests are related to #4634

@sylvestre sylvestre merged commit 76793d5 into uutils:main Mar 26, 2023
@sylvestre
Copy link
Contributor

thanks :)

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.

test '(' foo --1 should not panic test '(' foo should fail like GNU's
3 participants