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 output.to_stdout_from_any_process when $stdout has been reassigned #1486

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jdelStrother
Copy link

(An alternate fix for rspec/rspec-core#3113)

When running under rspec bisect runner, $stdout & $stderr are reassigned to a StringIO, which would cause "can't convert Tempfile into StringIO" when trying to reopen the stream in CaptureStreamToTempfile

Since CaptureStreamToTempfile reopens the stream rather than reassigning it, I think it makes sense to use STDOUT rather than whatever the current value of $stdout is.

(Please feel free to sanity-check that assumption. I've gone round in circles a few too many times on this)

When running under rspec bisect runner, $stdout & $stderr are reassigned
to a StringIO, which would introduce spec errors.

("can't convert Tempfile into StringIO", when trying to reopen the
stream in CaptureStreamToTempfile)

Since CaptureStreamToTempfile reopens the stream rather than reassigning
it, I _think_ it makes sense to use STDOUT rather than whatever the
current value of $stdout is.
@JonRowe
Copy link
Member

JonRowe commented Sep 20, 2024

I don't have any objections to this approach apart from the fact it breaks our build due to having already redeclared std out, I'm not sure how to fix this yet...

Otherwise it doesn't work when $stderr has previously been reassigned,
like in the rspec-core test suite
@jdelStrother
Copy link
Author

I've pushed a possible fix, where instead of reopening $stderr, it reopens STDERR and reassigns $stderr.
I'm not entirely sure it's going to work, but see what you think and I can squash it if desired.

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