Fix parallel testing (issue #48547) #48560
Merged
Conversation
This file contains 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
|
Thanks @eileencodes ! I've verified this fix works on my example app (sqlite3) |
eileencodes
force-pushed
the
fix-parallel-testing-48547
branch
3 times, most recently
from
dd88153
to
4b3f27d
Compare
Fixes 2 bugs in parallel testing. The first bug was related to changes made in rails#45450 which meant that we were no longer replacing the connection in parallel testing because the config object is equal (we simply merge a new db name but the object id of the config stays the same). This bug only manifested in mysql and sqlite3 interestingly. It would fail on the internal metadata tables because they were missing in the schema version check. To fix this I introduced a `clobber: true` kwarg onto the connection handler that allows us to bypass the functionality that won't make a new connection if the config is the same. This is an easy way to fall back to the old behavior from before this change. I only added `clobber` to the `reconstruct_from_schema` call because we need to actually replace the connection for these. It's not safe to add everywhere since we don't always want to replace the connection. After implementing this fix I was still seeing failures in the mysql demo app I made due to the fact that `purge` was not re-establishing the connection to a config that had a database defined. Neither sqlite3 or postgresql were missing this. I added a test for mysql2 so we don't have regressions in the future. I think this was missed because sqlite3 only demonstrates the bug if it was never successful on that worker and postgresql was fine. Fixes rails#48547
eileencodes
force-pushed
the
fix-parallel-testing-48547
branch
from
4b3f27d
to
5cf5a2a
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.
Fixes 2 bugs in parallel testing.
The first bug was related to changes made in #45450 which meant that we were no longer replacing the connection in parallel testing because the config object is equal (we simply merge a new db name but the object id of the config stays the same). This bug only manifested in mysql and sqlite3 interestingly. It would fail on the internal metadata tables because they were missing in the schema version check.
To fix this I introduced a
clobber: truekwarg onto the connection handler that allowsus to bypass the functionality that won't make a new connection if the config is the same. This is an easy way to fall back to the old behavior from before this change.After implementing this fix I was still seeing failures in the mysql demo app I made due to the fact that
purgewas not re-establishing the connection to a config that had a database defined. Neither sqlite3 or postgresql were missing this.I added a test for mysql2 so we don't have regressions in the future. I think this was missed because sqlite3 only demonstrates the bug if it was never successful on that worker and postgresql was fine.
Fixes #48547
cc/ @matthutchinson