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.
:::

3. 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)

3. 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.

Configuration used: CodeRabbit UI
Review profile: CHILL

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())
 }

Configuration used: CodeRabbit UI
Review profile: CHILL

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,

Configuration used: CodeRabbit UI
Review profile: CHILL

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

ctx_test.go (1)

3246-3249: Good addition of test timeout configuration.

The explicit timeout configuration with FailOnTimeout helps 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))

Configuration used: CodeRabbit UI
Review profile: CHILL

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,

Configuration used: CodeRabbit UI
Review profile: CHILL

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`.

-			return nil, errors.New("test: got empty response")
+			return nil, errors.New("test: got empty response")

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

Configuration used: CodeRabbit UI
Review profile: CHILL

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)

Configuration used: CodeRabbit UI
Review profile: CHILL

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 TestConfig as an argument to app.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 TestConfig is the equivalent of:
+2. Empty Config (when fiber.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 -->