Update to Rails 7.1 (including related updates) #2219
Conversation
Update to Rails 7.1. This requires other related updates. Oddly, 'rails-i18n' doesn't upgrade to the same number this time. It *will* later when we upgrade again, so be careful about that. I've added a spurious extra version number comment to hint at that for later. This updates paper_trail. However, it does *not* adjust the older data in paper_trail. Paper_trail records data as it was, and by default uses YAML. This is a problem due to a datatype change; YAML records the *actual* datatype, even when we don't want to use it. That should be addressed separately before merging this change. This PR removes the known-failing test on papertrail. This updates does *not* deal with a Rack deprecation. We'll need to deal with that separately, but since it's still supported, we can deal with it separately. It will currently show: > DEPRECATION WARNING: Setting action_dispatch.show_exceptions > to false is deprecated. Set to :none instead. > (called from call at ... /best-practices-badge/config/initializers/ > canonical_trailing_slash.rb:30) Signed-off-by: David A. Wheeler <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2219 +/- ##
==========================================
+ Coverage 98.08% 99.95% +1.86%
==========================================
Files 55 55
Lines 2144 2125 -19
==========================================
+ Hits 2103 2124 +21
+ Misses 41 1 -40 ☔ View full report in Codecov by Sentry. |
|
ran into this also (codecov showing decreased coverage) but I didn't figure out why |
This changes PaperTrail to store using jsonb, which should make this transition easier. Signed-off-by: David A. Wheeler <[email protected]>
Now that PaperTrail is fixed up, restore test using it. Signed-off-by: David A. Wheeler <[email protected]>
Signed-off-by: David A. Wheeler <[email protected]>
|
@andrewfader - Hi! Great to hear from you, and Merry Christmas! If you have any ideas, let me know. I'm starting by reviewing the File Explorer results e.g., here. Sometimes there's a race where CodeCov produces results before tests have completed, producing lower numbers. But usually it's re-run when the test complete & the coverage value updates. So I don't think that's it, it seems to actually have a problem. |
|
Well, it just gets weirder. For example, CodeCov says that lots of code in UsersController isn't run, including "index". We do have such tests. So the tests aren't running for some reasons, or the results aren't getting recorded. |
|
@david-a-wheeler yeah, it's almost like it bails out or crashes at some point and stops tracking the things getting run |
|
btw, I opened #2212 if you want a monkeypatch to silence that VCR error. Also you can manage webdrivers with selenium now |
|
Wow, thanks for creating #2212 ! Below is the output when I run the tests locally ("rake" which includes "rake test"). All the tests pass (0 failures). However, there are lots of warnings about failures between vcr and webmock. I suspect what's happening is that our old version of vcr isn't happy, leading to passing tests not always being recorded as passing. The vcr gem switched from an OSS license to a "Hippocratic Oath" license which is why we haven't upgraded it. I long ago raised a legal question about it, & then moved on to other things. |
|
I don't know if #2212 will solve this problem, but it seems like a good thing to merge for now. I hope to remove the monkeypatch eventually no matter what. |
|
Yeah, #2212 will silence those warning/errors, but I don't know about the codecov issue with rails 7.1. I suspect we should look into what has changed in rails 7.1 and see if it might be something we have or use or is part of an old version of another gem. We could also try upgrading other gems that are loaded in the path of the codecov run |
|
@david-a-wheeler I guess it fixed it! Or it just didn't even run codecov this time? where's the result? |
Signed-off-by: David A. Wheeler <[email protected]>
|
Nevermind, still same problem! |
|
CodeCov results are here: https://app.codecov.io/gh/coreinfrastructure/best-practices-badge (you can get there using the little badge on the README of the best practices badge project). |
|
I see it now, it just didn't load right away. Until it loads it looks like it passed with 7 checks, before the 8th shows up |
|
The build runs the tests & the tests pass. The CodeCov results for this pull request are shown here: https://app.codecov.io/gh/coreinfrastructure/best-practices-badge/pull/2219 |
|
While it'd be nice to have CodeCov working properly, what matters is that we have tests working properly. build-deploy runs all the tests, and while I'll double-check, it looks like they're all passing. |
|
@david-a-wheeler we could try moving the |
|
I looked at build-deploy which is the task that actually runs all the tests. Details. The "run test suite (both system and non-system tests)" runs this command: It reports some missing extensions, as well as a deprecation that must be dealt with before upgrading to Rails 7.2. |
|
CodeCov continues to show up late for the party, reporting only partial coverage. Hmmm. This StackOverflow answer says that what we're doing ( The documentation for Rails 7.1 says: "If you are using System Tests, bin/rails test will not run them, since they can be slow. To also run them, add an another CI step that runs bin/rails test:system, or change your first step to bin/rails test:all, which runs all tests including system tests." Rails 8 says something similar. Let's switch to |
Signed-off-by: David A. Wheeler <[email protected]>
|
@david-a-wheeler worth a try but also take a look at that simplecov issue I just posted above |
|
Oh, I didn't see your SimpleCov comment earlier. That definitely looks like something to check out, yes yes yes. |
|
Actually, looks like you fixed it! |
|
Yes indeed! It does look like it's fixed. We had a Stack Overflow answer that didn't work on a later version of some software. Let's just say I've seen that situation before :-). I was suspicious because I didn't see multi-argument support documented in the rails documentation. |
|
@andrewfader - Thanks so much for your help. Merging now. Of course, I'll need to turn out around and fix the deprecations so that we can upgrade further, but that's a later step. |
|
The weirdest thing about this was that I first needed to switch PaperTrail to use jsonb. Without that, it couldn't load old stored values, because it was storing in YAML and trying to create time objects in a format no longer supported. JSON doesn't allow arbitrary types, so switching to JSON meant we eliminated that problem. It's more efficient anyway (I'm using jsonb in PostgreSQL). |
|
+1 json in pg is the way to go |
I agree. PaperTrail defaults to YAML, presumably because it can back any type (so it's a more general solution). But in many cases the correct answer is JSONB. |
Update to Rails 7.1. This requires other related updates.
Oddly, 'rails-i18n' doesn't upgrade to the same number this time. It will later when we upgrade again, so be careful about that. I've added a spurious extra version number comment to hint at that for later.
This updates paper_trail. However, it does not adjust the older data in paper_trail. Paper_trail records data as it was, and by default uses YAML. This is a problem due to a datatype change; YAML records the actual datatype, even when we don't want to use it. That should be addressed separately before merging this change. This PR removes the known-failing test on papertrail.
This updates does not deal with a Rack deprecation. We'll need to deal with that separately, but since it's still supported, we can deal with it separately.
It will currently show: