Skip to content

Aggressively reload A/B tests during spec runs#11693

Merged
matthinz merged 1 commit intomainfrom
matthinz/aggressive-ab-test-cleanup
Dec 31, 2024
Merged

Aggressively reload A/B tests during spec runs#11693
matthinz merged 1 commit intomainfrom
matthinz/aggressive-ab-test-cleanup

Conversation

@matthinz
Copy link
Copy Markdown
Contributor

🛠 Summary of changes

TLDR: This PR updates our RSpec config to reload A/B tests after each spec file in a test run. This is because by default A/B tests are initialized once and only once, and this makes it possible for them to "hold on" to mocked configuration data, potentially leading to flakiness.

Previously in #11583 I added a cleanup hook to the ab_tests_spec.rb file, but there are two problems with this:

  1. Theoretically, A/B tests could get "stuck" by any spec, so it needs to be applied more broadly
  2. The way I added it didn't actually work. RSpec complained:
WARNING: `after(:suite)` hooks are only supported on the RSpec configuration object. This `after(:suite)` hook, registered on an example group, will be ignored. Called from /builds/lg/identity-idp/spec/config/initializers/ab_tests_spec.rb:6:in `block in <top (required)>'.

Thus, this PR adds the cleanup hook to the main RSpec config. I'm adding it as an after :context block, which here means "after each test file". We could theoretically run the same thing after each example, but that is probably overkill.

📜 Testing Plan

To see an example of this causing a problem on main, run:

bundle exec rspec --seed 4734 --fail-fast \
spec/config/initializers/ab_tests_spec.rb \
spec/features/saml/ial2_sso_spec.rb

ial2_sso_spec.rb will fail.

Switch to this branch, and the same invocation will pass.

Tests can get stuck with mocked configs, leading to flakiness.

[skip changelog]
@matthinz matthinz requested a review from a team December 30, 2024 23:48
Copy link
Copy Markdown
Contributor

@Sgtpluck Sgtpluck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this add time onto the spec run? (am curious if there's a way to benchmark that.)

@matthinz
Copy link
Copy Markdown
Contributor Author

Based on a quick test on my laptop, 1000 A/B test reloads execute in less than 1 second:

it 'tests how fast reloading is' do
    start = Time.zone.now
    1000.times do
      reload_ab_tests
    end
    expect(Time.zone.now - start).to be < 1.second
end

So I don't think this will add any appreciable time to CI runs.

@matthinz matthinz merged commit a9e50e4 into main Dec 31, 2024
@matthinz matthinz deleted the matthinz/aggressive-ab-test-cleanup branch December 31, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants