Fix test suite with Rails 7.2+ #220
Merged
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
|
NB: I'm fine with dropping 7.1 and older on CI if it's too annoying to support both the new and old API. |
This was something I was thinking about as I worked to update the test matrix in #219 - the API for |
As of rails/rails#49819 the internals of `ActionDispatch::Assertions::RoutingAssertions` changed and the way we were building our app instance was no compatible with it. This gets us back to passing tests for Rails 7.2+. I hope 🤞
byroot
force-pushed
the
fix-integration-2
branch
from
1d5a5ee to
e277341
Compare
|
Ok, I found the leak (or one of?) by bisecting the test suite, and fixed it in e77bcc3 |
stevenharman
added a commit
to docsend/activerecord-session_store
that referenced
this pull request
Mar 26, 2025
I took another look at the changes we had to make in rails#220 and compared that to how things work in the actiondispatch's Cookie Store tests and realized we were close, but just missing how we were setting the options for our store. So I've revamped the setup to be in line with what's in Rails' own tests. This should make it easier to keep things in line and working, going forward. See the following two files for how Cookie Store tests (and their base `ActionDispatch::IntegrationTest`) work today (as of Rails 8.0): * https://github.com/rails/rails/blob/8-0-stable/actionpack/test/dispatch/session/cookie_store_test.rb * https://github.com/rails/rails/blob/8-0-stable/actionpack/test/abstract_unit.rb NOTE:
stevenharman
added a commit
to docsend/activerecord-session_store
that referenced
this pull request
Mar 26, 2025
I took another look at the changes we had to make in rails#220 and compared that to how things work in `actiondispatch`'s Cookie Store tests and realized we were close, but missing how we were setting the options for our store. So I've revamped the setup to be in line with what's in Rails' own tests. This should make it easier to keep things in line and working, going forward. See the following two files for how Cookie Store tests (and their base `ActionDispatch::IntegrationTest`) work today (as of Rails 8.0): * https://github.com/rails/rails/blob/8-0-stable/actionpack/test/dispatch/session/cookie_store_test.rb * https://github.com/rails/rails/blob/8-0-stable/actionpack/test/abstract_unit.rb NOTE: The big downside to this is… it depends on some changes to how `#with_routing`, provided by Rails, works. Meaning it only works with Rails 7.2+.☹️
stevenharman
added a commit
to docsend/activerecord-session_store
that referenced
this pull request
Apr 2, 2025
I took another look at the changes we had to make in rails#220 and compared that to how things work in `actiondispatch`'s Cookie Store tests and realized we were close, but missing how we were setting the options for our store. So I've revamped the setup to be in line with what's in Rails' own tests. This should make it easier to keep things in line and working, going forward. We also need to patch in support for with_routing for integration tests which was added in Rails 7.2. So we do that, only when necessary. We can drop that patch when we drop Rails 7.1 support. See the following two files for how Cookie Store tests (and their base `ActionDispatch::IntegrationTest`) work today (as of Rails 8.0): * https://github.com/rails/rails/blob/8-0-stable/actionpack/test/dispatch/session/cookie_store_test.rb * https://github.com/rails/rails/blob/8-0-stable/actionpack/test/abstract_unit.rb
stevenharman
added a commit
to docsend/activerecord-session_store
that referenced
this pull request
Apr 2, 2025
I took another look at the changes we had to make in rails#220 and compared that to how things work in `actiondispatch`'s Cookie Store tests and realized we were close, but missing how we were setting the options for our store. So I've revamped the setup to be in line with what's in Rails' own tests. This should make it easier to keep things in line and working, going forward. We also need to patch in support for with_routing for integration tests which was added in Rails 7.2. So we do that, only when necessary. We can drop that patch when we drop Rails 7.1 support. See the following two files for how Cookie Store tests (and their base `ActionDispatch::IntegrationTest`) work today (as of Rails 8.0): * https://github.com/rails/rails/blob/8-0-stable/actionpack/test/dispatch/session/cookie_store_test.rb * https://github.com/rails/rails/blob/8-0-stable/actionpack/test/abstract_unit.rb
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.
Fix: #219
Starting from rails/rails#49819 most of the test suite is broken.
I'm trying to figure out how to fix it, but I'm really struggling to make sense of it.
@gmcgibbon would you have a bit of time to look at this? I think I managed to fix the
build_appandwith_routingthings, but now it seems like cookies aren't set.FYI: @stevenharman