-
Notifications
You must be signed in to change notification settings - Fork 108
feat: [#841] Add testing support for HTTP client [2] #1339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @krishankumar01, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request, which is the second part of adding HTTP client testing support, focuses on refining the testing infrastructure within the framework. It introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1339 +/- ##
==========================================
+ Coverage 70.22% 70.57% +0.35%
==========================================
Files 281 286 +5
Lines 16852 17519 +667
==========================================
+ Hits 11834 12364 +530
- Misses 4525 4635 +110
- Partials 493 520 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request significantly improves the HTTP testing support. The introduction of the TestingT interface is a great step towards making the testing helpers more generic and testable. Moving the Bind functionality from Request to Response is a logical API design improvement, making the testing flow more intuitive. The addition of comprehensive comments to the public interfaces and the improved test cases for AssertableJSON are also valuable contributions to the codebase's quality and maintainability. I have a couple of suggestions to further improve the new TestingT interface and fix a typo.
| "github.com/goravel/framework/contracts/testing/docker" | ||
| ) | ||
|
|
||
| type TestingT interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introducing this so that I can mock the assertions. This helps me test scenarios like AssertableJson, where, if a test case fails and I directly use *testing.T, it prints output to the console. By accepting this interface, I can mock it and then inspect whether the test case correctly triggers a failing assertion.
See:
| mockT := &MockTestingT{} |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a comprehensive testing framework for the HTTP client, which is a great addition. It allows for faking requests, creating response sequences, and making assertions on sent requests. The implementation is well-structured, introducing concepts like FakeState and FakeTransport to cleanly separate the mocking logic. The changes to the testing interfaces to use TestingT also improve the testability of the framework itself. I've identified a few areas for improvement, mainly concerning error handling and the robustness of the mock pattern matching, to make this new feature even more solid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces comprehensive testing support for the HTTP client, including faking requests, stubbing responses, and making assertions. The changes are extensive, adding new interfaces, implementations, and tests for fakes, response factories, and response sequences. The API design is clean and follows patterns seen in other popular frameworks. I've identified a couple of areas for improvement related to error handling in the fake factory and API consistency, but overall this is a solid and valuable feature addition.
hwbrzzl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR 👍 Left a few questions, but I haven't completed the review yet, will continue tomorrow.
http/client/fake_response.go
Outdated
| } | ||
| } | ||
|
|
||
| func (r *FakeResponse) File(path string, status int) client.Response { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about File(status int, path string)?
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a robust testing layer for the HTTP client, including request interception, mocking, and assertions. The changes are extensive, adding new interfaces and implementations for faking responses, sequences, and matching rules. The core HTTP client factory and request logic have been updated to integrate these testing capabilities, and the changes are well-supported by new tests.
One key issue was identified: the implementation for handling mock data doesn't fully align with the feature description, specifically for structured data like maps. A fix has been suggested to ensure this works as expected.
Additionally, it's worth noting that there's a breaking change in the testing API: the Bind method has been moved from the test Request to the Response object. This is a good architectural improvement, but it would be beneficial to highlight this in the pull request description to guide users through the migration.
hwbrzzl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, LGTM
|
|
||
| // Json creates a mock response with a JSON body and "application/json" content type. | ||
| Json(obj any, code int) Response | ||
| Json(code int, obj any) Response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Json(code int, obj any) Response | |
| Json(status int, obj any) Response |
📑 Description
Closes goravel/goravel#841
Introduces a robust testing layer for the HTTP Client, enabling request interception, stateful mocking, and traffic assertions without external network calls.
Key Features
Fake()accepts simple values (int,string), structured data (map,json), dynamic handlers (func), or stateful sequences.ResponseSequencewith repetition (PushStatus(200, 3)), strict exhaustion (error on overflow), and explicit fallbacks (WhenEmpty).PreventStrayRequests/AllowStrayRequeststo enforce hermetic testing boundaries.AssertSent,AssertNotSent,AssertSentCount,AssertNothingSent.Usage Examples
1. Advanced Mocking
Handle various scenarios in a single setup:
2. Assertions
Validate outgoing traffic structure:
3. Strict Mode Integration
Ensure tests fail on unmocked network calls:
Limitations
ResponseSequencedefaults to returning an error (HttpClientHandlerReturnedNil) when exhausted. Use.WhenEmpty()to define persistent behavior.Factoryinstance. Global singletons requireHttp.Reset()between tests.✅ Checks