Skip to content

Add Helm chart unittests for ray-cluster #3277

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.

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

Closed

Conversation

ChenYi015
Copy link
Contributor

@ChenYi015 ChenYi015 commented Apr 7, 2025

Why are these changes needed?

Make sure ray-cluster Helm chart can work as expected with various configurations by adding unit tests.

One can run Helm chart unit tests by:

$ helm plugin install https://github.com/helm-unittest/helm-unittest.git

$ helm unittest helm-chart/ray-cluster --strict --debug
### Chart [ ray-cluster ] helm-chart/ray-cluster

 PASS  Test RayCluster  helm-chart/ray-cluster/tests/raycluster_test.yaml

Charts:      1 passed, 1 total
Test Suites: 1 passed, 1 total
Tests:       96 passed, 96 total
Snapshot:    0 passed, 0 total
Time:        281.1805ms

Related issue number

Checks

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

@kevin85421
Copy link
Member

cc @rueian would you mind reviewing this PR? Thanks!

@kevin85421
Copy link
Member

@ChenYi015, thank you for your contribution! Are you on Ray Slack? If you're interested in the community, feel free to DM me on Slack (my handle is "Kai-Hsun Chen (ray team)"). I'd be happy to share more information about the community with you.

@ChenYi015 ChenYi015 force-pushed the helm/raycluster-unittest branch from a0a8b43 to af541f2 Compare April 12, 2025 02:21
Copy link
Contributor

@rueian rueian left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

Hi @ChenYi015, this PR overall looks good to me, but it's too big so that I may miss something. Would you mind splitting it into smaller PRs? For example,

  • 1 PR to update {{- if .Values.head.rayVersion }} to {{- with .Values.head.rayVersion }}.
  • 1 PR to update indent to nindent (btw, is there any linter for this?)
  • 1 PR to add tests

@ChenYi015
Copy link
Contributor Author

Hi @ChenYi015, this PR overall looks good to me, but it's too big so that I may miss something. Would you mind splitting it into smaller PRs?

Sure, I will split it into smaller ones.

@ChenYi015
Copy link
Contributor Author

Close this PR for it has already been split into smaller ones.

@ChenYi015 ChenYi015 closed this Apr 17, 2025
@ChenYi015 ChenYi015 deleted the helm/raycluster-unittest branch April 17, 2025 00:40
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