Support compound stream matchers #1460

nevinera · 2024-05-19T04:49:01Z

Currently, compound stream matchers aren't properly supported, because when the CompoundMatcher nests them, the inner matcher doesn't expose the captured output to the outer matcher. This results in #1391.

While resolving the issue for the to_stderr_from_any_process though, I found an additional problem, which is that this matcher didn't properly restore the RSpec::Support::StdErrSplitter to its original state after capturing (it leaves it pointed at a now-dead Tempfile stream).

This PR will be in draft state until I work out a resolution for that.

Fixes #1391

nevinera · 2024-05-20T02:10:51Z

I think this is the solution: rspec/rspec-support#598

nevinera · 2024-05-21T02:55:16Z

There does still seem to be some issue in JRuby - it looks like JRuby has a different implementation of Tempfile, and as a result, the existing stream-reopen behavior doesn't produce objects that can distinguished from the original as we're currently doing. I'll be back once I can get JRuby working locally, I've never done anything in there -.-

pirj · 2024-06-24T21:02:38Z

I can reproduce those two failures on JRuby 9.4.7.0 + OpenJDK 22, which were a breeze to install with rvm/homebrew.

The only difference I could find is:

But to_io makes them return true in both cases for is_a?(File), even though both before and after to_io those are of a class Tempfile.

Is there anything else I can dig for you? What are your suspects?

pirj · 2024-08-18T11:43:53Z

Gemfile

@@ -16,6 +16,8 @@ branch = File.read(File.expand_path("../maintenance-branch", __FILE__)).chomp
  end
 end

+gem "rspec-support", :git => "https://github.com/nevinera/rspec-support.git", :branch => "nev--override-stderr-splitter-clone"


This is not needed anymore when rspec/rspec-support#598 is merged?

I think that's correct, but I'll need a few minutes to refresh; I'll take a look later today. :-)

Ah, so the resolution on the other PR was just to declare that the behavior would remain incorrect on jruby for now, and skip the relevant tests there? Should I update this PR to match? I didn't feel comfortable skipping support for such a significant ruby, but I admit I didn't make any progress on it, and haven't found time to come back to the problem in a while now :-\

I am having a surprising amount of trouble successfully skipping these tests for the right containers. It sure seems like "JRuby 1.7 1.8 mode" has RSpec::Support::Ruby.jruby? returning true; do you know what's special about that setup? (I'll poke around again this evening)

Edit: I typo'd this comment, and it says the reverse of what I meant it to say. I was actually seeing that container apparently running the specs that were marked as 'pending if jruby?', which all of the other jruby containers correctly skipped.

"JRuby 1.7 1.8 mode" has RSpec::Support::Ruby.jruby? returning true

I think this is as expected - it's JRuby, too.

That's what this is - it's a type-level difference, not an api-level one, but I never found another way to disambiguate the two cases.

I called it a "bug" because it does appear to be intentional, but it's been under discussion as a problem in MRI a few times too; as far as I can tell, this is the only place in all of ruby that an object can change its type after instantiation. I would generally say that rubyists should avoid being concerned with the type of the object in the first place; that's just the only place the information I needed to check (is the the original tempfile, or has it been reopened?) seems to be held.

I.. could maybe patch File in rspec-support to track that explicitly; how offensive would that be?

A wild idea - can we use a proxy that would additionally tell us if it’s a fake stream (and use a Tempfile underneath)?

So... "yes, probably?", in two different ways.

One of those ways is to introduce something on the level of the StdErrSplitter, but for StdOut; I scoped it out and decided it was workable, but involved enough that I didn't want to try it unless it solved a bunch of other problems too (not just from the effort, I think it has the potential to break stuff in a lot of places).

The other is to update the matchers so that they wrap the thing being yielded with a delegating file-alike, which further matchers could then check against to determine "is the thing that was yielded to me an original stream or a wrapped (xN) stream?" to have different behavior. I have a question-mark this time because I have some doubts of my ability to implement a sufficiently correct file-alike, given how much trouble I had from a minor difference in jruby's implementation of File :-\

it all seems like.. too much effort and risk either way for a feature that gets so little use, and which will start to spontaneously work correctly in another jruby release or so. I don't know how fast those land, but if it's less than a year, I'd rather work on some other tickets, and just ship this (with a skip for older versions of jquery) after it changes Tempfile implementations.

