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

Update to Rails 7 #2324

Merged
merged 4 commits into from
Apr 21, 2023
Merged

Update to Rails 7 #2324

merged 4 commits into from
Apr 21, 2023

Conversation

pablobm
Copy link
Collaborator

@pablobm pablobm commented Jan 27, 2023

This description has been updated since the PR was first created. The first few comments in the thread reflect the conversation while trying to find a fix to the issue of lingering constants.


Administrate was already compatible with Rails 7, but its specs weren't. This PR includes all necessary fixes, including a workaround for a strange Ruby behaviour when undefining constants on Ruby 3.x.

Specifically, spec/generators/dashboard_generator_spec.rb creates a number of classes and then attempts to undefine them, but these appear to linger around, affecting results at spec/generators/routes_generator_spec.rb. From what I can tell, this affects the specs, but not real world usage.

After trying a few things, I ended up with a bit of a hack: in spec/generators/dashboard_generator_spec.rb I define those classes to be children of TestRecord. Then the routes generator explicitly filter those out.

@pablobm pablobm force-pushed the rails70 branch 4 times, most recently from ba0819b to ccdc161 Compare February 2, 2023 15:45
@pablobm
Copy link
Collaborator Author

pablobm commented Feb 2, 2023

@nickcharlton - Not sure what to do about this. I'm considering skipping the offending example under Ruby 3.x. Thoughts?

@nickcharlton
Copy link
Member

Hmm, interesting. On spec/generators/routes_generator_spec.rb:123, it does pass locally with Ruby 3.2. It looks like the failure is because the routes file contains foos#index but not customers#index, have we made any changes that might make that to be the case?

@pablobm
Copy link
Collaborator Author

pablobm commented Feb 6, 2023

The specs do pass individually. The problem is running them all together, particularly when spec/generators/dashboard_generator_spec.rb runs before spec/generators/routes_generator_spec.rb.

  1. The first one generates temporary models and dashboards, like Foo and FooDashboard at https://github.com/thoughtbot/administrate/blob/main/spec/generators/dashboard_generator_spec.rb#L28
  2. These temporary classes are theoretically cleaned up in ensure blocks at the end of each example: https://github.com/thoughtbot/administrate/blob/main/spec/generators/dashboard_generator_spec.rb#L60
  3. However they are still around when the spec for the routes generator runs, and they are returned by ActiveRecord::Base.descendants at https://github.com/thoughtbot/administrate/blob/main/lib/generators/administrate/routes/routes_generator.rb#L74.
  4. These "zombie" models make the route generator think that those are models that need routes, creating them when they shouldn't exist.

@nickcharlton
Copy link
Member

Oh, right I see! Yeah, I neglected to try not just running them standalone.

Any thoughts on why it might be showing up on 3.1/3.1 only?

@pablobm
Copy link
Collaborator Author

pablobm commented Feb 6, 2023

No idea at the moment. I tried running a bisect through Rails's commit history, but there is a large section around 7.0.0.alpha (or something like that) where specs fail for different reasons (bug in Rails at the time, from what I gather). I may give it another whirl this week, but I'm going to timebox it as I don't want to spend too much longer on this.

@pablobm
Copy link
Collaborator Author

pablobm commented Feb 10, 2023

I had another look. The other error that I mention appears for the first time with this Rails commit: rails/rails@cb82f5f This is too esoteric for me, but it does sound related to our issue.

I've been playing with it a bit at main...pablobm:administrate:rails70-bisect. I may give it another go, but this is well beyond my expertise.

@pablobm pablobm marked this pull request as ready for review April 18, 2023 15:54
@pablobm
Copy link
Collaborator Author

pablobm commented Apr 18, 2023

I appear to have found a fix for the issue of lingering constants (see PR description), so I have marked the PR as ready to review.

Copy link
Contributor

@littleforest littleforest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@pablobm pablobm merged commit 66e3e3e into thoughtbot:main Apr 21, 2023
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.

3 participants