pat / combustion

Simple, elegant testing for Rails Engines
MIT License
709 stars 51 forks source link

Support future rails versions without legacy_connection_handling config #125

Closed jrochkind closed 2 years ago

jrochkind commented 2 years ago

legacy_connection_handling= is already missing from current rails-edge, so combustion would presumably already fail it's own test suite on rails-edge.

Closes #123

I decided using a "feature detection" respond_to? approach really was the most reliable and cleanest way to do this. Note that such a feature detection approach is already present in this very file with return unless ::ActiveRecord.constants.include?(:MassAssignmentSecurity) already present.

@pat Considering that combustion already runs it's own CI on rails-edge and thus implies endorsement of the idea that this is a wise thing to do, and that this change is necessary to run combustion at all on rails-edge (not just to avoid deprecation warnings), perhaps you would consider doing a combustion release after all, so people can run a released combustion on rails-edge?

pat commented 2 years ago

@jrochkind thanks for the PRs :) When it comes to edge rails though, CI isn't happy. Are you able to sort that out? I can look into it when I have a spare moment, but I suspect you'll be faster :)

jrochkind commented 2 years ago

Weird. Judging by the labelling, it seems to not just be edge that's failing, right? (2.7, 2.1.4, mysql2) -- that shouldn't be edge? Although weirdly it is failing for reasons due to 'legacy_connection_handling=` according to the logs -- could it not really be running what it's supposed to be running?

I believe some of these tests were probably failing even before this PR -- there hasn't been a CI run in a while, not since legacy_connection_handling= was removed from rails main branch.

I can try to look at it this week, but if it gets really into the weeds I might run out of time.

Would you like a separate PR to add weekly CI builds, so you find out sooner if a gem release or something on Rails edge (which you are already testing in master branch CI) causes a failure? It will likely reveal that CI is already failing on master though.

jrochkind commented 2 years ago

Oh wait, right, every Github Actions Job individually runs the entire appraisal suite (or, somehow, the whole suite that the ruby version is compatible with?), so edge is actually included in every job (or every job with a compatible ruby version), the numbers in the jobs aren't the Rails version. So hard to keep track all the different ways that different repos have set things up!

jrochkind commented 2 years ago

Oh right, I remember now discovering how Rails had thwarted my attempt to feature-detect. OK, maybe I'll go back to version numbers after all, gah.

jrochkind commented 2 years ago

@pat tried to fix it, now I'm getting "1 workflow awaiting approval" again, it's not running CI. :(

jrochkind commented 2 years ago

Thanks for running the CI. Hm, failed again. Very frustrating.

Wait, looking at the diff... it seems to still not actually have the change I tried to make earlier today?

I haven't been able to figure out how to run the tests locally yet. Trying to get tests to run when I can't do that always feels like flying blind; not being able to trigger CI runs myself doesn't help!

I'll see what I can do. If you can figure out anything out, please do feel free!

pat commented 2 years ago

Handled by #126 instead.