Fix flaky session test#1895
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the unified spec test runner’s initial data setup to decide when to advance cluster time by additionally considering session entities configured with snapshot: true, aiming to reduce flakiness in session-related unified spec tests.
Changes:
- Pass
createEntitiesintoisAdvanceClusterTimeNeeded()when preparinginitialData. - Extend
isAdvanceClusterTimeNeeded()to returntruewhen a snapshot session is defined increateEntities. - Expand the method’s docblock to describe why snapshot sessions may require cluster time advancement.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2.x #1895 +/- ##
============================================
+ Coverage 87.70% 87.72% +0.02%
- Complexity 3324 3328 +4
============================================
Files 452 454 +2
Lines 6644 6656 +12
============================================
+ Hits 5827 5839 +12
Misses 817 817
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Cluster time advancement is needed for snapshot sessions, since initialData uses an internal client that does not gossip its cluster time to test entities. Without advancement, a snapshot session may establish its atClusterTime before the initial data is visible, causing snapshot reads to return no results.
paulinevos
force-pushed
the
flake-fix
branch
2 times, most recently
from
1e3b4b3 to
2133e5c
Compare
|
The test is still failing 😢 |
| foreach ($createEntities ?? [] as $entity) { | ||
| $session = $entity->session ?? null; | ||
| if ($session === null) { | ||
| return false; |
There was a problem hiding this comment.
This should be a continue -- there could still be an entity with a snapshot session later in the list that might necessitate a cluster time advancement.
| * atClusterTime before the initial data is visible, causing snapshot reads to return no results. | ||
| */ | ||
| private function isAdvanceClusterTimeNeeded(array $operations): bool | ||
| private function isAdvanceClusterTimeNeeded(array $operations, ?array $createEntities = null): bool |
There was a problem hiding this comment.
I suspect the nullable array is because we have tests without entities, in which case we get null? Would it make sense to normalise that to an empty array before we pass it around?
| foreach ($createEntities ?? [] as $entity) { | ||
| $session = $entity->session ?? null; | ||
| if ($session === null) { | ||
| return false; |
There was a problem hiding this comment.
This is the reason the test is still failing after this PR.
The createEntities array in snapshot-sessions.json starts with client, database, and collection entries before the session entries:
[{client: {id: "client0", ...}}, {database: {...}}, {collection: {...}}, {session: {snapshot: true}}, ...]
When the loop reaches the first entity ({client: {...}}), $entity->session is null (the entity has no session key), so it hits return false and exits immediately — never reaching the actual snapshot sessions.
This should be continue (as @alcaeus noted) so the loop keeps checking remaining entities.
Cluster time advancement is needed for snapshot sessions, since initialData uses an internal client that does not gossip its cluster time to test entities. Without advancement, a snapshot session may establish its atClusterTime before the initial data is visible, causing snapshot reads to return no results.