fix(tests): make segment rule and condition ordering deterministic - #8454
Conversation
SegmentRule and Condition use ConfiguredOrderManager, which only adds
`.order_by("id")` when the explicit-ordering settings are enabled. They
default to off, so in tests the queryset carries no ordering at all and
rows come back in whatever order the database happens to return.
Several tests depend on that order — the LaunchDarkly `large_segments`
snapshots compare serialised `rules_data`, so any reordering of rules or
conditions changes the output. They pass today because Postgres happens to
return rows in insertion order for these queries, not because anything
guarantees it. That is latent flakiness: nothing stops a plan change, an
update, or autovacuum altering it.
Enable both settings for tests so the ordering the code implicitly expects
is the ordering it gets. This also makes the same suites deterministic on
MySQL and Oracle, which is how it was found — Flagsmith/flagsmith-ee#224.
Both derived names are set rather than the
SEGMENT_RULES_CONDITIONS_EXPLICIT_ORDERING_ENABLED parent, because common.py
resolves the derived settings at import time, so setting only the parent in
test.py has no effect. For the same reason this cannot be done per test:
ConfiguredOrderManager reads the value when the manager is constructed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe test settings enable Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change makes segment-rule and condition ordering deterministic in tests, reducing database-specific snapshot instability without changing production behavior. No merge-blocking risk remains. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8454 +/- ##
==========================================
- Coverage 98.81% 98.81% -0.01%
==========================================
Files 1621 1621
Lines 66297 66299 +2
==========================================
+ Hits 65513 65514 +1
- Misses 784 785 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20157 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #20157 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20157 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20157 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20154 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20154 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20154 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20154 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
2 participants
Add settings to make segment ordering deterministic in the tests. Without this, downstream issues arise when using other database engines (e.g. MySQL, and Oracle)