Skip to content
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

Add basic Helm chart unittests for kuberay-operator #3253

Merged
merged 3 commits into from
Apr 10, 2025

Conversation

ChenYi015
Copy link
Contributor

@ChenYi015 ChenYi015 commented Mar 31, 2025

Why are these changes needed?

Make sure kuberay-operator 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/kuberay-operator --strict --debug
### Chart [ kuberay-operator ] helm-chart/kuberay-operator

 PASS  Test Deployment  helm-chart/kuberay-operator/tests/deployment_test.yaml
 PASS  Test ClusterRole helm-chart/kuberay-operator/tests/role_test.yaml
 PASS  Test ClusterRoleBinding  helm-chart/kuberay-operator/tests/rolebinding_test.yaml
 PASS  Test Service     helm-chart/kuberay-operator/tests/service_test.yaml
 PASS  Test ServiceAccount      helm-chart/kuberay-operator/tests/serviceaccount_test.yaml

Charts:      1 passed, 1 total
Test Suites: 5 passed, 5 total
Tests:       25 passed, 25 total
Snapshot:    0 passed, 0 total
Time:        118.062459ms

Related issue number

Closes #3244

Checks

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

@ChenYi015 ChenYi015 changed the title Add basic Helm chart unittests Add basic Helm chart unittests for kuberay-operator Mar 31, 2025
@ChenYi015
Copy link
Contributor Author

Hi @kevin85421, PTAL when you have time.

@kevin85421
Copy link
Member

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

@ChenYi015 ChenYi015 requested a review from win5923 April 5, 2025 01:50
Copy link
Contributor

@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

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.

@ChenYi015 are these tests running in the CI now? cc @MortalHappiness would you mind reviewing this PR? Thanks!

@ChenYi015
Copy link
Contributor Author

@ChenYi015 are these tests running in the CI now?

@kevin85421 Not yet. I will raise another PR to add Helm chart unit test to CI once this PR is merged.

@kevin85421
Copy link
Member

Not yet. I will raise another PR to add Helm chart unit test to CI once this PR is merged.

Got it. Would you mind adding a screenshot of the result from running helm unittest helm-chart/kuberay-operator --strict --debug in the PR description after @MortalHappiness approves the PR? Since it’s not currently running in CI, we can’t determine whether the test logic is correct based on the CI results alone. Thanks!

@ChenYi015
Copy link
Contributor Author

ChenYi015 commented Apr 7, 2025

Would you mind adding a screenshot of the result from running helm unittest helm-chart/kuberay-operator --strict --debug in the PR description after @MortalHappiness approves the PR?

Sure.

Since it’s not currently running in CI, we can’t determine whether the test logic is correct based on the CI results alone.

Have created an issue #3279 for tracking this and PR #3280 for closing it.

@kevin85421
Copy link
Member

Thank you!

@kevin85421
Copy link
Member

@MortalHappiness please take a look. Thanks.

Copy link
Member

@MortalHappiness MortalHappiness left a comment

Choose a reason for hiding this comment

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

LGTM. Just one question: are the tests and the YAML formatting auto-generated?

@ChenYi015
Copy link
Contributor Author

Just one question: are the tests and the YAML formatting auto-generated?

No. I wrote the tests and format the YAML manually.

@ChenYi015
Copy link
Contributor Author

Would you mind adding a screenshot of the result from running helm unittest helm-chart/kuberay-operator --strict --debug in the PR description after @MortalHappiness approves the PR?

Here is the screenshot of running helm unittest:

image

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.

Overall LGTM. I've left some comments out of curiosity.

{{- toYaml .Values.labels | nindent 4 }}
{{- end }}
{{ include "kuberay-operator.labels" . | nindent 4 }}
{{- with .Values.labels }}
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind explaining the rationale behind the change from if to with? The only difference seems to be able to use toYaml .?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with can be used to specify the context scope, so we can simplify the template code (especially when the name of values are long) without repeating the values.

{{- if .Values.labels }}
{{- toYaml .Values.labels | nindent 4 }}
{{- end }}
{{ include "kuberay-operator.labels" . | nindent 4 }}
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to change from indent to nindent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

metadata:
  labels:
{{ include "kuberay-operator.labels" . | indent 4 }}

metadata:
  labels:
    {{- include "kuberay-operator.labels" . | nindent 4 }}

The above two template blocks work the same. But since yaml uses indention to convey the struct information, it will be easier to read if we can adjust the indention of helm template code.

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.

Thank you!

@kevin85421 kevin85421 merged commit ef0129e into ray-project:master Apr 10, 2025
21 checks passed
@ChenYi015 ChenYi015 deleted the helm-unittest branch April 11, 2025 02:14
@kevin85421 kevin85421 mentioned this pull request Apr 11, 2025
4 tasks
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.

[Feature] Add Helm chart unit tests
4 participants