From 50bafd48cd263bccae1b056f2041a28959bc69a4 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 30 Nov 2023 10:49:55 +0800 Subject: [PATCH] FIX: Flaky spec due to incorrect Rack response body (#24640) Why was the problem? ActiveRecord's query cache for the connection pool wasn't disabled after the `with a fake provider runs 'other_phase' for enabled auth methods` test in `omniauth_callbacks_controller_spec.rb` was run. This was because the Rack response body in `FakeAuthenticator::Strategy::other_phase` did not adhere to the expected Rack body format which is "typically an Array of String instances". Because this expectation was broken, it cascaded the problem down where it resulted in the ActiveRecord's query cache for the connection pool not being disabled as it normally should when the response body is closed. When the query cache is left enabled, common assertions pattern in RSpec like `expect { something }.to change { Group.count }` will fail since the query cache is enabled and the call first call to `Group.count` will cache the result to be reused later on. To see the bug in action, one can run the following command: `bundle exec rspec --seed 44747 spec/requests/omniauth_callbacks_controller_spec.rb:1150 spec/models/group_spec.rb:283` --- spec/models/user_spec.rb | 8 ++++---- spec/requests/omniauth_callbacks_controller_spec.rb | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b82de2ce2c5..5ef16f9a7db 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -3048,19 +3048,19 @@ RSpec.describe User do it "tracks old user record correctly" do expect do user.update_ip_address!("127.0.0.1") end.to change { - UserIpAddressHistory.uncached { UserIpAddressHistory.where(user_id: user.id).count } + UserIpAddressHistory.where(user_id: user.id).count }.by(1) freeze_time 10.minutes.from_now expect do user.update_ip_address!("0.0.0.0") end.to change { - UserIpAddressHistory.uncached { UserIpAddressHistory.where(user_id: user.id).count } + UserIpAddressHistory.where(user_id: user.id).count }.by(1) freeze_time 11.minutes.from_now expect do user.update_ip_address!("127.0.0.1") end.to_not change { - UserIpAddressHistory.uncached { UserIpAddressHistory.where(user_id: user.id).count } + UserIpAddressHistory.where(user_id: user.id).count } expect( @@ -3070,7 +3070,7 @@ RSpec.describe User do freeze_time 12.minutes.from_now expect do user.update_ip_address!("0.0.0.1") end.not_to change { - UserIpAddressHistory.uncached { UserIpAddressHistory.where(user_id: user.id).count } + UserIpAddressHistory.where(user_id: user.id).count } expect(UserIpAddressHistory.where(user_id: user.id).pluck(:ip_address).map(&:to_s)).to eq( diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index c9ccf3bf4bd..e9dbb7f1587 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -1108,7 +1108,7 @@ RSpec.describe Users::OmniauthCallbacksController do class Strategy include OmniAuth::Strategy def other_phase - [418, {}, "I am a teapot"] + [418, {}, ["I am a teapot"]] end end