Skip to content

feat(ws): Ensure test files exist in backend for any executed code #381 #423

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

liavweiss
Copy link

This PR adds foundational unit tests for the server lifecycle in the backend/internal/server package. The tests verify that the server can start, listen on a port, and shut down gracefully, which improves our test coverage.

This work is part of the larger effort to resolve #381.

output from make test:

make test
go fmt ./...
go vet ./...
KUBEBUILDER_ASSETS="/home/liavweiss/projects/NB2/notebooks/workspaces/backend/bin/k8s/1.31.0-linux-amd64" go test ./... -coverprofile cover.out
github.com/kubeflow/notebooks/workspaces/backend/cmd coverage: 0.0% of statements
? github.com/kubeflow/notebooks/workspaces/backend/internal/config [no test files]
? github.com/kubeflow/notebooks/workspaces/backend/internal/models/health_check [no test files]
github.com/kubeflow/notebooks/workspaces/backend/internal/models/namespaces coverage: 0.0% of statements
github.com/kubeflow/notebooks/workspaces/backend/internal/repositories coverage: 0.0% of statements
github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/health_check coverage: 0.0% of statements
github.com/kubeflow/notebooks/workspaces/backend/internal/auth coverage: 0.0% of statements
github.com/kubeflow/notebooks/workspaces/backend/internal/models/workspacekinds coverage: 0.0% of statements
github.com/kubeflow/notebooks/workspaces/backend/internal/models/workspaces coverage: 0.0% of statements
github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/namespaces coverage: 0.0% of statements
github.com/kubeflow/notebooks/workspaces/backend/internal/helper coverage: 0.0% of statements
github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/workspacekinds coverage: 0.0% of statements
github.com/kubeflow/notebooks/workspaces/backend/internal/repositories/workspaces coverage: 0.0% of statements
github.com/kubeflow/notebooks/workspaces/backend/openapi coverage: 0.0% of statements
ok github.com/kubeflow/notebooks/workspaces/backend/api 26.840s coverage: 61.2% of statements
ok github.com/kubeflow/notebooks/workspaces/backend/internal/server 4.292s coverage: 5.8% of statements

related: #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

@liavweiss liavweiss force-pushed the test_coverage/#381 branch 3 times, most recently from 8db6966 to 592c28e Compare June 18, 2025 14:43
}
conn.Close()
return nil
}, "5s", "100ms").Should(Succeed())
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 pull the 5s and 100ms values into variables/constants to promote re-use.

While nothing else uses the 5s startup (yet?) - would help future tests be consistent.

Consistently(func() error {
_, err := net.Dial("tcp", serverAddr)
return err // An error is expected, so we return it.
}, "2s", "100ms").Should(HaveOccurred())
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 pull the 2s and 100ms values into variables/constants to promote re-use.

While nothing else uses the 2s shutdown (yet?) - would help future tests be consistent.

// Consistently checks that the port remains closed.
Consistently(func() error {
_, err := net.Dial("tcp", serverAddr)
return err // An error is expected, so we return it.
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 add an additional assertion here (if possible) to check for the specific error we expect to be returned...

As an example - if we saw some type of PortInUse error being thrown here.. the test would still pass - while obviously this is not the error situation we expected to happen.

cancel()

By("verifying the server's Start method returns nil for a clean shutdown")
Eventually(serverErrChan, "5s").Should(Receive(BeNil()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see we do actually reuse the 5s value here 😎

)

// findFreePort is a helper to get an available TCP port, preventing test conflicts.
findFreePort := func() int {
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this helper function to avoid potential flakiness 💯

I wonder, though, and I'm not necessarily asking for a change - but just want to know your thoughts.. if we should define this moreso as a true/proper helper function:

func findFreePort() (int, error) {
    listener, err := net.Listen("tcp", "localhost:0")
    if err != nil {
        return 0, err
    }
    defer listener.Close()
    return listener.Addr().(*net.TCPAddr).Port, nil
}

i.e. we don't embed the Expect assertion in here...

and then where it is invoked in BeforeEach - we could add an assertion there for NotTo(HaveOccurred())

It just feels "weird" to me (and could be an opinionated/biased "me thing") to have an Expect in a function that isn't itself really focused with asserting any testable code directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Great point on findFreePort! I agree that assertions in a pure helper function aren't ideal. I've updated it to return the error, with the Expect assertion now properly placed in the BeforeEach block, aligning with our other error checks. Thanks for the catch!

@andyatmiami
Copy link
Contributor

/ok-to-test

@liavweiss liavweiss force-pushed the test_coverage/#381 branch from 69d34b6 to 910d225 Compare June 23, 2025 13:39
@andyatmiami
Copy link
Contributor

/lgtm

Confirmed the added tests are passing successfully

➜ backend/ git:((HEAD detached at liav/test_coverage/#381)) $ gmake test
...
ok  	github.com/kubeflow/notebooks/workspaces/backend/internal/server	2.810s	coverage: 65.2% 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