Add Ruby 3.1 to CI and fix Rails 7 #2552

petergoldstein · 2022-01-09T18:44:59Z

The bulk of the changes in this PR are test changes. These include:

Updating the example app configuration for Rails 7, including disabling eager class loading on CI
Updating specs to filter out additional lines that may be generated when running commands
Removing ".html.erb" and ".xml.erb" suffixes in render calls
Updating specs to accomodate differences in Rails view scaffolding before and after Rails 7

Material code changes include:

Adding additional logic to the ActionMailer argument parsing to accomodate for differences under Ruby 3.1/Rails 6.1
Symbolizing the handler argument parsed from the spec description for view specs with an empty render

With the application of these changes to main all CI entries (including the new Ruby 3.1 entries and existing Rails 7 entries) run green.

.github/workflows/ci.yml

petergoldstein · 2022-01-10T19:52:37Z

@pirj Agreed. I'm trying to work that into a combined branch and deal with the remaining failures.

pirj · 2022-01-10T19:54:58Z

I'll go ahead and merge #2546 as soon as the build is green, so you could rebase and work on feature failures.

pirj · 2022-01-10T20:29:09Z

I've rebased this PR on top of main. ~~And dared to replace '3.1' with just 3.1 in ci.yml.~~ I think the problem is just with 3.0 being interpreted as 3 and later on taken as "latest 3.x" by ruby-setup.

edit: my changes to '3.1' disappeared, but that's fine.

petergoldstein · 2022-01-10T20:32:09Z

3.1 without quotes is fine. Yes, that's the issue for the use of an unquoted 3.0 - it rounds to 3 and gets interpreted as latest 3.

As you noted our force-pushes collided.

pirj · 2022-01-10T20:34:24Z

our force-pushes collided

🤜 🤛

Unfortunately, --force-with-lease doesn't work for contributor's repos (when I push to PRs).

pirj · 2022-01-10T20:43:54Z

Cool, now failures in this PR are indicative of what are the incompatibilities with Rails 7 are, not some random net- issues and RuboCop parser version mismatches 🎉

petergoldstein · 2022-01-10T21:41:59Z

I've got a handle on the first few failures. The default ERB template changed - rails/rails@164c2f6#diff-f00cb6b8a5dac4a72299b7c0fc2962b5fc554a44e5452a8c0a7fb47b97080ad2 - and a table layout is no longer being used, meaning the matchers are failing. Working on updating the specs now.

spec/generators/rspec/scaffold/scaffold_generator_spec.rb

pirj · 2022-01-10T22:23:16Z

lib/generators/rspec/scaffold/templates/index_spec.rb

@@ -18,8 +18,9 @@

  it "renders a list of <%= ns_table_name %>" do
    render
+    cell_selector = Rails::VERSION::STRING >= '7' ? 'div>p' : 'tr>td'


I suggest moving this inside <% %> so it doesn't appear in the resulting spec.

That won't help much. Something needs to appear in the resulting spec, and that something is going to be different for Rails 7.0 and for previous versions of Rails. So either we duplicate the logic in the spec (which seems kind of pointless) or we put it in a single variable here.

I imagine the flow as:

generate a scaffold

upgrade Rails from 6 to 7

I can't imagine a failure if we've hardcoded tr>td. Because both the index_spec and the index have it like that being generated by Rails 6.

But, if we keep this ternary, after Rails 7 upgrade, index_spec will start failing, because index has td inside tr, while the spec would attempt to select div>p.

No, that's exactly backwards.

The scaffold is generated dynamically as part of the spec run. With the version of Rails that is currently active. So for you get:

Rails 6.x - tr>td
Rails 7.x - div>p

Hardcoding either one in the scaffold spec causes the scaffold spec to fail in at least one case.

This is easy to confirm on the results on the current branch. With the current code the scaffold and the various index specs pass across all Ruby/Rails combos. Hardcoding will break it.

scaffold is generated dynamically as part of the spec run

You think RSpec CI, I mean a real app developer's local machine.

RSpec CI would be green both ways. Real app developer's local test run will fail with the way it is now.

