Fix flaky primary key test #48473
Merged
Fix flaky primary key test #48473
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
I noticed in a build that this test was flaky and causing subsequent tests to fail with a no connection pool error. I don't entirely get why this test was failing, it's not the only one where we remove a connection. When this test fails, `@automatic_reconnect` is `false`. After spending far too much time trying to figure out the combination of tests that break this I decided to fix it by making a brand new connection that we can remove. It's also a more accurate test because we're asserting the connection is actually not `connected?`. Note that I had to use the connection handler to remove the connection because there is an inconsitency in remove_connection on the model. Unfortunately, `NoConnection.remove_connection` will cause `connected?` to fall back to `ActiveRecord::Base` which is contrary to the behavior of all other methods on the active record models. I'm going to send a PR to fix that but for now this should fix the (rare) flaky test for a primary key with no connection. To reproduce I used this seed: `SEED=29535 bundle exec rake test:mysql2`
4 tasks
eileencodes
added a commit
to eileencodes/rails
that referenced
this pull request
Jun 15, 2023
This test was added in rails@b3407c8 at a time when Rails was reading the `primary_keys` off a hash stored on `connection`. We no longer do that in Active Record so this test isn't really necessary. I'm removing it because it was flaky. I attempted to fix it in rails#48473 but realized today it's not testing the same thing it was before. After discussing it with Matthew, I've decided to remove it.
zzak
added a commit
to zzak/rails
that referenced
this pull request
Jul 9, 2023
This test was added in rails/rails@b3407c8 at a time when Rails was reading the `primary_keys` off a hash stored on `connection`. We no longer do that in Active Record so this test isn't really necessary. I'm removing it because it was flaky. I attempted to fix it in rails#48473 but realized today it's not testing the same thing it was before. After discussing it with Matthew, I've decided to remove it. Co-authored-by: eileencodes <eileencodes@gmail.com>
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.
I noticed in a build that this test was flaky and causing subsequent tests to fail with a no connection pool error. I don't entirely get why this test was failing, it's not the only one where we remove a connection. When this test fails,
@automatic_reconnectisfalse. After spending far too much time trying to figure out the combination of tests that break this I decided to fix it by making a brand new connection that we can remove. It's also a more accurate test because we're asserting the connection is actually notconnected?. Note that I had to use the connection handler to remove the connection because there is an inconsitency in remove_connection on the model. Unfortunately,NoConnection.remove_connectionwill causeconnected?to fall back toActiveRecord::Basewhich is contrary to the behavior of all other methods on the active record models. I'm going to send a PR to fix that but for now this should fix the (rare) flaky test for a primary key with no connection.To reproduce I used this seed:
SEED=29535 bundle exec rake test:mysql2