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

Full refresh of framework dummy applications #47259

Merged
merged 6 commits into from Mar 1, 2023

Conversation

skipkayhil
Copy link
Member

Motivation / Background

Following some of the updates to environment files in #47138, #47143, etc. I noticed that the environment files weren't fully up to date.

Detail

All three dummy apps have been completely refreshed with the latest templates using a script:
https://github.com/skipkayhil/rails-bin/blob/cbad7eddcb439c44e5ec28ade3336245917542fe/bin/regen-dummy

Additional information

The full diff is pretty crazy, which is why I left each framework in a separate commit. I can split this PR into multiple if that would make this easier to review

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • [ ] Tests are added or updated if you fix a bug or add a feature.
  • [ ] CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@skipkayhil
Copy link
Member Author

Related: #47242 one of the manual changes needed was removing require <plugin name> from config/application.rb. It shouldn't be needed (as shown by the Rails dummy apps not having it) but I'm still working on test failures 😞

Generated using the following script and manually reviewed after:
skipkayhil/rails-bin@6898611

The :messages route was removed because there isn't actually a
MessagesController.

config/boot.rb also has its Gemfile path fixed to point at the root
Gemfile because Action Mailbox does not have a Gemfile.
Similar to 89a24d6, but importmaps are
not added because no javascript is used in the app
Generated using the following script and manually reviewed after:
skipkayhil/rails-bin@38e2aac
These are leftover after Webpacker was replaced with importmaps in
89a24d6
Generated using the following script and manually reviewed after:
skipkayhil/rails-bin@cbad7ed

The active_storage configuration had to be moved to the dummy app
because of the changed eager_load config. Now, the whole app is eagerly
loaded when the app's environment file is required which means that
changing configuration in the test_helper is too late.
@rafaelfranca
Copy link
Member

I'll merge this one, but we should not have dummy apps in the Rails framework. We should be testing those frameworks that depend on dummy apps using the same approach we use to test activerecord, actionpack, and all the older frameworks, by adding integration tests in the railties test suite.

In my option, It doesn't make sense to upgrade dummy apps inside the framework every time we change the framework itself. Probably those dummy apps get outdated daily, and it is very likely we will change Rails in a way that would break those frameworks but the test will not catch because the dummy app is outdated.

If you want to work in that direction, it would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants