rubyatscale / packs-rails

packs-rails establishes and implements a set of conventions for splitting up large monoliths.
MIT License
268 stars 26 forks source link

RSpec integration not working as expected #15

Closed iMacTia closed 2 years ago

iMacTia commented 2 years ago

I've currently setup packwerk and stimpack in my application and my expectation around RSpec was that when running rspec or bundle exec rspec then all tests contained in the standard spec/ folder and all tests in packs/**/spec/ folders would be executed.

However it seems like at the moment only tests in the global folder are being picked up and executed, the packs tests are being ignored. I'm using the new Stimpack.config.root setting but I don't think that has anything to do with this.

While debugging, I've managed to print out the value of RSpec.configuration.pattern after the Stimpack integration was loaded, and I got the following:

"**{,/*/**}/*_spec.rb,components/comp1/spec/**/*_spec.rb,components/comp2/spec/**/*_spec.rb,components/comp3/spec/**/*_spec.rb"

so I can see my components there, but the tests are still not included in the run. Is my expectation incorrect, or is this a bug with Stimpack?

For what is worth, I was previously able to make this work in my app before adding Stimpack, and the trick had to do with setting the --default-path option for RSpec. This is because, as far as I understand, RSpec assumes your path starts from spec/ and so all patterns are relative to that path, hence components/... will translate into spec/components... which is not what we want.

ngan commented 2 years ago

Hey @iMacTia -- do you have something in your .rspec, .rspec-local or somewhere else that overrides the pattern?

iMacTia commented 2 years ago

I don't think so, my .rspec file only sets other options which are unrelated from the --pattern one, but that's why I've added my RSpec.configuration.pattern to the issue description. Does that differ from the one you get in your application?

astyagun commented 2 years ago

I can see the same problem in my case. When I add raise '123' to a file at "packs/pack_name/spec/services/service_name_spec.rb" and then run rspec in terminal, I expect this command to fail because of that raise. But all tests start running instead (without those in "packs/*/spec/` directories).

If I change this code: https://github.com/rubyatscale/stimpack/blob/300b4621986e0d3dfce50a0d56ac1be9696d3d6b/lib/stimpack/integrations/rspec.rb#L15

to

          ::RSpec.configuration.pattern.concat(",../#{pack.relative_path.join("spec/**/*_spec.rb")}")

("../" added into the string), then rspec fails expectedly.

ngan commented 2 years ago

@astyagun which directory are you running your rspec command from?

astyagun commented 2 years ago

@ngan From project root directory, which contains spec and packs directories

iMacTia commented 2 years ago

@ngan to solve the impasse, I've just created a dummy rails application with a vanilla installation of stimpack.

If you clone this repository and you run bundle exec rspec, you'll see the component's specs are ignored. However if you run bundle exec rspec --default-path . or use the fix from @astyagun, the component's spec will be executed and you'll get an exception coming from the component's spec file

ngan commented 2 years ago

Thank you!! I’ll take a look later today!

astyagun commented 2 years ago

The problem with setting default path to "." is that pattern has "**{,/*/**}/*_spec.rb" by default. Which means, that spec files would be searched for in the whole root directory recursively and not only "spec" directory. Which seems unnecessary.

So another solution could be to set default path to "." AND change default pattern to "spec/**{,/*/**}/*_spec.rb" ("spec/" prepended). Then "../" prefix for package specs paths could be avoided and the whole pattern would look something like this: "spec/**{,/*/**}/*_spec.rb,packs/package_name1/spec/**/*_spec.rb,packs/package_name2/spec/**/*_spec.rb,...".

oleg-vinted commented 2 years ago

So another solution could be to set default path to "." AND change default pattern to "spec/*{,//*}/_spec.rb"

This didn't work for us: there are a bunch of require calls which are actually relative to /spec. We also have --require spec_helper which breaks with default path set to ..

ngan commented 2 years ago

Hey everyone, I think I know what's going on. We use spring at Gusto and one (quirky, but ok-yeah-that-makes-sense) thing it does is that it loads (and caches) the Rails application before RSpec. This means Rails and stimpack will boot and we will get to install our RSpec hook before RSpec has a chance to do its setup.

I think to help make everything work consistently, I'll be moving the rspec integration out to its own file so that you can make it an RSpec require: --require stimpack/rspec and it will do its thing.

astyagun commented 2 years ago

@ngan Does this mean, that using Spring should fix the problem? Because Spring is used in my case, but I still see the problem.

Also, I'm wondering, why adding "../" prefix for package specs paths helps in my case?

ngan commented 2 years ago

Actually, it wasn't because of Spring. Turns out we had --require spec_helper in our .rspec file. This caused spec/spec_helper.rb to boot before files are resolved by RSpec. This allowed us to load Stimpack with a simple require 'stimpack'. As a workaround, you can do the same.

That said, I've just put up a PR to refactor lots of Stimpack and improve RSpec support: https://github.com/rubyatscale/stimpack/pull/28

After that lands, you'll simply add --require stimpack/rspec to your .rspec or on your CLI run (bundle exec rspec --require stimpack/rspec)

astyagun commented 2 years ago

@ngan I've tried the workaround of requiring stimpack before anything else, this is what I have in .rspec file:

--color
--require stimpack
--require rails_helper

Unfortunately, this hasn't helped in my case. Maybe I'm missing something... 🤔

Oh the other hand @ngan, I wonder, what value do you have in RSpec.configuration.default_path?

alexevanczuk commented 2 years ago

@astyagun @iMacTia @oleg-vinted Can you all confirm your issues have been fixed with 0.7.0?

oleg-vinted commented 2 years ago

Seems to work in my limited testing so far. 👍

astyagun commented 2 years ago

Works for me now, thanks 🙏

iMacTia commented 2 years ago

I'll test this out once I'm back from my holiday, but the feedback is really positive so far! Thank you @AlexEvanczuk and @ngan for fixing this 🙏 !

iMacTia commented 2 years ago

I finally had a chance to test this, and adding --require stimpack/rspec to my .rspec raises the following error:

Failure/Error: require File.expand_path('../config/environment', __dir__)

FrozenError:
  can't modify frozen Array: [...]

# /Users/mattiagiuffrida/.rvm/gems/ruby-3.1.2/gems/railties-6.1.6.1/lib/rails/engine.rb:588:in `unshift'
# /Users/mattiagiuffrida/.rvm/gems/ruby-3.1.2/gems/railties-6.1.6.1/lib/rails/engine.rb:588:in `block in <class:Engine>'
# /Users/mattiagiuffrida/.rvm/gems/ruby-3.1.2/gems/railties-6.1.6.1/lib/rails/initializable.rb:32:in `instance_exec'
# /Users/mattiagiuffrida/.rvm/gems/ruby-3.1.2/gems/railties-6.1.6.1/lib/rails/initializable.rb:32:in `run'
# /Users/mattiagiuffrida/.rvm/gems/ruby-3.1.2/gems/railties-6.1.6.1/lib/rails/initializable.rb:61:in `block in run_initializers'
# /Users/mattiagiuffrida/.rvm/gems/ruby-3.1.2/gems/railties-6.1.6.1/lib/rails/initializable.rb:50:in `each'
# /Users/mattiagiuffrida/.rvm/gems/ruby-3.1.2/gems/railties-6.1.6.1/lib/rails/initializable.rb:50:in `tsort_each_child'

It seems as if for some reason the initialisation is running twice or something, I haven't really been able to dig further into it. Hoping @ngan might have seen this error already while working on the PR? I'm running v 0.7.1

iMacTia commented 2 years ago

I spent some extra time investigating this and opened a new issue with my findings.