Skip to content

Commit

Permalink
fix: Inconsistent and flaky unit-tests (#2892)
Browse files Browse the repository at this point in the history 
* Fixes for some of the failing tests

* Add readiness check to serverStart()

* Use net/http client for tests listen test

* Use different key for this test

* Run Proxy Middleware tests in parallel. Add nil checks for potential issues pointed by nilaway

* Enable parallel client tests

* Do not run timing sensitive tests in parallel

* Remove TODO

* Revert Test_Proxy_DoTimeout_Timeout, and remove t.Parallel() for it

* Do not calculate favicon len on each handler call

* Revert logic change

* Increase timeout of SaveFile tests

* Do not run time sensitive tests in parallel

* The Agent can't be run in parallel

* Do not run time sensitive tests in parallel

* Fixes based on uber/nilaway

* Revert change to Client test

* Run parallel

* Update client_test.go

* Update client_test.go

* Update cache_test.go

* Update cookiejar_test.go

* Remove parallel for test using timeouts

* Remove t.Parallel() from logger middleware tests

* Do not use testify.require in a goroutine

* Fix import, and update golangci-lint

* Remove changes to template_chain.go

* Run more tests in parallel

* Add more parallel tests

* Add more parallel tests

* SetLogger can't run in parallel

* Run more tests in parallel, fix issue with goroutine in limiter middleware

* Update internal/storage/memory, add more benchmarks

* Increase sleep for csrf test by 100 milliseconds. Implement asserted and parallel benchmarks for Session middleware

* Add 100 milliseconds to sleep during test

* Revert name change

* fix: Inconsistent and flaky unit-tests

* fix: Inconsistent and flaky unit-tests

* fix: Inconsistent and flaky unit-tests

* fix: Inconsistent and flaky unit-tests

* fix: Inconsistent and flaky unit-tests

* fix: Inconsistent and flaky unit-tests

* fix: Inconsistent and flaky unit-tests

* fix: Inconsistent and flaky unit-tests

* fix: Inconsistent and flaky unit-tests

* fix: Inconsistent and flaky unit-tests

---------

Co-authored-by: M. Efe Çetin <[email protected]>
Co-authored-by: René <[email protected]>
  • Loading branch information
@gaby @efectn @ReneWerner87
3 people authored Mar 8, 2024
1 parent 1fdef7f commit 0379cc5
Show file tree
Hide file tree
Showing 32 changed files with 691 additions and 294 deletions.
2 changes: 0 additions & 2 deletions .golangci.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ output:
linters-settings:
testifylint:
enable-all: true
disable:
- go-require
errcheck:
check-type-assertions: true
check-blank: true
Expand Down
91 changes: 60 additions & 31 deletions app_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (

"github.com/gofiber/utils/v2"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/valyala/fasthttp"
"github.com/valyala/fasthttp/fasthttputil"
Expand Down Expand Up @@ -197,6 +198,7 @@ func (*customConstraint) Execute(param string, args ...string) bool {
}

func Test_App_CustomConstraint(t *testing.T) {
t.Parallel()
app := New()
app.RegisterCustomConstraint(&customConstraint{})

Expand Down Expand Up @@ -821,20 +823,20 @@ func Test_App_ShutdownWithTimeout(t *testing.T) {
time.Sleep(5 * time.Second)
return c.SendString("body")
})

ln := fasthttputil.NewInmemoryListener()
go func() {
require.NoError(t, app.Listener(ln))
err := app.Listener(ln)
assert.NoError(t, err)
}()

time.Sleep(1 * time.Second)
go func() {
conn, err := ln.Dial()
if err != nil {
t.Errorf("unexepcted error: %v", err)
}
assert.NoError(t, err)

if _, err = conn.Write([]byte("GET / HTTP/1.1\r\nHost: google.com\r\n\r\n")); err != nil {
t.Errorf("unexpected error: %v", err)
}
_, err = conn.Write([]byte("GET / HTTP/1.1\r\nHost: google.com\r\n\r\n"))
assert.NoError(t, err)
}()
time.Sleep(1 * time.Second)

Expand Down Expand Up @@ -866,20 +868,18 @@ func Test_App_ShutdownWithContext(t *testing.T) {
ln := fasthttputil.NewInmemoryListener()

go func() {
require.NoError(t, app.Listener(ln))
err := app.Listener(ln)
assert.NoError(t, err)
}()

time.Sleep(1 * time.Second)

go func() {
conn, err := ln.Dial()
if err != nil {
t.Errorf("unexepcted error: %v", err)
}
assert.NoError(t, err)

if _, err = conn.Write([]byte("GET / HTTP/1.1\r\nHost: google.com\r\n\r\n")); err != nil {
t.Errorf("unexpected error: %v", err)
}
_, err = conn.Write([]byte("GET / HTTP/1.1\r\nHost: google.com\r\n\r\n"))
assert.NoError(t, err)
}()

time.Sleep(1 * time.Second)
Expand All @@ -903,6 +903,7 @@ func Test_App_ShutdownWithContext(t *testing.T) {

// go test -run Test_App_Static_Index_Default
func Test_App_Static_Index_Default(t *testing.T) {
t.Parallel()
app := New()

app.Static("/prefix", "./.github/workflows")
Expand Down Expand Up @@ -932,6 +933,7 @@ func Test_App_Static_Index_Default(t *testing.T) {

// go test -run Test_App_Static_Index
func Test_App_Static_Direct(t *testing.T) {
t.Parallel()
app := New()

app.Static("/", "./.github")
Expand Down Expand Up @@ -960,6 +962,7 @@ func Test_App_Static_Direct(t *testing.T) {

// go test -run Test_App_Static_MaxAge
func Test_App_Static_MaxAge(t *testing.T) {
t.Parallel()
app := New()

app.Static("/", "./.github", Static{MaxAge: 100})
Expand All @@ -974,6 +977,7 @@ func Test_App_Static_MaxAge(t *testing.T) {

// go test -run Test_App_Static_Custom_CacheControl
func Test_App_Static_Custom_CacheControl(t *testing.T) {
t.Parallel()
app := New()

app.Static("/", "./.github", Static{ModifyResponse: func(c Ctx) error {
Expand All @@ -994,6 +998,7 @@ func Test_App_Static_Custom_CacheControl(t *testing.T) {

// go test -run Test_App_Static_Download
func Test_App_Static_Download(t *testing.T) {
t.Parallel()
app := New()

app.Static("/fiber.png", "./.github/testdata/fs/img/fiber.png", Static{Download: true})
Expand All @@ -1008,6 +1013,7 @@ func Test_App_Static_Download(t *testing.T) {

// go test -run Test_App_Static_Group
func Test_App_Static_Group(t *testing.T) {
t.Parallel()
app := New()

grp := app.Group("/v1", func(c Ctx) error {
Expand Down Expand Up @@ -1037,6 +1043,7 @@ func Test_App_Static_Group(t *testing.T) {
}

func Test_App_Static_Wildcard(t *testing.T) {
t.Parallel()
app := New()

app.Static("*", "./.github/index.html")
Expand All @@ -1054,6 +1061,7 @@ func Test_App_Static_Wildcard(t *testing.T) {
}

func Test_App_Static_Prefix_Wildcard(t *testing.T) {
t.Parallel()
app := New()

app.Static("/test/*", "./.github/index.html")
Expand All @@ -1079,6 +1087,7 @@ func Test_App_Static_Prefix_Wildcard(t *testing.T) {
}

func Test_App_Static_Prefix(t *testing.T) {
t.Parallel()
app := New()
app.Static("/john", "./.github")

Expand Down Expand Up @@ -1109,6 +1118,7 @@ func Test_App_Static_Prefix(t *testing.T) {
}

func Test_App_Static_Trailing_Slash(t *testing.T) {
t.Parallel()
app := New()
app.Static("/john", "./.github")

Expand Down Expand Up @@ -1155,6 +1165,7 @@ func Test_App_Static_Trailing_Slash(t *testing.T) {
}

func Test_App_Static_Next(t *testing.T) {
t.Parallel()
app := New()
app.Static("/", ".github", Static{
Next: func(c Ctx) bool {
Expand All @@ -1168,6 +1179,7 @@ func Test_App_Static_Next(t *testing.T) {
})

t.Run("app.Static is skipped: invoking Get handler", func(t *testing.T) {
t.Parallel()
req := httptest.NewRequest(MethodGet, "/", nil)
req.Header.Set("X-Custom-Header", "skip")
resp, err := app.Test(req)
Expand All @@ -1182,6 +1194,7 @@ func Test_App_Static_Next(t *testing.T) {
})

t.Run("app.Static is not skipped: serving index.html", func(t *testing.T) {
t.Parallel()
req := httptest.NewRequest(MethodGet, "/", nil)
req.Header.Set("X-Custom-Header", "don't skip")
resp, err := app.Test(req)
Expand All @@ -1198,6 +1211,7 @@ func Test_App_Static_Next(t *testing.T) {

// go test -run Test_App_Mixed_Routes_WithSameLen
func Test_App_Mixed_Routes_WithSameLen(t *testing.T) {
t.Parallel()
app := New()

// middleware
Expand Down Expand Up @@ -1476,6 +1490,7 @@ func (invalidView) Render(io.Writer, string, any, ...string) error { panic("impl

// go test -run Test_App_Init_Error_View
func Test_App_Init_Error_View(t *testing.T) {
t.Parallel()
app := New(Config{Views: invalidView{}})

defer func() {
Expand Down Expand Up @@ -1542,23 +1557,23 @@ func Test_App_ReadTimeout(t *testing.T) {
time.Sleep(500 * time.Millisecond)

conn, err := net.Dial(NetworkTCP4, "127.0.0.1:4004")
require.NoError(t, err)
assert.NoError(t, err)
defer func(conn net.Conn) {
err := conn.Close()
require.NoError(t, err)
assert.NoError(t, err)
}(conn)

_, err = conn.Write([]byte("HEAD /read-timeout HTTP/1.1\r\n"))
require.NoError(t, err)
assert.NoError(t, err)

buf := make([]byte, 1024)
var n int
n, err = conn.Read(buf)

require.NoError(t, err)
require.True(t, bytes.Contains(buf[:n], []byte("408 Request Timeout")))
assert.NoError(t, err)
assert.True(t, bytes.Contains(buf[:n], []byte("408 Request Timeout")))

require.NoError(t, app.Shutdown())
assert.NoError(t, app.Shutdown())
}()

require.NoError(t, app.Listen(":4004", ListenConfig{DisableStartupMessage: true}))
Expand All @@ -1576,23 +1591,22 @@ func Test_App_BadRequest(t *testing.T) {
go func() {
time.Sleep(500 * time.Millisecond)
conn, err := net.Dial(NetworkTCP4, "127.0.0.1:4005")
require.NoError(t, err)
assert.NoError(t, err)
defer func(conn net.Conn) {
err := conn.Close()
require.NoError(t, err)
assert.NoError(t, err)
}(conn)

_, err = conn.Write([]byte("BadRequest\r\n"))
require.NoError(t, err)
assert.NoError(t, err)

buf := make([]byte, 1024)
var n int
n, err = conn.Read(buf)
require.NoError(t, err)

require.True(t, bytes.Contains(buf[:n], []byte("400 Bad Request")))

require.NoError(t, app.Shutdown())
assert.NoError(t, err)
assert.True(t, bytes.Contains(buf[:n], []byte("400 Bad Request")))
assert.NoError(t, app.Shutdown())
}()

require.NoError(t, app.Listen(":4005", ListenConfig{DisableStartupMessage: true}))
Expand All @@ -1612,12 +1626,12 @@ func Test_App_SmallReadBuffer(t *testing.T) {
go func() {
time.Sleep(500 * time.Millisecond)
req, err := http.NewRequestWithContext(context.Background(), MethodGet, "http://127.0.0.1:4006/small-read-buffer", nil)
require.NoError(t, err)
assert.NoError(t, err)
var client http.Client
resp, err := client.Do(req)
require.NoError(t, err)
require.Equal(t, 431, resp.StatusCode)
require.NoError(t, app.Shutdown())
assert.NoError(t, err)
assert.Equal(t, 431, resp.StatusCode)
assert.NoError(t, app.Shutdown())
}()

require.NoError(t, app.Listen(":4006", ListenConfig{DisableStartupMessage: true}))
Expand Down Expand Up @@ -1755,6 +1769,19 @@ func Test_App_Test_no_timeout_infinitely(t *testing.T) {
}
}

func Test_App_Test_timeout(t *testing.T) {
t.Parallel()

app := New()
app.Get("/", func(_ Ctx) error {
time.Sleep(1 * time.Second)
return nil
})

_, err := app.Test(httptest.NewRequest(MethodGet, "/", nil), 100*time.Millisecond)
require.Equal(t, errors.New("test: timeout error after 100ms"), err)
}

func Test_App_SetTLSHandler(t *testing.T) {
t.Parallel()
tlsHandler := &TLSHandler{clientHelloInfo: &tls.ClientHelloInfo{
Expand Down Expand Up @@ -1815,6 +1842,7 @@ func TestApp_GetRoutes(t *testing.T) {
}

func Test_Middleware_Route_Naming_With_Use(t *testing.T) {
t.Parallel()
named := "named"
app := New()

Expand Down Expand Up @@ -1874,6 +1902,7 @@ func Test_Middleware_Route_Naming_With_Use(t *testing.T) {
}

func Test_Route_Naming_Issue_2671_2685(t *testing.T) {
t.Parallel()
app := New()

app.Get("/", emptyHandler).Name("index")
Expand Down
19 changes: 11 additions & 8 deletions client/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/gofiber/fiber/v3/addon/retry"
"github.com/gofiber/fiber/v3/internal/tlstest"
"github.com/gofiber/utils/v2"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/valyala/bytebufferpool"
)
Expand Down Expand Up @@ -277,8 +278,8 @@ func Test_Client_ConcurrencyRequests(t *testing.T) {
go func(m string) {
defer wg.Done()
resp, err := client.Custom("http://example.com", m)
require.NoError(t, err)
require.Equal(t, "example.com "+m, utils.UnsafeString(resp.RawResponse.Body()))
assert.NoError(t, err)
assert.Equal(t, "example.com "+m, utils.UnsafeString(resp.RawResponse.Body()))
}(method)
}
}
Expand Down Expand Up @@ -1258,10 +1259,11 @@ func Test_Client_TLS(t *testing.T) {
})

go func() {
require.NoError(t, app.Listener(ln, fiber.ListenConfig{
assert.NoError(t, app.Listener(ln, fiber.ListenConfig{
DisableStartupMessage: true,
}))
}()
time.Sleep(1 * time.Second)

client := New()
resp, err := client.SetTLSConfig(clientTLSConf).Get("https://" + ln.Addr().String())
Expand Down Expand Up @@ -1291,10 +1293,11 @@ func Test_Client_TLS_Error(t *testing.T) {
})

go func() {
require.NoError(t, app.Listener(ln, fiber.ListenConfig{
assert.NoError(t, app.Listener(ln, fiber.ListenConfig{
DisableStartupMessage: true,
}))
}()
time.Sleep(1 * time.Second)

client := New()
resp, err := client.SetTLSConfig(clientTLSConf).Get("https://" + ln.Addr().String())
Expand All @@ -1321,10 +1324,11 @@ func Test_Client_TLS_Empty_TLSConfig(t *testing.T) {
})

go func() {
require.NoError(t, app.Listener(ln, fiber.ListenConfig{
assert.NoError(t, app.Listener(ln, fiber.ListenConfig{
DisableStartupMessage: true,
}))
}()
time.Sleep(1 * time.Second)

client := New()
resp, err := client.Get("https://" + ln.Addr().String())
Expand Down Expand Up @@ -1579,18 +1583,17 @@ func Test_Client_SetProxyURL(t *testing.T) {

func Test_Client_SetRetryConfig(t *testing.T) {
t.Parallel()

retryConfig := &retry.Config{
InitialInterval: 1 * time.Second,
MaxRetryCount: 3,
}

core, client, req := newCore(), New(), AcquireRequest()
req.SetURL("http://example.com")
req.SetURL("http://exampleretry.com")
client.SetRetryConfig(retryConfig)
_, err := core.execute(context.Background(), client, req)

require.NoError(t, err)
require.Error(t, err)
require.Equal(t, retryConfig.InitialInterval, client.RetryConfig().InitialInterval)
require.Equal(t, retryConfig.MaxRetryCount, client.RetryConfig().MaxRetryCount)
}
Expand Down
Loading

1 comment on commit 0379cc5

@ReneWerner87
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 0379cc5 Previous: d2b19e2 Ratio
Benchmark_Middleware_Favicon 216 ns/op 12 B/op 4 allocs/op 90.01 ns/op 3 B/op 1 allocs/op 2.40

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.