Skip to content

test: add timeout isolation regression coverage#118

Open
hwbrzzl wants to merge 3 commits into
masterfrom
test-timeout-regressions
Open

test: add timeout isolation regression coverage#118
hwbrzzl wants to merge 3 commits into
masterfrom
test-timeout-regressions

Conversation

@hwbrzzl
Copy link
Copy Markdown
Contributor

@hwbrzzl hwbrzzl commented May 24, 2026

Summary

  • Bump goravel/gin and goravel/fiber to the timeout-fix snapshots that preserve 408 Request Timeout responses without leaking stale writes into later requests
  • Add example-app timeout isolation coverage that proves a timed-out request returns Request Timeout and the next request still returns its own fresh body
  • Update the URL route assertions to match the extra timeout regression routes in the example app

Why

The timeout fixes in goravel/gin and goravel/fiber are about preserving the existing Timeout(...) behavior while preventing pooled request state from being reused after a request has already timed out. This example repo is the right place to pin those driver snapshots and prove the integration still behaves correctly under both HTTP drivers.

resp, err := s.Http(s.T()).Get("/timeout-isolated?token=stale")
s.Require().NoError(err)
resp.AssertStatus(contractshttp.StatusRequestTimeout)

content, err := resp.Content()
s.Require().NoError(err)
s.Equal("Request Timeout", content)

freshResp, err := s.Http(s.T()).Get("/timeout-after?token=fresh")
s.Require().NoError(err)
freshContent, err := freshResp.Content()
s.Require().NoError(err)
s.Equal("{\"token\":\"fresh\"}", freshContent)

Before these driver fixes, the timeout middleware could return 408 while the original handler kept running against pooled request and response wrappers. That created the bug class where a later write like late:stale could bleed into the next request. After the fixes, the timed-out request still yields Request Timeout, but the follow-up request gets its own clean response body instead of leaked output from the abandoned handler.

facades.Route().Get("timeout-isolated", func(ctx http.Context) http.Response {
	select {
	case <-time.After(4 * time.Second):
		return ctx.Response().Success().String("late:" + token) // before: this late write could leak into a later request
	case <-ctx.Done():
		return ctx.Response().Status(http.StatusRequestTimeout).String("Request Timeout") // after: timeout stays visible to the caller
	}
})

facades.Route().Get("timeout-after", func(ctx http.Context) http.Response {
	return ctx.Response().Success().Json(http.Json{
		"token": ctx.Request().Query("token", "missing"), // after: the next request still renders its own fresh body
	})
})

Pin goravel/gin and goravel/fiber to the timeout fix snapshots so the example app can verify 408 timeout responses and stale-response isolation under both drivers.
Copilot AI review requested due to automatic review settings May 24, 2026 04:52
@hwbrzzl hwbrzzl requested a review from a team as a code owner May 24, 2026 04:52
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the example app and its integration tests to cover a timeout “isolation” regression scenario across the Gin and Fiber HTTP drivers, while also bumping goravel/gin and goravel/fiber to snapshot versions that include the timeout fix.

Changes:

  • Add new example routes to simulate a timed-out request and a subsequent “fresh” request.
  • Add a new TestTimeoutIsolation integration test that asserts the timeout response and verifies the next response is not contaminated.
  • Update URL-route assertions to match the new closure ordering, and bump module dependencies to the referenced snapshots.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.

File Description
tests/feature/http_test.go Adds timeout-isolation regression test and updates URL route assertions.
routes/api.go Adds /timeout-isolated and /timeout-after routes used by the new regression test.
go.mod Bumps goravel/gin and goravel/fiber snapshots plus related indirect deps.
go.sum Updates checksums for the dependency changes.
Comments suppressed due to low confidence (1)

tests/feature/http_test.go:219

  • This assertion hard-codes the Go compiler's synthetic function name (Api.func13.2), which changes whenever new closures are added earlier in routes/api.go. To make this test resilient, consider decoding the JSON and asserting stable fields only, or asserting the handler has a stable prefix instead of the exact func index.
	content, err = resp.Content()
	s.Require().NoError(err)
	s.Equal(`{"full_url":"http://example.com/url/post/1?a=1\u0026b=2","info":{"handler":"goravel/routes.Api.func13.2","method":"POST","name":"url.post","path":"/url/post/{id}"},"info1":{"handler":"goravel/routes.Api.func13.2","method":"POST","name":"url.post","path":"/url/post/{id}"},"method":"POST","name":"url.post","origin_path":"/url/post/{id}","path":"/url/post/1","url":"/url/post/1?a=1\u0026b=2"}`, content)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread routes/api.go
Comment thread routes/api.go Outdated
Comment thread tests/feature/http_test.go Outdated
Comment thread tests/feature/http_test.go Outdated
hwbrzzl added 2 commits May 24, 2026 12:58
Derive timeout overlap timings from config, stop timers explicitly, and make route URL assertions resilient to compiler-generated handler names.
Copilot AI review requested due to automatic review settings May 24, 2026 05:14
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants