Chi is a dep even for pure go implementations, refactor its usage in the main package to pure go. #661
Conversation
WalkthroughThe changes in this pull request involve modifications to several test files related to the Huma API framework. Key updates include the introduction of a new test function in Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
autoregister_test.go (1)
42-42: LGTM! Good move towards standard library.The switch to
http.NewServeMux()aligns well with the goal of removing Chi dependencies and simplifying the implementation. Whilehttp.ServeMuxhas fewer features than Chi, it's sufficient for these tests and reduces external dependencies.adapters/humachi/humachi_test.go (1)
319-354: Good test coverage, consider these enhancementsThe test effectively verifies router prefix handling, but could be enhanced for more comprehensive coverage:
- Add assertions for the response body content
- Use a test-specific server URL instead of hardcoded "http://localhost:8888/api"
- Consider using table-driven tests for different path scenarios
Here's a suggested enhancement:
func TestChiRouterPrefix(t *testing.T) { + testCases := []struct { + name string + path string + expectedStatus int + expectedBody string + }{ + { + name: "base path", + path: "/api/test", + expectedStatus: http.StatusOK, + expectedBody: `{"field":""}`, + }, + { + name: "docs path", + path: "/api/docs", + expectedStatus: http.StatusOK, + }, + } + mux := chi.NewMux() var api huma.API mux.Route("/api", func(r chi.Router) { config := huma.DefaultConfig("My API", "1.0.0") - config.Servers = []*huma.Server{{URL: "http://localhost:8888/api"}} + config.Servers = []*huma.Server{{URL: "http://test-server/api"}} api = New(r, config) }) // ... rest of the setup code ... tapi := humatest.Wrap(t, New(mux, huma.DefaultConfig("Test", "1.0.0"))) - resp := tapi.Get("/api/test") - assert.Equal(t, http.StatusOK, resp.Code) - assert.Contains(t, resp.Header().Get("Link"), "/api/schemas/TestOutputBody.json") - - resp = tapi.Get("/api/docs") - assert.Equal(t, http.StatusOK, resp.Code) - assert.Contains(t, resp.Body.String(), "/api/openapi.yaml") + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + resp := tapi.Get(tc.path) + assert.Equal(t, tc.expectedStatus, resp.Code) + + if tc.expectedBody != "" { + assert.JSONEq(t, tc.expectedBody, resp.Body.String()) + } + + if tc.path == "/api/test" { + assert.Contains(t, resp.Header().Get("Link"), + "/api/schemas/TestOutputBody.json") + } else if tc.path == "/api/docs" { + assert.Contains(t, resp.Body.String(), "/api/openapi.yaml") + } + }) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
adapters/humachi/humachi_test.go(2 hunks)api_test.go(0 hunks)autoregister_test.go(1 hunks)huma_test.go(3 hunks)resolver_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- api_test.go
🔇 Additional comments (6)
resolver_test.go (1)
37-37: LGTM! Router change aligns with PR objectives.
The switch to http.NewServeMux() successfully removes the Chi dependency while maintaining the test's functionality. This change simplifies the codebase by using the standard library's router implementation.
Let's verify the consistency of router usage across the codebase:
adapters/humachi/humachi_test.go (2)
16-16: LGTM: Appropriate test dependencies added
The addition of humatest and testify/assert packages is appropriate for the new test function.
Also applies to: 18-18
Line range hint 23-318: LGTM: Appropriate placement of Chi-dependent benchmarks
The benchmark tests are correctly placed in the humachi adapter package, which is the designated location for Chi-specific implementations. This aligns with the PR's objective of removing Chi dependencies from the main package while maintaining the ability to benchmark Chi router performance.
huma_test.go (3)
25-25: LGTM! Import and variable changes align with PR objectives.
The changes correctly replace the Chi router dependency with the pure Go implementation by:
- Importing the humago adapter
- Updating NewExampleAPI to use humago.New
Also applies to: 30-30
1901-1906: LGTM! Router initialization properly migrated to pure Go.
The changes correctly:
- Use standard library's http.NewServeMux instead of Chi router
- Initialize the API with the humago adapter while maintaining the existing configuration
1918-1918: LGTM! Middleware integration maintained with new router.
The Recoverer middleware is properly integrated with the new router implementation, maintaining the existing error handling functionality.
| @@ -40,7 +39,7 @@ func (s *ItemsHandler) RegisterListItems(api huma.API) { | |||
|
|
|||
| func ExampleAutoRegister() { | |||
| // Create the router and API. | |||
| router := chi.NewMux() | |||
| router := http.NewServeMux() | |||
There was a problem hiding this comment.
💡 Codebase verification
Router initialization is inconsistent across test files
The verification reveals inconsistent router initialization patterns across test files:
autoregister_test.gouseshttp.NewServeMux()sse/sse_example_test.gouseschi.NewMux()humacli/humacli_test.gouseschi.NewRouter()
Additionally, Chi router imports are still present in multiple test files:
- adapters/adapters_test.go
- adapters/humachi/humachi_test.go
- humacli/humacli_test.go
- sse/sse_example_test.go
These inconsistencies should be addressed to maintain uniformity across the test suite.
🔗 Analysis chain
Verify consistent router changes across test files.
Let's ensure this router change is consistently applied across all test files to maintain uniformity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining Chi router usage in test files
# and verify consistency of http.NewServeMux usage
echo "Checking for any remaining Chi router imports or usage in test files..."
rg -l 'github.com/go-chi/chi' '*_test.go'
echo "Verifying consistent router initialization in test files..."
rg -A 1 'router :=' '*_test.go'
Length of output: 457
Script:
#!/bin/bash
# Let's first find all test files in the repository
echo "Finding all test files..."
fd "_test\.go$"
# Then check for Chi router imports and usage
echo -e "\nChecking for Chi router imports in test files..."
rg -l "github.com/go-chi/chi" $(fd "_test\.go$")
# Finally check router initialization patterns
echo -e "\nChecking router initialization patterns in test files..."
rg -C 2 "router\s*:=\s*" $(fd "_test\.go$")
Length of output: 3596
|
Took care of the two spots the rabbit pointed out and added an optional commit to remove the use of syscall which won't compile on a windows machine. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #661 +/- ##
==========================================
- Coverage 93.04% 92.76% -0.29%
==========================================
Files 22 22
Lines 4905 4905
==========================================
- Hits 4564 4550 -14
- Misses 298 308 +10
- Partials 43 47 +4 ☔ View full report in Codecov by Sentry. |
For starters, this project is awesome. I wish it existed in 2020 when I was over hauling an app.
I noticed chi in my deps when I was playing around with this, gave me an excuse to look into your code base. After this change I confirmed chi is removed from my deps. I didn't see a great place for
TestChiRouterPrefix, I went with adapters as that felt fitting. However, you all only have benches in there at the moment and it's not broken into it's own test package.If you have some idea's, I'd be happy put them in place.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor