Skip to content

test: replace fixed wait in RayCluster scale-down test - #5045

Open
Smallfu666 wants to merge 1 commit into
ray-project:masterfrom
Smallfu666:fix/raycluster-scale-down-wait
Open

test: replace fixed wait in RayCluster scale-down test#5045
Smallfu666 wants to merge 1 commit into
ray-project:masterfrom
Smallfu666:fix/raycluster-scale-down-wait

Conversation

@Smallfu666

Copy link
Copy Markdown

Why are these changes needed?

TestRayClusterScalingDown currently waits for a fixed five seconds before checking that exactly one worker Pod has been marked for deletion.

This change replaces the fixed sleep with the existing Gomega Eventually polling mechanism and waits directly for the same worker deletion condition. The redundant second worker Pod query and manual count assertion are removed.

This only changes E2E test synchronization. Production behavior is unchanged.

Related issue number

N/A — small test synchronization cleanup.

Labels

  • If this PR has user-facing changes that require documentation updates at release time, I have added the doc-updates-required label.
  • If this PR contains breaking changes, I have added the breaking-change label.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

Manual test instructions

Ran the targeted E2E test locally on kind:

KUBECONFIG=~/kubeconfig-kind \
KUBERAY_TEST_TIMEOUT_SHORT=1m \
go test -v -count=1 -timeout 30m \
-run '^TestRayClusterScalingDown$' ./test/e2e/

Results:

  • 20/20 development validation runs passed
  • 3/3 final smoke-test runs passed

@win5923 win5923 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@machichima machichima left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines +188 to +204
g.Eventually(func() (int, error) {
workerPods, err := GetWorkerPods(test, rayCluster)
if err != nil {
return 0, err
}

deletingCount := 0
for _, pod := range workerPods {
if pod.DeletionTimestamp != nil {
deletingCount++
}
}
return deletingCount, nil
}, TestTimeoutShort).Should(
Equal(1),
"Should have only one worker pod having deletionTimestamp",
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we simplify to this?

	g.Eventually(WorkerPods(test, rayCluster), TestTimeoutShort).Should(
		ConsistOf(
			HaveField("DeletionTimestamp", BeNil()),
			HaveField("DeletionTimestamp", Not(BeNil())),
		),
		"Should have only one worker pod having deletionTimestamp",
	)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as suggested—thanks!

@Smallfu666
Smallfu666 force-pushed the fix/raycluster-scale-down-wait branch from 52d2420 to 0993da4 Compare July 30, 2026 05:12
@machichima

Copy link
Copy Markdown
Collaborator

@Smallfu666 Do you mind rebase / merge master again? The failed CI test should be fixed.

Signed-off-by: Smallfu666 <62057986+Smallfu666@users.noreply.github.com>
@Smallfu666
Smallfu666 force-pushed the fix/raycluster-scale-down-wait branch from 3327190 to ee4d629 Compare July 30, 2026 12:47
@Smallfu666

Copy link
Copy Markdown
Author

@Smallfu666 Do you mind rebase / merge master again? The failed CI test should be fixed.

@machichima Rebased, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants