-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 utilities: easy way to simulate terminal context #5869
test utilities: easy way to simulate terminal context #5869
Conversation
83412c4
to
379a38c
Compare
2ac77c5
to
7bfed9e
Compare
7bfed9e
to
0e11cdc
Compare
Is it ready to merge already, or are we waiting for further improvements ? |
0e11cdc
to
a803ce1
Compare
What is missing is a review and approval from one of the maintainers. |
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.
I like it a lot! Excellent work! I only have some small questions.
I also think that we might want to be able to configure the terminal with this interface, but for now, the API is flexible enough I think. It's also about time we extract the test framework to a crate, so we can generate documentation for it. It's getting quite complex. That's something for another PR though.
I would like to wait for approval of one of the other maintainers before merging though.
One final question: have you seen any intermittently failing tests with this? Can we assume that this implementation is robust enough not to make our test suite fail unexpectedly (even more so then it already does 😅)
tests/by-util/test_env.rs
Outdated
fn test_simulation_of_terminal_false() { | ||
let scene = TestScenario::new(util_name!()); | ||
|
||
let out = scene.ucmd().arg("sh").arg("is_atty.sh").succeeds(); |
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.
Are these meaningful tests of env
? If not, it could still be useful to have them (we're going very meta with tests for our test framework 😄), but maybe somewhere else?
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.
ah, yes. This is a valid point. I used env
here as a tool rather than to test itself. This is something I should cleanup. Do you have a proposal for a better location of these tests?
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.
Maybe in the file where the test framework is defined? It's an interesting question, we don't really have tests for the tests yet 😄
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.
done
0d14526
to
66ded04
Compare
I think its stable. |
66ded04
to
d21d2f1
Compare
943c22b
to
b291cf3
Compare
b291cf3
to
a4d5def
Compare
GNU testsuite comparison:
|
1 similar comment
GNU testsuite comparison:
|
5d57805
to
ef02e43
Compare
ef02e43
to
d8b3b41
Compare
GNU testsuite comparison:
|
thanks |
addresses #5785
and addresses #1857
replaces if accepted PR #5858
let me know what you think. It's very easy to use. Just do a
.simulate_terminal(true)
and thats it.I implemented in such a way that also stdin piping still works and also the capturing of stdout and stderr works as usual.
EDIT: For cases where the terminal size matters, there is no an additional function available:
Example:
where
is_atty.sh
looks like this: