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 end to end tests to apiserver #1460

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

z103cb
Copy link
Contributor

@z103cb z103cb commented Sep 29, 2023

Why are these changes needed?

The kuberay api server does not have an easy to execute set of test cases that would allow for the validation of the integration between it and the kuberay operator. This PR provides a basic set of such integration test cases.

This PR:

  • Adds the testcases
  • Adds the make targets to facilitate the test cases
  • Updates the documentation for development of kuberay api server.
  • Updates the github CI pipeline to execute only the unit tests for api server
  • Minor documentation updates (formatting)

Related issue number

Closes #1388

Checks

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

@@ -144,7 +144,7 @@ jobs:
working-directory: ${{env.working-directory}}

- name: Test
run: go test ./...
run: go test ./pkg/... ./cmd/... -race -parallel 4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevin85421 let me know if this change is acceptable. If not made the e2e test would execute, and fail as there's no kind cluster setup at this point of the pipeline.

Copy link
Member

Choose a reason for hiding this comment

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

Can we use make test instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, I don't see a reason why make test cannot be here. I did not add it as it would have been different than what is happening for the other steps in the CI pipeline and I did not seem to be a changed related to the main scope of the PR.

I would prefer that we change the CI (Git hub actions) for the API Server in separate PR and perhaps integrate the execution of the e2e tests with build kite.

I would not change more in the GH action than I have already done.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good

apiserver/Makefile Outdated Show resolved Hide resolved
@z103cb
Copy link
Contributor Author

z103cb commented Sep 29, 2023

I still need to a a few more test cases and fixes, but I would like to get some feedback on what I have so far.

@tedhtchang, @blublinsky and @anishasthana can provide feedback ?
cc: @kevin85421

@blublinsky
Copy link
Contributor

This looks great. Makes things so much simpler

apiserver/Makefile Outdated Show resolved Hide resolved
@z103cb z103cb force-pushed the z103cb/issue1388 branch 3 times, most recently from 68ec386 to 1de910a Compare October 5, 2023 08:16
apiserver/DEVELOPMENT.md Outdated Show resolved Hide resolved
apiserver/Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@tedhtchang tedhtchang left a comment

Choose a reason for hiding this comment

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

I had some small issues(see comment). Otherwise, this looks good. I was able to run make operator-image local-e2e-test -e OPERATOR_IMAGE_TAG=latest -e GO_TEST_FLAGS="-v" successfully.

@z103cb z103cb mentioned this pull request Oct 10, 2023
1 task
@z103cb z103cb force-pushed the z103cb/issue1388 branch 3 times, most recently from a4e36c6 to 89193e5 Compare October 12, 2023 08:51
@z103cb
Copy link
Contributor Author

z103cb commented Oct 12, 2023

@astefanutti can you take a look ?

Copy link
Contributor

@astefanutti astefanutti left a comment

Choose a reason for hiding this comment

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

Looks great. Could a GH Actions workflow be created so it runs in CI?

apiserver/test/e2e/cluster_server_e2e_test.go Outdated Show resolved Hide resolved
apiserver/test/e2e/cluster_server_e2e_test.go Outdated Show resolved Hide resolved
.github/workflows/test-job.yaml Show resolved Hide resolved
@z103cb z103cb force-pushed the z103cb/issue1388 branch 4 times, most recently from 1be8fd7 to a95cb9c Compare October 23, 2023 07:27
@z103cb z103cb force-pushed the z103cb/issue1388 branch 2 times, most recently from 2336828 to 903f28b Compare October 27, 2023 09:49
@z103cb z103cb marked this pull request as ready for review October 27, 2023 09:50
Copy link
Contributor

@blublinsky blublinsky left a comment

Choose a reason for hiding this comment

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

Two suggestions, but overall looks good

apiserver/Volumes.md Outdated Show resolved Hide resolved
Copy link
Contributor

@blublinsky blublinsky left a comment

Choose a reason for hiding this comment

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

LGTM

@tedhtchang
Copy link
Contributor

I kept failing the same test for some reason:

              Error Trace:  /root/kuberay/apiserver/test/e2e/service_server_e2e_test.go:911
                                  /root/kuberay/apiserver/test/e2e/service_server_e2e_test.go:746
              Error:        Received unexpected error:
                            timed out waiting for the condition
              Test:         TestServiceServerV1Patch/Update_service_cluster_worker_group
              Messages:     No error expected when getting ray service: 'shepherd', err timed out waiting for the condition
FAIL
FAIL  github.com/ray-project/kuberay/apiserver/test/e2e 888.117s

Copy link
Contributor

@tedhtchang tedhtchang left a comment

Choose a reason for hiding this comment

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

/LGTM

@z103cb
Copy link
Contributor Author

z103cb commented Nov 1, 2023

@kevin85421, let me know if you have any concerns with this PR, if not can you please merge it ?

@kevin85421 kevin85421 self-assigned this Nov 13, 2023
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 for the contribution! Testing is always the most important part of the KubeRay community.

@kevin85421
Copy link
Member

Rerun the CI. There seem to be some changes in the Ray nightly build causing flakiness.

@kevin85421 kevin85421 merged commit ff45923 into ray-project:master Nov 15, 2023
24 checks passed
blublinsky pushed a commit to blublinsky/kuberay that referenced this pull request Nov 16, 2023
@z103cb z103cb deleted the z103cb/issue1388 branch February 5, 2024 14:36
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 end to end tests to apiserver
5 participants