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

stdin closed / unavailable during testing #3185

Open
t-bltg opened this issue Aug 30, 2022 · 13 comments · May be fixed by #3186
Open

stdin closed / unavailable during testing #3185

t-bltg opened this issue Aug 30, 2022 · 13 comments · May be fixed by #3186

Comments

@t-bltg
Copy link
Contributor

t-bltg commented Aug 30, 2022

#3065 broke Sixel.jl testing (this used to work on 1.6, 1.7, so I consider this a regession).

In our case, we need stdin opened in order to query / analyze terminal properties via control sequences.

Could we set an option to keep stdin opened if we cannot revert the PR ?

cc @johnnychen94, @IanButterworth

@t-bltg t-bltg linked a pull request Aug 30, 2022 that will close this issue
@t-bltg t-bltg changed the title stdin closed during testing stdin closed / unavailable during testing Aug 30, 2022
@oxinabox
Copy link
Contributor

oxinabox commented Nov 16, 2022

This also broke DataDeps.jl oxinabox/DataDeps.jl#104
where it is a feature that the user is prompted to confirm that they do infact want to download data during the test.
(which they can bypass by setting an enviroment variable).
I feature I am no longer sure the wisdom of, but given it has been working this way for 5 years, I am not inclined to change.

@oxinabox
Copy link
Contributor

also JuliaCI/PkgTemplates.jl#370

@IanButterworth
Copy link
Member

IanButterworth commented Nov 16, 2022

I don't understand what happened here.

#3065 removed stdin forwarding that was added in #2933 (because of #3062). Both of which were during development of 1.8.0

If this used to work before 1.8, the change & revert above isn't the reason for the breakage

@IanButterworth
Copy link
Member

1.7.3

p = run(ignorestatus(cmd))

1.8.0

p = run(pipeline(ignorestatus(cmd), stdout = sandbox_ctx.io, stderr = stderr_f()), wait = false)

@IanButterworth
Copy link
Member

IanButterworth commented Nov 16, 2022

Ok.

From the Base.run docs

When wait is true (default), I/O streams are shared with the parent process. Use pipeline to control I/O redirection.

@vtjnash in #3062 you said that one shouldn't forward stdin, but isn't that what run was doing by default before 1.8?

@vtjnash
Copy link
Member

vtjnash commented Nov 16, 2022

You should not forward and set wait=false. It is application dependent though if you need stdin

@IanButterworth
Copy link
Member

Ok. So would this be better?

p = run(ignorestatus(cmd), stdin, sandbox_ctx.io, stderr_f())

@vtjnash
Copy link
Member

vtjnash commented Nov 16, 2022

The Pkg tests are run on a worker which does not have access to stdin

@vtjnash
Copy link
Member

vtjnash commented Nov 16, 2022

Tests that need a TTY, should be BYO

@IanButterworth
Copy link
Member

So is this a way forward?

1 similar comment
@IanButterworth
Copy link
Member

So is this a way forward?

@IanButterworth
Copy link
Member

bump @vtjnash

@vtjnash
Copy link
Member

vtjnash commented Nov 28, 2022

Sure, we could move julia/test/testhelpers/FakePTYs.jl into the test harness itself. You would not want to revert #3065 though, since that brings back all sorts of other problems.

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 a pull request may close this issue.

4 participants