🩹 Fix: Fix app.Test() auto-failing when a connection is closed early #3279
Conversation
grivera64
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe pull request introduces enhancements to the error handling in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (5)
Finishing Touches
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 #3279 +/- ##
==========================================
+ Coverage 84.08% 84.15% +0.06%
==========================================
Files 116 116
Lines 11541 11541
==========================================
+ Hits 9704 9712 +8
+ Misses 1406 1400 -6
+ Partials 431 429 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app_test.go (1)
1513-1525: Test logic looks good, but needs formatting fixes.The test case correctly verifies that
c.Drop()returnsErrTestGotEmptyResponse. However, the code needs formatting fixes to meet Go standards.Run the following commands to fix the formatting:
-func Test_App_Test_drop_empty_response(t *testing.T) { - t.Parallel() - - app := New() - app.Get("/", func (c Ctx) error { - return c.Drop() - }) - - _, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), TestConfig{ - Timeout: 0, - FailOnTimeout: false, - }) - require.ErrorIs(t, err, ErrTestGotEmptyResponse) +func Test_App_Test_drop_empty_response(t *testing.T) { + t.Parallel() + + app := New() + app.Get("/", func(c Ctx) error { + return c.Drop() + }) + + _, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), TestConfig{ + Timeout: 0, + FailOnTimeout: false, + }) + require.ErrorIs(t, err, ErrTestGotEmptyResponse) }🧰 Tools
🪛 golangci-lint (1.62.2)
1514-1514: File is not
gofumpt-ed with-extra(gofumpt)
1514-1514: File is not
goimports-ed(goimports)
🪛 GitHub Actions: golangci-lint
[error] 1514-1514: File is not
gofmt-ed with-s. Code formatting does not meet Go standards.
[error] 1514-1514: File is not
gofumpt-ed with-extra. Code formatting does not meet enhanced Go standards.
[error] 1514-1514: File is not
goimports-ed. Import formatting does not meet Go standards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app.go(3 hunks)app_test.go(2 hunks)helpers.go(2 hunks)helpers_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
app_test.go
1514-1514: File is not gofumpt-ed with -extra
(gofumpt)
1514-1514: File is not goimports-ed
(goimports)
🪛 GitHub Actions: golangci-lint
app_test.go
[error] 1514-1514: File is not gofmt-ed with -s. Code formatting does not meet Go standards.
[error] 1514-1514: File is not gofumpt-ed with -extra. Code formatting does not meet enhanced Go standards.
[error] 1514-1514: File is not goimports-ed. Import formatting does not meet Go standards.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (7)
helpers.go (2)
615-616: LGTM! Good error handling practice.Defining
errTestConnClosedas a package-level variable follows Go's best practices for error handling, enabling error comparison usingerrors.Is().
636-637: LGTM! Consistent error handling.Using the predefined error variable instead of creating a new error improves consistency and enables proper error type checking.
helpers_test.go (1)
551-551: LGTM! Improved error type verification.Using
require.ErrorIs()instead of a generic error check ensures that the specificerrTestConnClosederror is returned when writing to a closed connection.app.go (2)
944-945: LGTM! Well-defined error variable.The new error variable
ErrTestGotEmptyResponseis appropriately defined at package level with a clear error message.
Line range hint
1027-1038: LGTM! Improved error handling for closed connections.The changes correctly handle early connection closure:
- Ignores
errTestConnClosedwhen combined withfasthttp.ErrGetOnly- Returns
ErrTestGotEmptyResponsefor empty responses, which is expected whenc.Drop()is calledapp_test.go (2)
1494-1494: LGTM! Improved error assertion.Using
require.ErrorIsis more precise than comparing error strings.
1510-1511: LGTM! Consistent error handling.The test now correctly checks for
ErrTestGotEmptyResponseinstead of a string comparison.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app_test.go (1)
1517-1517: Fix formatting in function declarationThe function declaration has incorrect spacing. Let's fix it to match the codebase style.
- app.Get("/", func (c Ctx) error { + app.Get("/", func(c Ctx) error {🧰 Tools
🪛 golangci-lint (1.62.2)
1517-1517: File is not
gofumpt-ed with-extra(gofumpt)
1517-1517: File is not
goimports-ed(goimports)
🪛 GitHub Actions: golangci-lint
[warning] 1517-1517: File is not
gofmt-ed with-s
[warning] 1517-1517: File is not
gofumpt-ed with-extra
[warning] 1517-1517: File is not
goimports-ed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app_test.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
app_test.go
1517-1517: File is not gofumpt-ed with -extra
(gofumpt)
1517-1517: File is not goimports-ed
(goimports)
1522-1522: File is not gofumpt-ed with -extra
(gofumpt)
🪛 GitHub Actions: golangci-lint
app_test.go
[warning] 1517-1517: File is not gofmt-ed with -s
[warning] 1517-1517: File is not gofumpt-ed with -extra
[warning] 1517-1517: File is not goimports-ed
[warning] 1522-1522: File is not gofumpt-ed with -extra
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (3)
app_test.go (3)
1494-1494: Great improvement in error checking!Using
require.ErrorIsinstead ofrequire.Equalis more idiomatic and provides better error type checking.
1510-1511: Appropriate error type for empty response!Using
ErrTestGotEmptyResponsefor timeout cases without failure flag is consistent with the PR's objective of improving error handling.
1513-1525: Well-structured test case that addresses the core issue!This test case effectively verifies that
app.Test()handlesc.Drop()correctly by returningErrTestGotEmptyResponseinstead of failing.🧰 Tools
🪛 golangci-lint (1.62.2)
1517-1517: File is not
gofumpt-ed with-extra(gofumpt)
1517-1517: File is not
goimports-ed(goimports)
1522-1522: File is not
gofumpt-ed with-extra(gofumpt)
🪛 GitHub Actions: golangci-lint
[warning] 1517-1517: File is not
gofmt-ed with-s
[warning] 1517-1517: File is not
gofumpt-ed with-extra
[warning] 1517-1517: File is not
goimports-ed
[warning] 1522-1522: File is not
gofumpt-ed with-extra
grivera64
force-pushed
the
bugfix(v3)/fix-autofail-app-test
branch
from
566cdb8 to
6456261
Compare
grivera64
force-pushed
the
bugfix(v3)/fix-autofail-app-test
branch
from
6456261 to
fd86be6
Compare
Description
This PR fixes the bug where
app.Test()auto-fails by returningtestConn is closedif the underlying connection is closed during afiber.Handler. Namely,c.Drop()falls under this category, as it callsc.RequestCtx().Conn().Close().To fix this, we do not return an error if the error matches with
errTestConnClosed. If there was truly an error, it should return asErrTestGotEmptyResponsedue to an empty response (which should be expected whenc.Drop()is called).Fixes #3278
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
app.Test()is working as expected for new corner cases introduced in v3.Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md