Skip to content

feat(ws): Notebooks 2.0 // Backend // Add tests to repositories/namespaces #411

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

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

Conversation

roee1313
Copy link

@roee1313 roee1313 commented Jun 10, 2025

this PR adds a test file to workspaces/backend/internal/repositories/namespaces/repo.go
Partially solves issue #381

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 kimwnasptd 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

@roee1313 roee1313 marked this pull request as draft June 10, 2025 04:54
@roee1313 roee1313 marked this pull request as ready for review June 10, 2025 16:22
@@ -93,6 +93,7 @@ require (
google.golang.org/genproto/googleapis/rpc v0.0.0-20240701130421-f6361c86f094 // indirect
google.golang.org/grpc v1.65.0 // indirect
google.golang.org/protobuf v1.34.2 // indirect
gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

might be nice to call out in PR description that this dependency is brought in as part of:

  • "sigs.k8s.io/controller-runtime/pkg/client/fake"

Copy link
Author

Choose a reason for hiding this comment

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

got it, will learn for next time :)
Thanks!

}

func (e *erroringClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
return errors.New("mocked list error")
Copy link
Contributor

Choose a reason for hiding this comment

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

I typically avoid the "magic strings" assertion pattern where we rely on setting an error string.. and then asserting on that error string in the test itself... its too brittle (when the values are hard-coded like this)... even declaring an additional var I don't like...

For your consideration - how do you feel about something like this?

type MockError struct {
    message string
}

func (e *MockError) Error() string {
    return e.message
}

// In the erroringClient
func (e *erroringClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
    return &MockError{message: "mocked list error"}
}

// In the test
It("should return an error", func() {
    namespaces, err := namespaceRepo.GetNamespaces(ctx)
    Expect(err).To(HaveOccurred())
    
    // Type assert to check for our specific error type
    var mockErr *MockError
    Expect(errors.As(err, &mockErr)).To(BeTrue())
    Expect(namespaces).To(BeNil())
})

Custom error types seem to be somewhat idiomatic in Go - and scales better if/when tests start becoming more complicated with need to test more side effects...

Copy link
Author

Choose a reason for hiding this comment

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

got your point and learnt from it, thanks for that!

@roee1313 roee1313 force-pushed the test_coverage_roee branch from 7e2907a to deb5fa4 Compare June 19, 2025 10:09
@roee1313 roee1313 force-pushed the test_coverage_roee branch from deb5fa4 to 2a9f95a Compare June 19, 2025 10:11
@andyatmiami
Copy link
Contributor

/ok-to-test

@andyatmiami
Copy link
Contributor

/lgtm

Verified running the make test command successfully executes the added tests

➜ backend/ git:((HEAD detached at roee/test_coverage_roee)) $ gmake test
go fmt ./...
go vet ./...
KUBEBUILDER_ASSETS="/Users/astonebe/Development/Code/GitHub/kubeflow-notebooks/workspaces/backend/bin/k8s/1.31.0-darwin-arm64" go test ./... -coverprofile cover.out
...
ok  	github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/namespaces	0.625s	coverage: 100.0% of statements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

2 participants