Refactor httpserver integration tests into per-domain files#80
Merged
Conversation
The OAuth integration tests were all in a single 4,389-line file
(oauth_integration_test.go) containing one OAuthFlowSuite with 88 test
methods spanning many unrelated domains. Split it into focused files
that share a common base:
- integration_common_test.go (suite setup, shared helpers, fixtures)
- oauth_flow_test.go
- oidc_test.go
- mfa_test.go
- email_verification_flow_test.go
- password_reset_flow_test.go
- authorize_consent_test.go
- introspection_revocation_test.go
- client_credentials_test.go
- admin_test.go
All 88 test methods stay on OAuthFlowSuite, so suite discovery is
unchanged.
Fixes inconsistent build-tag gating:
- oauth_integration_test.go had its //go:build integration tag
commented out ("// /* //go:build integration */"), so the
container-backed suite ran on every `go test`, while
avatar_integration_test.go (real tag) never ran in CI.
- All integration files now carry a live //go:build integration tag.
- avatar_integration_test.go referenced an undefined
generateCodeChallenge and so had never compiled under the tag; the
helper is now defined once in integration_common_test.go and reused
by both suites (replacing the inline S256 computation).
Wires integration tests into automation:
- Makefile: add `test-integration` target (go test -tags integration).
- CI: add a dedicated "Run integration tests" step that runs the
tagged suites (testcontainers needs the runner's Docker daemon).
Minor cleanup: email_verification_test.go replaces hand-rolled
contains/findSubstring helpers with strings.Contains.
The avatar suite had never compiled before the build-tag/generateCodeChallenge
fix, so two tests failed the first time CI actually ran them:
- TestUserInfoReturnsPicture: mustCompleteAuthorizationFlow asserted the
login POST redirect contained "code=", but the server redirects login ->
/oauth/authorize -> /oauth/consent and only issues a code after consent
is approved. The helper now drives the full login -> authorize -> consent
chain (mirroring OAuthFlowSuite.mustLoginAndConsent), handling both the
unauthenticated entry and the already-authenticated client passed by
TestUserInfoReturnsPicture.
- TestAvatarUpdate: asserted an uploaded avatar is fetchable via anonymous
http.Get on its public URL, but the MinIO test bucket was private, so the
GET returned 403. Added S3Storage.MakeBucketPublicRead (anonymous
s3:GetObject policy) and call it in SetupSuite, reproducing the public
bucket/CDN used to serve avatars in production.
The OAuthFlowSuite split is unaffected; it passed in CI.
There was a problem hiding this comment.
Pull request overview
Refactors the pkg/httpserver container-backed integration tests into smaller per-domain files that share a common OAuthFlowSuite base, fixes integration build-tag gating so these suites only run under -tags integration, and wires integration tests into local automation + CI. Also adds an S3 bucket policy helper used by avatar integration tests to allow fetching uploaded avatars via public URLs in MinIO.
Changes:
- Split the large OAuth integration suite into domain-focused
*_test.gofiles with shared setup/helpers inintegration_common_test.go. - Enforce
//go:build integrationgating across integration tests and add CI + Makefile support for running them. - Update avatar integration setup to grant anonymous read on the MinIO bucket and refactor the authorization-code helper logic.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/storage/s3.go | Adds MakeBucketPublicRead helper to set a public-read bucket policy (used by avatar integration tests). |
| pkg/httpserver/integration_common_test.go | Introduces shared OAuthFlowSuite setup/helpers and the integration build tag gate. |
| pkg/httpserver/oauth_flow_test.go | Adds/refactors OAuth flow integration tests into a focused file. |
| pkg/httpserver/oidc_test.go | Adds OIDC-focused integration tests (ID token, JWKS, discovery, userinfo, etc.). |
| pkg/httpserver/mfa_test.go | Adds MFA integration tests and regression coverage around OAuth context preservation. |
| pkg/httpserver/password_reset_flow_test.go | Adds password reset + forgot username integration tests. |
| pkg/httpserver/email_verification_flow_test.go | Adds email verification flow integration tests. |
| pkg/httpserver/introspection_revocation_test.go | Adds token introspection/revocation integration tests. |
| pkg/httpserver/client_credentials_test.go | Adds client-credentials integration tests. |
| pkg/httpserver/admin_test.go | Adds admin API integration tests using client credentials tokens. |
| pkg/httpserver/authorize_consent_test.go | Adds integration tests for authorize error handling, consent behavior, success page, and logout behavior. |
| pkg/httpserver/avatar_integration_test.go | Updates avatar tests to make the bucket publicly readable and refactors the auth-code flow helper. |
| pkg/httpserver/email_verification_test.go | Simplifies substring assertions by using strings.Contains. |
| Makefile | Adds test-integration target (go test -tags integration ./...). |
| .github/workflows/pull_request.yaml | Splits unit vs integration test steps and runs integration tests with the integration build tag. |
Comments suppressed due to low confidence (2)
pkg/httpserver/email_verification_flow_test.go:125
- Stray section header at the end of the file (no password reset tests follow). This looks like a leftover from the prior monolithic integration test file and can be removed to avoid confusion.
// Password Reset Tests
pkg/httpserver/client_credentials_test.go:259
- Trailing comment suggests an additional test but none follows in this file. This appears to be a leftover header and should be removed or completed to avoid confusion.
// TestAdminCreateUser_WithClientCredentials tests end-to-end: get token, create user
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+47
to
+51
| resp, err = s.httpClient.PostForm("http://localhost:8080/oauth/reset-password", url.Values{ | ||
| "token": {rawToken}, | ||
| "password": {secondPassword}, | ||
| "confirm_password": {secondPassword}, | ||
| }) |
Comment on lines
+67
to
+70
| // TestLogoutAcceptsGET verifies that the logout endpoint (advertised as | ||
| // end_session_endpoint in OIDC discovery) accepts GET requests per OIDC | ||
| // RP-Initiated Logout 1.0, which expects browsers to navigate to it via redirect. | ||
| func (s *OAuthFlowSuite) TestPasswordResetFlow() { |
Comment on lines
+103
to
+107
| s.True(tokens.Next(), "should have a password reset token") | ||
| var storedHash string | ||
| err = tokens.Scan(&storedHash) | ||
| s.Require().NoError(err) | ||
|
|
Comment on lines
+137
to
+139
| // TestTokenResponseIncludesIDToken verifies that the token response includes an id_token | ||
| // when the openid scope is requested, per OIDC Core Section 3.1.3.3. | ||
| func (s *OAuthFlowSuite) TestAuthorizationCodeCannotBeReused() { |
Comment on lines
+186
to
+189
| // TestPasswordResetTokenCannotBeReused verifies that a password reset token is | ||
| // single-use: the first reset succeeds, the second attempt with the same token | ||
| // is rejected. Guards against the TOCTOU on auth_email_tokens.used_at. | ||
| func (s *OAuthFlowSuite) TestLoginFailedPreservesAllScopes() { |
Comment on lines
+385
to
+389
| // TestAuthorizationCodeCannotBeReused verifies that an authorization code is | ||
| // single-use: the first token exchange succeeds, the second with the same code | ||
| // is rejected with invalid_grant. Guards against the TOCTOU where two concurrent | ||
| // token requests could both consume the same code. | ||
| func (s *OAuthFlowSuite) TestTokenResponseIDTokenAbsentWithoutOpenID() { |
Comment on lines
+725
to
+727
| // TestOAuthFlowWithMFA verifies that users with MFA enabled are redirected to the MFA page | ||
| // and can complete the flow by entering a valid TOTP code. | ||
| func (s *OAuthFlowSuite) TestOIDCDiscoveryEndpoint() { |
Comment on lines
+23
to
+25
| // TestTokenResponseIDTokenAbsentWithoutOpenID verifies that no id_token is returned | ||
| // when the openid scope is not requested. | ||
| func (s *OAuthFlowSuite) TestSuccessPageRequiresAuthentication() { |
Comment on lines
+73
to
+75
| // mustLoginAndGetAuthorizeClient is a helper that creates a user, logs in to get a session, | ||
| // and returns an HTTP client with the session cookie and the registered OAuth client. | ||
| func (s *OAuthFlowSuite) TestAuthorizeInvalidScopeRedirectsErrorToClient() { |
Comment on lines
+582
to
+585
| resp, err := client.Get(authURL) | ||
| s.Require().NoError(err) | ||
| defer resp.Body.Close() | ||
|
|
||
| s.Require().Equal(http.StatusFound, resp.StatusCode) | ||
| location := resp.Header.Get("Location") | ||
| s.Require().Contains(location, "code=") | ||
| resp.Body.Close() |
Address review feedback on the integration-test split: - The split left each function carrying the *next* function's doc comment (an artifact of slicing between a comment and its func). Re-derived every doc comment from the original monolith so each comment again describes the function it sits above, and removed dangling section-header comments left at end-of-file. - TestPasswordResetTokenCannotBeReused: the replay POST sent a "password" field, but the reset handler reads "new_password". The replay was rejected for the wrong reason (missing field) rather than proving token single-use. Use the correct field name so the test exercises real token-reuse rejection. - avatar mustCompleteAuthorizationFlow: assert the /oauth/authorize response is 302 before reading Location, so a regression to a non-redirect status fails with a clear message.
go test -tags integration ./... is additive: it rebuilds and re-runs every package's untagged unit tests on top of the integration suites, so the unit tests ran twice per CI run. Scope the integration step to the only package that has integration tests (pkg/httpserver) and to the two suite entrypoints via -run, so it runs just the container-backed suites. Also hoist the test JWT_PRIVATE_KEY/JWT_ISSUER to job-level env so the block is defined once instead of being duplicated across every step.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The OAuth integration tests were all in a single 4,389-line file
(oauth_integration_test.go) containing one OAuthFlowSuite with 88 test
methods spanning many unrelated domains. Split it into focused files
that share a common base:
All 88 test methods stay on OAuthFlowSuite, so suite discovery is
unchanged.
Fixes inconsistent build-tag gating:
commented out ("// /* //go:build integration */"), so the
container-backed suite ran on every
go test, whileavatar_integration_test.go (real tag) never ran in CI.
generateCodeChallenge and so had never compiled under the tag; the
helper is now defined once in integration_common_test.go and reused
by both suites (replacing the inline S256 computation).
Wires integration tests into automation:
test-integrationtarget (go test -tags integration).tagged suites (testcontainers needs the runner's Docker daemon).
Minor cleanup: email_verification_test.go replaces hand-rolled
contains/findSubstring helpers with strings.Contains.