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: Provide a way to run tests with PTY stdout #5785

Closed
RenjiSann opened this issue Jan 5, 2024 · 6 comments
Closed

tests: Provide a way to run tests with PTY stdout #5785

RenjiSann opened this issue Jan 5, 2024 · 6 comments

Comments

@RenjiSann
Copy link
Contributor

RenjiSann commented Jan 5, 2024

Some utils' behavior may change depending on the stdout().is_terminal() answer.
This is the case for ls which adapts its default quoting style depending on STDOUT being a TTY or not.

The issue is that from what I saw, the testing utilities do not provide any way to test these TTY behaviors.

Maybe we could adapt the utilities to offer such verifications.
A straighforward way to go would be to use the pty_process crate.

@cre4ture
Copy link
Contributor

cre4ture commented Jan 20, 2024

I was implementing something like this for the "nohup" tests. Its still pending as PR: #5858

I was testing multiple different external crates. Somehow I was falling back to implement it manually.
But this decision was based on a mistake that I did myself and only recognised later on.

I think if we decide to have it as part of the testing utilities then it makes sense to investigate again and a bit deeper. Especially because there are actually multiple crates that provides a similar functionality:

pty-process = { version = "0.4.0", features = ["async"] }
fake-tty = "0.3.1"
tty-spawn = "0.3.1"
ptyprocess = "0.4.1
portable-pty = "0.8.1"

Was there a special reason why you suggest pty-process?

I did a very first rougth test with all of them and currently I think that "ptyprocess" (without the dash) is probably a good candidate as it seems to be easy to use and connects well with the std::process::Command.

@cre4ture
Copy link
Contributor

cre4ture commented Jan 21, 2024

update: After some testing, I have the feeling that all of the crates have some issue such that we can't directly use it.
Some even come from a time where the rustlib probably didn't provide much. But this is different today.
Such that I think we should try to make our own implementation similar as I did already. This works fine and is much less code than the implementation of any of the crates.

There is only on special thing to mention: portable-pty provides a implementation even for windows. I will investigate a bit more into that to see if we can also support even windows there.

@cre4ture
Copy link
Contributor

the windows implementation for a pty in the rustlib is appartently not a new topic:
Tracking Issue for windows_process_extensions_raw_attribute
Add ability to spawn Windows process with Proc Thread Attributes

I think the windows implementation is not urgend and thus we should wait for the rustlib being extened to support it with the std::Command.

@cre4ture
Copy link
Contributor

let me know what you think from proposal #5869

@RenjiSann
Copy link
Contributor Author

let me know what you think from proposal #5869

Thank you !
The reason I only proposed the pty-process crate was that it felt similar to the way I we handle commands here.
However, the more I investigated, the more I realized I was missing knowledge and context about the deep functioning of PTYs.
Also I think I am not fluent enough in Rust yet to understand your whole PR, so not surprisingly, I wouldn't have proposed something better (although I keep hope in becoming better someday as I understand a bit of it).

Anyway, from what I read and understood, that's exactly how I imagined it.
👍 Thank you !

@RenjiSann
Copy link
Contributor Author

Fixed by #5869

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

No branches or pull requests

2 participants