|
| 1 | +# Replace migueleliasweb/go-github-mock with stretchr/testify/mock |
| 2 | + |
| 3 | +**Type:** Enhancement / Refactoring |
| 4 | +**Labels:** `enhancement`, `testing`, `technical-debt` |
| 5 | + |
| 6 | +--- |
| 7 | + |
| 8 | +### Describe the feature or problem you'd like to solve |
| 9 | + |
| 10 | +The current test suite uses [migueleliasweb/go-github-mock](https://github.com/migueleliasweb/go-github-mock) for mocking GitHub REST API responses in unit tests. While this library has served us well, there are several reasons to consider replacing it with [stretchr/testify/mock](https://github.com/stretchr/testify): |
| 11 | + |
| 12 | +1. **Dependency Consolidation**: We already use `stretchr/testify` for assertions (`assert` and `require`). Using `testify/mock` would consolidate our testing dependencies. |
| 13 | + |
| 14 | +2. **Interface-based Mocking**: `testify/mock` encourages interface-based mocking, which leads to better separation of concerns and more flexible test design. |
| 15 | + |
| 16 | +3. **Maintenance & Activity**: `stretchr/testify` is one of the most widely used Go testing libraries with active maintenance. |
| 17 | + |
| 18 | +4. **Type Safety**: Interface-based mocking provides compile-time type checking, whereas HTTP-level mocking relies on runtime matching. |
| 19 | + |
| 20 | +5. **Test Clarity**: Mock expectations at the interface level make tests more readable and focused on behavior rather than HTTP transport details. |
| 21 | + |
| 22 | +### Current State |
| 23 | + |
| 24 | +The codebase currently uses `go-github-mock` extensively (metrics from `grep` search): |
| 25 | + |
| 26 | +| Metric | Count | |
| 27 | +|--------|-------| |
| 28 | +| Files using go-github-mock | 16 | |
| 29 | +| `mock.NewMockedHTTPClient` calls | ~449 | |
| 30 | +| `mock.WithRequestMatchHandler` calls | ~267 | |
| 31 | +| `mock.WithRequestMatch` calls | ~79 | |
| 32 | +| Unique mock endpoint patterns | ~80+ | |
| 33 | + |
| 34 | +*Note: Run `grep -c "mock.NewMockedHTTPClient" pkg/github/*_test.go` to verify current counts.* |
| 35 | + |
| 36 | +**Files affected:** |
| 37 | +- `pkg/github/*_test.go` (14 files) |
| 38 | +- `pkg/raw/raw_test.go` |
| 39 | +- `pkg/raw/raw_mock.go` |
| 40 | + |
| 41 | +### Proposed solution |
| 42 | + |
| 43 | +Replace HTTP-level mocking with interface-based mocking using `testify/mock`: |
| 44 | + |
| 45 | +#### Phase 1: Create Mock Interfaces |
| 46 | +1. Define interfaces for GitHub client operations (if not already present) |
| 47 | +2. Create mock implementations using `testify/mock` |
| 48 | +3. Update the codebase to depend on interfaces rather than concrete clients |
| 49 | + |
| 50 | +#### Phase 2: Migrate Tests Incrementally |
| 51 | +1. Start with a single test file to establish patterns |
| 52 | +2. Create helper functions for common mock setups |
| 53 | +3. Migrate remaining test files one at a time |
| 54 | +4. Remove `go-github-mock` dependency when complete |
| 55 | + |
| 56 | +#### Example Migration |
| 57 | + |
| 58 | +**Before (HTTP-level mocking):** |
| 59 | +```go |
| 60 | +mockedClient := mock.NewMockedHTTPClient( |
| 61 | + mock.WithRequestMatch( |
| 62 | + mock.GetReposIssuesByOwnerByRepoByIssueNumber, |
| 63 | + mockIssue, |
| 64 | + ), |
| 65 | +) |
| 66 | +client := github.NewClient(mockedClient) |
| 67 | +``` |
| 68 | + |
| 69 | +**After (Interface-based mocking):** |
| 70 | +```go |
| 71 | +mockClient := new(MockGitHubClient) |
| 72 | +mockClient.On("GetIssue", ctx, "owner", "repo", 42).Return(mockIssue, nil) |
| 73 | +``` |
| 74 | + |
| 75 | +### Benefits |
| 76 | + |
| 77 | +1. **Simpler Test Setup**: No need to construct HTTP responses |
| 78 | +2. **Better Error Testing**: Easy to mock error conditions without crafting HTTP error responses |
| 79 | +3. **Faster Tests**: No HTTP round-trip overhead (even if mocked) |
| 80 | +4. **Clearer Intent**: Tests read more like specifications |
| 81 | +5. **Reduced Dependencies**: One less external dependency to maintain |
| 82 | + |
| 83 | +### Considerations |
| 84 | + |
| 85 | +1. **Migration Effort**: This is a significant refactoring with ~449 mock usages to update |
| 86 | +2. **Interface Design**: Need to carefully design interfaces that balance granularity and usability |
| 87 | +3. **GraphQL Mocking**: The existing `githubv4mock` for GraphQL is **out of scope** for this issue and will remain unchanged. It already provides a different mocking approach specific to GraphQL. |
| 88 | +4. **Breaking Changes**: Test file changes only, no production code changes expected |
| 89 | + |
| 90 | +### Implementation Plan |
| 91 | + |
| 92 | +- [ ] Audit current mock usage patterns to identify common interfaces needed |
| 93 | +- [ ] Design and implement mock interfaces for GitHub REST API client |
| 94 | +- [ ] Create helper functions and test utilities for common mock scenarios |
| 95 | +- [ ] Migrate test files incrementally (suggest starting with smallest files): |
| 96 | + - [ ] `pkg/github/code_scanning_test.go` (~4 mocks) |
| 97 | + - [ ] `pkg/github/secret_scanning_test.go` (~5 mocks) |
| 98 | + - [ ] `pkg/github/dependabot_test.go` (~6 mocks) |
| 99 | + - [ ] Continue with remaining files... |
| 100 | +- [ ] Update `docs/testing.md` to reflect new mocking patterns |
| 101 | +- [ ] Remove `go-github-mock` from `go.mod` after complete migration |
| 102 | + |
| 103 | +### Additional context |
| 104 | + |
| 105 | +**Current testing documentation reference:** |
| 106 | +> Mocking is performed using [go-github-mock](https://github.com/migueleliasweb/go-github-mock) or `githubv4mock` for simulating GitHub REST and GQL API responses. |
| 107 | +
|
| 108 | +This will need to be updated to: |
| 109 | +> Mocking is performed using [testify/mock](https://github.com/stretchr/testify#mock-package) for interface-based mocking or `githubv4mock` for simulating GitHub GQL API responses. |
| 110 | +
|
| 111 | +### Related |
| 112 | + |
| 113 | +- [stretchr/testify documentation](https://pkg.go.dev/github.com/stretchr/testify/mock) |
| 114 | +- Current testing guidelines: [`docs/testing.md`](../testing.md) |
0 commit comments