Skip to content

Conversation

owenowenisme
Copy link
Collaborator

@owenowenisme owenowenisme commented Jul 23, 2025

Why are these changes needed?

Closes #3889

Related issue number

Checks

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

@owenowenisme owenowenisme marked this pull request as ready for review July 23, 2025 05:42
@owenowenisme
Copy link
Collaborator Author

@rueian PTAL

@400Ping
Copy link
Contributor

400Ping commented Jul 23, 2025

LGTM

@AndySung320
Copy link

Hi, I'm still learning the codebase, but would it be cleaner to pass the Gomega instance as a parameter rather than creating a new one inside the function? This way we avoid duplicate NewWithT(test.T()) calls and use the same instance as the caller. Let me know if I'm missing something!

Comment on lines 8 to 9
"github.com/onsi/gomega"
. "github.com/onsi/gomega"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove one of them to avoid confusion.

@owenowenisme owenowenisme force-pushed the e2e-make-kill-headpod-to-test-utils branch from 6ec09d3 to b6fe5bd Compare July 24, 2025 08:39
Signed-off-by: You-Cheng Lin <[email protected]>
Signed-off-by: You-Cheng Lin <[email protected]>
@owenowenisme owenowenisme force-pushed the e2e-make-kill-headpod-to-test-utils branch from b6fe5bd to cfa43e5 Compare July 24, 2025 08:47
Signed-off-by: You-Cheng Lin <[email protected]>
@owenowenisme owenowenisme requested a review from win5923 July 24, 2025 09:02
Signed-off-by: You-Cheng Lin <[email protected]>
Copy link
Collaborator

@win5923 win5923 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

| nameOverride | string | `"kuberay"` | String to partially override release name. |
| fullnameOverride | string | `""` | String to fully override release name. |
| imagePullSecrets | list | `[]` | Secrets with credentials to pull images from a private registry |
| gcsFaultTolerance.enabled | bool | `false` | |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for fixing it. It's my bad..

@rueian rueian merged commit a851490 into ray-project:master Jul 25, 2025
25 checks passed
@owenowenisme
Copy link
Collaborator Author

owenowenisme commented Jul 25, 2025

Hi, I'm still learning the codebase, but would it be cleaner to pass the Gomega instance as a parameter rather than creating a new one inside the function? This way we avoid duplicate NewWithT(test.T()) calls and use the same instance as the caller. Let me know if I'm missing something!

@AndySung320
You're right, we could avoid duplicate if we pass gomega instance, but I think we might have other test that doesn't use gomega so usingNewWithT(test.T()) inside will allow them to pass test without creating gomega in their test func.

And since this is only for testing purpose duplicate Gomega instance is acceptable IMO.

Thanks for commenting!

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.

Add a test util function for killing the head Pod and wait.
5 participants