Test failures - Githubissues

utkarsh2102 commented 4 years ago

Hi,

There are some test failures:

FFF..............

Failures:

  1) TTY::Which#which with extension handles path with executable file C:\Program Files\Git\bin\git
     Failure/Error: expect(Which.which(path_with_exe_file)).to eq(expected_path)

     RuntimeError:
       CRITICAL: RUBYGEMS_ACTIVATION_MONITOR.owned?: before false -> after true
     # ./spec/unit/which_spec.rb:86:in `block (3 levels) in <top (required)>'
     # ------------------
     # --- Caused by: ---
     # NameError:
     #   uninitialized constant RSpec::Support::Differ
     #   ./spec/unit/which_spec.rb:86:in `block (3 levels) in <top (required)>'

  2) TTY::Which#which with extension searches path for executable git
     Failure/Error: expect(Which.which(cmd)).to eq(expected_path)

     NameError:
       uninitialized constant RSpec::Support::Differ
     # ./spec/unit/which_spec.rb:122:in `block (3 levels) in <top (required)>'

  3) TTY::Which#which with extension searches path for executable git.exe
     Failure/Error: expect(Which.which(cmd)).to eq(expected_path)

     NameError:
       uninitialized constant RSpec::Support::Differ
     # ./spec/unit/which_spec.rb:103:in `block (3 levels) in <top (required)>'

Top 2 slowest examples (0.01244 seconds, 36.8% of total time):
  TTY::Which#which with extension handles path with executable file C:\Program Files\Git\bin\git
    0.00952 seconds ./spec/unit/which_spec.rb:72
  TTY::Which#exist? fails to find executable in the path
    0.00292 seconds ./spec/unit/exist_spec.rb:11

Top 2 slowest example groups:
  TTY::Which#which
    0.00319 seconds average (0.02234 seconds / 7 examples) ./spec/unit/which_spec.rb:3
  TTY::Which#exist?
    0.00185 seconds average (0.0037 seconds / 2 examples) ./spec/unit/exist_spec.rb:3

Finished in 0.03384 seconds (files took 0.1605 seconds to load)
17 examples, 3 failures

Failed examples:

rspec ./spec/unit/which_spec.rb:72 # TTY::Which#which with extension handles path with executable file C:\Program Files\Git\bin\git
rspec ./spec/unit/which_spec.rb:107 # TTY::Which#which with extension searches path for executable git
rspec ./spec/unit/which_spec.rb:89 # TTY::Which#which with extension searches path for executable git.exe

Randomized with seed 58345

Any clue about the fix?

piotrmurach commented 4 years ago

Hi Utkarsh,

Thanks for using the gem. Looking at the output, I can see that the failure is not caused by the tty-which gem. The uninitialized constant RSpec::Support::Differ means that you have an unsupported version of RSpec. Try upgrading to at least version 3.

geor-g commented 4 years ago

Hi @piotrmurach,

I'm running into this as well, and don't understand your recommendation: The version of rspec in use is 3.9.0, as shipped in Debian unstable.

Any further idea?

Thanks!

piotrmurach commented 4 years ago

Hi @ge-fa,

To run specs you need to preload all dependencies, the easiest and most common way in Ruby world is to use bundler.

So using bundler and rake run the following:

bundle exec rake spec

Alternatively, you can skip using rake and run rspec directly on the test directory:

bundle exec rspec spec/

geor-g commented 4 years ago

Thanks, I'm aware of the workings of bundler. This issue is specifically about Debian, and bundler isn't used to package stuff. I think it's related to the recent switch in Debian making Ruby 2.7 the default, however, I'm unsure about the specific reason.

Note: This issue is not only related to this lib, it poped up elsewhere as well, see for example this issue.

piotrmurach commented 4 years ago

I run the test suite on Ruby 2.6.5 using only rspec executable and all the tests pass fine. On Ruby 2.7.0 running the same tests fails as demonstrated in the bug report. When I dug deeper into one of the tests I could see that mocking a Ruby core class in RSpec:

allow(::File).to receive(:join).with(...).and_return(...)

fails due to arguments to with being actual the current test file location and rspec matcher object which is completely wrong and totally weird. The expected arguments are never received and hence the spec fails. I could replace the test with

allow(::File).to receive(:join).and_return(...)