Please don't get me wrong, I suggest to:

it "renders a list of <%= ns_table_name %>" do render <% for attribute in output_attributes -%> assert_select <% Rails::VERSION::STRING >= '7' ? 'div>p' : 'tr>td' %>, text: Regexp.new(<%= value_for(attribute) %>.to_s), count: 2 <% end -%> end

On Rails 6, it will generate a spec with lines like:

assert_select 'tr>td', text: Regexp.new(3.to_s), count: 2

I think given the HTML table template Rails 6 used to generate, on developer's machine after update to Rails 7 this code will remain working.

What makes this code problematic?

we've hardcoded

Sorry for being ambiguous.
By we I didn't mean me or you, I meant the template, lib/generators/rspec/scaffold/templates/index_spec.rb.
And by the product that will undergo hardcoding I meant the resulting index_spec.rb spec file on the real project's developer machine, not the template in this git repository.

So the suggested approach doesn't work. It breaks in CI and breaks on a developer's box.

CI - for Rails 7 your approach will work for the generated index spec you reference (because the tr>td will be replaced in the generated spec. But the scaffold spec - spec/generators/rspec/scaffold/scaffold_generator_spec.rb - will fail, because it will have a hard-coded tr>td

Developer's Box - We have the same issue for the scaffold spec if we generate the app at one Rails version and switch to another. If a developer builds the smoke app under Rails 7, then the specs now fail. Why does Rails 6 have a preferential position over Rails 7? What happens if I generate the app on Rails 7 and move to Rails 6.

When a developer changes RAILS_VERSION then they should run bundle exec rake clobber generate:app generate:stuff. That solves the upgrade problem entirely.

c68b568#diff-94efcbcbb995cd8f60c1956dbb889ec2af5999f3aef85e686278dd96b9a3be16R21

A competitive commit coincidentally implementing the approach I suggest.

Why does Rails 6 have a preferential position over Rails 7? What happens if I generate the app on Rails 7 and move to Rails 6.

There is no preferential approach. If you were on Rails 7, generated those specs with index with <p><strong>.
If you move to Rails 6 after, index_spec that will now start asserting that index should have tr>td. But it doesn't, it still has <p><strong>.

There seem to be a massive misunderstanding on this.

There is no misunderstanding. You've implemented a version of the alternative that I described here. Specifically:

So either we duplicate the logic in the spec (which seems kind of pointless) or we put it in a single variable here.

They've duplicated the logic from the index generator in the previously static scaffold spec. Note there is no mention of such a change in this comment. As I noted, the issue is we have dynamically generated things (index specs) and one statically generated thing (scaffold spec). Without the modification of the scaffold spec the specs would fail.

To me it seems that duplicating the logic in two places (as is done in the referenced commit) is less ideal, but it's not a hill I want to die on. If there's a consensus that this approach is "better" I'm fine with that. As noted elsewhere, my goal is to get things working.

Keep in mind that regardless of this spec's behavior, switching Rails versions without regenerating the smoke app would count as a bad idea. There's no guarantee that the generated app would i) run at all or ii) run consistently because there may be Rails version dependent changes. My earlier comment about blowing away and regenerating the Rails app when switching Rails versions still holds.

spec/generators/rspec/scaffold/scaffold_generator_spec.rb

petergoldstein · 2022-01-11T01:08:30Z

Ok, the Rails 6.1/Ruby 3.1 spec failures are...interesting. In the Ruby 3.1 case the arguments are being treated as keyword arguments. They look like:

{"args"=>["required"], "_aj_ruby2_keywords"=>["args"]}

If you run the same Rails version (6.1) under Ruby 3.0, the arguments passed to the jobs ar3:

"required"

So basically we need to deconstruct this. I can fix the failing specs pretty easily, but I'm not sure if this indicates a wider issue with ActiveJob arguments in this combination.

pirj · 2022-01-11T06:11:13Z

Rails 6.1/Ruby 3.1 spec failures are...interesting

Indeed. And so is the fact the same spec passed with Rails 7.
Wondering if it's this line that makes it fail.

