Flaky tests fix: Migration shim can apply stashed v1 ops to v1 state and v2 ops to v2 state #26148
Conversation
MarioJGMsoft
requested review from
a team,
WillieHabi,
Copilot,
jason-ha,
jatgarg,
kian-thompson,
markfields and
steffenloesch
There was a problem hiding this comment.
Pull request overview
This PR fixes flaky tests by removing unnecessary synchronization calls in two test cases that validate stashed operation replay behavior. The tests were timing out due to ensureSynchronized() calls that weren't needed since the tests only verify local state.
Key Changes:
- Removed
await provider.ensureSynchronized()after loading container with pending state in "MigrationShim can apply stashed v1 ops to v1 state" test - Removed
await provider.ensureSynchronized()after loading container with pending state in "MigrationShim can apply stashed v2 ops to v2 state" test
|
|
||
| const loader = provider.createLoader([[provider.defaultCodeDetails, runtimeFactory2]]); | ||
| const container2 = await loader.resolve({ url }, pendingState); | ||
| await provider.ensureSynchronized(); |
There was a problem hiding this comment.
Without this, I think it's possible that container2 would be missing state from container1 that predates the local pending state...? But maybe that's not possible, I think it only applies pending state once all ops up to the refSeq are processed. So... maybe it is ok.
What exactly were the "timing issues" - timeouts? I'm unsure how else removing an await could fix the flakiness. And I also feel like if this were timing out we should open a bug to investigate root cause there. ensureSynchronized hanging usually indicates a bug (e.g. around dirty/saved state tracking)
There was a problem hiding this comment.
The issues were that the container2 wasn't being saved properly and it was left hanging, causing the test to timeout when it was ran against FRS
markfields
left a comment
markfields
left a comment
There was a problem hiding this comment.
Approving but please look at the comment I left and let's chat
2 participants
Description
Currently, both tests are being flaky in FRS because of timing issues with the container creation and the
await provider.ensureSynchronized()promise. Since the tests are only validating the local state ofcontainer2, we can remove the function call and fix the timeout issue.Fixes:
AB#49800
AB#49818
Reviewer Guidance
The review process is outlined on this wiki page.