Skip to content

[Feat] Add e2e test for applying ray-job.interactive-mode.yaml #3779

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 16, 2025
Merged

[Feat] Add e2e test for applying ray-job.interactive-mode.yaml #3779

merged 3 commits into from
Jun 16, 2025

Conversation

CheyuWu
Copy link
Contributor

@CheyuWu CheyuWu commented Jun 14, 2025 •
edited
Loading

Why are these changes needed?

There is no testing for ray-job.interactive-mode.yaml

Manual testing

$ helm repo update
$ helm install kuberay-operator kuberay/kuberay-operator --version v1.4.0-rc.1
$ kubectl apply -f ray-operator/config/samples/ray-job.interactive-mode.yaml

Related issue number

Closes #3778

Checks

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

CheyuWu
CheyuWu commented Jun 14, 2025
View reviewed changes
ray-operator/config/samples/ray-job.interactive-mode.yaml
@@ -2,7 +2,6 @@ apiVersion: ray.io/v1
kind: RayJob
metadata:
name: rayjob-interactive-mode
namespace: default
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove this to allow E2E to be assigned to other namespaces.

@CheyuWu
Copy link
Contributor Author

CheyuWu commented Jun 14, 2025

Hi @MortalHappiness PTAL

MortalHappiness
MortalHappiness reviewed Jun 14, 2025
View reviewed changes
ray-operator/test/sampleyaml/rayjob_test.go Outdated
Comment on lines 79 to 83
// There is not deployment in `ray-job.interactive-mode.yaml`, so we skip the deployment status check for this yaml file.
if tt.name != "ray-job.interactive-mode.yaml" {
g.Eventually(RayJob(test, namespace.Name, rayJobFromYaml.Name), TestTimeoutMedium).Should(WithTransform(RayJobDeploymentStatus, Equal(rayv1.JobDeploymentStatusComplete)))
g.Eventually(RayJob(test, namespace.Name, rayJobFromYaml.Name), TestTimeoutMedium).Should(WithTransform(RayJobStatus, Equal(rayv1.JobStatusSucceeded)))
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to add a separate test for ray-job.interactive-mode.yaml instead of mixing it with other tests. Otherwise if there are some other special cases for other yaml files in the future, we need to create a lot of if-else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, no problem

kevin85421
kevin85421 reviewed Jun 15, 2025
View reviewed changes
ray-operator/test/sampleyaml/rayjob_test.go

g.Eventually(RayJob(test, namespace.Name, rayJobFromYaml.Name), TestTimeoutMedium).Should(WithTransform(RayJobDeploymentStatus, Equal(rayv1.JobDeploymentStatusComplete)))
Copy link
Member

Choose a reason for hiding this comment

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

why remove this?

Copy link
Contributor Author

@CheyuWu CheyuWu Jun 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I removed the wrong code, sorry about this.

ray-operator/test/sampleyaml/rayjob_test.go

g.Eventually(RayJob(test, namespace.Name, rayJobFromYaml.Name), TestTimeoutMedium).Should(WithTransform(RayJobStatus, Equal(rayv1.JobStatusSucceeded)))
Copy link
Member

Choose a reason for hiding this comment

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

why remove this?

Copy link
Contributor Author

@CheyuWu CheyuWu Jun 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I removed the wrong code, sorry about this.

kevin85421
kevin85421 reviewed Jun 15, 2025
View reviewed changes
ray-operator/test/sampleyaml/rayjob_test.go
}
g.Eventually(WorkerPods(test, rayCluster), TestTimeoutShort).Should(HaveLen(int(desiredWorkerPods)))
g.Expect(GetRayCluster(test, namespace.Name, rayCluster.Name)).To(WithTransform(RayClusterDesiredWorkerReplicas, Equal(desiredWorkerPods)))
rayCluster := checkRayJobCluster(test, g, tt.name)

Copy link
Member

Choose a reason for hiding this comment

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

			// Check if the head pod is ready
			g.Eventually(HeadPod(test, rayCluster), TestTimeoutShort).Should(WithTransform(IsPodRunningAndReady, BeTrue()))

			// Check if all worker pods are ready
			g.Eventually(WorkerPods(test, rayCluster), TestTimeoutShort).Should(WithTransform(AllPodsRunningAndReady, BeTrue()))

this is duplicate with checkRayJobCluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied the wrong code, sorry about this.

@MortalHappiness
Copy link
Member

Wait, I found that I can successfully apply this yaml file for the nightly version and v1.4.0-rc.1. So I think we should find out some other people to test this.

image

@MortalHappiness MortalHappiness dismissed their stale review June 15, 2025 06:27

Dismiss stale review

@CheyuWu
fix: yaml file validation error
a2ed1c8 
fix: remove default namespace

fix: rayjob no deployment in yaml

refacator: separate to two type of testcase

fix: wrong copy code
@fscnick
Copy link
Contributor

fscnick commented Jun 15, 2025

Hi, I tested this yaml on the mac (arm). It could successfully apply.
image

@CheyuWu CheyuWu force-pushed the fix/yaml/validation branch from 6f33699 to 51cb45f Compare June 15, 2025 12:18
@CheyuWu CheyuWu changed the title [Fix] Validation error occurs when applying ray-job.interactive-mode.yaml [Feat] Add e2e test for applying ray-job.interactive-mode.yaml Jun 15, 2025
@CheyuWu
Copy link
Contributor Author

CheyuWu commented Jun 15, 2025

Hi @MortalHappiness, I have changed the description and title for this pr

kevin85421
kevin85421 approved these changes Jun 16, 2025
View reviewed changes
ray-operator/test/sampleyaml/rayjob_test.go Outdated
rayJob, err = GetRayJob(test, rayJob.Namespace, rayJob.Name)
g.Expect(err).NotTo(HaveOccurred())
g.Expect(rayJob).NotTo(BeNil())
func TestRayJobMode(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func TestRayJobMode(t *testing.T) {
func TestRayJobInteractiveMode(t *testing.T) {

@kevin85421 kevin85421 merged commit 35b96f1 into ray-project:master Jun 16, 2025
25 checks passed
MortalHappiness pushed a commit to MortalHappiness/kuberay that referenced this pull request Jun 17, 2025
@CheyuWu @MortalHappiness
[Feat] Add e2e test for applying ray-job.interactive-mode.yaml (ray…
92f7bd8 
…-project#3779)
MortalHappiness added a commit that referenced this pull request Jun 17, 2025
@MortalHappiness @CheyuWu
[Feat] Add e2e test for applying ray-job.interactive-mode.yaml (#3779…
cfd0bfa 
…) (#3787)

Co-authored-by: Cheyu Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevin85421 kevin85421 kevin85421 approved these changes

@MortalHappiness MortalHappiness MortalHappiness approved these changes

Assignees

@kevin85421 kevin85421

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feat] Add e2e test for applying ray-job.interactive-mode.yaml
4 participants
@CheyuWu @MortalHappiness @fscnick @kevin85421