🔥 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.70% 82.85% +0.15%
==========================================
Files 114 114
Lines 11163 11189 +26
==========================================
+ Hits 9232 9271 +39
+ Misses 1529 1520 -9
+ Partials 402 398 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![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")
|
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 |
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 -->
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
ctx_test.go (1)
3246-3249: Good addition of test timeout configuration.The explicit timeout configuration with
FailOnTimeouthelps prevent test hangs and ensures proper test failure behavior. Consider also adding test cases to verify timeout behavior:// Test timeout scenario resp, err := app.Test(req, TestConfig{ Timeout: 1 * time.Millisecond, FailOnTimeout: true, }) require.Error(t, err) require.True(t, errors.Is(err, ErrTimeout))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
ctx_test.go(1 hunks)docs/api/app.md(2 hunks)docs/whats_new.md(2 hunks)middleware/logger/logger_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api/app.md
- middleware/logger/logger_test.go
🔇 Additional comments (1)
docs/whats_new.md (1)
194-210: Enhance the example to better demonstrate TestConfig's capabilities.
A past review comment already suggested improving this example to better showcase the streaming and timeout handling capabilities. The suggestion remains valid and aligns with the PR objectives of demonstrating how TestConfig helps with testing streaming responses and timeout scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
docs/whats_new.md (2)
78-79: Clarify the timeout value change.The documentation about timeout values needs more clarity. Consider adding an explanation of why the timeout representation changed from -1 to 0.
- Test -> Replaced timeout with a config parameter - - -1 represents no timeout -> 0 represents no timeout + - Changed timeout representation: Previously -1 represented no timeout, now 0 represents no timeout + - This change aligns with Go's common practice of using 0 to represent "no timeout" in time.Duration
188-214: Enhance the TestConfig example for better clarity.The current example doesn't effectively demonstrate the key features of TestConfig, particularly the timeout and error handling capabilities.
```go // 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("/", func(c fiber.Ctx) error { + // Simulate a slow response + time.Sleep(100 * time.Millisecond) + 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: 0, - FailOnTimeout: false, + // Set timeout to 50ms to simulate a timeout scenario + Timeout: 50 * time.Millisecond, + // When false, returns 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 { + // Handle timeout error + log.Printf("Test error: %v", err) +}</blockquote></details> <details> <summary>app_test.go (2)</summary><blockquote> `1128-1130`: **Consider using a more explicit value for "no timeout".** Using `Timeout: 0` is not immediately clear whether it means "no timeout" or "immediate timeout". Consider using `-1` or a named constant to make the intention more explicit. ```diff - Timeout: 0, + Timeout: -1, // No timeout
1139-1142: Consider increasing the timeout duration to prevent flaky tests.Using a very short timeout of 20ms could lead to flaky tests on slower systems or under high load. Consider increasing this to a more reasonable duration (e.g., 100ms) while still keeping the test fast.
- Timeout: 20 * time.Millisecond, + Timeout: 100 * time.Millisecond,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app.go(4 hunks)app_test.go(5 hunks)docs/api/app.md(2 hunks)docs/whats_new.md(2 hunks)middleware/keyauth/keyauth_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api/app.md
- middleware/keyauth/keyauth_test.go
🧰 Additional context used
📓 Learnings (1)
app.go (1)
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
🔇 Additional comments (7)
docs/whats_new.md (1)
215-221: 🛠️ Refactor suggestion
Add context about TestConfig struct fields.
The signature change section should include details about the available TestConfig fields and their purposes.
Note: This comment was previously made in a past review and is still valid. The documentation should explain the TestConfig struct fields to help users understand the available configuration options.
app.go (4)
906-918: LGTM! Well-structured and documented TestConfig.
The struct is well-designed with clear documentation and appropriate field names. The FailOnTimeout field name is a good choice, making it clear that it controls the error behavior on timeout.
923-932: LGTM! Clean implementation of the new Test method signature.
The method properly handles the new TestConfig parameter with sensible defaults. Breaking change is acceptable as this is for version 3 of the framework.
971-979: LGTM! Proper timeout handling with os.ErrDeadlineExceeded.
The timeout implementation is clean and follows best practices:
- Uses select for timeout handling
- Returns os.ErrDeadlineExceeded as recommended
- Properly closes the connection on timeout
997-999: 🛠️ Refactor suggestion
Simplify error creation using errors.New.
For static error messages without formatting, it's more efficient to use errors.New.
Apply this diff:
- return nil, errors.New("test: got empty response")
+ return nil, errors.New("test: got empty response")Likely invalid or redundant comment.
app_test.go (2)
1471-1476: LGTM! Good error handling test.
The test properly verifies that the correct error type (os.ErrDeadlineExceeded) is returned when a timeout occurs. The timeout duration is reasonable, and the test provides good coverage of the error handling path.
1478-1491: LGTM! Good test coverage for empty response scenario.
This test case nicely complements the timeout tests by covering the specific case where FailOnTimeout is false and an empty response is received. The error message is clear and descriptive, making debugging easier.
|
Thanks for the feedback @ReneWerner87 ! I've updated the code and documentation per the requests. Please let me know if we should make any more changes or additions to the PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/whats_new.md (2)
223-229: Add blank lines around lists for better readability.The list of configuration options should be surrounded by blank lines for better formatting and readability.
The `TestConfig` struct provides the following configuration options: + - `Timeout`: - The duration to wait before timing out the test. Use 0 for no timeout. - `FailOnTimeout`: - When true, the test will return an `os.ErrDeadlineExceeded` if the test exceeds the `Timeout` duration. - When false, the test will return the partial response received before timing out. +🧰 Tools
🪛 LanguageTool
[uncategorized] ~226-~226: Loose punctuation mark.
Context: ... Use 0 for no timeout. -FailOnTimeout: - When true, the test will return an ...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint
224-224: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
🪛 GitHub Check: markdownlint
[failure] 224-224: Lists should be surrounded by blank lines
docs/whats_new.md:224 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "-Timeout:"] https://github.com/DavidAnson/markdownlint/blob/v0.36.1/doc/md032.md
188-249: Documentation looks good with clear examples and explanations!The Test Config section effectively documents the new feature with:
- Clear explanation of the API changes
- Well-structured example code
- Detailed configuration options
- Default values and important notes
Consider adding cross-references to related sections or examples where this feature is particularly useful, such as streaming response testing or connection timeout scenarios.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~226-~226: Loose punctuation mark.
Context: ... Use 0 for no timeout. -FailOnTimeout: - When true, the test will return an ...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint
224-224: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
🪛 GitHub Check: markdownlint
[failure] 224-224: Lists should be surrounded by blank lines
docs/whats_new.md:224 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "-Timeout:"] https://github.com/DavidAnson/markdownlint/blob/v0.36.1/doc/md032.md
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/whats_new.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/whats_new.md
[uncategorized] ~226-~226: Loose punctuation mark.
Context: ... Use 0 for no timeout. - FailOnTimeout: - When true, the test will return an ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Markdownlint
docs/whats_new.md
224-224: null
Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
🪛 GitHub Check: markdownlint
docs/whats_new.md
[failure] 224-224: Lists should be surrounded by blank lines
docs/whats_new.md:224 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- Timeout:"] https://github.com/DavidAnson/markdownlint/blob/v0.36.1/doc/md032.md
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/whats_new.md (1)
225-230: Fix formatting inconsistencies in configuration options.The bullet points for configuration options have inconsistent formatting with extra spaces. Consider standardizing the format:
- - `Timeout`: - - The duration to wait before timing out the test. Use 0 for no timeout. - - `FailOnTimeout`: - - When true, the test will return an `os.ErrDeadlineExceeded` if the test exceeds the `Timeout` duration. - - When false, the test will return the partial response received before timing out. +- `Timeout`: The duration to wait before timing out the test. Use 0 for no timeout. +- `FailOnTimeout`: Controls the behavior when a timeout occurs: + - When true, the test will return an `os.ErrDeadlineExceeded` if the test exceeds the `Timeout` duration. + - When false, the test will return the partial response received before timing out.🧰 Tools
🪛 LanguageTool
[uncategorized] ~225-~225: Loose punctuation mark.
Context: ...wing configuration options: -Timeout: - The duration to wait before timing ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~227-~227: Loose punctuation mark.
Context: ... Use 0 for no timeout. -FailOnTimeout: - When true, the test will return an ...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/whats_new.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/whats_new.md
[uncategorized] ~225-~225: Loose punctuation mark.
Context: ...wing configuration options: - Timeout: - The duration to wait before timing ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~227-~227: Loose punctuation mark.
Context: ... Use 0 for no timeout. - FailOnTimeout: - When true, the test will return an ...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (1)
docs/whats_new.md (1)
195-211: Enhance example to demonstrate practical TestConfig usage.
The current example doesn't effectively showcase the key benefits of TestConfig, particularly for streaming responses and timeout handling.
Note: A previous review comment already suggested an improved example that better demonstrates TestConfig's features, particularly for streaming responses. That suggestion remains valid and applicable.
…nfig options description
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
docs/whats_new.md (3)
195-211: Enhance the example to demonstrate practical use cases.The current example is too basic and doesn't showcase the key benefits of the new TestConfig. Consider adding examples that demonstrate:
- Testing streaming responses with timeouts
- Handling partial responses
- Testing error scenarios
// 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") + + // Simulate a slow streaming response + return c.SendStreamWriter(func(w *bufio.Writer) { + for i := 0; i < 3; i++ { + fmt.Fprintf(w, "data: Message %d\n\n", i) + w.Flush() + time.Sleep(500 * 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: 0, - FailOnTimeout: false, + // Set timeout to 1 second to capture partial response + Timeout: time.Second, + // Don't fail on timeout to examine partial response + FailOnTimeout: false, } // Test the handler using the request and testConfig resp, err := app.Test(req, testConfig) +if err != nil { + log.Fatal(err) +} + +// Read the partial response received before timeout +body, _ := io.ReadAll(resp.Body) +fmt.Println(string(body))
223-229: Improve formatting of configuration options.The configuration options section needs better formatting for clarity. Consider using a table format or better bullet points.
The `TestConfig` struct provides the following configuration options: - - `Timeout`: The duration to wait before timing out the test. Use 0 for no timeout. - - `FailOnTimeout`: Controls the behavior when a timeout occurs: - - When true, the test will return an `os.ErrDeadlineExceeded` if the test exceeds the `Timeout` duration. - - When false, the test will return the partial response received before timing out. +| Option | Type | Description | +|--------|------|-------------| +| `Timeout` | `time.Duration` | The duration to wait before timing out the test. Use 0 for no timeout. | +| `FailOnTimeout` | `bool` | Controls the behavior when a timeout occurs:<br>• `true`: Returns `os.ErrDeadlineExceeded` if test exceeds `Timeout`<br>• `false`: Returns partial response received before timeout |🧰 Tools
🪛 LanguageTool
[uncategorized] ~225-~225: Loose punctuation mark.
Context: ...wing configuration options: -Timeout: The duration to wait before timing out ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~226-~226: Loose punctuation mark.
Context: ... Use 0 for no timeout. -FailOnTimeout: Controls the behavior when a timeout oc...(UNLIKELY_OPENING_PUNCTUATION)
230-249: Clarify default configuration behavior.The section about default configuration needs to better explain the behavioral differences between using default config vs empty config.
If a custom `TestConfig` isn't provided, then the following will be used: ```go testConfig := fiber.TestConfig{ Timeout: time.Second, FailOnTimeout: true, }-Note: Using this default is NOT the same as providing an empty
TestConfigas an argument toapp.Test().
+Important Behavior Differences:
+1. Default Config (when no config is provided):
- Times out after 1 second
- Returns error on timeout
- Suitable for most test cases
-An empty
TestConfigis the equivalent of:
+2. Empty Config (whenfiber.TestConfig{}is provided):
- Never times out (equivalent to infinite timeout)
- Never fails on timeout
- Useful for long-running tests or streaming responses
-```go
-testConfig := fiber.TestConfig{
- Timeout: 0,
- FailOnTimeout: false,
-}
-```
+Choose the appropriate configuration based on your testing needs.</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 be650d6276ec4d659176098eae0eb6d3ab992594 and 250b836f094e8753a021f837f6d52ec51846a425. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `docs/whats_new.md` (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/whats_new.md</summary> [uncategorized] ~225-~225: Loose punctuation mark. Context: ...wing configuration options: - `Timeout`: The duration to wait before timing out ... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~226-~226: Loose punctuation mark. Context: ... Use 0 for no timeout. - `FailOnTimeout`: Controls the behavior when a timeout oc... (UNLIKELY_OPENING_PUNCTUATION) </details> </details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
Thanks for the approval @efectn ! For some reason, the change request from a few days ago hasn't been cleared after pushing changes and resolving the comment. @ReneWerner87 Please let me know if I missed something from the change request that could be causing this issue. Thanks again! |
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.