This makes the test pass. But what is the point of a test that doesn't test the implementation? I feel as though here we're working to fix how rspec works on Ruby 2.7.0 and 2.7.1. Again, running via bundler loads files correctly and hence makes all the tests pass.

When you look closely at the error message:

RuntimeError:
       CRITICAL: RUBYGEMS_ACTIVATION_MONITOR.owned?: before false -> after true

You can see that's something to do with Rubygems threading issues and thus failing to load files correctly on Ruby 2.7.x. Hence the RSpec::Support::Differ missing - which indicates that rspec-support is not loaded correctly.

Bundler is a default gem packaged with Ruby. This issue doesn't affect how the test suite works in my current workflow that uses bundler. The tests are here to support releases. I'm in two minds about changing tests and making them less useful to work around a bug in Ruby 2.7 and Rubygems integration.

geor-g commented 4 years ago

Thanks for your follow-up and your findings, I appreciate this a lot. I'll try to find the cause and see what we're able to do in Debian about this.

piotrmurach commented 4 years ago

Thanks for making me dig a little bit deeper into this problem. I'm not necessarily sure what the core problem is here but everything seems to point to some issue with how rubygems loads dependencies in Ruby 2.7.x. I checked some of my other tty projects by running rspec without the bundler against an entire test suite and they don't 'suffer' from this issue. I'd be curious to know more if you manage to find the cause. Maybe there is something in tty-which setup that triggers this issue. It may be a good port of call to report this on the rubygems project to see if they have some ideas.

geor-g commented 4 years ago

ACK, thanks, I'll let you know, in case I'm able to find anything valuable.

utkarsh2102 commented 4 years ago

(forwarded this issue to the rubygems folk!) :smile:

Also, similar issues:

geor-g commented 4 years ago

@utkarsh2102 Could you provide the link to the rubygems issue?

piotrmurach commented 4 years ago

@utkarsh2102 The chefspec issue is also dealing with mocking a core Ruby File class. The allow(File).to receive(...).with(...) fails in this instance as well. The mocking somehow 'leaks out' from the test and mocks File class inside RSpec leading to failure.

utkarsh2102 commented 4 years ago

@ge-fa, I've forwarded this issue link to David, he'll take a look. And I'll write back as soon as I hear from him :)

geor-g commented 4 years ago

@utkarsh2102 Thanks!

deivid-rodriguez commented 4 years ago

Hello!! :wave: :wave:

This is quite convoluted :laughing:.

So, this is the backtrace I get printed together with the "CRITICAL: RUBYGEMS_ACTIVATION_MONITOR.owned?: before false -> after true" error:

#<Thread:0x0000561f76d66de8 run>
uninitialized constant RSpec::Support::Differ
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/error_generator.rb:305:in `differ'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/error_generator.rb:289:in `diff_message'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/error_generator.rb:275:in `error_message'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/error_generator.rb:62:in `raise_missing_default_stub_error'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/proxy.rb:225:in `raise_missing_default_stub_error'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/proxy.rb:210:in `message_received'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/proxy.rb:345:in `message_received'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/method_double.rb:77:in `proxy_method_invoked'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/verifying_proxy.rb:161:in `proxy_method_invoked'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/method_double.rb:64:in `block (2 levels) in define_proxy_method'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_require.rb:59:in `block (2 levels) in require'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_require.rb:57:in `each'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_require.rb:57:in `block in require'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_require.rb:46:in `each'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_require.rb:46:in `require'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/error_generator.rb:305:in `differ'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/error_generator.rb:289:in `diff_message'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/error_generator.rb:275:in `error_message'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/error_generator.rb:62:in `raise_missing_default_stub_error'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/proxy.rb:225:in `raise_missing_default_stub_error'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/proxy.rb:210:in `message_received'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/proxy.rb:345:in `message_received'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/method_double.rb:77:in `proxy_method_invoked'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/verifying_proxy.rb:161:in `proxy_method_invoked'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/method_double.rb:64:in `block (2 levels) in define_proxy_method'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_require.rb:59:in `block (2 levels) in require'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_require.rb:57:in `each'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_require.rb:57:in `block in require'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_require.rb:46:in `each'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_require.rb:46:in `require'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-expectations-3.9.2/lib/rspec/matchers.rb:573:in `eql'
/home/deivid/Code/tty-which/spec/unit/executable_file_spec.rb:17:in `block (2 levels) in <top (required)>'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/example.rb:257:in `instance_exec'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/example.rb:257:in `block in run'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/example.rb:503:in `block in with_around_and_singleton_context_hooks'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/example.rb:460:in `block in with_around_example_hooks'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/hooks.rb:481:in `block in run'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/hooks.rb:619:in `run_around_example_hooks_for'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/hooks.rb:481:in `run'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/example.rb:460:in `with_around_example_hooks'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/example.rb:503:in `with_around_and_singleton_context_hooks'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/example.rb:254:in `run'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/example_group.rb:644:in `block in run_examples'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/example_group.rb:640:in `map'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/example_group.rb:640:in `run_examples'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/example_group.rb:606:in `run'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/runner.rb:121:in `block (3 levels) in run_specs'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/runner.rb:121:in `map'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/runner.rb:121:in `block (2 levels) in run_specs'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/configuration.rb:2058:in `with_suite_hooks'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/runner.rb:116:in `block in run_specs'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/reporter.rb:74:in `report'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/runner.rb:115:in `run_specs'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/runner.rb:89:in `run'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/runner.rb:71:in `run'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib/rspec/core/runner.rb:45:in `invoke'
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/exe/rspec:4:in `<top (required)>'
/home/deivid/.rbenv/versions/2.7.1/bin/rspec:23:in `load'
/home/deivid/.rbenv/versions/2.7.1/bin/rspec:23:in `<main>'

If we observe the backtrace bottom-up, we see the following stuff:

First important line is

/home/deivid/Code/tty-which/spec/unit/executable_file_spec.rb:17:in `block (2 levels) in <top (required)>

