Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🔥 Feature: Add TestConfig to app.Test() for configurable testing #3161

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

grivera64
Copy link
Contributor

@grivera64 grivera64 commented Oct 9, 2024

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 of c.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

  • Documentation Update: Update old documentation of app.Test() in /docs/api/app.md
  • Changelog/What's New: Add TestConfig struct to app.Test() for configurable testing
  • Migration Guide: If necessary, provide a guide or steps for users to migrate their existing code to accommodate these changes.
    • May be a good idea to include this change in the migration guide.
  • Examples:
app := fiber.New()
app.Get("/", func(c Ctx) error {
  return c.Status(fiber.StatusOK).SendString("Hello World!")
}
resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil), TestConfig{
  Timeout: -1, // No timeout
  ErrOnTimeout: false,
})

Type of change

  • New feature (non-breaking change which adds functionality)
    • Adds the preservation of a timed-out response
    • Adds thread-safety to testConn
  • Enhancement (improvement to existing features and functionality)
    • Add a TestConfig{} struct to perform the same functionality as before
    • Add unit tests for testConn
  • Documentation update (changes to documentation)
    • Adds details about using the new version of the app.Test() method

Checklist

Before you submit your pull request, please make sure you meet these requirements:

  • Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
  • Conducted a self-review of the code and provided comments for complex or critical parts.
  • Updated the documentation in the /docs/ directory for Fiber's documentation.
  • Added or updated unit tests to validate the effectiveness of the changes or new features.
  • Ensured that new and existing unit tests pass locally with the changes.
  • Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
  • Aimed for optimal performance with minimal allocations in the new code.
  • Provided benchmarks for the new code to analyze and improve upon.

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 grivera64 requested a review from a team as a code owner October 9, 2024 19:03
@grivera64 grivera64 requested review from gaby, sixcolors, ReneWerner87 and efectn and removed request for a team October 9, 2024 19:03
Copy link
Contributor

coderabbitai bot commented Oct 9, 2024

Walkthrough

The changes introduce a new TestConfig struct to the fiber package, enhancing the app.Test method by allowing it to accept configuration settings for testing, including timeout duration and error handling for timeouts. This modification is reflected across various test files, where existing timeout parameters are replaced with instances of TestConfig. Additionally, several new tests and benchmarks have been added to improve coverage and performance measurement, particularly for context methods and connection handling.

Changes

File Change Summary
app.go Introduced TestConfig struct; modified Test method to accept TestConfig instead of timeout.
app_test.go Updated Test_App_Test to use TestConfig; added Test_App_Test_timeout_empty_response test case.
ctx_test.go Added benchmark tests and new test cases for context methods; enhanced error handling tests.
docs/api/app.md Updated Test method documentation to reflect changes to TestConfig.
helpers.go Modified testConn struct for thread safety; added locking mechanisms to Read, Write, and Close.
helpers_test.go Added tests for testConn operations; included benchmarks for new test functions.
middleware/compress/compress_test.go Updated tests to use TestConfig for timeout handling.
middleware/idempotency/idempotency_test.go Updated doReq function to use TestConfig in Test_Idempotency.
middleware/keyauth/keyauth_test.go Updated tests to utilize TestConfig for timeout settings.
middleware/logger/logger_test.go Updated logger tests to use TestConfig for request testing.
middleware/pprof/pprof_test.go Updated tests to utilize TestConfig for timeout settings.
middleware/proxy/proxy_test.go Updated tests to use TestConfig for request testing.
middleware/static/static_test.go Updated static file tests to use TestConfig for timeout and error handling.

Assessment against linked issues

Objective Addressed Explanation
Add TestConfig struct as an optional parameter to app.Test() for testing.
Allow app.Test() to specify error handling behavior for timeouts.
Ensure that existing functionality of app.Test() remains unchanged.

Poem

🐰 In the meadow where the code does play,
A new TestConfig hops in today!
With timeouts set and errors in sight,
Testing is clearer, everything feels right.
So let’s cheer for the changes, oh what a delight! 🎉


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.15%. Comparing base (48e82e7) to head (78e32a7).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3161      +/-   ##
==========================================
- Coverage   82.38%   82.15%   -0.24%     
==========================================
  Files         113      113              
  Lines        8466    11038    +2572     
==========================================
+ Hits         6975     9068    +2093     
- Misses       1088     1566     +478     
- Partials      403      404       +1     
Flag Coverage Δ
unittests 82.15% <100.00%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (13)
middleware/idempotency/idempotency_test.go (1)

85-88: Excellent update to use fiber.TestConfig!

This change aligns perfectly with the PR objectives by introducing the TestConfig struct for configurable testing. The update maintains the existing 15-second timeout while adding the ErrOnTimeout: true setting. 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 ErrOnTimeout setting 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 TestConfig

The changes consistently apply fiber.TestConfig across all test functions, which is excellent. To further improve code maintainability and reduce duplication, consider creating a helper function that returns the common TestConfig. 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 the TestConfig parameter.

