-
Notifications
You must be signed in to change notification settings - Fork 539
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimizations for alias tests which are flaky and timeout #23996
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR addresses flaky and intermittent timeouts in aliasing end‐to‐end tests by removing unnecessary synchronization calls and fixing a bug in the summarizer initialization order. Key changes include:
- Removing waitForContainerConnection and an extra provider.ensureSynchronized call in the beforeEach block.
- Adding the syncSummarizer: true option when getting the test object provider.
- Reordering the provider.ensureSynchronized call after creating the summarizer in one test to ensure proper summarizer synchronization.
Reviewed Changes
File | Description |
---|---|
packages/test/test-end-to-end-tests/src/test/rootDatastores.spec.ts | Removed redundant synchronization calls and reordered summarizer initialization to fix a bug |
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
packages/test/test-end-to-end-tests/src/test/rootDatastores.spec.ts:337
- [nitpick] The provider.ensureSynchronized() call has been moved after creating the summarizer to address a timing bug. Consider adding an inline comment here to clarify the importance of this ordering for future maintainers.
await provider.ensureSynchronized();
packages/test/test-end-to-end-tests/src/test/rootDatastores.spec.ts:46
- [nitpick] The introduction of the syncSummarizer: true option in the test object provider should be validated to ensure it doesn't introduce unintended delays or race conditions. It would be beneficial to add or update tests that cover the summarizer synchronization behavior.
provider = getTestObjectProvider({
There are a couple of aliasing end-to-end tests that are flaky and intermittently time out. See AB#31828 and AB#31831. This PR attempts to fix that by optimizing a couple of steps and fixing a bug:
beforeEach
, the test waits for container connection and callsprovider.ensureSynchronized
. Both of these are not needed so removing them.Assign multiple data stores to the same alias, first write wins, different containers
, aprovider.ensureSynchronized
is not needed so removing it.Assign multiple data stores to the same alias, first write wins, different containers from snapshot
there is a bug where the test expects the summarizer to summarize the data store that was created and aliased. However, that may not happen because theprovider.ensureSynchronized
is called before the summarizer is created - it should be called after.Also, the test object provider needs to be created with
syncSummarizer: true
option for it to sync changes to the summarizer client.timeoutAwait
so it's more debuggable.