MAYBE: Harden spec suite against test-order pollution (Faker reseed + Current reset + Faker-order address flake) - #2187
Open
maebeale wants to merge 3 commits into
Open
Conversation
…er pollution Current.user/source is only auto-reset by the executor around real requests/jobs. Controller/view/service specs set it in the test thread with no executor to clear it, so a stale Current leaks into later examples and silently flips model behavior that branches on it (Organization affiliation-lock validation, AhoyTrackable lifecycle tracking). Mirror the existing travel_back / Warden.test_reset! hooks with an after-each reset, plus an order:defined regression guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Faker::Config uses one shared, order-seeded stream (Random.new(42)), so a factory's random country/street/zip is determined by the example's position in that stream. Under some seeds (e.g. 62433) the existing address's country came out as 'Canada' — the exact value the upsert sets — so 'Country' dropped out of the reported changes and the example failed. Pin the changed fields to fixed, distinct values so the assertion is order-independent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
maebeale
changed the title
maebeale
marked this pull request as ready for review
August 13, 2026 13:57
Faker draws from one shared global RNG stream, so a factory's value depends on how many Faker calls ran earlier in the suite — making data order-dependent and specs flaky under some seeds (the upsert_address country=Canada collision was one instance). Reseed Random.new(42) before each example so every example draws the same fixed sequence regardless of order. Faker's .unique generator remembers used values for the whole run, so a bare reseed replays them and hits RetryLimitExceeded (1782 failures); clearing it per example fixes that. Verified green on the full suite (system specs included) at seeds 11334 and 18114: 6330 examples, 0 failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
maebeale
changed the title
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
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.
🤖 suggested review level: 3 Read 📖 spec-infra only (two after/before hooks + a factory-value pin); no app code — but touches all specs via the Faker hook, so worth a careful read
Hardens the spec suite against test-order pollution. Spec-only; no app code changes.
1. Reseed Faker per example (root fix for order-dependent faked data)
spec/support/faker.rbnow reseedsRandom.new(42)in abefore(:each)so every example draws the same fixed sequence regardless of order. Faker's.uniquegenerator remembers used values for the whole run, so a bare reseed replays them and hitsRetryLimitExceeded(that alone caused 1782 failures) — clearing it per example fixes that.2. Reset
Current(ActiveSupport::CurrentAttributes) between examplesCurrent.user/Current.sourceis only auto-reset by the executor around real requests/jobs. Controller/view/service specs set it in the test thread with nothing to clear it, so a staleCurrentleaks into later examples and silently flips model behavior that branches on it —Organization#affiliation_dates_locked(organization.rb:58) andAhoyTrackable(ahoy_trackable.rb:266).config.after { Current.reset }beside the existingtravel_back/Warden.test_reset!hooks, plus anorder: :definedregression spec (red without the hook, green with it).3. Pin address fields in
upsert_address_spec"Canada"— the value the upsert sets — so"Country"dropped from the reported changes. Pinned the changed fields to fixed, distinct values. Now also covered by fix but kept for clarity/robustness.Scope note
Currentor committed "AgeRange"CategoryType. Those look environment-specific (residual data in that workspace's test DB, or a selenium flake);RAILS_ENV=test bin/rails db:resetin that workspace is the likely remedy. The changes here fix the order-dependence hazards that are reproducible and real.