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

tests: test multi-call logic #6198

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

BenWiederhake
Copy link
Collaborator

@BenWiederhake BenWiederhake commented Apr 6, 2024

I noticed that src/bin/coreutils.rs had abysmal coverage. Turns out, we don't have any tests for most features of our "multi-call binary" logic!

This PR fixes that by adding a few tests, namely:

  • Calling with an unusual name
  • Calling with a non-UTF8-name
  • Calling with invalid arguments (thus exercising the top-level argument error handling)
  • Generating a manpage
  • Generating a completion

Thankfully, this revealed no bugs. Let's prevent regressions!

EDIT: From 19% to 68%: https://app.codecov.io/gh/uutils/coreutils/blob/BenWiederhake%2Fcoreutils-rs%3Adev-better-test-coverage/src%2Fbin%2Fcoreutils.rs

@BenWiederhake BenWiederhake marked this pull request as draft April 6, 2024 22:25
Thankfully, this revealed no bugs. Let's prevent regressions!
@BenWiederhake BenWiederhake marked this pull request as ready for review April 6, 2024 23:36
@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

  • Rebased to current main
  • Changed the test with non-UTF-8 filename to require not just cfg(unix) but cfg(target_os = "linux"), and document the exact reason. Long story short, it would otherwise break in MacOS CI.

Copy link

github-actions bot commented Apr 7, 2024

GNU testsuite comparison:

Congrats! The gnu test tests/env/env is no longer failing!


let scenario = TestScenario::new("invalid_name");
symlink_file(scenario.bin_path, scenario.fixtures.plus("invalid_name")).unwrap();
let child = Command::new(scenario.fixtures.plus("invalid_name"))
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to adapt the test framework to support this so that we can write this test witht it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, sure, it is possible to do so, but I don't see the benefit?

The tests "unrecognized name" and "non-UTF-8 name" are likely going to be the only use cases for this. Perhaps one more edge case that I overlooked, but I really don't see the advantage to make this more generally available.

@sylvestre
Copy link
Contributor

codecov/project Successful in 2s — 87.27% (+0.13%) compared to abdeead
nice

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.

3 participants