petergoldstein · 2022-01-11T06:57:44Z

Rails 6.1/Ruby 3.1 spec failures are...interesting

Indeed. And so is the fact the same spec passed with Rails 7. Wondering if it's this line that makes it fail.

I think I've almost got this. I've fixed 2 out of the 3 failures in this case.

And I'm making headway on the remaining Rails 7 issues - I've basically got to get through the verify_mailer_preview_path_spec.rb fixes and that should be that.

pirj · 2022-01-11T08:33:46Z

Fantastic! ⚡ Thank you!

petergoldstein · 2022-01-11T22:29:22Z

A quick update on this PR / Rails 7 issues in general.

I think our issues with Rails 7 fall into a few categories:

Scaffold template changes - at this point well understood, even if there's debate on how to handle
New output lines when running commands - these need to be filtered out, and for the most part that's done at this point. Things like new deprecation warnings
Class loading changes with zeitwerk - this is still not fully understood

The 3rd is the one we're still wrestling with. I think it's ultimately the cause of the two-ish remaining issues on the Rails 7 runs:

Not requiring action_mailer/railtie causes specs to fail - zeitwerk appears to be eager loading the app, and fails when it loads the existing mailer class. NotificationsMailer is loaded and fails
The No ActiveRecord smoke test fails - I think zeitwerk is loading ActiveRecord at some point, even though the test app doesn't have it by default.

@pirj I think we've had trouble reproducing this locally because the config/environments/test.rb file in the example app has the following line:

config.eager_load = ENV["CI"].present?

petergoldstein · 2022-01-11T22:36:44Z

@pirj By changing the value of config.eager_load I'm now able to reproduce locally.

petergoldstein · 2022-01-12T04:05:59Z

Ok, I've got the actual spec failure in Ruby 3.1 / Rails 6.1 fixed. But that branch is still failing because Rubocop with 3.1 is running into an error.

@pirj and @JonRowe is there any objection to me pulling out the rubocop run into a single, dedicated job - similar to what was just done on the rspec-core repo?

EDITED - Here's a PR for that change #2555

petergoldstein · 2022-01-12T06:56:54Z

I've rebased in the externalized Rubocop run. That should allow the Ruby 3.1/Rails6.1 branch to pass.

Tomorrow I'm going to look at the ActionMailer issue and see if I can wrap the mailer classes in if statements that are aware of the environment variable. I think that will get the previews running completely green on the standard smoke app. Then we just need to figure out where ActiveRecord is being pulled in for the no AR app.

EDITED: Confirmed that the branch passes - https://github.com/petergoldstein/rspec-rails/runs/4785785586 .

pirj · 2022-01-12T07:10:03Z

Thanks for your hard work!

pirj · 2022-01-18T17:59:11Z

     98:       it 'respects the setting from `show_previews`' do
     99:         x= capture_exec(custom_env.merge('SHOW_PREVIEWS' => 'true'), exec_script)
    100:         require 'pry'; binding.pry
 => 101:         expect(
    102:           x
    103:         ).to eq("#{::Rails.root}/spec/mailers/previews")
    104:       end
    105:
    106:       it 'allows initializers to set options' do

