Add Tests#3
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors services and handlers to use interfaces, adds a comprehensive test suite for webhook handlers, and adjusts CI/Makefile for coverage reporting.
- Introduce
AuthServiceinterface and rename concrete implementation toauthService - Update router and handler constructors to accept service interfaces and enforce early returns on errors
- Add handler tests and configure
make coverageand CI to run tests
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/service/auth.go | Added AuthService interface, renamed struct to authService |
| internal/handlers/webhook.go | Updated signature to use interfaces; added missing return after errors; removed default status fallback |
| cmd/server/server.go | Incorrect dereference when passing request service to router |
| internal/metrics/recorder.go | Recorder interface missing methods used in tests |
| internal/handlers/webhook_test.go | Added extensive webhook handler test suite |
Comments suppressed due to low confidence (2)
internal/handlers/webhook.go:60
- Removing the default status fallback causes
responseCodeto remain 0 if the form value is empty; consider restoringif responseCode == 0 { responseCode = http.StatusOK }to preserve prior behavior.
responseCode, _ := strconv.Atoi(r.FormValue("response_code"))
internal/metrics/recorder.go:4
- The
Recorderinterface lacks anIncWebhooksUpdated()method, which is invoked in handler tests; add this method (and any others tests rely on) to the interface.
type Recorder interface {
There was a problem hiding this comment.
Pull Request Overview
This PR adds test coverage for several handlers and services while refactoring service interfaces to be passed by value rather than by pointer. Key changes include updating method signatures in AuthService, modifying router function parameter types, and adding comprehensive tests for webhook handlers.
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/service/auth.go | Refactored AuthService method receivers and return type |
| internal/routers/* | Updated function signatures to use interface values |
| internal/repository/mocks/* | Updated mocks to match new interface signatures |
| internal/handlers/* | Updated handler constructions and error handling improvements |
| cmd/server/server.go, go.mod, Makefile | Minor updates to imports and test-related commands |
| .github/workflows/ci.yaml | Uncommented test step in CI workflow |
Comments suppressed due to low confidence (1)
internal/handlers/webhook_api.go:21
- The type name 'WebhookAiHandler' appears to be a typo; consider renaming it to 'WebhookApiHandler' for consistency with the file name and its usage.
type WebhookAiHandler struct {
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors service and handler layers to depend on interfaces and adds tests for the webhook handlers.
- Refactor constructors to accept service interfaces rather than concrete pointers
- Add testify-based mocks and a comprehensive test suite for
WebhookHandler - Update CI/Makefile to run tests and generate coverage
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/service/auth.go | Refactor AuthService to use an interface and add mockgen tag |
| internal/routers/webhook.go | Update NewWebhookRouter signature to accept interface types |
| internal/routers/web.go | Update NewWebRouter signature and handler argument unpacking |
| internal/routers/api.go | Update NewApiRouter signature to accept interface types |
| internal/repository/mocks/webhook_mock.go | Add WebhookRepositoryMock using testify |
| internal/repository/mocks/user_mock.go | Add UserRepositoryMock using testify |
| internal/middlewares/middleware.go | Update RequireAPIKey to accept AuthService interface |
| internal/metrics/mocks/recorder_mock.go | Add RecorderMock for metrics.Recorder |
| internal/handlers/webhook_test.go | Add WebhookHandler test suite using testify/suite |
| internal/handlers/webhook_requests.go | Refactor WebhookRequestHandler to use interfaces |
| internal/handlers/webhook_api.go | Update WebhookAiHandler to use WebhookService interface |
| internal/handlers/webhook.go | Refactor WebhookHandler, improve error handling returns |
| internal/handlers/home.go | Update HomeHandler to use interfaces |
| internal/handlers/auth.go | Refactor AuthHandler to use AuthService interface |
| go.mod | Remove duplicate testify require, add it once |
| cmd/server/server.go | Reorder imports for routers, service, store |
| Makefile | Add coverage target |
| .vscode/settings.json | Disable Makefile auto-configure |
| .github/workflows/ci.yaml | Re-enable tests step in CI |
Comments suppressed due to low confidence (6)
internal/handlers/webhook_api.go:21
- [nitpick] The struct is named
WebhookAiHandlerbut the constructor and file refer toApi. Rename toWebhookApiHandlerfor consistency.
type WebhookAiHandler struct {
internal/service/auth.go:32
- [nitpick] The comment refers to
AuthServicebut the actual type is nowauthService. Update the doc comment to match the type name or adjust naming.
// AuthService holds user business logic
internal/service/auth.go:42
- The
quitchannel passed toPeriodicCleanupis never closed, causing the cleanup goroutine to run indefinitely. Consider closing the channel or using a context to manage its lifecycle.
quit := make(chan struct{})
internal/handlers/webhook.go:58
- [nitpick] Defaulting
responseCodeto 0 may produce an invalid HTTP status. Reintroduce a fallback (e.g.,http.StatusOK) when parsing yields 0.
responseCode, _ := strconv.Atoi(r.FormValue("response_code"))
internal/middlewares/middleware.go:14
- The
RequireAPIKeymiddleware is untested. Add unit tests to validate behavior for valid, missing, and invalid API keys.
func RequireAPIKey(auth service.AuthService) func(http.Handler) http.Handler {
internal/handlers/webhook_requests.go:28
- The
sessionStorefield is never initialized inNewWebhookRequestHandlerand is not used. Remove it or initialize it to avoid dead code.
sessionStore gormstore.Store
There was a problem hiding this comment.
Pull Request Overview
This PR adds tests and updates service and handler signatures to use value types instead of pointers, along with improving error handling and updating CI and build configurations.
- Refactors AuthService and other service interfaces to use non-pointer types and renames the concrete type.
- Updates router and handler functions accordingly and adds missing return statements after error responses.
- Includes new test cases, updated mocks, and enhancements to CI workflows and Makefile for test coverage.
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/service/auth.go | Refactored AuthService interface and implementation; renamed struct type. |
| internal/routers/*.go | Converted service dependency parameters from pointers to values. |
| internal/repository/mocks/*.go | Added/updated mocks for Webhook and User repositories. |
| internal/handlers/*.go | Updated handlers to match new service signatures and improved error flows. |
| internal/handlers/webhook_test.go | Added comprehensive tests for webhook endpoints. |
| go.mod, Makefile, .github/workflows/ci.yaml | Updated dependencies and added test coverage commands in CI and Makefile. |
| .vscode/settings.json | Added editor configuration for Makefile. |
Comments suppressed due to low confidence (1)
internal/handlers/webhook_api.go:19
- The struct name 'WebhookAiHandler' is inconsistent with the constructor 'NewWebhookApiHandler'. Consider renaming it to 'WebhookApiHandler' for consistency.
type WebhookAiHandler struct {
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive unit tests, introduces mock implementations, and standardizes handler and router constructor signatures.
- Refactor router and handler constructors to accept service interfaces instead of pointers
- Introduce mocks for repository, metrics, and database layers
- Extend HTTP handlers with early returns on error and add full test coverage for
WebhookHandler
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/routers/webhook.go | Update NewWebhookRouter to use interface types for services |
| internal/routers/web.go | Refactor NewWebRouter signatures and fix metrics parameter |
| internal/routers/api.go | Change NewApiRouter to use service interfaces |
| internal/repository/mocks/webhook_mock.go | Add mock for WebhookRepository |
| internal/repository/mocks/user_mock.go | Add mock for UserRepository |
| internal/middlewares/middleware.go | Update RequireAPIKey to accept interface type |
| internal/metrics/mocks/recorder_mock.go | Add mock for metrics recorder |
| internal/handlers/webhook_test.go | Add full suite of tests for WebhookHandler |
| internal/handlers/webhook_requests.go | Update WebhookRequestHandler to use interfaces |
| internal/handlers/webhook_api.go | Change WebhookAiHandler (API) to use interface types |
| internal/handlers/webhook.go | Add early returns, update logger usage, remove default code bug |
| internal/handlers/home.go | Refactor HomeHandler constructors to use interface types |
| internal/handlers/auth.go | Simplify AuthHandler, removed custom password validation |
| internal/db/mocks/db.go | Add GORM DB mock connection helper |
| go.mod | Tidy dependencies and add testify |
| cmd/server/server.go | Update auth service initialization with hasher and validator |
| Makefile | Add coverage target |
| .github/workflows/ci.yaml | Enable running tests in CI |
No description provided.