The updated method signature correctly reflects the introduction of the TestConfig struct, 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 TestConfig parameter immediately after the signature to help users understand its purpose and usage.


569-593: Improve structure and clarity of TestConfig information.

The added information about TestConfig is 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:

  1. Move the default TestConfig information (lines 569-576) closer to the method signature for immediate context.
  2. 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.
:::
  1. Consider adding a brief example of how to use TestConfig with custom values to illustrate its practical application.

Line range hint 543-594: Enhance integration of new TestConfig information with existing documentation.

While the changes to the Test method 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:

  1. Update the introductory paragraph (lines 543-545) to mention the new TestConfig option alongside the existing timeout information.

  2. 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)
  1. Add a brief explanation of when and why a user might want to use custom TestConfig settings, to provide context for this new feature.

These changes will help users understand the new TestConfig feature in the context of the existing Test method 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_ReadWrite function effectively tests both read and write operations on the testConn structure. 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_Write function 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.

  1. Consider handling the error from conn.Close() instead of ignoring it with a linter directive. This would make the test more robust.
  2. 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_ReadWrite and Test_Utils_TestConn_Closed_Write significantly improves the test coverage for the testConn structure. 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:

  1. Adding error case scenarios for both read and write operations.
  2. Ensuring all errors are properly handled and checked, including those from Close() operations.
  3. 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 testConn structure, ensuring its reliability across various scenarios.

helpers.go (3)

629-637: LGTM! Thread-safe Write method with closed state check.

The Write method has been improved with two key changes:

  1. It now uses a mutex lock to ensure thread-safety when writing to the buffer.
  2. It checks the isClosed state before writing, preventing writes to a closed connection.

These changes enhance the reliability and correctness of the Write operation.

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 Close method has been updated with important improvements:

  1. It now uses a mutex lock to ensure thread-safety when closing the connection.
  2. It sets the isClosed state to true, preventing further operations on the closed connection.

These changes enhance the reliability and correctness of the Close operation.

Consider adding a check to prevent multiple calls to Close from changing the state:

func (c *testConn) Close() error {
    c.Lock()
    defer c.Unlock()

    if !c.isClosed {
        c.isClosed = true
    }
    return nil
}

This ensures that isClosed is 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 testConn struct and its methods (Read, Write, and Close) significantly enhance the reliability and correctness of the test connection implementation:

  1. Thread-safety: The addition of sync.Mutex and its usage in all methods prevents race conditions in concurrent scenarios.
  2. Connection state tracking: The isClosed flag helps manage the connection state, preventing operations on closed connections.
  3. Consistent locking pattern: All methods now use the same locking pattern, ensuring uniform behavior across the struct.

These improvements make the testConn more 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 the testConn struct. This would allow reusing the same testConn instance 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 TestConfig here effectively tests the new timeout functionality. Setting ErrOnTimeout: true allows 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 TestConfig struct 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

📥 Commits

Files that changed from the base of the PR and between c86c3c0 and 6112c04.

📒 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 with fiber.TestConfig

The changes here align well with the PR objectives. By introducing fiber.TestConfig, you've enhanced the test's flexibility and error handling capabilities. The ErrOnTimeout flag 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 configurability

The change from a simple timeout to fiber.TestConfig aligns with the PR objectives. The new ErrOnTimeout: true setting allows for more precise control over timeout behavior, which is particularly useful for testing streaming responses.


78-81: LGTM: Consistent TestConfig usage

The change to use fiber.TestConfig is 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 implementation

The change to use fiber.TestConfig maintains 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 consistently

The implementation of fiber.TestConfig for 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 test

The use of fiber.TestConfig in 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 scenario

The consistent use of fiber.TestConfig extends 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 TestConfig feature for the Test method, aligning well with the PR objectives. The updated method signature and the addition of TestConfig information enhance the testing capabilities of the Fiber framework.

However, there are opportunities to improve the documentation:

  1. Restructure the presentation of TestConfig information for better clarity.
  2. Integrate the new feature more seamlessly with the existing content.
  3. Provide more examples and context for using custom TestConfig settings.

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 TestConfig

The change to use fiber.TestConfig in the app.Test method call is consistent with the PR objectives. The configuration maintains the previous behavior:

  • Timeout: -1 indicates no timeout, which is equivalent to the previous implementation.
  • ErrOnTimeout: false ensures 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 call

This change is consistent with the previous update to the app.Test method call. It introduces fiber.TestConfig with 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 TestConfig

This change maintains consistency with the previous updates to the app.Test method calls. The fiber.TestConfig is applied with the same parameters:

  • Timeout: -1
  • ErrOnTimeout: false

The 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 implementation

This change continues the pattern of updating app.Test method calls with fiber.TestConfig. The configuration remains consistent:

  • Timeout: -1
  • ErrOnTimeout: false

The 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 TestConfig

