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

fix(tests): restrict 'cat' tests to unix environments. Fixes #776 #777

Merged
merged 7 commits into from
Jan 4, 2025

Conversation

ritvikos
Copy link
Contributor

Tests using the cat command are specific to Unix-like environments and will fail on unsupported platforms.
Restrict these tests to unix environments only.

Tests using the 'cat' command are specific
to unix-like environments and
fail on unsupported platforms.
@sharkdp
Copy link
Owner

sharkdp commented Dec 1, 2024

Thank you for reporting this and for opening a PR. It would be great if we could test this behavior on Windows somehow. Maybe we can find a Windows replacement for cat? Doesn't need to be a cat-equivalent. Just something that we can pass input to. And check if it received the input correctly.

@ritvikos
Copy link
Contributor Author

ritvikos commented Dec 2, 2024

Great idea!
Found that type is the replacement of cat in Windows.
Now the tests work fine 😄

.arg("--runs=1")
.arg("--input=example_input_file.txt")
.arg("--show-output")
.arg("type example_input_file.txt")
Copy link
Owner

Choose a reason for hiding this comment

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

This is not testing the right thing, I believe? The cat test above on Linux makes sure that we can pipe input from example_input_file.txt into the benchmarked command. But this looks to me like you ignore that and simply output the contents if example_input_file.txt using type.

@ritvikos
Copy link
Contributor Author

type cannot handle stdin like cat, just prints the content of the file.

Whoops! I missed that detail 😅
Nice catch!

Although, found 2 alternatives that can handle pipe stdin on Windows,
let me know if you think it looks acceptable or you want me to look into other alternatives 😄

  1. findstr x*: StackOverflow

someprog | findstr x* or any other single character followed by
asterisk copies all lines from the pipe stdin to stdout.

~\Desktop\oss\hyperfine\tests> hyperfine --runs=1 --input=example_input_file.txt --show-output "findstr x*"
Benchmark 1: findstr x*
This text is part of a file
  Time (abs ≡):         21.8 ms               [User: 0.0 ms, System: 4.7 ms]

  1. more command: Displays one screen of output at a time.
    Microsoft - More and Unixtutorial.org
~\Desktop\oss\hyperfine\tests> hyperfine --runs=1 --input=example_input_file.txt --show-output "more"
Benchmark 1: more
This text is part of a file
  Time (abs ≡):         16.2 ms               [User: 0.0 ms, System: 5.9 ms]

@sharkdp
Copy link
Owner

sharkdp commented Dec 27, 2024

findstr x*: StackOverflow

Thanks for looking into this. I think this is an acceptable hack. We should add a link to that stackoverflow answer to explain what's going on though.

@ritvikos
Copy link
Contributor Author

ritvikos commented Jan 2, 2025

Thank you for reviewing this.
Fixed the issue and added the link as well.

@sharkdp
Copy link
Owner

sharkdp commented Jan 4, 2025

Thank you. I think we can use something like let command = if cfg!(windows) { … } else { … }; to merge the tests for windows and linux again, to remove some duplication.

@ritvikos
Copy link
Contributor Author

ritvikos commented Jan 4, 2025

That makes sense!
How about defining a const for platform-specific standard I/O utility (not sure about naming) with this approach? 🤔

@sharkdp sharkdp merged commit 3bd38f2 into sharkdp:master Jan 4, 2025
15 checks passed
@sharkdp
Copy link
Owner

sharkdp commented Jan 4, 2025

Thank you!

rbeneyton pushed a commit to rbeneyton/hyperfine that referenced this pull request Jan 23, 2025
 (sharkdp#777)

Tests using the 'cat' command are specific
to unix-like environments and
fail on unsupported platforms.
@ritvikos ritvikos deleted the tests branch February 1, 2025 11:55
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.

2 participants