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

[wip] Rework spec for different JRuby implementation. #611

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JonRowe
Copy link
Member

@JonRowe JonRowe commented Aug 19, 2024

No description provided.

@JonRowe JonRowe changed the title Jruby fix [wip] Rework spec for different JRuby implementation. Aug 19, 2024
@JonRowe JonRowe force-pushed the jruby-fix branch 5 times, most recently from 118c0f5 to c114a2c Compare August 19, 2024 13:46
@pirj
Copy link
Member

pirj commented Aug 19, 2024

@nevinera jfyi

tempfile.close
tempfile.unlink
end
expect($stderr.to_io).not_to be_a(File)
expect(splitter.to_io.path).not_to eq tempfile.path
Copy link
Contributor

Choose a reason for hiding this comment

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

While I think this is a valid test for the code in StderrSplitter, I don't think it exercises the case I was trying to rely on in rspec/rspec-expectations#1460 anymore. Depending on whether we actually want to release that PR, that may be a good thing? I hadn't actually intended this PR to be merged until we had consensus on that one.

@JonRowe JonRowe force-pushed the jruby-fix branch 3 times, most recently from 21556a5 to 582cce3 Compare August 20, 2024 12:31

tempfile = Tempfile.new("foo")
begin
splitter.reopen(tempfile)
expect(splitter.to_io).to be_a(File)
expect(splitter.to_io).to_not be_stderr
Copy link
Member Author

Choose a reason for hiding this comment

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

@nevinera Unless I'm mistaken this represents the same effect as what you were testing, I'm not sure if I will merge this yet but it at least works for some JRuby

Copy link
Contributor

@nevinera nevinera Aug 21, 2024

Choose a reason for hiding this comment

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

Ah, I think the part I didn't follow was the bottom method - I assumed be_stderr was calling a stderr? method I wasn't aware of on the stream/splitter >.<

I'm not confident about how consistent that inspect is, but I'll trust your plan :-)

(Is that trick sturdy enough to use in here?)

@JonRowe JonRowe force-pushed the jruby-fix branch 2 times, most recently from cf25a08 to 43db5ab Compare September 6, 2024 11:31
@JonRowe JonRowe force-pushed the jruby-fix branch 2 times, most recently from b853a35 to fa11ed1 Compare September 10, 2024 11:03
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.

3 participants