chore: move testing.http to a single folder #814
Conversation
WalkthroughThe pull request introduces a comprehensive refactoring of the HTTP testing infrastructure within the Goravel framework. The changes primarily focus on enhancing the Changes
Sequence DiagramsequenceDiagram
participant TestCase
participant TestRequest
participant Route
participant Session
participant JSON
TestCase->>TestRequest: Create with dependencies
TestRequest->>Route: Inject route
TestRequest->>Session: Inject session manager
TestRequest->>JSON: Inject JSON handler
TestRequest-->>TestCase: Return configured test request
Possibly related PRs
Finishing Touches
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: 0
🧹 Nitpick comments (6)
testing/http/assertable_json.go (1)
117-122: Refactor duplicate code into a helper functionThe code for marshaling items and creating a new
AssertableJsoninstance is duplicated in theFirst,HasWithScope, andEachmethods. To improve maintainability and reduce code duplication, consider abstracting this logic into a helper function.Apply this refactor:
+// Helper function to create AssertableJson from an item +func (r *AssertableJson) newAssertableJsonFromItem(item any, errorMessage string) (contractstesting.AssertableJSON, bool) { + itemJson, err := r.json.Marshal(item) + if !assert.NoError(r.t, err, errorMessage) { + return nil, false + } + newJson, err := NewAssertableJSON(r.t, r.json, string(itemJson)) + if !assert.NoError(r.t, err, errorMessage) { + return nil, false + } + return newJson, true +} // Inside `First` method: - itemJson, err := r.json.Marshal(firstItem) - if assert.NoError(r.t, err, "Failed to marshal the first item") { - newJson, err := NewAssertableJSON(r.t, r.json, string(itemJson)) - if assert.NoError(r.t, err, "Failed to create AssertableJSON for first item") { - callback(newJson) - } - } + if newJson, ok := r.newAssertableJsonFromItem(firstItem, "Failed to create AssertableJSON for first item"); ok { + callback(newJson) + } // Inside `HasWithScope` method: - itemJson, err := r.json.Marshal(array[0]) - if !assert.NoError(r.t, err, "Failed to marshal the first item of array") { - return r - } - newJson, err := NewAssertableJSON(r.t, r.json, string(itemJson)) - if !assert.NoError(r.t, err, "Failed to create AssertableJSON for first item in scoped array") { - return r - } + if newJson, ok := r.newAssertableJsonFromItem(array[0], "Failed to create AssertableJSON for first item in scoped array"); ok { + callback(newJson) + } // Inside `Each` method: - itemJson, err := r.json.Marshal(item) - if !assert.NoError(r.t, err) { - continue - } - newJson, err := NewAssertableJSON(r.t, r.json, string(itemJson)) - if !assert.NoError(r.t, err) { - continue - } + if newJson, ok := r.newAssertableJsonFromItem(item, "Failed to create AssertableJSON for item in Each"); ok { + callback(newJson) + }Also applies to: 144-150, 172-178
testing/test_case.go (1)
10-10: Consider using an alias for the http package import.Since we're moving testing.http to a dedicated folder, consider using an alias like
httptestto avoid confusion with the standardhttppackage.-import "github.com/goravel/framework/testing/http" +import httptest "github.com/goravel/framework/testing/http"testing/http/test_request_test.go (1)
34-43: Consider extracting test constants.The initialization of test request contains several magic strings and empty maps. Consider extracting these into named constants or test fixtures.
+const ( + defaultTestContext = "test_context" +) + s.testRequest = &TestRequest{ t: s.T(), - ctx: context.Background(), + ctx: defaultTestContext, defaultHeaders: make(map[string]string), defaultCookies: make(map[string]string), sessionAttributes: make(map[string]any), json: &testJson{}, route: s.mockRoute, session: s.mockSessionManager, }testing/http/assertable_json_test.go (1)
26-26: Consider using a test helper for AssertableJSON initialization.The initialization of
&testJson{}is duplicated across multiple test cases. Consider extracting this into a test helper function to improve maintainability.+func newTestAssertableJSON(t *testing.T, jsonStr string) (contractstesting.AssertableJSON, error) { + return NewAssertableJSON(t, &testJson{}, jsonStr) +} -assertable, err := NewAssertableJSON(s.T(), &testJson{}, validJSON) +assertable, err := newTestAssertableJSON(s.T(), validJSON)Also applies to: 30-30, 37-37, 52-52, 63-63, 75-75, 87-87, 99-99, 118-118, 139-139, 152-152, 172-172, 194-194, 210-210
testing/http/test_response_test.go (2)
20-20: Extract test response creation into a helper function.The creation of test responses with
NewTestResponseis duplicated across multiple test cases. Consider extracting this into a helper function to reduce duplication and improve maintainability.+func newTestResponse(t *testing.T, res *http.Response) contractstesting.TestResponse { + return NewTestResponse(t, res, &testJson{}, nil) +} -r := NewTestResponse(t, res, &testJson{}, nil) +r := newTestResponse(t, res)Also applies to: 26-26, 32-32, 40-40, 46-46, 52-52, 58-58, 64-64, 70-70, 76-76, 82-82, 88-88, 94-94, 100-100, 106-106, 112-112, 118-118, 124-124, 130-130, 136-136, 142-142, 148-148, 154-154, 160-160
344-346: Consider using a builder pattern for TestResponseImpl initialization.The initialization of TestResponseImpl with session is repeated across test cases. Consider implementing a builder pattern to make the test setup more readable and maintainable.
+type TestResponseBuilder struct { + response *TestResponseImpl +} + +func NewTestResponseBuilder() *TestResponseBuilder { + return &TestResponseBuilder{ + response: &TestResponseImpl{}, + } +} + +func (b *TestResponseBuilder) WithSession(session interface{}) *TestResponseBuilder { + b.response.session = session + return b +} + +func (b *TestResponseBuilder) Build() *TestResponseImpl { + return b.response +} -testResponse := &TestResponseImpl{ - session: mockSessionManager, -} +testResponse := NewTestResponseBuilder(). + WithSession(mockSessionManager). + Build()Also applies to: 358-360, 373-375, 382-384
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
testing/http/assertable_json.go(5 hunks)testing/http/assertable_json_test.go(13 hunks)testing/http/test_request.go(6 hunks)testing/http/test_request_test.go(2 hunks)testing/http/test_response.go(7 hunks)testing/http/test_response_test.go(12 hunks)testing/test_case.go(1 hunks)
⏰ Context from checks skipped due to timeout of 300000ms (2)
- GitHub Check: test / windows (1.23)
- GitHub Check: test / windows (1.22)
🔇 Additional comments (3)
testing/http/test_request.go (1)
28-30: Ensurerouteis properly initializedIn the
callmethod, there's a check forr.routebeingnilthat results in a fatal error if true. To prevent unexpected test terminations, ensure thatrouteis properly initialized before making requests.Also applies to: 165-169
testing/http/test_response.go (1)
72-89: Handlesessioninitialization errorsIn the
Sessionmethod, whenr.sessionisnil, an errorerrors.SessionFacadeNotSetis returned. Verify that this error is properly defined and handled, and consider providing a more descriptive error message to aid in debugging.testing/test_case.go (1)
17-17:⚠️ Potential issueFix undefined variable references.
The implementation references undefined variables
json,routeFacade, andsessionFacade. These dependencies need to be properly initialized or injected.Let's verify if these variables are defined elsewhere:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #814 +/- ##
==========================================
- Coverage 69.57% 69.56% -0.01%
==========================================
Files 215 215
Lines 18499 18508 +9
==========================================
+ Hits 12870 12876 +6
- Misses 4962 4965 +3
Partials 667 667 ☔ View full report in Codecov by Sentry. |
📑 Description
Summary by CodeRabbit
Refactor
New Features
Chores