This is the line inside specs were the error happens.
Next two lines also give us useful information:

/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-expectations-3.9.2/lib/rspec/matchers.rb:573:in `eql'" /home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_require.rb:46:in `require'

This indicates that we immediately go into rubygems internal require code, because RSpec matchers are autoloaded (lazily required). While doing its thing, rubygems ends up calling File.file?, causing the problem. This explains why bundler does not suffer from the issue, since it does not monkeypatch require, and thus it doesn't end up calling File.file?.
When File.file? gets called, rspec mocking code gets run, and a few lines above we can see something interesting:

/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib/rspec/mocks/proxy.rb:225:in `raise_missing_default_stub_error'

RSpec is detecting that File.file? is being used with a value different to the one being mocked, and it's about to give an error that reads like this:
```
#<File (class)> received :file? with unexpected arguments
expected: ("ruby")
got: ("/home/deivid/Code/tty-which/spec/rspec/matchers/built_in/eql")
Please stub a default value first if message might be received with other args as well. 
```
This shows a potential resolution for this issue, probably the one that makes more sense: allow(File).to receive(:exist?).and_call_original, as suggested in https://github.com/chefspec/chefspec/issues/766#issuecomment-396631456.
But the error does not end up being displayed (to see it I manually added RSpec::Support.require_rspec_support "differ" on top of the error_generator.rb file). The problem is that the constant RSpec::Support::Differ, needed to show the error is also autoloaded, so we go to rubygems require again, call File.file? again, and so on. Rubygems detects this situation and fails.

So, maybe there's an issue in rspec, because it may be argued that RSpec should "undo" any mockings temporarily to run its own code (or any dependencies like rubygems in this case).

But probably the easiest way out is to use allow(File).to receive(:exist?).and_call_original before mocking calls with specific arguments?

The other failures should be similar.

Hope it helps!

piotrmurach commented 4 years ago

@deivid-rodriguez Thank you for the quick reply and your investigation!

I have reservations about concluding that rubygems is not at fault here. I'm not saying it is but I also cannot see evidence that it isn't. I think this is a bug indicative of some significant regression someplace and it may be that it is manifested in RSpec failure but the root cause is someplace else.

Let's look at the provided example snippet and the comment:

RSpec is detecting that File.file? is being used with a value different to the one being mocked, and it's about to give an error that reads like this:

#<File (class)> received :file? with unexpected arguments
expected: ("ruby")
got: ("/home/deivid/Code/tty-which/spec/rspec/matchers/built_in/eql")
Please stub a default value first if message might be received with other args as well.

The method definition is as simple as delegating passed value:

def executable_file?(filename)
  ::File.file?(filename) && ::File.executable?(filename)
end

and the corresponding test looks like this:

allow(::File).to receive(:file?).with("ruby").and_return(true)
allow(::File).to receive(:executable?).with("ruby").and_return(true)

expect(TTY::Which.executable_file?("ruby")).to eql(true)

Can you please explain to me how ""/home/deivid/Code/tty-which/spec/rspec/matchers/built_in/eql"" ends up being a value passed into the ::File.file?? This doesn't make much sense. Where is this value from? Why RSpec matcher eql is used as an argument?

Using theallow(File).to receive(:exist?).and_call_original is not a solution. It doesn't solve the problem but 'papers over' the bug. This is equivalent to mocking any exist? call with any arguments like so:

allow(File).to receive(:exist?)

Which matches the automated suggestion from the failure "Please stub a default value first if message might be received with other args as well. " But you shouldn't have to do it. The below passes:

# allow(::File).to receive(:file?).with("ruby").and_return(true)
# replaced with
allow(::File).to receive(:file?).with(any_args).and_return(true)
allow(::File).to receive(:executable?).with("ruby").and_return(true)

expect(TTY::Which.executable_file?("ruby")).to eql(true)

But we haven't solved the problem and made the test 'weaker' as it no longer tests that filename argument is passed correctly.

My other question is that, if this is rspec issue why does it work fine in other Rubies apart from Ruby 2.7.x? If this was autoloading bug I should've come across is much earlier since I've been using the allow(File).to receive(:file?).with(...).and_return(...) for many years without any issues?

Hi @JonRowe - do you have any experience with the above? Does anything jump out for you as the cause of the issue? It's been suggested that rspec and its autoloading of dependencies may be a problem here.

deivid-rodriguez commented 4 years ago

Can you please explain to me how ""/home/deivid/Code/tty-which/spec/rspec/matchers/built_in/eql"" ends up being a value passed into the ::File.file?? This doesn't make much sense. Where is this value from? Why RSpec matcher eql is used as an argument at all?

It's rubygems making that call, it's not your code. Rubygems code gets run because RSpec matchers are autoload'ed, and that runs Kernel.require, which is monkeypatched by rubygems, and that runs a bunch of ruby code, including some File.file? calls.

deivid-rodriguez commented 4 years ago

I honestly think there's no good solution here. Even if rubygems wouldn't monkeypatch require, end users mocking stdlib methods that RSpec uses internally would lead to a similar thing.

deivid-rodriguez commented 4 years ago

My other question is that, if this is rspec issue why does it work fine in other Rubies apart from Ruby 2.7.x? If this was autoloading bug I should've come across is much earlier since I've been using the allow(File).to receive(:file?).with(...).and_return(...) for many years without any issues?

I'm not saying that this is an RSpec issue, or that you should change your code, I'm just trying to explain the situation. The problem is actually not dependent on ruby 2.7, but on the rubygems version that you are running (which is higher on ruby 2.7). It also depends on the different gems you have installed on your system, since that triggers different code paths in rubygems that might or might not call File.file?. For example, using rubygems master version, and cleaning up my installed gems, the bug doesn't trigger for me anymore.

JonRowe commented 4 years ago

Hi @JonRowe - do you have any experience with the above? Does anything jump out for you as the cause of the issue? It's been suggested that rspec and its autoloading of dependencies may be a problem here.

If there are different versions of files being loaded its because the host system has more than one version of them installed and bundler is not being used, autoload and require being core Ruby methods (with or without rubygems) they depend on your path, so when you are not using bundler you are responsible for ensuring you have the correct version of gems in your path for your code.

Bundler solves many of these problems by manipulating the path to be correct for the current installed versions of dependencies. Without it you must do this manually. No other package manager is currently aware of Ruby dependencies to my knowledge.

deivid-rodriguez commented 4 years ago

Just to clarify my suggestion, I meant to add

allow(::File).to receive(:file?).and_call_original

on top of the other mocks, not instead of them, so in case other code external to your library, be it RSpec or rubygems, ends up calling File.file?, the original implementation gets called.

And I also meant it as a patch to be added by Debian to workaround the unfortunate situation, not as something to be changed upstream. The current implementation of the test looks just fine to me.

So, maybe there's an issue in rspec, because it may be argued that RSpec should "undo" any mockings temporarily to run its own code (or any dependencies like rubygems in this case).

Also, this was just a quick thought, but it sounds like something quite hard to implement, for little benefit.

If there are different versions of files being loaded its because the host system has more than one version of them installed and bundler is not being used, autoload and require being core Ruby methods (with or without rubygems) they depend on your path, so when you are not using bundler you are responsible for ensuring you have the correct version of gems in your path for your code.

Actually, rubygems require has some basic support for this, that's why you can run require "rspec" in IRB and get the latest version of rspec installed on your system activated, together with its dependencies, without it being in the $LOAD_PATH.

irb(main):001:0> puts $LOAD_PATH
/home/deivid/.rbenv/rbenv.d/exec/gem-rehash
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/x86_64-linux
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/vendor_ruby/2.7.0
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/vendor_ruby/2.7.0/x86_64-linux
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/vendor_ruby
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/2.7.0
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/2.7.0/x86_64-linux
=> nil
irb(main):002:0> require 'rspec'
=> true
irb(main):003:0> puts $LOAD_PATH
/home/deivid/.rbenv/rbenv.d/exec/gem-rehash
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/diff-lcs-1.4.4/lib
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-mocks-3.9.1/lib
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-3.9.0/lib
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-support-3.9.3/lib
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rspec-core-3.9.2/lib
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby/2.7.0/x86_64-linux
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/site_ruby
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/vendor_ruby/2.7.0
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/vendor_ruby/2.7.0/x86_64-linux
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/vendor_ruby
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/2.7.0
/home/deivid/.rbenv/versions/2.7.1/lib/ruby/2.7.0/x86_64-linux
=> nil

But unfortunately, it uses File.file? in several places to implement this.

piotrmurach commented 4 years ago

@deivid-rodriguez Thanks a lot for explaining this. It makes a lot more sense now. I see also why you're suggesting to use: allow(::File).to receive(:file?).and_call_original.

@JonRowe Would it make sense and is it even possible for rspec to limit its mock checks on things like allow(File).to receive(:method).with(...).and_return(...) to only file paths that relate to the namespace currently tested?

piotrmurach commented 4 years ago

@deivid-rodriguez I read your first comment with all the backtraces in more detail and they make perfect sense now. It seems like mocking File.join and File.file? are best to be done cautiously in the future as they are used here: https://github.com/rubygems/rubygems/blob/master/lib/rubygems/core_ext/kernel_require.rb#L59-L60

It seems like rspec could/should call allow(File).to receive(:method).and_call_original by default when allow(File).to receive(:method).with(...).and_return(...) is invoked. That's probably the easiest and surest way to prevent this from happening again? I will add the and_call_original to the tests to ensure this issue doesn't come back again.

deivid-rodriguez commented 4 years ago

@deivid-rodriguez I read your first comment with all the backtraces in more detail and they make perfect sense now. It seems like mocking File.join and File.file? are best to be done cautiously in the future as they are used here: https://github.com/rubygems/rubygems/blob/master/lib/rubygems/core_ext/kernel_require.rb#L59-L60

Great! This should be a rare case, since it needs specs not be run with bundler, which most people do. But the number of linked tickets suprised me...

It seems like rspec could/should call allow(File).to receive(:method).and_call_original by default when allow(File).to receive(:method).with(...).and_return(...) is invoked. That's probably the easiest and surest way to prevent this from happening again?

But then users would miss actual unintended calls with other arguments to File.file?, right? I think RSpec is ok here. The only thing I believe RSpec could do is check whether the immediate caller location of a method double is a file inside RbConfig::CONFIG["rubylibdir"], RbConfig::CONFIG["rubysitedir"] or another "system file", and in that case use the original implementation. But I understand if they don't want to do it.

JonRowe commented 4 years ago

Actually, rubygems require has some basic support for this, that's why you can run require "rspec" in IRB and get the latest version of rspec installed on your system activated, together with its dependencies, without it being in the $LOAD_PATH.

Rubygems also manipulates your path... its also intertwined with bundler these days. There are still issues if it is not limited to the current Gemfile's dependencies. Using gems from linux package managers is known to be generally problematic.

Would it make sense and is it even possible for rspec to limit its mock checks on things like allow(File).to receive(:method).with(...).and_return(...) to only file paths that relate to the namespace currently tested?

No it is not possible, we don't know wether the code is intentionally called or unintentionally called. Stubs only exist for the lifecycle of your test, they are cleaned up automatically so they are directly or indirectly invoked by you.

It seems like rspec could/should call allow(File).to receive(:method).and_call_original by default when allow(File).to receive(:method).with(...).and_return(...) is invoked. That's probably the easiest and surest way to prevent this from happening again? I will add the and_call_original to the tests to ensure this issue doesn't come back again.

If you stub a core method you are responsible for restoring any behaviour you depend upon elsewhere in your tests. This can be as simple as:

allow(File).to receive(:method).and_call_original
expect(File).to receive(:method).and_return(value)

It is an impossible task to ask RSpec to special case all of the core library to ensure some special behaviour.

The only thing I believe RSpec could do is check whether the immediate caller location of a method double is a file inside RbConfig::CONFIG["rubylibdir"], RbConfig::CONFIG["rubysitedir"] or another "system file", and in that case use the original implementation. But I understand if they don't want to do it.

This is an interesting idea but in practise I don't think it would work, we only know wether code is in the current working directory, or not, so much core Ruby behaviour is now gem based we wouldn't be able to distinguish between core Ruby libraries and other gems, and most people stub gems not Ruby features.

deivid-rodriguez commented 4 years ago

This is an interesting idea but in practise I don't think it would work, we only know wether code is in the current working directory, or not, so much core Ruby behaviour is now gem based we wouldn't be able to distinguish between core Ruby libraries and other gems, and most people stub gems not Ruby features.

Yeah, even if the first caller_location was specially guarded to be a rubygems file, that would break people intentionally stubbing stuff in rubygems for some reason. And feels not right anyways.

I don't think there's anything actionable here other than people adding their workaround of choice like allow(File).to receive(:method).and_call_original, or eagerly loading the RSpec parts that need to be early loaded. Or if @piotrmurach wants to support running specs in this repo without Bundler, then he can also add allow(File).to receive(:method).and_call_original to the specs giving trouble.

deivid-rodriguez commented 4 years ago

Rubygems also manipulates your path... its also intertwined with bundler these days.

I don't think rubygems changes your PATH in any way, I'm not even sure if it uses it at all (definitely not for a lot of things). It's your ruby version manager making sure rspec is found at the proper path. And if running rspec from a Debian-installed gem, I assume it's already in your PATH. Not sure how it relates to this issue, but I think you're trying to say that you don't want to change anything in RSpec to support this case. That's fine to me :+1:.

There are still issues if it is not limited to the current Gemfile's dependencies. Using gems from linux package managers is known to be generally problematic.

Indeed, but it's getting better. The Debian ruby team has packaged hundreds of gems which are used by people. And they seem to work :laughing:. I think it's a great way to make general purpose ruby tools more widely available and I really appreciate the work they do :heart:.

JonRowe commented 4 years ago

I don't think rubygems changes your PATH in any way, I'm not even sure if it uses it at all (definitely not for a lot of things). It's your ruby version manager making sure rspec is found at the proper path. And if running rspec from a Debian-installed gem, I assume it's already in your PATH.

It's not just your ruby version manager doing it, its the entire system, at the end of the day the load path has usually been touched by several things and has an order. The first file matching your require in the load path will be the one that is loaded.

Not sure how it relates to this issue,

I was attempting (badly it seems) to explain why File was seeing different paths like /home/deivid/Code/tty-which/spec/rspec/matchers/built_in/eql being checked, its Ruby searching the load path for the file.

deivid-rodriguez commented 4 years ago

Sorry, I understand that you said "path" where you meant "load path". In that case, sure, I already mentioned this in previous comments.

deivid-rodriguez commented 4 years ago

I was attempting (badly it seems) to explain why File was seeing different paths like /home/deivid/Code/tty-which/spec/rspec/matchers/built_in/eql being checked, its Ruby searching the load path for the file.

I think we all got why this happens at this point :+1:

piotrmurach commented 4 years ago

@deivid-rodriguez I decided to fix the failures to help make Debian releases easier. Thanks again for investigating and explaining the issue in such detail, that was mega useful.

@JonRowe Thanks for your perspective, I ensured that the original version of the mocked call is executed.

deivid-rodriguez commented 4 years ago

That's really kind of you @piotrmurach :purple_heart:!

piotrmurach / tty-which

Test failures #4