Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

agarwal-navin
Copy link
Contributor

@agarwal-navin agarwal-navin commented Mar 6, 2025

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:

  1. In beforeEach, the test waits for container connection and calls provider.ensureSynchronized. Both of these are not needed so removing them.
  2. In Assign multiple data stores to the same alias, first write wins, different containers, a provider.ensureSynchronized is not needed so removing it.
  3. In 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 the provider.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.
  4. Changed the awaits in the flaky tests to timeoutAwait so it's more debuggable.

@agarwal-navin agarwal-navin requested review from jenn-le, a team, pragya91, Copilot, markfields, jason-ha, jatgarg, kian-thompson, WillieHabi and rajatch-ff and removed request for a team March 6, 2025 01:24
@agarwal-navin agarwal-navin changed the title Optimizations to alias test which are flaky and timeout Optimizations to alias tests which are flaky and timeout Mar 6, 2025
@github-actions github-actions bot added base: main PRs targeted against main branch area: tests Tests to add, test infrastructure improvements, etc labels Mar 6, 2025
@agarwal-navin agarwal-navin changed the title Optimizations to alias tests which are flaky and timeout Optimizations for alias tests which are flaky and timeout Mar 6, 2025

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({
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants