Skip to content
This repository has been archived by the owner on Nov 30, 2024. It is now read-only.

Override RSpec::Support::StdErrSplitter#clone #598

Merged
merged 3 commits into from
Aug 18, 2024

Conversation

nevinera
Copy link
Contributor

With the default definition, cloning a StdErrSplitter results in the clone having the same @orig_stderr instance as the original Splitter has, which means that reopening either Splitter affects both (since the underlying stream is not cloned).

This is different from the behavior of an actual stream, in a way that causes a problem for the CaptureStreamToTempfile in rspec-matchers (used by the output.to_stderr_from_any_process matcher), because it clones the original stream to store it for restoration, and then reopens it at a Tempfile temporarily. But then when it goes back to restore the stream, it doesn't actually do so, since the clone's stream reference is the same instance as the original Splitter has - it just reopens it with the Tempfile again.

This is a supporting PR for rspec/rspec-expectations#1460 - before I can make compound to_stderr_from_any_process matchers work, I need the "restore the stream after capturing" behavior to actually work.

nevinera added 2 commits May 19, 2024 21:36
With the default definition, cloning a StdErrSplitter results in the
clone having the same @orig_stderr instance as the original Splitter
has, which means that reopening either Splitter affects both (since the
underlying stream is not cloned).

This is different from the behavior of an actual stream, in a way that
causes a problem for the CaptureStreamToTempfile in rspec-matchers (used
by the `output.to_stderr_from_any_process` matcher), because it clones
the original stream to store it for restoration, and then `reopen`s it
at a Tempfile temporarily. But then when it goes back to _restore_ the
stream, it doesn't actually do so, since the clone's stream reference is
the same instance as the original Splitter has - it just reopens it with
the Tempfile again.
This is what the matcher is trying to actually do, so ensuring that it
works makes a stronger test.
tempfile.close
tempfile.unlink
end
expect($stderr.to_io).not_to be_a(File)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test fails on this line without the change - $stderr is actually still pointed at the Tempfile (despite that it's closed), and not the original process stream.

@nevinera
Copy link
Contributor Author

Ugh, I'll put this in draft until I can figure out how to get jruby installed on an m2 -.-

@nevinera nevinera marked this pull request as draft May 27, 2024 03:44
@JonRowe
Copy link
Member

JonRowe commented Jun 13, 2024

If it helps the legacy ruby builds are all docker containers from https://github.com/rspec/docker-ci

@nevinera
Copy link
Contributor Author

If it helps the legacy ruby builds are all docker containers from https://github.com/rspec/docker-ci

Yeah, I stalled out on installing jruby locally and decided I'd need to work out how to dev in a container. I will get back to this, I'm just slammed at work currently -.-

@pirj
Copy link
Member

pirj commented Jun 15, 2024

I wonder if Tempfile.new("foo").is_a?(File) is even true in JRuby

https://github.com/jruby/jruby/blob/02ad2dd02dabac041e6d0d680b0b41459ce2b72f/lib/ruby/stdlib/tempfile.rb#L84

Is it in MRI?

@JonRowe
Copy link
Member

JonRowe commented Jun 15, 2024

My read of that is that JRuby returns a file from those methods so should respond true

@nevinera
Copy link
Contributor Author

nevinera commented Jun 30, 2024

Hm. I finally found time/energy to dig back into this and got jruby set up locally (I'm on an m3 now!), but.. the test passes on jruby-9.1.17.0 locally for me.

~/src/rspec-support nev--override-stderr-splitter-clone
❯ bundle exec rspec ./spec/rspec/support/spec/stderr_splitter_spec.rb:104
Run options: include {:locations=>{"./spec/rspec/support/spec/stderr_splitter_spec.rb"=>[104]}}

Randomized with seed 53186

RSpec::Support::StdErrSplitter
  is able to restore the stream from a cloned StdErrSplitter

Finished in 0.00311 seconds (files took 0.04087 seconds to load)
1 example, 0 failures

Randomized with seed 53186


~/src/rspec-support nev--override-stderr-splitter-clone
❯ bundle exec ruby --version
jruby 9.1.17.0 (2.3.3) 2018-04-20 d8b1ff9 OpenJDK 64-Bit Server VM 22.0.1 on 22.0.1 +jit [darwin-aarch64]

I definitely assumed this was a general jruby problem; any obvious special-ness about jruby-in-ci?

I do get two other failures locally when I run the full suite:

  1) RSpec::Support::ShellOut shells out and returns stdout and stderr
     Failure/Error: DEFAULT_FAILURE_NOTIFIER = lambda { |failure, _opts| raise failure }

       expected: "yes"
            got: ""

       (compared using ==)
     # ./spec/rspec/support/spec/shell_out_spec.rb:8:in `block (2 levels) in <top (required)>'

  2) RSpec::Support::ShellOut returns the exit status as the third argument
     Failure/Error: DEFAULT_FAILURE_NOTIFIER = lambda { |failure, _opts| raise failure }

       expected: 0
            got: 1

       (compared using ==)
     # ./spec/rspec/support/spec/shell_out_spec.rb:14:in `block (2 levels) in <top (required)>'

