unify the default timeout

[skeeey]

No description provided.

[coderabbitai]

Walkthrough

The PR centralizes Eventually timing by setting global defaults in suite_test.go and removes explicit timeout and polling interval arguments from Eventually calls across multiple e2e test files; one AfterSuite cleanup Eventually was given a longer explicit WithTimeout(5 minutes). No functional logic or public signatures changed.

Changes

Cohort / File(s)	Summary
Test Suite Configuration test/e2e/pkg/suite_test.go	Added SetDefaultEventuallyTimeout(2 * time.Minute) and SetDefaultEventuallyPollingInterval(2 * time.Second) in BeforeSuite; changed the cleanupResources Eventually call in AfterSuite to use WithTimeout(5 * time.Minute) (longer explicit timeout).
E2E Tests — Eventually time args removed test/e2e/pkg/cert_rotation_test.go, test/e2e/pkg/resources_test.go, test/e2e/pkg/sourceclient_test.go, test/e2e/pkg/spec_resync_test.go, test/e2e/pkg/status_resync_test.go	Removed explicit timeout and polling interval parameters from multiple Eventually(...) assertions (e.g., 1*time.Minute, 1*time.Second and 30*time.Second, 1*time.Second), relying on the new global defaults. No other logic changes.
E2E Tests — grpc test/e2e/pkg/grpc_test.go	Removed explicit timeout/polling args from Eventually assertions and removed the now-unused time import. No other logic changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	No description was provided by the author; this is a minimal but acceptable omission since the title and changes clearly convey the intent.	Consider adding a brief description explaining why default timeouts are being unified and what the new defaults are (2 minute timeout, 2 second polling interval).

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'unify the default timeout' is directly related to the main change, which removes explicit timeout parameters across multiple test files and establishes default timeout/polling behavior in the suite setup.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test/e2e/pkg/resources_test.go (1)

307-318: ⚠️ Potential issue | 🟡 Minor

Add explicit duration to strengthen the "not updated" verification.

The test suite sets defaults for Eventually (2 minutes) but does not configure Consistently defaults. Without an explicit duration, Consistently uses Gomega's brief built-in default, which weakens the guarantee that the nested work was not updated over a meaningful observation window. Specify a duration to ensure sufficient observation time.

Proposed adjustment (example duration)

-			Consistently(func() error {
+			Consistently(func() error {
 				nestedWork, err := agentTestOpts.workClientSet.WorkV1().ManifestWorks(nestedWorkNamespace).Get(ctx, nestedWorkName, metav1.GetOptions{})
 				if err != nil {
 					return err
 				}
@@
-				return nil
-			}).Should(BeNil())
+				return nil
+			}, 10*time.Second, 1*time.Second).Should(BeNil())

🤖 Fix all issues with AI agents

In `@test/e2e/pkg/sourceclient_test.go`:
- Around line 206-211: The Consistently call used to assert
otherConsumerWatcherResult.WatchedWorks stays empty needs an explicit longer
duration; update the Consistently invocation in the test (the call that checks
otherConsumerWatcherResult.WatchedWorks) to include a meaningful timeout (e.g.,
5*time.Second) as a second argument and ensure the time package is imported if
missing so the assertion runs over the intended window rather than the 100ms
default.

In `@test/e2e/pkg/suite_test.go`:
- Around line 199-201: The AfterSuite cleanup uses Eventually without an
explicit timeout which can flake on slow clusters; update the call that invokes
cleanupResources via Eventually to set a longer explicit timeout (e.g., call
Eventually(...).WithTimeout(5 * time.Minute).ShouldNot(HaveOccurred())) so the
cleanupResources(ctx) invocation has enough time to list/delete ManifestWorks
and AppliedManifestWorks; modify the Eventually invocation wrapping
cleanupResources to include WithTimeout and import time if not already present.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

test/e2e/pkg/resources_test.go (1)

307-318: ⚠️ Potential issue | 🟠 Major

Add explicit duration to Consistently block to meaningfully verify nested work remains unchanged.

The SetDefaultEventuallyTimeout set in suite_test.go affects only Eventually, not Consistently. The default Consistently duration is 100ms, which allows only ~10 polling checks before completing. This is insufficient to meaningfully assert that the nested work generation remains stable.

Add explicit durations to ensure the assertion runs long enough to catch any spurious updates:

Proposed fix

 			Consistently(func() error {
 				nestedWork, err := agentTestOpts.workClientSet.WorkV1().ManifestWorks(nestedWorkNamespace).Get(ctx, nestedWorkName, metav1.GetOptions{})
 				if err != nil {
 					return err
 				}

 				if nestedWork.Generation != 1 {
 					return fmt.Errorf("nested work generation is changed to %d", nestedWork.Generation)
 				}

 				return nil
-			}).Should(BeNil())
+			}, 30*time.Second, 1*time.Second).Should(BeNil())

[skeeey]

/cc @clyang82

[clyang82]

LGTM