adding parrlised and test isolation#131
Conversation
WalkthroughConverts multiple e2e tests to run as parallel, isolated subtests with per-subtest namespaces, clients, and Gomega instances. Adds a new ParallelTest helper to manage parallel lifecycle, and refactors the test support API (generic Option and NewTestNamespace). Also adds timestamped test logging utility. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Caller
participant ParallelTest as ParallelTest helper
participant Namespace as K8s Namespace
participant Resources as ConfigMaps/Quota/Finalizers
participant Cleanup as Teardown
Test->>ParallelTest: WithParallel(t)
activate ParallelTest
note right of ParallelTest: Per-subtest lifecycle created
Test->>ParallelTest: NewTestNamespace()
ParallelTest->>Namespace: create isolated namespace
ParallelTest->>Resources: WithConfigMaps / WithResourceQuota / BeforeEach hooks
ParallelTest->>Test: provide subtest client, ctx, gomega
rect rgb(220,255,220)
Test->>Namespace: Run subtest logic (Apply/Get/Delete/Wait)
Namespace->>Test: status/events
end
ParallelTest->>Cleanup: execute registered cleanup (reverse order)
Cleanup->>Resources: delete objects, remove finalizers
Cleanup->>Namespace: collect logs/events, delete namespace
deactivate ParallelTest
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
f5e7e3d to
d4e2ac2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ray-operator/test/e2e/raycluster_gcs_ft_test.go (1)
315-316: Use the subtest’s testing.T with require.- require.False(t, tc.redisPasswordEnv != "" && tc.redisPasswordInRayStartParams != "" && tc.redisPasswordInRayStartParams != "$REDIS_PASSWORD", "redisPasswordEnv and redisPasswordInRayStartParams are both set") + require.False(subtest.T(), tc.redisPasswordEnv != "" && tc.redisPasswordInRayStartParams != "" && tc.redisPasswordInRayStartParams != "$REDIS_PASSWORD", "redisPasswordEnv and redisPasswordInRayStartParams are both set")
🧹 Nitpick comments (15)
ray-operator/test/e2e/rayjob_test.go (3)
19-19: Remove unused top-level WithParallel(t)This constructs a client and registers cleanup but the instance isn’t used. Drop it to avoid resource waste.
- _ = WithParallel(t) + // Subtests invoke WithParallel(t) themselves; no need to pre-create here.
24-24: Drop redundant t.Parallel() — WithParallel(t) already calls itKeep a single source of truth to avoid confusion.
Example for one subtest:
- t.Parallel() subtest := WithParallel(t)Please apply similarly at the other marked lines.
Also applies to: 85-85, 132-132, 176-176, 210-210, 233-233, 261-261
54-55: Add explicit timeouts to Eventually/Consistently to reduce flakesA few waits rely on Gomega defaults (short). Use the shared constants.
Suggested edits:
@@ - g.Eventually(RayJob(subtest, rayJob.Namespace, rayJob.Name)). + g.Eventually(RayJob(subtest, rayJob.Namespace, rayJob.Name), TestTimeoutMedium). Should(WithTransform(RayJobDeploymentStatus, Equal(rayv1.JobDeploymentStatusComplete))) @@ - g.Eventually(Jobs(subtest, namespace.Name)).ShouldNot(BeEmpty()) + g.Eventually(Jobs(subtest, namespace.Name), TestTimeoutShort).ShouldNot(BeEmpty()) @@ - g.Consistently(RayJob(subtest, rayJob.Namespace, rayJob.Name)). + g.Consistently(RayJob(subtest, rayJob.Namespace, rayJob.Name), 3*time.Second, 250*time.Millisecond). Should(WithTransform(RayJobDeploymentStatus, Equal(rayv1.JobDeploymentStatusComplete))) @@ - g.Eventually(RayJob(subtest, rayJob.Namespace, rayJob.Name)). + g.Eventually(RayJob(subtest, rayJob.Namespace, rayJob.Name), TestTimeoutMedium). Should(WithTransform(RayJobDeploymentStatus, Equal(rayv1.JobDeploymentStatusNew))) @@ - g.Eventually(Jobs(subtest, namespace.Name)).Should(BeEmpty()) + g.Eventually(Jobs(subtest, namespace.Name), TestTimeoutShort).Should(BeEmpty())Also applies to: 69-69, 75-77, 290-292, 299-299
ray-operator/test/e2e/rayjob_recovery_test.go (3)
27-27: Prefer WithNamespace() for consistency- namespace := subtest.NewTestNamespace() + namespace := subtest.WithNamespace()
49-51: Replace fixed sleep with a readiness EventuallyThe 15s sleep adds flakiness and test time. Poll for at least one running Job pod before deletion.
Example:
- LogWithTimestamp(t, "Sleep RayJob %s/%s 15 seconds", rayJob.Namespace, rayJob.Name) - time.Sleep(15 * time.Second) + LogWithTimestamp(t, "Waiting for at least one running submitter Pod for %s/%s", rayJob.Namespace, rayJob.Name) + g.Eventually(func() ([]corev1.Pod, error) { + pods, err := subtest.Client().Core().CoreV1().Pods(namespace.Name).List(subtest.Ctx(), + metav1.ListOptions{LabelSelector: fmt.Sprintf("job-name=%s", rayJob.Name)}) + return pods.Items, err + }, TestTimeoutMedium).Should(WithTransform(func(pods []corev1.Pod) bool { + for _, p := range pods { if p.Status.Phase == corev1.PodRunning { return true } } + return false + }, BeTrue()))
61-61: Tiny nit: double space in log message- LogWithTimestamp(t, "Delete Pod %s from namespace %s", pod.Name, rayJob.Namespace) + LogWithTimestamp(t, "Delete Pod %s from namespace %s", pod.Name, rayJob.Namespace)ray-operator/test/support/log.go (1)
11-16: LGTM — helpful, minimal logging utilityConsider optionally including t.Name() in the prefix when logs are aggregated from many parallel subtests, but t.Log already scopes by test name in -v runs. No action required.
ray-operator/test/e2e/rayjob_cluster_selector_test.go (1)
21-21: Remove redundant t.Parallel() — WithParallel(t) already marks parallel- t.Parallel() subtest := WithParallel(t)Also applies to: 66-66, 108-108, 154-154
ray-operator/test/e2e/rayjob_retry_test.go (3)
28-31: Redundant t.Parallel; WithParallel already sets parallel.Each subtest calls t.Parallel and then WithParallel(t), which also calls t.Parallel(). Keep either; prefer relying on WithParallel for consistency.
45-45: Use the subtest’s T for logging.Pass subtest.T() to LogWithTimestamp to keep logs grouped per subtest.
- LogWithTimestamp(t, "Created RayJob %s/%s successfully", rayJob.Namespace, rayJob.Name) + LogWithTimestamp(subtest.T(), "Created RayJob %s/%s successfully", rayJob.Namespace, rayJob.Name)Apply similarly to all shown lines.
Also applies to: 47-47, 78-78, 113-115, 147-147, 171-175, 203-206, 225-229
50-57: Deflake with Eventually for status/reason/counters.Several immediate g.Expect(GetRayJob(...)).To(...) checks race with controller updates. Prefer Eventually for these, as done elsewhere.
Example:
- g.Expect(GetRayJob(subtest, rayJob.Namespace, rayJob.Name)). - To(WithTransform(RayJobStatus, Equal(rayv1.JobStatusFailed))) + g.Eventually(GetRayJob(subtest, rayJob.Namespace, rayJob.Name), TestTimeoutMedium). + Should(WithTransform(RayJobStatus, Equal(rayv1.JobStatusFailed)))Apply the same pattern to Reason/Failed/Succeeded assertions in each subtest.
Also applies to: 60-63, 117-125, 127-131, 176-185, 220-223
ray-operator/test/e2e/raycluster_gcs_ft_test.go (2)
70-73: Use subtest.T() for logs to keep output scoped per subtest.- LogWithTimestamp(t, "Created RayCluster %s/%s successfully", rayCluster.Namespace, rayCluster.Name) + LogWithTimestamp(subtest.T(), "Created RayCluster %s/%s successfully", rayCluster.Namespace, rayCluster.Name)Apply to the other listed lines.
Also applies to: 79-83, 89-90, 247-254, 361-366
102-104: Add explicit timeout to Eventually.Provide a deterministic bound consistent with surrounding checks.
- g.Eventually(HeadPod(subtest, rayCluster)). + g.Eventually(HeadPod(subtest, rayCluster), TestTimeoutMedium). Should(WithTransform(PodState, Equal("Running")))ray-operator/test/e2e/raycluster_test.go (2)
155-160: Use subtest.T() for logs.- LogWithTimestamp(t, "Created RayCluster %s/%s successfully", rayCluster.Namespace, rayCluster.Name) + LogWithTimestamp(subtest.T(), "Created RayCluster %s/%s successfully", rayCluster.Namespace, rayCluster.Name)Apply to the other listed lines.
Also applies to: 187-193
205-206: Avoid fixed sleeps; use Eventually for stable scale-down assertions.Replace time.Sleep with Eventually on desired conditions to deflake under load.
Example:
- time.Sleep(5 * time.Second) + g.Eventually(func() int { + pods, _ := GetWorkerPods(subtest, rayCluster) + n := 0 + for _, p := range pods { + if p.DeletionTimestamp != nil { n++ } + } + return n + }, TestTimeoutMedium).Should(Equal(1))Keep the separate headPod non-deletion assertion with Eventually as well.
Also applies to: 213-220
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ray-operator/test/e2e/raycluster_gcs_ft_test.go(6 hunks)ray-operator/test/e2e/raycluster_test.go(1 hunks)ray-operator/test/e2e/rayjob_cluster_selector_test.go(5 hunks)ray-operator/test/e2e/rayjob_recovery_test.go(3 hunks)ray-operator/test/e2e/rayjob_retry_test.go(5 hunks)ray-operator/test/e2e/rayjob_suspend_test.go(3 hunks)ray-operator/test/e2e/rayjob_test.go(4 hunks)ray-operator/test/support/log.go(1 hunks)ray-operator/test/support/parallel_test.go(1 hunks)ray-operator/test/support/test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
ray-operator/test/e2e/rayjob_test.go (8)
ray-operator/test/support/parallel_test.go (1)
WithParallel(28-61)ray-operator/test/support/client.go (1)
Client(15-20)ray-operator/test/support/support.go (3)
TestApplyOptions(14-14)TestTimeoutMedium(17-17)TestTimeoutShort(16-16)ray-operator/test/support/log.go (1)
LogWithTimestamp(11-16)ray-operator/apis/ray/v1/rayjob_types.go (15)
RayJob(257-264)RayJobStatus(199-243)IsJobTerminal(36-42)JobStatusSucceeded(22-22)JobDeploymentStatusComplete(51-51)RayJobSpec(113-196)JobStatusFailed(23-23)JobDeploymentStatusFailed(52-52)AppFailed(75-75)JobStatusNew(18-18)SubmissionFailed(73-73)JobDeploymentStatusRunning(50-50)JobStatusStopped(21-21)JobDeploymentStatusNew(48-48)DeadlineExceeded(74-74)ray-operator/test/support/ray.go (7)
RayJob(17-21)RayJobStatus(27-29)GetRayJob(23-25)RayJobDeploymentStatus(31-33)GetRayCluster(67-69)RayJobReason(39-41)RayJobManagedBy(35-37)ray-operator/test/support/batch.go (1)
Jobs(9-15)ray-operator/controllers/ray/utils/constant.go (1)
KubeRayController(216-216)
ray-operator/test/e2e/raycluster_gcs_ft_test.go (5)
ray-operator/test/support/parallel_test.go (1)
WithParallel(28-61)ray-operator/test/support/support.go (1)
TestApplyOptions(14-14)ray-operator/test/support/log.go (1)
LogWithTimestamp(11-16)ray-operator/test/support/ray.go (4)
StatusCondition(75-86)MatchCondition(117-119)GetHeadPod(135-147)HeadPod(129-133)ray-operator/test/support/core.go (2)
ExecPodCmd(71-104)Pods(21-33)
ray-operator/test/e2e/raycluster_test.go (6)
ray-operator/test/support/parallel_test.go (1)
WithParallel(28-61)ray-operator/test/support/ray.go (8)
RayCluster(61-65)RayClusterState(71-73)RayClusterManagedBy(192-194)StatusCondition(75-86)MatchCondition(117-119)MatchConditionContainsMessage(121-123)GetHeadPod(135-147)GetWorkerPods(155-164)ray-operator/controllers/ray/utils/constant.go (2)
KubeRayController(216-216)ErrFailedCreateHeadPod(257-257)ray-operator/test/support/support.go (1)
TestApplyOptions(14-14)ray-operator/test/support/log.go (1)
LogWithTimestamp(11-16)ray-operator/test/support/environment.go (1)
GetRayVersion(23-25)
ray-operator/test/e2e/rayjob_recovery_test.go (3)
ray-operator/test/support/parallel_test.go (1)
WithParallel(28-61)ray-operator/test/support/log.go (1)
LogWithTimestamp(11-16)ray-operator/test/support/core.go (1)
Pods(21-33)
ray-operator/test/e2e/rayjob_retry_test.go (6)
ray-operator/test/support/parallel_test.go (1)
WithParallel(28-61)ray-operator/test/support/log.go (1)
LogWithTimestamp(11-16)ray-operator/test/support/client.go (1)
Client(15-20)ray-operator/test/support/support.go (4)
TestApplyOptions(14-14)TestTimeoutLong(18-18)TestTimeoutShort(16-16)TestTimeoutMedium(17-17)ray-operator/test/support/ray.go (8)
RayJob(17-21)RayJobDeploymentStatus(31-33)GetRayJob(23-25)RayJobStatus(27-29)RayJobReason(39-41)RayJobFailed(43-48)RayJobSucceeded(50-55)GetRayCluster(67-69)ray-operator/test/support/batch.go (1)
Jobs(9-15)
ray-operator/test/e2e/rayjob_suspend_test.go (7)
ray-operator/test/support/parallel_test.go (1)
WithParallel(28-61)ray-operator/test/support/client.go (1)
Client(15-20)ray-operator/test/support/support.go (3)
TestApplyOptions(14-14)TestTimeoutMedium(17-17)TestTimeoutShort(16-16)ray-operator/test/support/log.go (1)
LogWithTimestamp(11-16)ray-operator/test/support/ray.go (5)
RayJob(17-21)RayJobDeploymentStatus(31-33)GetRayCluster(67-69)GetRayJob(23-25)RayJobStatus(27-29)ray-operator/test/support/batch.go (1)
Jobs(9-15)ray-operator/test/support/core.go (1)
Pods(21-33)
ray-operator/test/e2e/rayjob_cluster_selector_test.go (7)
ray-operator/test/support/parallel_test.go (1)
WithParallel(28-61)ray-operator/pkg/client/applyconfiguration/ray/v1/raycluster.go (1)
RayCluster(22-29)ray-operator/test/support/ray.go (5)
RayCluster(61-65)RayClusterState(71-73)RayJob(17-21)RayJobStatus(27-29)GetRayJob(23-25)ray-operator/test/support/support.go (2)
TestApplyOptions(14-14)TestTimeoutMedium(17-17)ray-operator/test/support/log.go (1)
LogWithTimestamp(11-16)ray-operator/pkg/client/applyconfiguration/ray/v1/rayjob.go (1)
RayJob(22-29)ray-operator/apis/ray/v1/rayjob_types.go (7)
RayJob(257-264)RayJobStatus(199-243)IsJobTerminal(36-42)JobStatusSucceeded(22-22)JobStatusFailed(23-23)JobDeploymentStatus(45-45)JobDeploymentStatusNew(48-48)
ray-operator/test/support/parallel_test.go (2)
ray-operator/test/support/test.go (1)
Option(12-12)ray-operator/test/support/core.go (1)
Pods(21-33)
ray-operator/test/support/test.go (1)
ray-operator/test/support/client.go (1)
Client(15-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Ray Kubectl plugin
- GitHub Check: Build Apiserver and Docker Images
🔇 Additional comments (3)
ray-operator/test/e2e/rayjob_recovery_test.go (1)
23-24: The review comment is factually incorrect about the WithConfigMaps return signature.The function definition at
ray-operator/test/support/parallel_test.go:220showsWithConfigMapsreturns a single value (*corev1.ConfigMap), not two values. Lines 18 and 23 inrayjob_recovery_test.goattempt to unpack two values from a one-value return, which is a Go assignment arity error. The proposed diff compounds this by adding_to capture a non-existent second return value.The correct fix is:
subtest := WithParallel(t) jobs := subtest.WithConfigMaps("long_running_counter.py")This matches the actual function signature and aligns with most other call sites in the codebase (e.g.,
rayjob_suspend_test.go,rayjob_cluster_selector_test.go).Likely an incorrect or invalid review comment.
ray-operator/test/e2e/rayjob_cluster_selector_test.go (2)
22-23: Dismiss this review comment; WithConfigMaps returns a single value, not two.The function signature is
func (pt *ParallelTest) WithConfigMaps(fileNames ...string) *corev1.ConfigMap, returning one value of type*corev1.ConfigMap. The review's claim that "elsewhere the function returns (namespace, jobs)" is incorrect. The current code patternjobs := subtest.WithConfigMaps(...)is correct and should not be changed; the suggested fix would introduce a compile error.Likely an incorrect or invalid review comment.
60-62: The review comment is incorrect — this is not a compile errorThe code uses valid gomega syntax.
Expect()with functions returning multiple values applies the matcher to the first return value and requires subsequent returns (the error) to be nil. The widespread usage of this pattern across 25+ instances in the codebase confirms it compiles and runs successfully.While refactoring to explicitly unwrap and check the error first could improve clarity, the current code is not invalid.
Likely an incorrect or invalid review comment.
| _ = WithParallel(t) | ||
|
|
||
| // Create a namespace | ||
| namespace := test.NewTestNamespace() | ||
| testScriptAC := newConfigMap(namespace.Name, files(test, "test_detached_actor_1.py", "test_detached_actor_2.py")) | ||
| testScript, err := test.Client().Core().CoreV1().ConfigMaps(namespace.Name).Apply(test.Ctx(), testScriptAC, TestApplyOptions) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| // Create a namespace and ConfigMap | ||
| test := WithParallel(t) | ||
| namespace := test.WithNamespace() | ||
| _, testScript := test.WithConfigMaps("test_detached_actor_1.py", "test_detached_actor_2.py") |
There was a problem hiding this comment.
Fix redundant WithParallel call and incorrect multiple assignment.
- Remove the no-op
_ = WithParallel(t). - WithConfigMaps returns a single value; assign to one variable.
- _ = WithParallel(t)
-
// Create a namespace and ConfigMap
test := WithParallel(t)
- namespace := test.WithNamespace()
- _, testScript := test.WithConfigMaps("test_detached_actor_1.py", "test_detached_actor_2.py")
+ namespace := test.WithNamespace()
+ testScript := test.WithConfigMaps("test_detached_actor_1.py", "test_detached_actor_2.py")Note: WithConfigMaps must create immediately in the current namespace; see suggested support changes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ = WithParallel(t) | |
| // Create a namespace | |
| namespace := test.NewTestNamespace() | |
| testScriptAC := newConfigMap(namespace.Name, files(test, "test_detached_actor_1.py", "test_detached_actor_2.py")) | |
| testScript, err := test.Client().Core().CoreV1().ConfigMaps(namespace.Name).Apply(test.Ctx(), testScriptAC, TestApplyOptions) | |
| g.Expect(err).NotTo(HaveOccurred()) | |
| // Create a namespace and ConfigMap | |
| test := WithParallel(t) | |
| namespace := test.WithNamespace() | |
| _, testScript := test.WithConfigMaps("test_detached_actor_1.py", "test_detached_actor_2.py") | |
| // Create a namespace and ConfigMap | |
| test := WithParallel(t) | |
| namespace := test.WithNamespace() | |
| testScript := test.WithConfigMaps("test_detached_actor_1.py", "test_detached_actor_2.py") |
🤖 Prompt for AI Agents
In ray-operator/test/e2e/raycluster_gcs_ft_test.go around lines 26 to 31, remove
the redundant no-op `_ = WithParallel(t)` and change the multiple-assignment to
a single variable by calling the method on the test instance: call
`test.WithConfigMaps("test_detached_actor_1.py", "test_detached_actor_2.py")`
and assign its single return value to `testScript`; ensure you call the method
on the `test` (namespace-scoped) helper so the ConfigMap(s) are created
immediately in the current namespace.
| t.Parallel() | ||
| subtest := WithParallel(t). | ||
| WithResourceQuota("test-quota", "0.1", "0.1Gi") | ||
| g := subtest.Gomega() | ||
|
|
There was a problem hiding this comment.
*Chaining requires WithResourceQuota to return ParallelTest.
This assignment won’t compile unless WithResourceQuota returns *ParallelTest. Update the helper to return the receiver (see support diff). No change needed here once helper is fixed.
🤖 Prompt for AI Agents
In ray-operator/test/e2e/raycluster_test.go around lines 142 to 146, the
chaining of WithParallel(t).WithResourceQuota(...) requires WithResourceQuota to
return *ParallelTest; update the WithResourceQuota helper (the ParallelTest
method) signature to return *ParallelTest and ensure it returns the receiver at
the end (e.g., return p) so the method is chainable; keep the method behavior
the same otherwise.
| test, _ := WithParallel(t).WithConfigMaps("long_running_counter.py") | ||
| g := test.Gomega() | ||
|
|
There was a problem hiding this comment.
Remove unused pre-work; it will not compile
The variables created here are unused and WithParallel(t).WithConfigMaps creates resources you don’t use.
- test, _ := WithParallel(t).WithConfigMaps("long_running_counter.py")
- g := test.Gomega()
+ // Subtests set up their own isolated context and resources.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test, _ := WithParallel(t).WithConfigMaps("long_running_counter.py") | |
| g := test.Gomega() | |
| // Subtests set up their own isolated context and resources. |
🤖 Prompt for AI Agents
In ray-operator/test/e2e/rayjob_recovery_test.go around lines 18 to 20, the call
assigning test, _ := WithParallel(t).WithConfigMaps("long_running_counter.py")
and the following g := test.Gomega() are unused and cause compilation failures;
remove these unused pre-work lines (or, alternatively, if the intent was to use
the test harness, replace the unused assignment by invoking the necessary setup
functions where their results are actually consumed). Ensure no unused variables
remain and that any created ConfigMap resources are either used later in the
test or the setup call is removed.
| // Create a namespace | ||
| namespace := test.NewTestNamespace() | ||
|
|
||
| // Job scripts | ||
| jobsAC := newConfigMap(namespace.Name, files(test, "fail.py")) | ||
| jobs, err := test.Client().Core().CoreV1().ConfigMaps(namespace.Name).Apply(test.Ctx(), jobsAC, TestApplyOptions) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| LogWithTimestamp(test.T(), "Created ConfigMap %s/%s successfully", jobs.Namespace, jobs.Name) | ||
| jobs := test.NewConfigMap(namespace.Name, jobsAC.Name, jobsAC.Data) | ||
| LogWithTimestamp(t, "Created ConfigMap %s/%s successfully", jobs.Namespace, jobs.Name) | ||
|
|
There was a problem hiding this comment.
Per-subtest isolation is broken (shared namespace/ConfigMap). Move setup inside each subtest.
A single namespace and ConfigMap are created once and reused by all parallel subtests, causing interference and flakes. Create them inside each subtest with the subtest context.
Example patch (first subtest shown; replicate for others):
- // Create a namespace
- namespace := test.NewTestNamespace()
-
- // Job scripts
- jobsAC := newConfigMap(namespace.Name, files(test, "fail.py"))
- jobs := test.NewConfigMap(namespace.Name, jobsAC.Name, jobsAC.Data)
- LogWithTimestamp(t, "Created ConfigMap %s/%s successfully", jobs.Namespace, jobs.Name)
+ // subtest-scoped resources
+ namespace := subtest.NewTestNamespace()
+ jobsAC := newConfigMap(namespace.Name, files(subtest, "fail.py"))
+ jobs := subtest.NewConfigMap(namespace.Name, jobsAC.Name, jobsAC.Data)
+ LogWithTimestamp(subtest.T(), "Created ConfigMap %s/%s successfully", jobs.Namespace, jobs.Name)Apply the same pattern to all subtests to ensure isolation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ray-operator/test/e2e/rayjob_retry_test.go around lines 19-26, the test
currently creates a single namespace and ConfigMap once and reuses them across
parallel subtests causing interference; move creation of the namespace and the
ConfigMap inside each subtest closure so each subtest gets its own
test.NewTestNamespace() and newConfigMap(...), perform creation/registration
using the subtest's context, call t.Parallel() only after setup (or omit if
using shared setup), and ensure you clean up or rely on the test harness
cleanup; apply this pattern to every subtest to guarantee per-subtest isolation.
| subtest := WithParallel(t) | ||
| jobs := subtest.WithConfigMaps("long_running.py", "counter.py") | ||
| g := subtest.Gomega() |
There was a problem hiding this comment.
Fix WithConfigMaps assignment arity and drop redundant t.Parallel()
Use the two-value form consistently and let WithParallel mark parallelism.
- t.Parallel()
- subtest := WithParallel(t)
- jobs := subtest.WithConfigMaps("long_running.py", "counter.py")
+ subtest := WithParallel(t)
+ _, jobs := subtest.WithConfigMaps("long_running.py", "counter.py")
@@
- t.Parallel()
- subtest := WithParallel(t)
- jobs := subtest.WithConfigMaps("long_running.py", "counter.py")
+ subtest := WithParallel(t)
+ _, jobs := subtest.WithConfigMaps("long_running.py", "counter.py")Also applies to: 78-80
🤖 Prompt for AI Agents
In ray-operator/test/e2e/rayjob_suspend_test.go around lines 21-23 (and
similarly at 78-80), the call to WithConfigMaps is using the single-value
assignment and t.Parallel() is being invoked redundantly; change these to the
two-value form (e.g., subtest, jobs := WithParallel(t) followed by jobs :=
subtest.WithConfigMaps(...)? -- instead use subtest, jobs := WithParallel(t)
then jobs, err := subtest.WithConfigMaps("long_running.py","counter.py")?);
remove any explicit t.Parallel() calls so WithParallel controls parallelism.
Ensure the variables are assigned with the correct two-value returns from
WithParallel/WithConfigMaps and adjust subsequent code to use the returned
instances accordingly.
| // NewTestNamespace creates a new namespace for this test with a unique name | ||
| func (pt *ParallelTest) NewTestNamespace(opts ...Option[*corev1.Namespace]) *corev1.Namespace { | ||
| pt.t.Helper() | ||
|
|
||
| name := fmt.Sprintf("test-%s", pt.t.Name()) | ||
| name = strings.Map(func(r rune) rune { | ||
| if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' { | ||
| return r | ||
| } | ||
| return '-' | ||
| }, name) | ||
| if len(name) > 63 { | ||
| name = name[:63] | ||
| } | ||
| name = strings.TrimSuffix(name, "-") | ||
|
|
||
| namespace := &corev1.Namespace{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| }, | ||
| } | ||
|
|
||
| for _, opt := range opts { | ||
| opt(namespace) | ||
| } | ||
|
|
||
| namespace, err := pt.client.Core().CoreV1().Namespaces().Create(pt.ctx, namespace, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| pt.t.Fatalf("Error creating namespace: %v", err) | ||
| } | ||
|
|
||
| for _, fn := range pt.beforeEachFns { | ||
| if err := fn(namespace); err != nil { | ||
| pt.t.Fatalf("BeforeEach failed: %v", err) | ||
| } | ||
| } | ||
|
|
||
| pt.cleanup = append(pt.cleanup, func() { | ||
| storeAllPodLogs(pt, namespace) | ||
| storeEvents(pt, namespace) | ||
|
|
||
| err := pt.client.Core().CoreV1().Namespaces().Delete(pt.ctx, namespace.Name, metav1.DeleteOptions{}) | ||
| if err != nil { | ||
| pt.t.Errorf("Error deleting namespace: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| return namespace | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Ensure BeforeEach runs and remember the namespace; fail early on errors.
Set lastNamespace and run beforeEachFns; keep behavior.
func (pt *ParallelTest) NewTestNamespace(opts ...Option[*corev1.Namespace]) *corev1.Namespace {
pt.t.Helper()
@@
- for _, opt := range opts {
- opt(namespace)
- }
+ for _, opt := range opts {
+ if err := opt.applyTo(namespace); err != nil {
+ pt.t.Fatalf("Option failed: %v", err)
+ }
+ }
@@
namespace, err := pt.client.Core().CoreV1().Namespaces().Create(pt.ctx, namespace, metav1.CreateOptions{})
if err != nil {
pt.t.Fatalf("Error creating namespace: %v", err)
}
+ pt.lastNamespace = namespace
+
for _, fn := range pt.beforeEachFns {
if err := fn(namespace); err != nil {
pt.t.Fatalf("BeforeEach failed: %v", err)
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ray-operator/test/support/parallel_test.go around lines 63 to 111, set
pt.lastNamespace = namespace before executing the beforeEachFns and ensure each
beforeEach function is run immediately and any error causes an immediate test
failure (use pt.t.Fatalf on error) so we fail early and preserve existing
cleanup behavior; make sure pt.cleanup is still appended (so namespace and logs
are stored and deletion attempted) and that namespace creation remains
unchanged.
| // WithNamespace creates a new namespace with a unique name based on the test name | ||
| // This is a convenience wrapper around BeforeEach for creating and cleaning up namespaces | ||
| func (pt *ParallelTest) WithNamespace() *corev1.Namespace { | ||
| name := fmt.Sprintf("test-%s", pt.t.Name()) | ||
| name = strings.Map(func(r rune) rune { | ||
| if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' { | ||
| return r | ||
| } | ||
| return '-' | ||
| }, name) | ||
| if len(name) > 63 { | ||
| name = name[:63] | ||
| } | ||
| name = strings.TrimSuffix(name, "-") | ||
|
|
||
| namespace := &corev1.Namespace{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| }, | ||
| } | ||
|
|
||
| namespace, err := pt.client.Core().CoreV1().Namespaces().Create(pt.ctx, namespace, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| pt.t.Fatalf("Error creating namespace: %v", err) | ||
| } | ||
|
|
||
| pt.cleanup = append(pt.cleanup, func() { | ||
| storeAllPodLogs(pt, namespace) | ||
| storeEvents(pt, namespace) | ||
|
|
||
| err := pt.client.Core().CoreV1().Namespaces().Delete(pt.ctx, namespace.Name, metav1.DeleteOptions{}) | ||
| if err != nil { | ||
| pt.t.Errorf("Error deleting namespace: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| return namespace | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Delegate WithNamespace to NewTestNamespace to execute BeforeEach and avoid duplication.
func (pt *ParallelTest) WithNamespace() *corev1.Namespace {
- name := fmt.Sprintf("test-%s", pt.t.Name())
- ...
- return namespace
+ return pt.NewTestNamespace()
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ray-operator/test/support/parallel_test.go around lines 179 to 216, the
WithNamespace implementation duplicates namespace creation and cleanup logic;
replace its body to delegate to the existing NewTestNamespace helper so
BeforeEach runs and duplication is removed. Change WithNamespace to simply call
and return the value from NewTestNamespace (ensuring any required
context/receiver usage matches the helper signature) and remove the duplicated
create/cleanup code block so that namespace creation, log/event storage, and
deletion are handled by NewTestNamespace.
| // WithConfigMaps creates ConfigMaps in the test namespace with the given files | ||
| // This is a convenience wrapper around BeforeEach for the common case of creating ConfigMaps | ||
| func (pt *ParallelTest) WithConfigMaps(fileNames ...string) *corev1.ConfigMap { | ||
| var ctx testContext | ||
| pt.BeforeEach(func(namespace *corev1.Namespace) error { | ||
| name := fmt.Sprintf("test-%s", pt.t.Name()) | ||
| name = strings.Map(func(r rune) rune { | ||
| if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' { | ||
| return r | ||
| } | ||
| return '-' | ||
| }, name) | ||
| if len(name) > 63 { | ||
| name = name[:63] | ||
| } | ||
| name = strings.TrimSuffix(name, "-") | ||
|
|
||
| configMap := &corev1.ConfigMap{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| Namespace: namespace.Name, | ||
| }, | ||
| Data: make(map[string]string), | ||
| } | ||
|
|
||
| for _, fileName := range fileNames { | ||
| data, err := os.ReadFile(filepath.Join("test", "data", fileName)) | ||
| if err != nil { | ||
| pt.t.Fatalf("Error reading file %s: %v", fileName, err) | ||
| } | ||
| configMap.Data[fileName] = string(data) | ||
| } | ||
|
|
||
| configMap, err := pt.client.Core().CoreV1().ConfigMaps(namespace.Name).Create(pt.ctx, configMap, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| pt.t.Fatalf("Error creating ConfigMap: %v", err) | ||
| } | ||
|
|
||
| pt.cleanup = append(pt.cleanup, func() { | ||
| err := pt.client.Core().CoreV1().ConfigMaps(namespace.Name).Delete(pt.ctx, configMap.Name, metav1.DeleteOptions{}) | ||
| if err != nil { | ||
| pt.t.Errorf("Error deleting ConfigMap: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| ctx.configMap = configMap | ||
| return nil | ||
| }) | ||
| return ctx.configMap | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Create ConfigMap immediately in the current namespace; don’t rely on deferred BeforeEach.
Fail fast if a namespace hasn’t been created yet.
func (pt *ParallelTest) WithConfigMaps(fileNames ...string) *corev1.ConfigMap {
- var ctx testContext
- pt.BeforeEach(func(namespace *corev1.Namespace) error {
- name := fmt.Sprintf("test-%s", pt.t.Name())
- ...
- configMap, err := pt.client.Core().CoreV1().ConfigMaps(namespace.Name).Create(pt.ctx, configMap, metav1.CreateOptions{})
- ...
- ctx.configMap = configMap
- return nil
- })
- return ctx.configMap
+ if pt.lastNamespace == nil {
+ pt.t.Fatalf("WithConfigMaps requires a namespace; call WithNamespace() or NewTestNamespace() first")
+ }
+ name := fmt.Sprintf("test-%s", pt.t.Name())
+ name = strings.TrimSuffix(strings.Map(func(r rune) rune {
+ if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' { return r }
+ return '-'
+ }, name), "-")
+ cmData := make(map[string]string)
+ for _, fileName := range fileNames {
+ data, err := os.ReadFile(filepath.Join("test", "data", fileName))
+ if err != nil {
+ pt.t.Fatalf("Error reading file %s: %v", fileName, err)
+ }
+ cmData[fileName] = string(data)
+ }
+ return pt.NewConfigMap(pt.lastNamespace.Name, name, cmData)
}Committable suggestion skipped: line range outside the PR's diff.
| // WithResourceQuota creates a resource quota in the test namespace | ||
| // This is a convenience wrapper around BeforeEach for the common case of creating resource quotas | ||
| func (pt *ParallelTest) WithResourceQuota(name string, cpu string, memory string) { | ||
| pt.BeforeEach(func(namespace *corev1.Namespace) error { | ||
| quota := &corev1.ResourceQuota{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| Namespace: namespace.Name, | ||
| }, | ||
| Spec: corev1.ResourceQuotaSpec{ | ||
| Hard: corev1.ResourceList{ | ||
| corev1.ResourceCPU: resource.MustParse(cpu), | ||
| corev1.ResourceMemory: resource.MustParse(memory), | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| quota, err := pt.client.Core().CoreV1().ResourceQuotas(namespace.Name).Create(pt.ctx, quota, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| pt.t.Fatalf("Error creating resource quota: %v", err) | ||
| } | ||
|
|
||
| pt.cleanup = append(pt.cleanup, func() { | ||
| err := pt.client.Core().CoreV1().ResourceQuotas(namespace.Name).Delete(pt.ctx, quota.Name, metav1.DeleteOptions{}) | ||
| if err != nil { | ||
| pt.t.Errorf("Error deleting resource quota: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| return nil | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Make WithResourceQuota chainable and ensure cleanup runs; execute via BeforeEach.
-func (pt *ParallelTest) WithResourceQuota(name string, cpu string, memory string) {
+func (pt *ParallelTest) WithResourceQuota(name string, cpu string, memory string) *ParallelTest {
pt.BeforeEach(func(namespace *corev1.Namespace) error {
quota := &corev1.ResourceQuota{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace.Name,
},
...
})
return nil
})
+ return pt
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // WithResourceQuota creates a resource quota in the test namespace | |
| // This is a convenience wrapper around BeforeEach for the common case of creating resource quotas | |
| func (pt *ParallelTest) WithResourceQuota(name string, cpu string, memory string) { | |
| pt.BeforeEach(func(namespace *corev1.Namespace) error { | |
| quota := &corev1.ResourceQuota{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: name, | |
| Namespace: namespace.Name, | |
| }, | |
| Spec: corev1.ResourceQuotaSpec{ | |
| Hard: corev1.ResourceList{ | |
| corev1.ResourceCPU: resource.MustParse(cpu), | |
| corev1.ResourceMemory: resource.MustParse(memory), | |
| }, | |
| }, | |
| } | |
| quota, err := pt.client.Core().CoreV1().ResourceQuotas(namespace.Name).Create(pt.ctx, quota, metav1.CreateOptions{}) | |
| if err != nil { | |
| pt.t.Fatalf("Error creating resource quota: %v", err) | |
| } | |
| pt.cleanup = append(pt.cleanup, func() { | |
| err := pt.client.Core().CoreV1().ResourceQuotas(namespace.Name).Delete(pt.ctx, quota.Name, metav1.DeleteOptions{}) | |
| if err != nil { | |
| pt.t.Errorf("Error deleting resource quota: %v", err) | |
| } | |
| }) | |
| return nil | |
| }) | |
| } | |
| // WithResourceQuota creates a resource quota in the test namespace | |
| // This is a convenience wrapper around BeforeEach for the common case of creating resource quotas | |
| func (pt *ParallelTest) WithResourceQuota(name string, cpu string, memory string) *ParallelTest { | |
| pt.BeforeEach(func(namespace *corev1.Namespace) error { | |
| quota := &corev1.ResourceQuota{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: name, | |
| Namespace: namespace.Name, | |
| }, | |
| Spec: corev1.ResourceQuotaSpec{ | |
| Hard: corev1.ResourceList{ | |
| corev1.ResourceCPU: resource.MustParse(cpu), | |
| corev1.ResourceMemory: resource.MustParse(memory), | |
| }, | |
| }, | |
| } | |
| quota, err := pt.client.Core().CoreV1().ResourceQuotas(namespace.Name).Create(pt.ctx, quota, metav1.CreateOptions{}) | |
| if err != nil { | |
| pt.t.Fatalf("Error creating resource quota: %v", err) | |
| } | |
| pt.cleanup = append(pt.cleanup, func() { | |
| err := pt.client.Core().CoreV1().ResourceQuotas(namespace.Name).Delete(pt.ctx, quota.Name, metav1.DeleteOptions{}) | |
| if err != nil { | |
| pt.t.Errorf("Error deleting resource quota: %v", err) | |
| } | |
| }) | |
| return nil | |
| }) | |
| return pt | |
| } |
🤖 Prompt for AI Agents
ray-operator/test/support/parallel_test.go around lines 269-301: make
WithResourceQuota chainable and ensure cleanup runs by changing its signature to
return *ParallelTest, register the creation logic via pt.BeforeEach as it
already does, replace the pt.t.Fatalf call inside the BeforeEach handler with
returning the error (e.g. return fmt.Errorf("creating resource quota: %w", err))
so the BeforeEach can handle failures without calling Fatalf (which would skip
cleanup), add fmt to imports if needed, and at the end of WithResourceQuota
return pt so callers can chain calls.
| // Option is a function that modifies a value of type T | ||
| // This pattern allows for flexible configuration of objects with optional settings | ||
| type Option[T any] func(T) | ||
|
|
There was a problem hiding this comment.
Option type is incompatible with existing callers (expects applyTo).
Other helpers (e.g., Pods in core.go) call option.applyTo(...). Restore the interface shape to avoid compile errors.
-// Option is a function that modifies a value of type T
-// This pattern allows for flexible configuration of objects with optional settings
-type Option[T any] func(T)
+// Option modifies a target value; implementations may validate and return errors.
+type Option[T any] interface {
+ applyTo(T) error
+}
+
+// OptionFunc adapts a function into an Option.
+type OptionFunc[T any] func(T) error
+
+func (f OptionFunc[T]) applyTo(to T) error { return f(to) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Option is a function that modifies a value of type T | |
| // This pattern allows for flexible configuration of objects with optional settings | |
| type Option[T any] func(T) | |
| // Option modifies a target value; implementations may validate and return errors. | |
| type Option[T any] interface { | |
| applyTo(T) error | |
| } | |
| // OptionFunc adapts a function into an Option. | |
| type OptionFunc[T any] func(T) error | |
| func (f OptionFunc[T]) applyTo(to T) error { return f(to) } |
🤖 Prompt for AI Agents
In ray-operator/test/support/test.go around lines 10 to 13, the current Option
type is declared as a function type (type Option[T any] func(T)) which breaks
callers that expect an Option with an applyTo method (e.g.,
option.applyTo(...)). Change the API back to an interface shape: declare
Option[T any] as an interface with an applyTo(T) method, and update any existing
helper implementations (previously functions) to either implement that interface
(add a small struct or method wrapper) or provide a thin adapter that implements
applyTo by invoking the existing function, so all callers using
option.applyTo(...) compile without errors.
1 participant
some suggested changes to improve flaky e2e tests
Summary by CodeRabbit
Tests
Refactor