[1] pry(#<RSpec::ExampleGroups::ActionMailerRailtieHook::InANonDevelopmentEnvironment>)> puts x.io

/Users/pirj/source/rspec-dev/repos/bundle/ruby/3.1.0/gems/activerecord-7.0.1/lib/active_record.rb:26:in `<top (required)>': ololololo (RuntimeError)
        from /Users/pirj/source/rspec-dev/repos/bundle/ruby/3.1.0/gems/activerecord-7.0.1/lib/active_record/railtie.rb:3:in `require'
        from /Users/pirj/source/rspec-dev/repos/bundle/ruby/3.1.0/gems/activerecord-7.0.1/lib/active_record/railtie.rb:3:in `<top (required)>'
        from /Users/pirj/source/rspec-dev/repos/bundle/ruby/3.1.0/gems/actionmailbox-7.0.1/lib/action_mailbox/engine.rb:6:in `require'
        from /Users/pirj/source/rspec-dev/repos/bundle/ruby/3.1.0/gems/actionmailbox-7.0.1/lib/action_mailbox/engine.rb:6:in `<top (required)>'
        from /Users/pirj/source/rspec-dev/repos/rspec-rails/tmp/no_ar_example_app/spec/support/default_preview_path:31:in `require'
        from /Users/pirj/source/rspec-dev/repos/rspec-rails/tmp/no_ar_example_app/spec/support/default_preview_path:31:in `block (2 levels) in <main>'
        from /Users/pirj/source/rspec-dev/repos/rspec-rails/tmp/no_ar_example_app/spec/support/default_preview_path:4:in `require_file_stub'
        from /Users/pirj/source/rspec-dev/repos/rspec-rails/tmp/no_ar_example_app/spec/support/default_preview_path:17:in `block in <main>'
        from /Users/pirj/source/rspec-dev/repos/rspec-rails/tmp/no_ar_example_app/spec/support/default_preview_path:4:in `require_file_stub'
        from /Users/pirj/source/rspec-dev/repos/rspec-rails/tmp/no_ar_example_app/spec/support/default_preview_path:15:in `<main>'

pirj · 2022-01-18T18:08:28Z

example_app_generator/spec/support/default_preview_path

@@ -26,6 +26,10 @@ require_file_stub 'config/environment' do
    require "action_controller/railtie"
    require "action_mailer/railtie" unless ENV['NO_ACTION_MAILER']
    require "action_view/railtie"
+    if Rails::VERSION::STRING >= '6'
+      require "action_cable/engine"
+      require "action_mailbox/engine"


It's this line that's causing the "ActiveRecord is defined but should not be!" message in no_ar_example_app.

action_mailbox requires active_record.

🎉

Good detective work.

I think the fix is slightly more complex than just removing that line, but we should be able to get everything but the ActionMailer dependency fixed pretty quickly. As noted elsewhere, the ActionMailer fix requires us to modify the mailer classes.

Some work on my machine indicates that these changes may do it - petergoldstein#5 . I want to ensure they run green on CI before pushing to this branch.

petergoldstein · 2022-01-18T18:48:01Z

Ok, this last commit ran green. And it fixes everything but the ActionMailer dependency on my side, even when eager loading.

To fix the ActionMailer dependency we'll need to wrap the definitions of the mailer classes in conditionals on that environment variable.

JonRowe · 2022-01-18T23:05:03Z

No need to revert, see #2560, I pulled out 5-1-maintenance, main is now essentially 6-0-dev.

JonRowe · 2022-01-18T23:05:35Z

We do need Ruby 3.1 fixes backported seperately to Rails 7 fixes.

pirj · 2022-01-20T10:31:28Z

I've merged #2521, so those parts are no longer needed.

pirj · 2022-01-23T10:16:22Z

I have merged conflicting changes in #2563 to main and am going to rebase this PR.

All the have_enqueued_mail changes done here will remain, but I intend to extract some code to FeatureCheck like in #2563.

example_app_generator/generate_stuff.rb

pirj · 2022-01-24T21:27:05Z

@petergoldstein I dared approaching the problem with AR loaded in no_ar_example_app slightly differently.
And changed a couple of other things. Appreciate if you glance over.

I didn't change the way you approach scaffold spec generator, though, as I didn't take time to gather arguments to convince you with my approach.

petergoldstein · 2022-01-24T21:28:54Z

example_app_generator/spec/verify_mailer_preview_path_spec.rb

@@ -108,6 +111,8 @@ def have_no_preview
      end

      it 'handles action mailer not being available' do
+        skip "Rails 7 forces eager loading on CI, loads app/mailers and fails" if Rails::VERSION::STRING.to_f >= 7.0


@pirj Sure, this is a reasonable way to address the issue.

petergoldstein · 2022-01-24T21:30:05Z

@pirj I think this all looks fine. Thanks for pulling it together.

lib/rspec/rails/feature_check.rb

pirj · 2022-01-24T21:34:52Z

lib/rspec/rails/feature_check.rb

@@ -43,8 +43,11 @@ def has_action_mailbox?
        defined?(::ActionMailbox)
      end

-      def ruby_3_1?
-        RUBY_VERSION >= "3.1"
+      # TODO: add a self-explanatory method name. See https://github.com/rspec/rspec-rails/pull/2552#issuecomment-1009508693 for hints


I couldn't. Can you please help me? Is there some explanation, like "the last hash argument is now treated as a positional argument instead of being auto-expanded into kwargs"?
I searched in https://rubyreferences.github.io/rubychanges/3.1.html and couldn't find anything related.

So I'm not sure there's a simple description other than "this is how it works for this combo". The Argument processing in the Rails codebase is pretty complex. There's an intersection of several changes happening at once, combined with an goal to keep support working from Ruby 2.5 to Ruby 3.1. That's a lot of moving parts, and I didn't try and dissect exactly what causes the change on input to rspec-rails.

Then rspec-rails is post-processing the arguments with its own conditions, because of how it processes arguments to the matchers. So making that all line up is complicated, and I basically looked at the discrepancies and found the simplest resolution for the failing case.

pirj · 2022-01-24T21:36:10Z

example_app_generator/generate_action_mailer_specs.rb

@@ -31,6 +31,7 @@
  if skip_active_record?
    comment_lines 'spec/support/default_preview_path', /active_record/
    comment_lines 'spec/support/default_preview_path', /active_storage/
+    comment_lines 'spec/support/default_preview_path', /action_mailbox/


This is the key change to skip loading AR in no_ar_example_app, it's patching default_preview_path.

The bulk of the changes in this PR are test changes. These include: 1. Updating the example app configuration for Rails 7, including disabling eager class loading on CI 2. Updating specs to filter out additional lines that may be generated when running commands 3. Removing ".html.erb" and ".xml.erb" suffixes in render calls 4. Updating specs to accomodate differences in Rails view scaffolding before and after Rails 7 Material code changes include: 1. Adding additional logic to the ActionMailer argument parsing to accomodate for differences under Ruby 3.1/Rails 6.1 2. Symbolizing the handler argument parsed from the spec description for view specs with an empty render

…rrors

…action_mailbox was

Action Mailbox requires Active Record

See rails/rails#43508

JonRowe · 2022-04-03T21:23:46Z

Thanks for this, I've merged the initial CI work bar the mailer stuff that clashes, that could be rebased if desired or closed

pirj · 2022-07-26T23:11:08Z

It appears that all of this has been merged in parts.
Thanks a lot, @petergoldstein !

pirj reviewed Jan 10, 2022

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

pirj force-pushed the feature/ruby_3_1_for_rails_6_1 branch from 15423a7 to 581f14a Compare January 10, 2022 20:26

petergoldstein force-pushed the feature/ruby_3_1_for_rails_6_1 branch from 581f14a to 0b3c744 Compare January 10, 2022 20:28

pirj reviewed Jan 10, 2022

View reviewed changes

spec/generators/rspec/scaffold/scaffold_generator_spec.rb Show resolved Hide resolved

pirj reviewed Jan 10, 2022

View reviewed changes

spec/generators/rspec/scaffold/scaffold_generator_spec.rb Show resolved Hide resolved

pirj changed the title ~~Add Ruby 3.1 to CI and passing for Rails 6.1~~ Add Ruby 3.1 to CI and fix Rails 7 Jan 10, 2022

pirj mentioned this pull request Jan 10, 2022

Fix Ruby 3.1 on CI #2553

Merged

petergoldstein force-pushed the feature/ruby_3_1_for_rails_6_1 branch from 15d0688 to 0e15994 Compare January 11, 2022 00:57

petergoldstein force-pushed the feature/ruby_3_1_for_rails_6_1 branch from a55cbab to aa3cc96 Compare January 11, 2022 08:11

pirj mentioned this pull request Jan 11, 2022

Fix Ruby 3.1 on CI for dependent builds (this time for real) #2554

Closed

pirj referenced this pull request Jan 12, 2022

Fixed rails7 specs

c68b568

petergoldstein force-pushed the feature/ruby_3_1_for_rails_6_1 branch from dca3d7f to cc555ec Compare January 12, 2022 06:52

pirj added the rails7 label Jan 12, 2022

pirj reviewed Jan 18, 2022

View reviewed changes

JonRowe mentioned this pull request Jan 18, 2022

RSpec Rails 6.0 Release Plan #2560

Closed

21 tasks

pirj added this to the 6.0 milestone Jan 20, 2022

pirj force-pushed the feature/ruby_3_1_for_rails_6_1 branch from 63d4131 to 1f9c0ff Compare January 23, 2022 11:47

pirj reviewed Jan 23, 2022

View reviewed changes

example_app_generator/generate_stuff.rb Outdated Show resolved Hide resolved

pirj force-pushed the feature/ruby_3_1_for_rails_6_1 branch from 7398042 to ca8385f Compare January 24, 2022 21:00

petergoldstein commented Jan 24, 2022

View reviewed changes

pirj reviewed Jan 24, 2022

View reviewed changes

lib/rspec/rails/feature_check.rb Show resolved Hide resolved

pirj reviewed Jan 24, 2022

View reviewed changes

petergoldstein and others added 6 commits January 25, 2022 17:37

Update requires to avoid pulling in ActiveRecord, fix other require e…

e820894

…rrors

wip: Extract arg condition method

64d7e33

fixup! Remove hack that prevented active_record to be preloaded when …

8a550cf

…action_mailbox was

Skip Action Mailbox for No AR example app

d23598e

Action Mailbox requires Active Record

Skip "action mailer missing" example for Rails 7

c7bba85

See rails/rails#43508

pirj force-pushed the feature/ruby_3_1_for_rails_6_1 branch from ca8385f to c7bba85 Compare January 25, 2022 14:39

pirj mentioned this pull request Jan 29, 2022

Fix mailer argument deserialisation for 6.1 on Ruby 3.1 #2570

Merged

pirj closed this Jul 26, 2022

pirj mentioned this pull request Sep 15, 2022

use the tag itself instead of the variable #2619

Closed

Add Ruby 3.1 to CI and fix Rails 7 #2552

Add Ruby 3.1 to CI and fix Rails 7 #2552

Conversation

petergoldstein commented Jan 9, 2022 • edited

petergoldstein commented Jan 10, 2022

pirj commented Jan 10, 2022

pirj commented Jan 10, 2022 • edited

petergoldstein commented Jan 10, 2022

pirj commented Jan 10, 2022

pirj commented Jan 10, 2022

petergoldstein commented Jan 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petergoldstein Jan 12, 2022 • edited

Choose a reason for hiding this comment

petergoldstein commented Jan 11, 2022

pirj commented Jan 11, 2022

petergoldstein commented Jan 11, 2022

pirj commented Jan 11, 2022

petergoldstein commented Jan 11, 2022

petergoldstein commented Jan 11, 2022 • edited

petergoldstein commented Jan 12, 2022 • edited

petergoldstein commented Jan 12, 2022 • edited

pirj commented Jan 12, 2022

pirj commented Jan 18, 2022

Choose a reason for hiding this comment

petergoldstein Jan 18, 2022 • edited

Choose a reason for hiding this comment

petergoldstein commented Jan 18, 2022

JonRowe commented Jan 18, 2022

JonRowe commented Jan 18, 2022

pirj commented Jan 20, 2022

pirj commented Jan 23, 2022

pirj commented Jan 24, 2022

Choose a reason for hiding this comment

petergoldstein commented Jan 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonRowe commented Apr 3, 2022 • edited

pirj commented Jul 26, 2022

petergoldstein commented Jan 9, 2022 •

edited

pirj commented Jan 10, 2022 •

edited

petergoldstein Jan 12, 2022 •

edited

petergoldstein commented Jan 11, 2022 •

edited

petergoldstein commented Jan 12, 2022 •

edited

petergoldstein commented Jan 12, 2022 •

edited

petergoldstein Jan 18, 2022 •

edited

JonRowe commented Apr 3, 2022 •

edited