The changes in this file consistently update all app.Test method calls to use the new fiber.TestConfig structure. Key points:

  1. All updates use the same configuration (Timeout: -1, ErrOnTimeout: false), maintaining the previous behavior.
  2. The changes are applied systematically throughout the file, demonstrating thoroughness.
  3. 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 configuration

The update to use fiber.TestConfig with 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 improvement

The update to use fiber.TestConfig with 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 improvement

The update to use fiber.TestConfig with 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 improvement

The update to use fiber.TestConfig with 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 improvement

The update to use fiber.TestConfig with 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 improvement

The update to use fiber.TestConfig with 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 improvement

The update to use fiber.TestConfig with 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 improvement

The update to use fiber.TestConfig with 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 improvement

The update to use fiber.TestConfig with 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 note

The update to use fiber.TestConfig with 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 improvement

The update to use fiber.TestConfig with 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 consistency

The changes made to this file consistently update the app.Test() calls to use fiber.TestConfig with 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 isClosed boolean and sync.Mutex to the testConn struct improves its functionality:

  1. isClosed allows tracking the connection state.
  2. sync.Mutex ensures thread-safety for concurrent operations on the connection.

These changes enhance the reliability and safety of the testConn struct in multi-threaded scenarios.


622-627: LGTM! Thread-safe Read method implementation.

The Read method has been updated to use a mutex lock, which ensures thread-safety when reading from the buffer. The defer statement 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 testConn struct.

app_test.go (2)

1127-1130: LGTM! TestConfig struct improves test configurability.

The introduction of the TestConfig struct enhances the flexibility of the app.Test method. Setting Timeout: -1 and ErrOnTimeout: false maintains 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 TestConfig struct, 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: Add TestConfig struct to enhance test configuration.

The introduction of the TestConfig struct provides flexibility in configuring the app.Test method, 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: Update app.Test method to accept TestConfig.

The app.Test method now accepts a variadic TestConfig parameter instead of a timeout duration. This change enhances functionality by allowing more configuration options. Ensure that existing code that calls app.Test is 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.Test with timeout durations are handled properly.


933-941: Implement timeout handling using TestConfig.

The updated logic correctly utilizes the Timeout and ErrOnTimeout fields from TestConfig to manage timeouts during testing. This provides flexible error handling and improves the robustness of the test method.

ctx_test.go (1)

3145-3148: 🛠️ Refactor suggestion

Improve test validation by checking Content-Encoding

Instead of comparing the Content-Length to a hard-coded value, consider verifying that the Content-Encoding header 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 of fiber.TestConfig in test.

The test has been appropriately updated to use fiber.TestConfig, enhancing control over timeout and error handling during testing, in line with the new TestConfig enhancements.


348-351: Consistent application of fiber.TestConfig in tests.

The use of fiber.TestConfig in this test function ensures consistency and takes advantage of the new testing configuration options.

Comment on lines 138 to 141
resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, target, nil), fiber.TestConfig{
Timeout: 5 * time.Second,
ErrOnTimeout: true,
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Comment on lines 741 to 744
resp, err := app.Test(req, fiber.TestConfig{
Timeout: 10 * time.Second,
ErrOnTimeout: true,
})
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grivera64 Can you make this change

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

app.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
app.go (2)

885-894: LGTM! Improved flexibility with TestConfig.

The updated Test method now accepts a more versatile TestConfig, 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 TestConfig in 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 with TestConfig.

The updated timeout handling logic correctly utilizes the TestConfig fields, 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.New instead of fmt.Errorf for 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

📥 Commits

Files that changed from the base of the PR and between 6112c04 and 4a97d7b.

📒 Files selected for processing (1)
  • app.go (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app.go (2)

868-880: LGTM! Well-structured and documented TestConfig struct.

The new TestConfig struct 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.New for the static error message is a good change. It improves consistency with the rest of the codebase and addresses the previous review comment.

@gaby gaby added this to the v3 milestone Oct 10, 2024
app.go Show resolved Hide resolved
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this something we could replace with os.ErrDeadlineExceeded() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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"

- Rename TestConfig.ErrOnTimeout to TestConfig.FailOnTimeout
- Update documentation to use changed name
  - Also fix stale documentation about passing Timeout as a
    single argument
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Files that changed from the base of the PR and between 4a97d7b and 78e32a7.

📒 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 suggestion

Enhance empty response error handling.

As suggested in the past review, we should use errors.New for 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")

Comment on lines +933 to +941
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)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider improving timeout error handling.

The timeout handling could be more idiomatic and provide more context in the error message.

Consider this improvement:

- return nil, fmt.Errorf("test: timeout error after %s", cfg.Timeout)
+ return nil, fmt.Errorf("test: request timed out waiting for response after %s", 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.

Suggested change
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("test: request timed out waiting for response after %s", cfg.Timeout)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

📝 [Proposal]: Add TestConfig to app.Test() for configurable testing
2 participants