Upgrade: cleanup before the storm + Rails 4.2.11.3 (latest patch release) #1511
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ysbaddaden
added
dependencies
ysbaddaden
changed the title
|
I added some backward compatible changes (ApplicationRecord, ApplicationJob, rename That should good to merge. Now I'll try upgrading to Rails 5, following the fixes and gem version locks that @ftarulla did 🤞 |
ysbaddaden
force-pushed
the
chore/upgrade-to-rails-5.0
branch
from
9df29f0 to
981d5c4
Compare
straight-shoota
approved these changes
Mar 9, 2022
|
This is an awesome cleanup! ❤️ I'm wondering why the upgrade to 4.2.11 failed in #1472. Maybe it was really caused by one of the removed dependencies. |
|
The error that popped in CI was the same: the RSpec version was incompatible with Rake that got updated by Bundler for an unknown reason. Maybe you then tried more upgrades? I chose to merely downgraded Rake back to its previous version, because there was no reason for Bundler to upgrade Rake in the first place. |
ysbaddaden
force-pushed
the
chore/upgrade-to-rails-5.0
branch
from
981d5c4 to
31bccc3
Compare
ysbaddaden
force-pushed
the
chore/upgrade-to-rails-5.0
branch
2 times, most recently
from
ae7d4ec to
e7accfc
Compare
This removes around 10 direct dependencies, and a bunch more dependencies. I checked each and every removed dependency, and I couldn't find any single use of them left. Unit and integration specs are still passing, and the app still boots. I also removed shoulda because we only used a single rspec matcher (validate_presence_of) and there was only 5 occurrences of it.
It's only used in a few controllers, so the induced pattern is quite odd compared to the whole application. We can replace the gem with a few methods. One less dependency for less hidden magic.
This is to understand where and what tests are missing in the application, to have a global understanding of the test quality of the application. As of this commit the overall coverage is 80%, which looks great, but looking more closely we see that 9 controllers are 0% hence totally untested, and 6 controllers are below 70% coverage.
ysbaddaden
force-pushed
the
chore/upgrade-to-rails-5.0
branch
2 times, most recently
from
556ca1c to
d8580bd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes a dozen unused gems, and removes the
decent_exposuregem that wasn't used much. This is less gems to worry about during the upgrade. There are probably more gems that aren't used much that we could remove. For example RestClient is used for a single HTTP call!I also tried to group gems into categories (databases, models, views, assets, ...).
Also adds support for code coverage (on demand) using simplecov to understand what features are missing tests, as well as where to focus QA for the upgrade process. We have a good average (80%) but we also have lots of fully untested or under tested controllers, which decreases the overall confidence in the upgrade.
refs #1235