Support compound stream matchers #1460
Conversation
|
I think this is the solution: rspec/rspec-support#598 |
nevinera
force-pushed
the
nev--1391--handle-compound-stream-matchers-correctly
branch
from
2b07db6 to
9127806
Compare
|
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 -.- |
|
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 Is there anything else I can dig for you? What are your suspects? |
pirj
force-pushed
the
nev--1391--handle-compound-stream-matchers-correctly
branch
from
9127806 to
17dfbee
Compare
Gemfile
Outdated
| @@ -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" | |||
There was a problem hiding this comment.
This is not needed anymore when rspec/rspec-support#598 is merged?
There was a problem hiding this comment.
I think that's correct, but I'll need a few minutes to refresh; I'll take a look later today. :-)
There was a problem hiding this comment.
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 :-\
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
"JRuby 1.7 1.8 mode" has RSpec::Support::Ruby.jruby? returning true
I think this is as expected - it's JRuby, too.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
A wild idea - can we use a proxy that would additionally tell us if it’s a fake stream (and use a Tempfile underneath)?
There was a problem hiding this comment.
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 :-\
There was a problem hiding this comment.
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 #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.
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
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.
nevinera
force-pushed
the
nev--1391--handle-compound-stream-matchers-correctly
branch
from
8fc34dc to
829f7fe
Compare
| # 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
|
I'm closing this as part of the monorepo migration because its a draft, but if you wanted to reopen it @nevinera it should be easier to work with there. |
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 rspec/rspec#89.
While resolving the issue for the
to_stderr_from_any_processthough, I found an additional problem, which is that this matcher didn't properly restore theRSpec::Support::StdErrSplitterto 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 rspec/rspec#89