Skip to content
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

Merged
merged 6 commits into from
Nov 8, 2023
Merged

Build and support on Rails 7.1 #3099

merged 6 commits into from
Nov 8, 2023

Conversation

jrochkind
Copy link
Member

@jrochkind jrochkind commented Nov 7, 2023

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!).

  1. 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.

  2. 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.

    • I patched that implementation in to rspec-rails, in our local spec_helper.rb. The patch expires with a subsequent rspec-rails release. Conditional implementation on Rails version, like rspec-rails was trying.
      • I think it could be a while until rspec-rails has a release, and is acceptable to use a local patch here, that only effects test harness and not production code, in order to get a blacklight release that supports rails 7.1.
    • The codebase has three other places where we copy-paste-modify rspec-rails stub_template implementation to work in non-view-specs. I modified these too, each still inline one-off, to use new implementation that works.
  3. Was getting a deprecation warning on serialize :query_params, Blacklight::SearchParamsYamlCoder telling me it should be serialize :query_params, coder: Blacklight::SearchParamsYamlCoder, so went ahead and fixed that with a conditional for Rails version too.

@jrochkind
Copy link
Member Author

jrochkind commented Nov 7, 2023

OK getting close -- having one failure in Rails 7.1 CI that does not reproduce locally for me.

Done generating test app
/opt/hostedtoolcache/Ruby/3.2.2/x64/bin/ruby -I/opt/hostedtoolcache/Ruby/3.2.2/x64/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/lib:/opt/hostedtoolcache/Ruby/3.2.2/x64/lib/ruby/gems/3.2.0/gems/rspec-support-3.12.1/lib /opt/hostedtoolcache/Ruby/3.2.2/x64/lib/ruby/gems/3.2.0/gems/rspec-core-3.12.2/exe/rspec --pattern spec/\*\*/\*_spec.rb

An error occurred while loading spec_helper.
Failure/Error: EngineCart.load_application!

Zeitwerk::NameError:
  expected file /home/runner/work/blacklight/blacklight/.internal_test_app/lib/generators/test_app_generator.rb to define constant Generators::TestAppGenerator, but didn't
# ./.internal_test_app/config/environment.rb:5:in `<top (required)>'
# ./spec/spec_helper.rb:15:in `<top (required)>'
No examples found.

Actually I'll see if I can reproduce locally running rake all-in-one instead of creating engine cart separately and then running tests as I've done. Then I'll see if I can fix it.

Can repro locally if we export CI=true to turn on eager loading in tests. It is of course a zeitwerk issue. Working on it.

…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?
@jrochkind
Copy link
Member Author

jrochkind commented Nov 7, 2023

OK, so engine_cart (I guess!) puts a generator into the local test app (.internal_test_app), ./lib/generators. test_app_generator.rb, TestAppGenerator

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 Generators::TestAppGenerator. The zeitwerk docs do talk about this lib/generators use case in fact, although when the lib is in a gem, it's not expecting you to put it into an app like we do.

For example, if the gem has Rails generators under lib/generators, by convention that directory defines a Generators Ruby module. If generators is just a container for non-autoloadable code and templates, not acting as a project namespace, you need to setup things accordingly.

If the warning is legit, just tell the loader to ignore the offending file or directory:

loader.ignore("#{__dir__}/generators")

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 ./lib/generators folder.

While this could conceivably effect someone if they actually wanted to use ./lib/generators for something namespaced Generators? This seems unlikely? And maybe would conflict with Blacklight already?

If someone has a better solution, it's welcome!

update: repoted to engine_cart at cbeer/engine_cart#117

@jrochkind jrochkind force-pushed the rails_7.1_jrochkind branch 2 times, most recently from f96b59e to 3dd752b Compare November 7, 2023 18:14
@jrochkind jrochkind requested review from jcoyne and removed request for jcoyne November 7, 2023 19:47
…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.
@tpendragon tpendragon merged commit 92a0da7 into main Nov 8, 2023
15 checks passed
@tpendragon tpendragon deleted the rails_7.1_jrochkind branch November 8, 2023 22:00
@jcoyne jcoyne mentioned this pull request Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants