-
Notifications
You must be signed in to change notification settings - Fork 256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Build and support on Rails 7.1 #3099
Conversation
c116f80
to
25e5750
Compare
OK getting close -- having one failure in Rails 7.1 CI that does not reproduce locally for me.
Actually I'll see if I can reproduce locally running Can repro locally if we |
…-rails that does not yet include it rspec/rspec-rails@4d65bea rspec/rspec-rails#2696
…7.1 compat implementation I guess we had to implement these custom inline to use in places other than view specs, with the right method path to get to the view_context? We needed to provide an alternate implementation that works for new rspec-rails way of doing this that is compat with Rails 7.1: rspec/rspec-rails@4d65bea
…ite_primary_key? and #has_query_constraints?
25e5750
to
a97dcc5
Compare
OK, so engine_cart (I guess!) puts a generator into the local test app ( I'm not sure why this is happening, why is it putting the generator for a test app into the generated test app? But it confuses zeitwerk, because it's not called
What's weird here is that the generators dir is in our generated app, not in the gem. I am trying to avoid having to get engine_cart to do something different, because that way lies tragedy. So one fix is having the blacklight gem configure the whole local app to tell zeitwerk to ignore the local app's whole While this could conceivably effect someone if they actually wanted to use If someone has a better solution, it's welcome! update: repoted to engine_cart at cbeer/engine_cart#117 |
f96b59e
to
3dd752b
Compare
…app ./lib/generators Not sure why we are generating a generator INTO the local test app. But rather than figure this out and change it, we want to make zeitwerk okay with it. We are telling zeitwerk to ignore the WHOLE RAILS_ROOT/lib/generators directory, just to deal with the one test_app_generator.rb file we are putting there. But hopefully that is okay.
3dd752b
to
e45b762
Compare
gemspec already (mistakenly?) allows Rails 7.1, even though build does not pass on Rails 7.1, including for at least one non-test-harness reason.
This is passing for me locally on Rails 7.1.1, let's see how it does in CI.
There were actually only a couple fairly simple issues (although not always simple to fix!).
The "ActiveModelShim" needed a couple more boilerplate methods in order to pass tests. I don't really understand what's going on here, especially the
has_query_constraints?
method. (and things like primary keys suggest to me that what we're testing isn't really an ActiveModel shim but an ActiveRecord one?). Not sure what this is all used for, but some simple changes make tests green.RSpec-rails does not currently have a
stub_template
method that works in Rails 7.1. There is an unreleased one in rspec-rails main that tries, but suffers from problems with stubs persisting to subsequent tests. I found an implementation that appears to work though, and does let our tests pass.spec_helper.rb
. The patch expires with a subsequent rspec-rails release. Conditional implementation on Rails version, like rspec-rails was trying.stub_template
implementation to work in non-view-specs. I modified these too, each still inline one-off, to use new implementation that works.Was getting a deprecation warning on
serialize :query_params, Blacklight::SearchParamsYamlCoder
telling me it should beserialize :query_params, coder: Blacklight::SearchParamsYamlCoder
, so went ahead and fixed that with a conditional for Rails version too.