🔥 Feature: Add TestConfig to app.Test() for configurable testing #3161
Conversation
This commit is summarized as: - Add the struct `TestConfig` as a parameter for `app.Test()` instead of `timeout` - Add documentation of `TestConfig` to docs/api/app.md and in-line - Modify middleware to use `TestConfig` instead of the previous implementation Fixes gofiber#3149
- Fixes Test_Utils_TestConn_Closed_Write - Fixes missing regular write test
grivera64
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe changes introduce a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3161 +/- ##
==========================================
+ Coverage 82.75% 82.85% +0.10%
==========================================
Files 114 114
Lines 11151 11177 +26
==========================================
+ Hits 9228 9261 +33
+ Misses 1523 1518 -5
+ Partials 400 398 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (13)
middleware/idempotency/idempotency_test.go (1)
85-88: Excellent update to usefiber.TestConfig!This change aligns perfectly with the PR objectives by introducing the
TestConfigstruct for configurable testing. The update maintains the existing 15-second timeout while adding theErrOnTimeout: truesetting. This enhancement will improve error handling during tests, especially for scenarios involving streaming or long-running operations.Consider adding a test case that specifically verifies the timeout behavior with this new configuration. This would ensure that the
ErrOnTimeoutsetting is working as expected and would provide coverage for timeout scenarios.middleware/compress/compress_test.go (1)
Line range hint
42-192: Consider creating a helper function for TestConfigThe changes consistently apply
fiber.TestConfigacross all test functions, which is excellent. To further improve code maintainability and reduce duplication, consider creating a helper function that returns the commonTestConfig. This would centralize the configuration and make future changes easier.Example:
func getTestConfig() fiber.TestConfig { return fiber.TestConfig{ Timeout: 10 * time.Second, ErrOnTimeout: true, } }Then, in each test:
resp, err := app.Test(req, getTestConfig())This approach would make it easier to modify the test configuration globally if needed in the future.
docs/api/app.md (3)
543-544: LGTM! Consider adding a brief explanation of theTestConfigparameter.The updated method signature correctly reflects the introduction of the
TestConfigstruct, which aligns with the PR objectives. This change enhances the testing capabilities of the framework by allowing for more flexible configuration.Consider adding a brief explanation of the
TestConfigparameter immediately after the signature to help users understand its purpose and usage.
569-593: Improve structure and clarity ofTestConfiginformation.The added information about
TestConfigis crucial for users to understand the default behavior and potential pitfalls. However, the structure and presentation of this information could be improved for better clarity and readability.Consider the following suggestions:
- Move the default
TestConfiginformation (lines 569-576) closer to the method signature for immediate context.- Restructure the caution note (lines 578-593) to make it more concise and easier to understand. For example:
:::caution Be careful when using an empty `TestConfig{}`. It is not equivalent to the default configuration and may result in unexpected behavior: - Default: `Timeout: 1 second, ErrOnTimeout: true` - Empty `TestConfig{}`: `Timeout: 0, ErrOnTimeout: false` An empty `TestConfig{}` will cause the test to time out immediately, always resulting in a "test: empty response" error. :::
- Consider adding a brief example of how to use
TestConfigwith custom values to illustrate its practical application.
Line range hint
543-594: Enhance integration of newTestConfiginformation with existing documentation.While the changes to the
Testmethod documentation are consistent with the PR objectives, there's an opportunity to improve the integration of the new information with the existing content.Consider the following suggestions:
Update the introductory paragraph (lines 543-545) to mention the new
TestConfigoption alongside the existing timeout information.Modify the example (lines 547-568) to demonstrate the use of
TestConfig. For instance:// Example with default TestConfig resp, _ := app.Test(req) // Example with custom TestConfig customConfig := fiber.TestConfig{ Timeout: 2 * time.Second, ErrOnTimeout: false, } resp, _ := app.Test(req, customConfig)
- Add a brief explanation of when and why a user might want to use custom
TestConfigsettings, to provide context for this new feature.These changes will help users understand the new
TestConfigfeature in the context of the existingTestmethod functionality.🧰 Tools
🪛 LanguageTool
[grammar] ~597-~597: Did you mean “are” or “were”?
Context: ... response" error. ::: ## Hooks Hooks is a method to return hooks ...(SENT_START_NNS_IS)
helpers_test.go (3)
517-538: LGTM! Consider adding error case tests.The
Test_Utils_TestConn_ReadWritefunction effectively tests both read and write operations on thetestConnstructure. It verifies data integrity in both directions, which is crucial for ensuring the correct behavior of the connection.Consider adding test cases for error scenarios, such as reading or writing with a closed connection, to ensure robust error handling.
540-557: LGTM! Consider handling Close() error and adding read test.The
Test_Utils_TestConn_Closed_Writefunction effectively tests the behavior of writing to a closed connection. It verifies that writing after closing fails and that only data from successful writes is present.
- Consider handling the error from
conn.Close()instead of ignoring it with a linter directive. This would make the test more robust.- Add a test case for reading from a closed connection to ensure comprehensive coverage of closed connection behavior.
Example improvement for point 1:
- conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here + err = conn.Close() + require.NoError(t, err, "Failed to close connection")
517-557: Overall, the new test functions enhance the test coverage.The addition of
Test_Utils_TestConn_ReadWriteandTest_Utils_TestConn_Closed_Writesignificantly improves the test coverage for thetestConnstructure. These tests effectively verify the behavior of read and write operations, including edge cases like writing to a closed connection.To further improve the robustness of these tests, consider:
- Adding error case scenarios for both read and write operations.
- Ensuring all errors are properly handled and checked, including those from
Close()operations.- Adding a test for reading from a closed connection to complement the existing write test.
These additions would provide a more comprehensive test suite for the
testConnstructure, ensuring its reliability across various scenarios.helpers.go (3)
629-637: LGTM! Thread-safe Write method with closed state check.The
Writemethod has been improved with two key changes:
- It now uses a mutex lock to ensure thread-safety when writing to the buffer.
- It checks the
isClosedstate before writing, preventing writes to a closed connection.These changes enhance the reliability and correctness of the
Writeoperation.Consider using a custom error type or a package-level error variable for the "testConn is closed" error. This would allow for easier error checking by consumers of this method. For example:
var ErrConnClosed = errors.New("testConn is closed") // In the Write method if c.isClosed { return 0, ErrConnClosed }
639-645: LGTM! Thread-safe Close method implementation.The
Closemethod has been updated with important improvements:
- It now uses a mutex lock to ensure thread-safety when closing the connection.
- It sets the
isClosedstate to true, preventing further operations on the closed connection.These changes enhance the reliability and correctness of the
Closeoperation.Consider adding a check to prevent multiple calls to
Closefrom changing the state:func (c *testConn) Close() error { c.Lock() defer c.Unlock() if !c.isClosed { c.isClosed = true } return nil }This ensures that
isClosedis set to true only once, which could be useful for debugging or tracking the connection state.
616-645: Overall improvements in thread-safety and connection state management.The changes made to the
testConnstruct and its methods (Read,Write, andClose) significantly enhance the reliability and correctness of the test connection implementation:
- Thread-safety: The addition of
sync.Mutexand its usage in all methods prevents race conditions in concurrent scenarios.- Connection state tracking: The
isClosedflag helps manage the connection state, preventing operations on closed connections.- Consistent locking pattern: All methods now use the same locking pattern, ensuring uniform behavior across the struct.
These improvements make the
testConnmore robust and suitable for use in multi-threaded test environments. The changes align well with best practices for concurrent programming in Go.Consider adding a
Reset()method to thetestConnstruct. This would allow reusing the sametestConninstance across multiple tests, potentially improving performance in test suites with many connection-based tests.app_test.go (2)
1139-1142: Approved: TestConfig usage enhances timeout testing.The use of
TestConfighere effectively tests the new timeout functionality. SettingErrOnTimeout: trueallows testing of the error-on-timeout feature.Consider adding a comment explaining the purpose of the 20ms timeout, e.g.:
// Set a short timeout to trigger the timeout scenario quickly Timeout: 20 * time.Millisecond,
1479-1493: Excellent addition: Test case for timeout with empty response.This new test function enhances the test coverage by specifically addressing the scenario of a timeout with an empty response. It effectively uses the new
TestConfigstruct and verifies the correct error message.Consider adding a brief comment explaining the purpose of this test case, e.g.:
// Test_App_Test_timeout_empty_response verifies that a timeout with an empty response // returns the expected error message when ErrOnTimeout is false.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- app.go (4 hunks)
- app_test.go (4 hunks)
- ctx_test.go (1 hunks)
- docs/api/app.md (2 hunks)
- helpers.go (1 hunks)
- helpers_test.go (1 hunks)
- middleware/compress/compress_test.go (6 hunks)
- middleware/idempotency/idempotency_test.go (1 hunks)
- middleware/keyauth/keyauth_test.go (4 hunks)
- middleware/logger/logger_test.go (2 hunks)
- middleware/pprof/pprof_test.go (2 hunks)
- middleware/proxy/proxy_test.go (11 hunks)
- middleware/static/static_test.go (5 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
app.go
[failure] 960-960:
fmt.Errorf can be replaced with errors.New (perfsprint)
🔇 Additional comments (35)
middleware/pprof/pprof_test.go (1)
108-111: Improved test configuration withfiber.TestConfigThe changes here align well with the PR objectives. By introducing
fiber.TestConfig, you've enhanced the test's flexibility and error handling capabilities. TheErrOnTimeoutflag set to true is particularly useful for testing timeout scenarios, ensuring that the test can properly handle and report timed-out responses.This modification maintains the existing timeout duration while adding more granular control over the test behavior. It's a positive step towards more robust and configurable testing.
middleware/compress/compress_test.go (6)
42-45: LGTM: TestConfig usage enhances test configurabilityThe change from a simple timeout to
fiber.TestConfigaligns with the PR objectives. The newErrOnTimeout: truesetting allows for more precise control over timeout behavior, which is particularly useful for testing streaming responses.
78-81: LGTM: Consistent TestConfig usageThe change to use
fiber.TestConfigis consistent with the previous test function and aligns with the PR objectives. This consistency across tests is good for maintainability.
108-111: LGTM: Consistent TestConfig implementationThe change to use
fiber.TestConfigmaintains consistency with the previous test functions and adheres to the PR objectives. This uniformity across different compression algorithms (here, deflate) is commendable.
135-138: LGTM: TestConfig applied consistentlyThe implementation of
fiber.TestConfigfor the Brotli compression test maintains the consistency observed in previous test functions. This uniform approach across different compression algorithms enhances the overall test suite coherence.
162-165: LGTM: TestConfig applied to Zstd compression testThe use of
fiber.TestConfigin the Zstd compression test maintains the consistency seen throughout the file. This uniform approach across all compression algorithms ensures a standardized testing methodology.
189-192: LGTM: TestConfig applied to disabled compression scenarioThe consistent use of
fiber.TestConfigextends to the disabled compression test, maintaining uniformity across all test scenarios. This is particularly important as it ensures that the new configuration doesn't inadvertently affect scenarios where compression is intentionally disabled.docs/api/app.md (1)
Line range hint
543-594: Overall assessment: Documentation updates align with PR objectives but could be further improved.The changes introduced in this file successfully document the new
TestConfigfeature for theTestmethod, aligning well with the PR objectives. The updated method signature and the addition ofTestConfiginformation enhance the testing capabilities of the Fiber framework.However, there are opportunities to improve the documentation:
- Restructure the presentation of
TestConfiginformation for better clarity.- Integrate the new feature more seamlessly with the existing content.
- Provide more examples and context for using custom
TestConfigsettings.These improvements would make the new feature more accessible and understandable to users of the Fiber framework.
🧰 Tools
🪛 LanguageTool
[grammar] ~597-~597: Did you mean “are” or “were”?
Context: ... response" error. ::: ## Hooks Hooks is a method to return hooks ...(SENT_START_NNS_IS)
middleware/keyauth/keyauth_test.go (5)
107-110: LGTM: Updated app.Test call with TestConfigThe change to use
fiber.TestConfigin theapp.Testmethod call is consistent with the PR objectives. The configuration maintains the previous behavior:
Timeout: -1indicates no timeout, which is equivalent to the previous implementation.ErrOnTimeout: falseensures that no error is returned on timeout, preserving the existing functionality.This update enhances the test's configurability while maintaining backward compatibility.
215-218: LGTM: Consistent update to app.Test callThis change is consistent with the previous update to the
app.Testmethod call. It introducesfiber.TestConfigwith the same configuration:
Timeout: -1(no timeout)ErrOnTimeout: false(no error on timeout)The consistency in applying these changes across the test file is commendable.
235-238: LGTM: Consistent application of TestConfigThis change maintains consistency with the previous updates to the
app.Testmethod calls. Thefiber.TestConfigis applied with the same parameters:
Timeout: -1ErrOnTimeout: falseThe uniform application of these changes throughout the test file demonstrates a thorough and systematic approach to updating the test configurations.
362-365: LGTM: Consistent TestConfig implementationThis change continues the pattern of updating
app.Testmethod calls withfiber.TestConfig. The configuration remains consistent:
Timeout: -1ErrOnTimeout: falseThe uniform application of these changes throughout the entire test file demonstrates a comprehensive approach to implementing the new TestConfig feature.
Line range hint
1-665: Overall: Successful implementation of TestConfigThe changes in this file consistently update all
app.Testmethod calls to use the newfiber.TestConfigstructure. Key points:
- All updates use the same configuration (
Timeout: -1, ErrOnTimeout: false), maintaining the previous behavior.- The changes are applied systematically throughout the file, demonstrating thoroughness.
- No modifications were made to the existing test logic, preserving test coverage and behavior.
These updates successfully implement the TestConfig feature as outlined in the PR objectives, enhancing the configurability of the tests while maintaining backward compatibility.
middleware/proxy/proxy_test.go (12)
113-116: LGTM: Improved test configurationThe update to use
fiber.TestConfigwith a specific timeout and error handling for timeouts is a good improvement. This change enhances the test's reliability and error handling capabilities.
178-181: LGTM: Consistent test configuration improvementThe update to use
fiber.TestConfigwith a specific timeout and error handling for timeouts is consistent with the previous change. This improves the test's reliability and maintains consistency across test cases.
204-207: LGTM: Consistent test configuration improvementThe update to use
fiber.TestConfigwith a specific timeout and error handling for timeouts is consistent with the previous changes. This maintains the improved test reliability and consistency across test cases.
233-236: LGTM: Consistent test configuration improvementThe update to use
fiber.TestConfigwith a specific timeout and error handling for timeouts is consistent with the previous changes. This maintains the improved test reliability and consistency across test cases.
414-417: LGTM: Consistent test configuration improvementThe update to use
fiber.TestConfigwith a specific timeout and error handling for timeouts is consistent with the previous changes. This maintains the improved test reliability and consistency across test cases.
441-444: LGTM: Consistent test configuration improvementThe update to use
fiber.TestConfigwith a specific timeout and error handling for timeouts is consistent with the previous changes. This maintains the improved test reliability and consistency across test cases.
542-545: LGTM: Consistent test configuration improvementThe update to use
fiber.TestConfigwith a specific timeout and error handling for timeouts is consistent with the previous changes. This maintains the improved test reliability and consistency across test cases.
583-586: LGTM: Consistent test configuration improvementThe update to use
fiber.TestConfigwith a specific timeout and error handling for timeouts is consistent with the previous changes. This maintains the improved test reliability and consistency across test cases.
607-610: LGTM: Consistent test configuration improvementThe update to use
fiber.TestConfigwith a specific timeout and error handling for timeouts is consistent with the previous changes. This maintains the improved test reliability and consistency across test cases.
654-657: LGTM: Consistent test configuration improvement with a noteThe update to use
fiber.TestConfigwith a specific timeout and error handling for timeouts is consistent with the previous changes. However, note that this test uses a 1-second timeout instead of the 2-second timeout used in other tests. Ensure this difference is intentional and aligns with the specific requirements of this test case.
750-753: LGTM: Consistent test configuration improvementThe update to use
fiber.TestConfigwith a specific timeout and error handling for timeouts is consistent with the majority of the previous changes. This maintains the improved test reliability and consistency across test cases.
Line range hint
1-824: Overall improvement in test reliability and consistencyThe changes made to this file consistently update the
app.Test()calls to usefiber.TestConfigwith explicit timeout handling. This improvement enhances the reliability of the tests and provides better error handling for timeout scenarios. The consistent application of these changes across multiple test cases is commendable and aligns with good testing practices.One test case (lines 654-657) uses a 1-second timeout instead of the 2-second timeout used in other tests. Ensure this difference is intentional and aligns with the specific requirements of that test case.
These changes contribute to a more robust and maintainable test suite for the proxy middleware.
helpers.go (2)
616-619: LGTM! Thread-safe connection state tracking added.The addition of
isClosedboolean andsync.Mutexto thetestConnstruct improves its functionality:
isClosedallows tracking the connection state.sync.Mutexensures thread-safety for concurrent operations on the connection.These changes enhance the reliability and safety of the
testConnstruct in multi-threaded scenarios.
622-627: LGTM! Thread-safe Read method implementation.The
Readmethod has been updated to use a mutex lock, which ensures thread-safety when reading from the buffer. Thedeferstatement guarantees that the lock is always released, even if an error occurs during the read operation.This change prevents potential race conditions in concurrent scenarios, improving the overall reliability of the
testConnstruct.app_test.go (2)
1127-1130: LGTM! TestConfig struct improves test configurability.The introduction of the
TestConfigstruct enhances the flexibility of theapp.Testmethod. SettingTimeout: -1andErrOnTimeout: falsemaintains the existing behavior while allowing for future extensibility.
Line range hint
1127-1493: Overall assessment: Well-implemented TestConfig functionality.The changes in this file effectively introduce and utilize the new
TestConfigstruct, aligning perfectly with the PR objectives. The modifications enhance the testing capabilities of the Fiber framework, particularly for configurable timeouts and error handling in streaming scenarios. The new test cases provide good coverage for different timeout situations, including empty responses.app.go (3)
868-881: AddTestConfigstruct to enhance test configuration.The introduction of the
TestConfigstruct provides flexibility in configuring theapp.Testmethod, allowing customization of timeout durations and error handling on timeouts. This enhances the testing capabilities and allows for more granular control during tests.
885-894: Updateapp.Testmethod to acceptTestConfig.The
app.Testmethod now accepts a variadicTestConfigparameter instead of a timeout duration. This change enhances functionality by allowing more configuration options. Ensure that existing code that callsapp.Testis updated accordingly, and consider documenting this change clearly for users.If backward compatibility is a concern, you might want to verify that existing calls to
app.Testwith timeout durations are handled properly.
933-941: Implement timeout handling usingTestConfig.The updated logic correctly utilizes the
TimeoutandErrOnTimeoutfields fromTestConfigto manage timeouts during testing. This provides flexible error handling and improves the robustness of the test method.ctx_test.go (1)
3145-3148: 🛠️ Refactor suggestionImprove test validation by checking
Content-EncodingInstead of comparing the
Content-Lengthto a hard-coded value, consider verifying that theContent-Encodingheader matches the expected compression algorithm. This ensures that the response is compressed using the expected algorithm and makes the test more robust.Apply this diff to improve the test:
- require.NotEqual(t, "58726", resp.Header.Get(HeaderContentLength)) + require.Equal(t, algo, resp.Header.Get(HeaderContentEncoding))Likely invalid or redundant comment.
middleware/logger/logger_test.go (2)
303-306: Correct implementation offiber.TestConfigin test.The test has been appropriately updated to use
fiber.TestConfig, enhancing control over timeout and error handling during testing, in line with the newTestConfigenhancements.
348-351: Consistent application offiber.TestConfigin tests.The use of
fiber.TestConfigin this test function ensures consistency and takes advantage of the new testing configuration options.
middleware/pprof/pprof_test.go
Outdated
| resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, target, nil), fiber.TestConfig{ | ||
| Timeout: 5 * time.Second, | ||
| ErrOnTimeout: true, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistent use of fiber.TestConfig across tests
The changes here mirror those in the Test_Pprof_Subs function, which is good for consistency. The use of fiber.TestConfig brings the same benefits of improved flexibility and error handling to this test function.
While the current implementation is correct, there's an opportunity to reduce code duplication:
Consider extracting the common test configuration into a shared variable or helper function. This would make the tests more maintainable and reduce the risk of inconsistencies if the configuration needs to be updated in the future. For example:
var defaultTestConfig = fiber.TestConfig{
Timeout: 5 * time.Second,
ErrOnTimeout: true,
}
// Then in your tests:
resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, target, nil), defaultTestConfig)This approach would make it easier to maintain consistent test configurations across multiple test functions.
middleware/static/static_test.go
Outdated
| resp, err := app.Test(req, fiber.TestConfig{ | ||
| Timeout: 10 * time.Second, | ||
| ErrOnTimeout: true, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated fiber.TestConfig instances to adhere to DRY principle
The fiber.TestConfig{Timeout: 10 * time.Second, ErrOnTimeout: true} configuration is repeated multiple times across your test functions. To enhance maintainability and reduce code duplication, consider defining this configuration once and reusing it.
You can define a variable at the beginning of your test file or within a common setup function:
var testConfig = fiber.TestConfig{
Timeout: 10 * time.Second,
ErrOnTimeout: true,
}Then, update your test calls to use the shared configuration:
- resp, err := app.Test(req, fiber.TestConfig{
- Timeout: 10 * time.Second,
- ErrOnTimeout: true,
- })
+ resp, err := app.Test(req, testConfig)This refactoring improves code readability and makes it easier to manage timeout settings across your tests.
Also applies to: 754-757, 778-781, 804-807, 848-851
There was a problem hiding this comment.
@grivera64 Can you make this change
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@gaby I've made this change for a few of the files in the middleware that use a single testConfig. A few other middleware used 2 or more different configs, so I didn't use a variable for those to avoid confusion between the configs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app.go (2)
885-894: LGTM! Improved flexibility withTestConfig.The updated
Testmethod now accepts a more versatileTestConfig, enhancing test customization. The default configuration is correctly set up, and user-provided config is properly applied if available.Consider adding a comment explaining that the first
TestConfigin the variadic parameter is used if provided, for added clarity:// Override config if provided if len(config) > 0 { + // Use the first TestConfig if multiple are provided cfg = config[0] }
933-941: LGTM! Improved timeout handling withTestConfig.The updated timeout handling logic correctly utilizes the
TestConfigfields, providing more flexibility and control over test behavior. The inclusion of the actual timeout duration in the error message is a nice touch for debugging.For consistency with the rest of the codebase, consider using
errors.Newinstead offmt.Errorffor the static error message:if cfg.ErrOnTimeout { - return nil, fmt.Errorf("test: timeout error after %s", cfg.Timeout) + return nil, errors.New("test: timeout error after " + cfg.Timeout.String()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app.go (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app.go (2)
868-880: LGTM! Well-structured and documentedTestConfigstruct.The new
TestConfigstruct is a great addition, providing users with more control over the test behavior. The clear documentation for each field, including default values, enhances usability.
959-961: LGTM! Consistent error handling.The update to use
errors.Newfor the static error message is a good change. It improves consistency with the rest of the codebase and addresses the previous review comment.
app.go
Outdated
| case <-time.After(cfg.Timeout): | ||
| conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here | ||
| if cfg.ErrOnTimeout { | ||
| return nil, fmt.Errorf("test: timeout error after %s", cfg.Timeout) |
There was a problem hiding this comment.
Is this something we could replace with os.ErrDeadlineExceeded() ?
There was a problem hiding this comment.
@gaby I believe we can, ErrDeadlineExceeded seems to be used by the net package according to the os package comment. To follow common convention, we can return this error.
Should we also wrap this error with any extra details about the timeout? Like for example:
fmt.Errorf("%w: failed after %s", os.ErrDeadlineExceeded, cfg.Timeout)
// Returns wrapped error: "i/o timeout: failed after 1s"There was a problem hiding this comment.
@gaby I've change it to use os.ErrDeadlineExceeded as the error type. Thanks for the recommendation!
- Rename TestConfig.ErrOnTimeout to TestConfig.FailOnTimeout
- Update documentation to use changed name
- Also fix stale documentation about passing Timeout as a
single argument
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
middleware/logger/logger_test.go (1)
348-351: Consider standardizing timeout duration across similar tests.While the implementation is correct, I notice that this test uses a 2-second timeout while the previous test uses 3 seconds. Consider standardizing the timeout duration across similar test cases for consistency.
- Timeout: 2 * time.Second, + Timeout: 3 * time.Second,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- app.go (4 hunks)
- app_test.go (4 hunks)
- ctx_test.go (1 hunks)
- docs/api/app.md (2 hunks)
- middleware/compress/compress_test.go (6 hunks)
- middleware/idempotency/idempotency_test.go (1 hunks)
- middleware/keyauth/keyauth_test.go (4 hunks)
- middleware/logger/logger_test.go (2 hunks)
- middleware/pprof/pprof_test.go (2 hunks)
- middleware/proxy/proxy_test.go (11 hunks)
- middleware/static/static_test.go (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- app_test.go
- ctx_test.go
- docs/api/app.md
- middleware/compress/compress_test.go
- middleware/idempotency/idempotency_test.go
- middleware/keyauth/keyauth_test.go
- middleware/pprof/pprof_test.go
- middleware/proxy/proxy_test.go
- middleware/static/static_test.go
🧰 Additional context used
🔇 Additional comments (4)
middleware/logger/logger_test.go (1)
303-306: LGTM! Clean transition to TestConfig.The update properly implements the new TestConfig struct with appropriate timeout and failure behavior settings.
app.go (3)
868-880: LGTM! Well-structured TestConfig implementation.The struct is well-designed with clear documentation and sensible defaults, perfectly aligning with the PR objectives for configurable testing.
883-894: LGTM! Clean implementation of TestConfig handling.The method signature change and default configuration setup are well-implemented, maintaining backward compatibility while adding new functionality.
959-961: 🛠️ Refactor suggestionEnhance empty response error handling.
As suggested in the past review, we should use
errors.Newfor static error messages and provide more context.Apply this improvement:
- return nil, errors.New("test: got empty response") + return nil, errors.New("test: received empty response from server")
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
middleware/logger/logger_test.go (1)
347-350: Consider standardizing timeout durations across similar tests.While the implementation is correct, the timeout duration (2 seconds) differs from the similar test above (3 seconds). Consider standardizing the timeout duration across similar test cases for better maintainability.
- Timeout: 2 * time.Second, + Timeout: 3 * time.Second,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app.go(4 hunks)app_test.go(4 hunks)ctx_test.go(1 hunks)middleware/logger/logger_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ctx_test.go
🔇 Additional comments (7)
middleware/logger/logger_test.go (1)
303-306: LGTM! TestConfig implementation looks good.
The change properly implements the new TestConfig struct with appropriate timeout settings for the latency test.
app.go (2)
905-917: LGTM! Well-structured and documented TestConfig.
The struct is well-designed with clear documentation and sensible defaults. It effectively addresses the requirements outlined in issue #3149 for configurable testing options.
996-998: LGTM! Clean error handling implementation.
The error handling for empty responses is well-implemented, using errors.New for static messages as recommended by the linter.
app_test.go (4)
1127-1130: LGTM! Clear test configuration using the new TestConfig struct.
The change appropriately uses the new TestConfig struct with an infinite timeout (-1) and FailOnTimeout set to false, maintaining the original test behavior.
1139-1142: LGTM! Appropriate test configuration for timeout verification.
The test correctly uses TestConfig to verify timeout behavior with a 20ms timeout and FailOnTimeout set to true, ensuring errors are properly handled.
1479-1493: LGTM! Well-structured test case for empty response on timeout.
The new test case improves coverage by verifying the behavior when a timeout occurs with FailOnTimeout set to false. The test structure and error message are clear and consistent with other timeout tests.
1441-1444: LGTM! Consistent usage of TestConfig across test cases.
The modifications successfully update existing tests to use the new TestConfig struct while maintaining their original test objectives and timeout behaviors.
Also applies to: 1472-1475
app.go
Outdated
| if cfg.Timeout >= 0 { | ||
| // With timeout | ||
| select { | ||
| case err = <-channel: | ||
| case <-time.After(to): | ||
| return nil, fmt.Errorf("test: timeout error after %s", to) | ||
| case <-time.After(cfg.Timeout): | ||
| conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here | ||
| if cfg.FailOnTimeout { | ||
| return nil, fmt.Errorf("test: timeout error after %s", cfg.Timeout) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance timeout error message and use os.ErrDeadlineExceeded.
The timeout error handling could be improved by:
- Using
os.ErrDeadlineExceededas the base error - Providing more context in the error message
Apply this diff to enhance the error handling:
- return nil, fmt.Errorf("test: timeout error after %s", cfg.Timeout)
+ return nil, fmt.Errorf("%w: request timed out waiting for response after %s", os.ErrDeadlineExceeded, cfg.Timeout)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if cfg.Timeout >= 0 { | |
| // With timeout | |
| select { | |
| case err = <-channel: | |
| case <-time.After(to): | |
| return nil, fmt.Errorf("test: timeout error after %s", to) | |
| case <-time.After(cfg.Timeout): | |
| conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here | |
| if cfg.FailOnTimeout { | |
| return nil, fmt.Errorf("test: timeout error after %s", cfg.Timeout) | |
| } | |
| if cfg.Timeout >= 0 { | |
| // With timeout | |
| select { | |
| case err = <-channel: | |
| case <-time.After(cfg.Timeout): | |
| conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here | |
| if cfg.FailOnTimeout { | |
| return nil, fmt.Errorf("%w: request timed out waiting for response after %s", os.ErrDeadlineExceeded, cfg.Timeout) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
middleware/compress/compress_test.go (1)
19-22: Consider adding documentation for the test configuration.The test configuration looks good, but adding a comment explaining the rationale behind the 10-second timeout and FailOnTimeout settings would help other contributors understand the test requirements.
+// testConfig defines standard test timeout settings for compression tests. +// A 10-second timeout is used to ensure sufficient time for compression operations, +// and FailOnTimeout is enabled to catch potential hanging or deadlock scenarios. var testConfig = fiber.TestConfig{ Timeout: 10 * time.Second, FailOnTimeout: true, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
app.go(4 hunks)app_test.go(5 hunks)middleware/compress/compress_test.go(7 hunks)middleware/keyauth/keyauth_test.go(5 hunks)middleware/pprof/pprof_test.go(3 hunks)middleware/proxy/proxy_test.go(12 hunks)middleware/static/static_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app_test.go
- middleware/keyauth/keyauth_test.go
- middleware/pprof/pprof_test.go
- middleware/proxy/proxy_test.go
- middleware/static/static_test.go
🔇 Additional comments (2)
middleware/compress/compress_test.go (2)
47-47: LGTM! Consistent application of test configuration.
The test configuration has been consistently applied across all relevant test methods, improving the standardization of timeout handling.
Also applies to: 80-80, 107-107, 131-131, 155-155, 179-179
47-47: Verify timeout appropriateness for compression operations.
While the 10-second timeout is applied consistently, let's verify it's appropriate for all compression algorithms and levels.
Also applies to: 80-80, 107-107, 131-131, 155-155, 179-179
| if cfg.Timeout >= 0 { | ||
| // With timeout | ||
| select { | ||
| case err = <-channel: | ||
| case <-time.After(to): | ||
| return nil, fmt.Errorf("test: timeout error after %s", to) | ||
| case <-time.After(cfg.Timeout): | ||
| conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here | ||
| if cfg.FailOnTimeout { | ||
| return nil, os.ErrDeadlineExceeded | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use zero value for Timeout to disable timeout for clarity
Using -1 to disable the timeout may be unintuitive for users. In Go, it's conventional to use a zero time.Duration to represent the absence of a timeout. Adjusting this will make the API more user-friendly and align with Go standards.
Update the comments and logic as follows:
-// Config is optional and defaults to a 1s error on timeout,
-// -1 timeout will disable it completely.
+// Config is optional and defaults to a 1-second timeout with error.
+// A Timeout of zero disables the timeout.
...
-if cfg.Timeout >= 0 {
+if cfg.Timeout > 0 {
// With timeout
select {
case err = <-channel:
case <-time.After(cfg.Timeout):
conn.Close() //nolint:errcheck, revive // It is fine to ignore the error here
if cfg.FailOnTimeout {
return nil, os.ErrDeadlineExceeded
}
}
} else {
// Without timeout
err = <-channel
}This change enhances readability and aligns with common Go practices regarding timeouts.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@ReneWerner87 I believe that using 0 for no timeout would be fine here. I don't believe that users will intentionally want to instant-timeout a test as possible with the current configuration, and making this change will reduce the number of people that make the mistake. As 0 is the default, you could even not include the Timeout field to imply no timeout in the test.
I can make this change and add it to the PR.
|
I have added commits to address previous comments. I think the only failing check is due to an unrelated method, so this should be good from the checks perspective. Please let me know if you would like to make any more changes to the PR. |
|
@grivera64 The benchmarks are failing related to the github-action we are using. I have reported the issue but there's no fix yet. |
|
@grivera64 Don't forget to update the "What's New" section, since this changes how using https://github.com/gofiber/fiber/blob/main/docs/whats_new.md |
Interesting, would this affect the ability to work on Pull Requests? |
Thanks for the reminder @gaby , I've added an entry for |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
docs/whats_new.md (2)
189-190: Enhance the introduction of TestConfig.The current introduction could be more informative. Consider adding details about the key benefits and use cases of the new
TestConfig.-The `app.Test()` method now allows users to customize their test configurations: +The `app.Test()` method now accepts a `TestConfig` struct that allows users to customize test behavior, including: +- Timeout duration for test execution +- Control over response handling when timeouts occur +- Enhanced testing capabilities for streaming responses
214-220: Add more context about the signature change.The documentation should explain why the signature was changed and the benefits it brings.
-To provide configurable testing capabilities, we had to change -the signature of the `Test` method. +The signature of the `Test` method has been updated to provide more control over test behavior: + +- The new `TestConfig` struct replaces the simple timeout parameter +- This change enables advanced features like: + - Configurable timeout handling for streaming responses + - Control over response preservation on timeout + - Future extensibility for additional test configurations + ```diff - Test(req *http.Request, timeout ...time.Duration) (*http.Response, error) + Test(req *http.Request, config ...fiber.TestConfig) (*http.Response, error)</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 438b6302de020557ae165d42ba006f7e412bfa70 and 0326c07a1b17b1134987b37a906efa36e9316896. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `docs/whats_new.md` (2 hunks) </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| ```go | ||
| // Create a test app with a handler to test | ||
| app := fiber.New() | ||
| app.Get("/", func(c fiber.Ctx) { | ||
| return c.SendString("hello world") | ||
| }) | ||
|
|
||
| // Define the HTTP request and custom TestConfig to test the handler | ||
| req := httptest.NewRequest(MethodGet, "/", nil) | ||
| testConfig := fiber.TestConfig{ | ||
| Timeout: -1, | ||
| FailOnTimeout: false, | ||
| } | ||
|
|
||
| // Test the handler using the request and testConfig | ||
| resp, err := app.Test(req, testConfig) | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the example to showcase key features.
The current example could better demonstrate the practical benefits of TestConfig, particularly for streaming responses and timeout handling.
// Create a test app with a handler to test
app := fiber.New()
-app.Get("/", func(c fiber.Ctx) {
- return c.SendString("hello world")
+app.Get("/stream", func(c fiber.Ctx) error {
+ c.Set("Content-Type", "text/event-stream")
+ c.Set("Transfer-Encoding", "chunked")
+
+ // Simulate a streaming response
+ return c.SendStreamWriter(func(w *bufio.Writer) {
+ for i := 0; i < 5; i++ {
+ fmt.Fprintf(w, "data: Message %d\n\n", i)
+ w.Flush()
+ time.Sleep(100 * time.Millisecond)
+ }
+ })
})
// Define the HTTP request and custom TestConfig to test the handler
-req := httptest.NewRequest(MethodGet, "/", nil)
+req := httptest.NewRequest(MethodGet, "/stream", nil)
testConfig := fiber.TestConfig{
- Timeout: -1,
- FailOnTimeout: false,
+ // Allow 1 second for the stream to complete
+ Timeout: 1 * time.Second,
+ // Preserve the partial response even if timeout occurs
+ FailOnTimeout: false,
}
// Test the handler using the request and testConfig
resp, err := app.Test(req, testConfig)
+if err != nil {
+ log.Fatal(err)
+}
+
+// Read and verify the streamed response
+body, _ := io.ReadAll(resp.Body)
+fmt.Println(string(body))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```go | |
| // Create a test app with a handler to test | |
| app := fiber.New() | |
| app.Get("/", func(c fiber.Ctx) { | |
| return c.SendString("hello world") | |
| }) | |
| // Define the HTTP request and custom TestConfig to test the handler | |
| req := httptest.NewRequest(MethodGet, "/", nil) | |
| testConfig := fiber.TestConfig{ | |
| Timeout: -1, | |
| FailOnTimeout: false, | |
| } | |
| // Test the handler using the request and testConfig | |
| resp, err := app.Test(req, testConfig) | |
| ``` | |
| ```go | |
| // Create a test app with a handler to test | |
| app := fiber.New() | |
| app.Get("/stream", func(c fiber.Ctx) error { | |
| c.Set("Content-Type", "text/event-stream") | |
| c.Set("Transfer-Encoding", "chunked") | |
| // Simulate a streaming response | |
| return c.SendStreamWriter(func(w *bufio.Writer) { | |
| for i := 0; i < 5; i++ { | |
| fmt.Fprintf(w, "data: Message %d\n\n", i) | |
| w.Flush() | |
| time.Sleep(100 * time.Millisecond) | |
| } | |
| }) | |
| }) | |
| // Define the HTTP request and custom TestConfig to test the handler | |
| req := httptest.NewRequest(MethodGet, "/stream", nil) | |
| testConfig := fiber.TestConfig{ | |
| // Allow 1 second for the stream to complete | |
| Timeout: 1 * time.Second, | |
| // Preserve the partial response even if timeout occurs | |
| FailOnTimeout: false, | |
| } | |
| // Test the handler using the request and testConfig | |
| resp, err := app.Test(req, testConfig) | |
| if err != nil { | |
| log.Fatal(err) | |
| } | |
| // Read and verify the streamed response | |
| body, _ := io.ReadAll(resp.Body) | |
| fmt.Println(string(body)) | |
| ``` |
There was a problem hiding this comment.
This does sound like a much better example to show off, but I think it would be best to add this example use case in this separate PR:
🔥 Feature (v3): Add buffered streaming support #3131
Before doing so, I think we would need to merge these changes first so we can add unit tests using the new fiber.TestConfig{} feature from this PR.
Please let me know your thoughts!
There was a problem hiding this comment.
@grivera64, that sounds like a great plan. Adding the example in PR #3131 after merging this PR makes sense. Let me know if you need any assistance!
(^.^)
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), -1) | ||
| resp, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), TestConfig{ | ||
| Timeout: -1, | ||
| FailOnTimeout: false, |
There was a problem hiding this comment.
@grivera64 if failOnTimeout is false, do we really need a -1 for the timeout? couldn't we do this implicitly?
There was a problem hiding this comment.
@ReneWerner87 FailOnTimeout does not directly relate to the Timeout field, but the naming may mislead users to believe so.
When true, FailOnTimeout tells the test to return an error after timing out. If FailOnTimeout is false, the test will return the most up-to-date response before timing out without returning an error. If there is no timeout (Timeout: -1), then the value of FailOnTimeout doesn't affect the test. For clarity, I kept FailOnTimeout as false since there is no timeout that can lead to a failing test in the first place.
Maybe a less misleading name for the field would be something like IgnoreResponseOnTimeout or something along those lines.
What are your thoughts about this?
Description
This pull request modifies
app.Test()in v3 to use a TestConfig{} struct to configure internal testing of app routes. This is due to the introduction ofc.SendStreamWriter()in PR #3131, we need support for reading the response of a route after a certain timeout to mimic a client disconnection. It is important to keep this in account when streaming a response to the client, since you may need to do cleanup or rollback after a closed connection.Fixes #3149
Changes introduced
app.Test()in /docs/api/app.mdTestConfigstruct toapp.Test()for configurable testingType of change
testConnTestConfig{}struct to perform the same functionality as beforetestConnapp.Test()methodChecklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.