Skip to content

test(ws): Notebooks 2.0 // Backend // Add tests #410

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: notebooks-v2
Choose a base branch
from
Open

test(ws): Notebooks 2.0 // Backend // Add tests #410

wants to merge 1 commit into from

Conversation

yehudit1987
Copy link

This PR solves partially issue #381

@github-project-automation github-project-automation bot moved this to Needs Triage in Kubeflow Notebooks Jun 9, 2025
@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign thesuperzapper for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@yehudit1987 yehudit1987 marked this pull request as draft June 9, 2025 11:57
@yehudit1987 yehudit1987 changed the title Add tests to Backend test(ws): Notebooks 2.0 // Backend // Add tests Jun 9, 2025
@yehudit1987 yehudit1987 changed the title test(ws): Notebooks 2.0 // Backend // Add tests test(ws): Notebooks 2.0 // Backend // Add tests Jun 9, 2025
@yehudit1987 yehudit1987 marked this pull request as ready for review June 9, 2025 12:24
@yehudit1987 yehudit1987 marked this pull request as draft June 11, 2025 10:00
@yehudit1987 yehudit1987 force-pushed the dev/#381 branch 4 times, most recently from 78d7e48 to b8ab4b0 Compare June 17, 2025 13:06
@yehudit1987 yehudit1987 marked this pull request as ready for review June 17, 2025 13:16
andyatmiami
andyatmiami reviewed Jun 17, 2025
View reviewed changes
workspaces/backend/go.mod Outdated
@@ -9,6 +9,7 @@ require (
github.com/kubeflow/notebooks/workspaces/controller v0.0.0
github.com/onsi/ginkgo/v2 v2.19.0
github.com/onsi/gomega v1.33.1
github.com/stretchr/testify v1.9.0
Copy link
Contributor

Choose a reason for hiding this comment

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

do we actually have a dependency on testify ? I don't see it referenced anywhere...

andyatmiami
andyatmiami reviewed Jun 17, 2025
View reviewed changes
workspaces/backend/internal/auth/authentication_test.go
. "github.com/onsi/gomega"
)

var _ = Describe("NewRequestAuthenticator", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

admittedly could be viewed as "overkill" here - but seems like we could write a generic helper function to run the authenticate tests

By simply providing user + groups + prefx as input to said generic function - it could do the invoking and also dynamically make the right assertions based on input being defined/empty

andyatmiami
andyatmiami reviewed Jun 17, 2025
View reviewed changes
workspaces/backend/internal/helper/validation_test.go
Describe("ValidateFieldIsDNS1123Subdomain", func() {
path := field.NewPath("metadata", "name")

DescribeTable("should validate DNS1123 subdomain",
Copy link
Contributor

Choose a reason for hiding this comment

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

nice use of DescribeTable 💯

andyatmiami
andyatmiami reviewed Jun 17, 2025
View reviewed changes
workspaces/backend/internal/repositories/workspaces/suite_test.go Outdated
Comment on lines 112 to 114
BeforeEach(func() {
// Set up once for all tests
})
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove this empty BeforeEach if there is no setup required

andyatmiami
andyatmiami reviewed Jun 17, 2025
View reviewed changes
workspaces/backend/internal/repositories/workspaces/suite_test.go Outdated
Comment on lines 107 to 110
ws1 = &kubefloworgv1beta1.Workspace{ObjectMeta: metav1.ObjectMeta{Name: "ws1", Namespace: ns1}, Spec: kubefloworgv1beta1.WorkspaceSpec{Kind: "kind1"}}
ws2 = &kubefloworgv1beta1.Workspace{ObjectMeta: metav1.ObjectMeta{Name: "ws2", Namespace: ns1}, Spec: kubefloworgv1beta1.WorkspaceSpec{Kind: "kind-nonexistent"}}
ws3 = &kubefloworgv1beta1.Workspace{ObjectMeta: metav1.ObjectMeta{Name: "ws3", Namespace: ns2}, Spec: kubefloworgv1beta1.WorkspaceSpec{Kind: "kind1"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

While I wholly appreciate what this test setup is trying to do - I feel a little uneasy about constructing this scenario in a way that is literally impossible to encounter in the "real world"...

i.e. if we are declaring a WorkspaceSpec - it feels weird/wrong to me to set the Kind to some value other than Workspace

Don't feel strongly about the change - but I wonder if we could simply include some non-Workspace objects in the initObjs parameter that then more closely simulates how this works in reality...

andyatmiami
andyatmiami reviewed Jun 17, 2025
View reviewed changes
workspaces/backend/internal/repositories/workspaces/suite_test.go
Expect(wss).To(BeEmpty())
})

It("returns all workspaces in the cluster", func() {
Copy link
Contributor

@andyatmiami andyatmiami Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Given we declared ws2 as such:

ws2 = &kubefloworgv1beta1.Workspace{ObjectMeta: metav1.ObjectMeta{Name: "ws2", Namespace: ns1}, Spec: kubefloworgv1beta1.WorkspaceSpec{Kind: "kind-nonexistent"}}

It seems odd to me (at face value) we would then expect this result to appear when getting all workspaces 🤔

I'm probably reading the code too literally - but worry it could be a head-scratcher in the future...

Copy link
Contributor

Choose a reason for hiding this comment

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

i was getting confused on kind vs. spec.kind - which @yehudit1987 kinda clearly up for me...

This comment can be ignored.

@andyatmiami
Copy link
Contributor

/ok-to-test

@andyatmiami
Copy link
Contributor

/lgtm

Great (and much needed) foundation for getting tests added in critical areas 💯

➜ backend/ git:((HEAD detached at yehudit/dev/#381)) $ gmake test
...
ok  	github.com/kubeflow/notebooks/workspaces/backend/internal/auth	1.372s	coverage: 64.3% of statements
ok  	github.com/kubeflow/notebooks/workspaces/backend/internal/helper	1.562s	coverage: 93.8% of statements
ok  	github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/workspacekinds	1.104s	coverage: 88.9% of statements
ok  	github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/workspaces	1.422s	coverage: 78.0% of statements

@google-oss-prow google-oss-prow bot added the lgtm label Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyatmiami andyatmiami andyatmiami approved these changes

@kimwnasptd kimwnasptd Awaiting requested review from kimwnasptd

@thesuperzapper thesuperzapper Awaiting requested review from thesuperzapper

Assignees

@andyatmiami andyatmiami

Labels
lgtm ok-to-test size/XL
Projects
Kubeflow Notebooks
Status: Needs Triage
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yehudit1987 @andyatmiami