-
Notifications
You must be signed in to change notification settings - Fork 16
Add unit tests for support package #13
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
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Raghul-M The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ommon into unittestsupport#239
Hey @sutaakar , I've added unit tests for Applicable Modules in the Support Package . However, some modules are pending ( Feel free to review it. |
support/batch_test.go
Outdated
// testInstance := &T{ | ||
// WithT: gomega.NewWithT(t), | ||
// t: t, | ||
// ctx: ctx, | ||
// client: &testClient{ | ||
// core: fakeClient, | ||
// }, | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I will remove it
support/environment_test.go
Outdated
if version != "1.2.3" { | ||
t.Errorf("Expected version 1.2.3, but got %s", version) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the code above there is used Gomega for assertions.
Can you leverage it also here (and in other places) to keep consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure , will do. thanks for the suggestion
support/ingress_test.go
Outdated
"k8s.io/client-go/kubernetes/fake" | ||
) | ||
|
||
func NewFakeKubeClientForIngress(objects ...runtime.Object) *fake.Clientset { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function seems to be same as NewFakeKubeClientWithObjects
, IMHO it would be good to reuse the existing one (and possibly move it some dedicated file to keep it on one place independent on specific test files).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept this function in a separate file and reused it other files
@@ -9,9 +9,11 @@ require ( | |||
github.com/openshift/client-go v0.0.0-20221019143426-16aed247da5c | |||
github.com/project-codeflare/multi-cluster-app-dispatcher v1.37.0 | |||
github.com/ray-project/kuberay/ray-operator v0.0.0-20231016183545-097828931d15 | |||
github.com/stretchr/testify v1.8.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is testify used for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed running go mod tidy
. testify
It's not needed.
6545a72
to
369b325
Compare
Closing this PR due to Merge Conflicts. So Created New PR with Same changes : New PR |
Issue link
Closes project-codeflare/codeflare-operator#239
What changes have been made
Added Unit test for the Support Package Applicable Modules
Added Workflow file to test the Unit tests
Verification steps
RUN -
go test ./support -v
Checks