This is the issue reported in rspec#1391 - expecting like expect { puts "foobar" } .to output.to_stdout(/foo/) .and output.to_stdout(/bar/) fails, because the two matchers get nested, and inner matcher catches the stream write, hiding it from the outer one.

@pirj

For the simple matchers, the solution is straightforward (and @pirj supplied it in the issue thread). Pass the output along to the original stream after storing it, unless the original stream is the _real_ original stream.

Sadly, it doesn't quite work on StdErrSplitter, because of the existing (but possibly non-impactful?) bug around restoring the original stream during the ensure block.

lib/rspec/matchers/built_in/output.rb

JonRowe · 2024-08-19T14:11:00Z

lib/rspec/matchers/built_in/output.rb

+          # we have access to is the stream itself, and in the case of stderr,
+          # that stream is really a RSpec::Support::StdErrSplitter (which is why
+          # we're testing `is_a?(File)` in such an obnoxious way).
+          inner_matcher = stream.to_io.is_a?(File)


Is there not a way to assert against one of our internal classes here? checking to see if we're already in a splitter etc

Only in some of the cases. We're interacting with whatever's in $stderr and $stdout, so I think we're reliably getting an internal class for the former but not the latter. Wrapping with an internal class for the latter as well did occur to me as a potential solution, but it was large and involved enough that I wasn't willing to attempt it for such a minor issue :-\ (also I'm scared)

This was referenced May 19, 2024

Compound expectations of OutputMatcher do not receive captured output #1391

Open

Override RSpec::Support::StdErrSplitter#clone rspec/rspec-support#598

Merged

nevinera force-pushed the nev--1391--handle-compound-stream-matchers-correctly branch from 2b07db6 to 9127806 Compare May 20, 2024 02:59

pirj force-pushed the nev--1391--handle-compound-stream-matchers-correctly branch from 9127806 to 17dfbee Compare August 18, 2024 11:36

pirj reviewed Aug 18, 2024

View reviewed changes

nevinera added 2 commits August 18, 2024 14:15

Pass output along, if not in the outer matcher

1bc9e69

For the simple matchers, the solution is straightforward (and @pirj supplied it in the issue thread). Pass the output along to the original stream after storing it, unless the original stream is the _real_ original stream.

nevinera force-pushed the nev--1391--handle-compound-stream-matchers-correctly branch 2 times, most recently from de7a375 to 3fbe8a0 Compare August 18, 2024 18:43

nevinera and others added 3 commits August 18, 2024 15:10

Nearly support compound matchers on CaptureStreamToTempfile

7e7d458

Sadly, it doesn't quite work on StdErrSplitter, because of the existing (but possibly non-impactful?) bug around restoring the original stream during the ensure block.

Add a changelog entry

4175430

Mark the two compound-stream matcher tests pending for jruby

829f7fe

nevinera force-pushed the nev--1391--handle-compound-stream-matchers-correctly branch from 8fc34dc to 829f7fe Compare August 18, 2024 19:11

JonRowe requested changes Aug 19, 2024

View reviewed changes

nevinera mentioned this pull request Aug 19, 2024

[wip] Rework spec for different JRuby implementation. rspec/rspec-support#611

Draft

Use 'captured_stream' instead of 'strm' (avoid abbreviations)

d44f61f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support compound stream matchers #1460

Support compound stream matchers #1460

nevinera commented May 19, 2024 •

edited by pirj

Loading

nevinera commented May 20, 2024

nevinera commented May 21, 2024 •

edited

Loading

pirj commented Jun 24, 2024

pirj Aug 18, 2024

nevinera Aug 18, 2024

nevinera Aug 18, 2024

nevinera Aug 18, 2024 •

edited

Loading

pirj Aug 18, 2024

nevinera Aug 19, 2024

nevinera Aug 19, 2024

pirj Aug 19, 2024

nevinera Aug 19, 2024

nevinera Aug 19, 2024

JonRowe Aug 19, 2024

nevinera Aug 19, 2024 •

edited

Loading

Support compound stream matchers #1460

Are you sure you want to change the base?

Support compound stream matchers #1460

Conversation

nevinera commented May 19, 2024 • edited by pirj Loading

nevinera commented May 20, 2024

nevinera commented May 21, 2024 • edited Loading

pirj commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nevinera Aug 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nevinera Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

nevinera commented May 19, 2024 •

edited by pirj

Loading

nevinera commented May 21, 2024 •

edited

Loading

nevinera Aug 18, 2024 •

edited

Loading

nevinera Aug 19, 2024 •

edited

Loading