Skip to content

chore: improve testing coverage on ExtractProvisionerDaemonAuthenticated middleware #15622

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

Merged
merged 8 commits into from
Nov 26, 2024

Conversation

defelmnq
Copy link
Contributor

This one aims to resolve #15604

Created some table tests for the main cases -
also preferred to create two isolated cases for the most complicated cases in order to keep table tests simple enough.

Give us full coverage on the middleware logic, for both optional and non optional cases - PSK and ProvisionerKey.

r = r.WithContext(context.WithValue(r.Context(), chi.RouteCtxKey, routeCtx))
res := httptest.NewRecorder()

r.Header.Set(codersdk.ProvisionerDaemonKey, "5Hl2Qw9kX3nM7vB4jR8pY6tA1cF0eD5uI2oL9gN3mZ4")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

review: Just generated a random string which should be different from the one generate in CreateProvisionerKey
The objective here is to use a non-existing key to have a 404-like from DB.

@defelmnq defelmnq changed the title chore(coderd): Improve testing coverage on ExtractProvisionerDaemonAuthenticated middleware chore(coderd): improve testing coverage on ExtractProvisionerDaemonAuthenticated middleware Nov 22, 2024
@defelmnq defelmnq changed the title chore(coderd): improve testing coverage on ExtractProvisionerDaemonAuthenticated middleware chore: improve testing coverage on ExtractProvisionerDaemonAuthenticated middleware Nov 23, 2024
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Firstly, thanks for following up on this!

image

There's this block which seems to not be covered by tests, and this feels like the most important block. Is there a reason you're not testing that one specifically?

@defelmnq
Copy link
Contributor Author

Thanks @dannykopping - I indeed was missing some test cases - I added one for the compare failing, and another one in case the DB fails - so we now have 100% test coverage on this middleware 👀

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@defelmnq defelmnq merged commit 60ddcf5 into main Nov 26, 2024
27 checks passed
@defelmnq defelmnq deleted the 15604-improve-middleware-coverage branch November 26, 2024 03:02
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.

Improve testing coverage on ExtractProvisionerDaemonAuthenticated middleware
3 participants