Finished in 2.13 seconds (files took 0.09453 seconds to load)
568 examples, 2 failures, 2 pending

I don't think it's related though - that seems to be a problem with my jruby setup (it's failing to run ruby from inside the house, and the output contains a message like LoadError: no such file to load -- bundler/setup. I may poke at that some more, but I doubt it's pertinent.

(Setting export JRUBY_OPTS="--dev --server -Xcompile.invokedynamic=false" doesn't appear to affect either situation).

@nevinera
Copy link
Contributor Author

nevinera commented Jun 30, 2024

I wonder if Tempfile.new("foo").is_a?(File) is even true in JRuby

https://github.com/jruby/jruby/blob/02ad2dd02dabac041e6d0d680b0b41459ce2b72f/lib/ruby/stdlib/tempfile.rb#L84

Is it in MRI?

Well.. so no; Tempfile.new("foo").is_a?(File) returns false in both jruby and mri. But $stderr.reopen(Tempfile.new("foo")); $stderr.is_a?(File) returns true in both for me locally, and appears not to do so in CI on jrubies. :-(

MRI

3.2.2 :001 > Tempfile.new("bar").is_a?(File)
 => false
3.2.2 :002 > $stderr.reopen(Tempfile.new("foo")); $stderr.is_a?(File)
 => true
3.2.2 :003 > RUBY_VERSION
 => "3.2.2"

JRuby

 :001 > Tempfile.new("bar").is_a?(File)
 => false
 :002 > $stderr.reopen(Tempfile.new("foo")); $stderr.is_a?(File)
 => true
 :003 > RUBY_VERSION
 => "2.6.10"

It's a bit misleading, because Tempfile delegates most of its methods (including its console representations) to the File it wraps.

I found an interesting thread discussing this particular method. Apparently it's the only case where an object's class and singleton class can be dynamically changed.

@nevinera nevinera force-pushed the nev--override-stderr-splitter-clone branch from ceb2c96 to da06053 Compare June 30, 2024 03:45
@pirj
Copy link
Member

pirj commented Jun 30, 2024

I commented on the wrong PR with my findings.

@pirj
Copy link
Member

pirj commented Jun 30, 2024

On my Linux x86-64 machine.

JRuby 9.1.17.0:

jruby-9.1.17.0 :003 > Tempfile.new("bar").is_a?(File)
 => true

but:

jruby-9.1.17.0 :004 > Tempfile.new("bar") === File
 => false

Same for OpenJDK 22.0.1.u0-1 and OpenJDK 8.412.u08-1.

Same on JRuby 9.2.14.

For some reason, after the update there are less JRuby versions (no 9.3, no 9.4) to experiment with in RVM.
jruby-head:

> RUBY_DESCRIPTION
=> "jruby 9.4.8.0-SNAPSHOT (3.1.4) 2024-06-30 6655bb60d8 OpenJDK 64-Bit Server VM 22 on 22 +jit [x86_64-linux]"
> Tempfile.new('bar').is_a?(File)
=> true

At a glance, this seems to be a MacOS vs Linux thing.

Unless you'll succeed with replacing is_a? with a ===, I suggest marking the spec as failing on JRuby. But not as we usually do with pending, but rather with skip to avoid failures (due to the passing spec) on MacOS:

            if RSpec::Support::Ruby.jruby?
              skip "Failing on JRuby on Linux"
            end

@headius just a heads-up.

@nevinera
Copy link
Contributor Author

-.- of course, osx. So we might be looking at a jruby bug then? I wouldn't expect a behavioral difference here, but it does seem like an edge that might have easily been missed; I saw several other bug reports about jruby and IO#reopen..

I'm not really happy leaving that behavioral difference in place in jruby+Linux, though I guess it's a fairly niche feature. I will see if I can come up with any other way to detect the state first.

@headius
Copy link

headius commented Jul 1, 2024

JRuby has since 2009 implemented Tempfile natively as a subclass of File, because it made sense to us to do so and because at the time the tempfile.rb library was poorly designed. Generally you should see this reflected in is_a?:

$ jruby -v -rtempfile -e 'p Tempfile.new("foo").is_a?(File)'
jruby 9.4.8.0-SNAPSHOT (3.1.4) 2024-06-28 6655bb60d8 OpenJDK 64-Bit Server VM 17.0.5+8 on 17.0.5+8 +jit [arm64-darwin]
true

The tempfile.rb version has improved over the years, and as of JRuby 10 (next major, due later this year) we will switch to using the same library for JRuby's Tempfile. This version works as in CRuby, with Tempfile delegating to a wrapped File:

$ jruby -v -rtempfile -e 'p Tempfile.new("foo").is_a?(File)'
jruby 10.0.0.0-SNAPSHOT (3.4.0) 2024-07-01 55b7eb378c OpenJDK 64-Bit Server VM 17.0.5+8 on 17.0.5+8 +jit [arm64-darwin]
false

If you are on a JRuby version prior to 10, I believe it is possible to activate and use the tempfile.rb library from the gem, which then makes it behavior like CRuby. This may be the source of some confusion:

$ jruby -v -rtempfile -e 'p $".grep(/tempfile/)'
jruby 9.4.8.0-SNAPSHOT (3.1.4) 2024-07-01 5c7b6173d6 OpenJDK 64-Bit Server VM 17.0.5+8 on 17.0.5+8 +jit [arm64-darwin]
["/Users/headius/work/jruby/lib/ruby/stdlib/tempfile.rb"]
$ gem install tempfile
Fetching tempfile-0.2.1.gem
Successfully installed tempfile-0.2.1
Parsing documentation for tempfile-0.2.1
Installing ri documentation for tempfile-0.2.1
Done installing documentation for tempfile after 0 seconds
1 gem installed
$ jruby -v -e 'gem "tempfile"; require "tempfile"; p $".grep(/tempfile/); p Tempfile.new("foo").is_a?(File)'
jruby 9.4.8.0-SNAPSHOT (3.1.4) 2024-07-01 5c7b6173d6 OpenJDK 64-Bit Server VM 17.0.5+8 on 17.0.5+8 +jit [arm64-darwin]
["/Users/headius/work/jruby/lib/ruby/gems/shared/gems/tempfile-0.2.1/lib/tempfile.rb"]
false

The alternative hierarchy is certainly a difference but I don't know if I'd classify it as a bug per se; both versions of Tempfile should behave the same from an API perspective.

I'm happy to answer any other questions about this!

JRuby historically used its own implementation of Tempfile. This spec relies on a check that fails on JRuby.
Evidently, JRuby starting from version 10 is going to behave similarly to CRuby.
@pirj pirj marked this pull request as ready for review August 18, 2024 11:24
@pirj pirj merged commit 7378952 into rspec:main Aug 18, 2024
30 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants