From b648b951b067f71f10d0211487ca8f485754acbf Mon Sep 17 00:00:00 2001 From: Gani Georgiev Date: Sat, 22 Aug 2026 11:33:18 +0300 Subject: [PATCH] fix some of the flaky tests --- CHANGELOG.md | 6 +- apis/cron_test.go | 7 +- apis/middlewares_rate_limit_test.go | 20 +++--- core/base_test.go | 42 ++++++----- core/settings_model.go | 13 ---- tests/api.go | 1 + tools/cron/cron_test.go | 105 ++++++++++++++-------------- tools/filesystem/filesystem.go | 7 +- 8 files changed, 101 insertions(+), 100 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ae09e99..df85b6d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,10 +4,10 @@ _⚠️ Note that this could be a slight breaking change in case you are chaining PocketBase commands and relied on the previous `0` exit status for `Command.RunE` returned errors._ _Or in other words, if you have `./pocketbase invalid && someothercommand` and previously relied that `someothercommand` will be always executed then this is no longer the case and you'll have to adjust it or replace `&&` with `;`._ -- Added helper `filesystem` methods: +- Added new `filesystem` low-level helper methods: - `filesystem.NewWriter(key, opts)` to allow direct file create from an `io.Reader` value. - - `filesystem.OnNewWriter()` low-level hook to allow listening for new/to-be-creaded files _(app level equivalent hook is also available but not exposed for now to avoid introducing breaking changes)_. - - `filesystem.OnDelete()` low-level hook to allow listening for deleted files _(app level equivalent hook is also available but not exposed for now to avoid introducing breaking changes)_. + - `filesystem.OnNewWriter()` hook to allow listening for new/to-be-creaded files _(app level hook is not exposed for now to avoid introducing breaking changes)_. + - `filesystem.OnDelete()` hook to allow listening for deleted files _(app level hook is not exposed for now to avoid introducing breaking changes)_. - Added new `DELETE /api/logs` endpoint and UI control to delete all logs without changing the `maxDays` retention setting. diff --git a/apis/cron_test.go b/apis/cron_test.go index a72fb2f5..b30f03f7 100644 --- a/apis/cron_test.go +++ b/apis/cron_test.go @@ -77,10 +77,13 @@ func TestCronsRun(t *testing.T) { app.Cron().Add("test", "* * * * *", func() { app.Store().Set("testJobCalls", cast.ToInt(app.Store().Get("testJobCalls"))+1) }) + app.Cron().Stop() } expectedCalls := func(expected int) func(t testing.TB, app *tests.TestApp, res *http.Response) { return func(t testing.TB, app *tests.TestApp, res *http.Response) { + time.Sleep(50 * time.Millisecond) + total := cast.ToInt(app.Store().Get("testJobCalls")) if total != expected { t.Fatalf("Expected total testJobCalls %d, got %d", expected, total) @@ -93,7 +96,6 @@ func TestCronsRun(t *testing.T) { Name: "unauthorized", Method: http.MethodPost, URL: "/api/crons/test", - Delay: 50 * time.Millisecond, BeforeTestFunc: beforeTestFunc, AfterTestFunc: expectedCalls(0), ExpectedStatus: 401, @@ -107,7 +109,6 @@ func TestCronsRun(t *testing.T) { Headers: map[string]string{ "Authorization": "eyJhbGciOiJIUzI1NiJ9.eyJpZCI6IjRxMXhsY2xtZmxva3UzMyIsInR5cGUiOiJhdXRoIiwiY29sbGVjdGlvbklkIjoiX3BiX3VzZXJzX2F1dGhfIiwiZXhwIjoyNTI0NjA0NDYxLCJyZWZyZXNoYWJsZSI6dHJ1ZX0.ZT3F0Z3iM-xbGgSG3LEKiEzHrPHr8t8IuHLZGGNuxLo", }, - Delay: 50 * time.Millisecond, BeforeTestFunc: beforeTestFunc, AfterTestFunc: expectedCalls(0), ExpectedStatus: 403, @@ -121,7 +122,6 @@ func TestCronsRun(t *testing.T) { Headers: map[string]string{ "Authorization": "eyJhbGciOiJIUzI1NiJ9.eyJpZCI6InN5d2JoZWNuaDQ2cmhtMCIsInR5cGUiOiJhdXRoIiwiY29sbGVjdGlvbklkIjoicGJjXzMxNDI2MzU4MjMiLCJleHAiOjI1MjQ2MDQ0NjEsInJlZnJlc2hhYmxlIjp0cnVlfQ.UXgO3j-0BumcugrFjbd7j0M4MQvbrLggLlcu_YNGjoY", }, - Delay: 50 * time.Millisecond, BeforeTestFunc: beforeTestFunc, AfterTestFunc: expectedCalls(0), ExpectedStatus: 404, @@ -135,7 +135,6 @@ func TestCronsRun(t *testing.T) { Headers: map[string]string{ "Authorization": "eyJhbGciOiJIUzI1NiJ9.eyJpZCI6InN5d2JoZWNuaDQ2cmhtMCIsInR5cGUiOiJhdXRoIiwiY29sbGVjdGlvbklkIjoicGJjXzMxNDI2MzU4MjMiLCJleHAiOjI1MjQ2MDQ0NjEsInJlZnJlc2hhYmxlIjp0cnVlfQ.UXgO3j-0BumcugrFjbd7j0M4MQvbrLggLlcu_YNGjoY", }, - Delay: 50 * time.Millisecond, BeforeTestFunc: beforeTestFunc, AfterTestFunc: expectedCalls(1), ExpectedStatus: 204, diff --git a/apis/middlewares_rate_limit_test.go b/apis/middlewares_rate_limit_test.go index 3114c25d..af19d1e1 100644 --- a/apis/middlewares_rate_limit_test.go +++ b/apis/middlewares_rate_limit_test.go @@ -1,8 +1,10 @@ package apis_test import ( + "fmt" "net/http/httptest" "testing" + "testing/synctest" "time" "github.com/pocketbase/pocketbase/apis" @@ -127,27 +129,29 @@ func TestDefaultRateLimitMiddleware(t *testing.T) { {"/rate/guest", 0, true, 429}, } - for _, s := range scenarios { - t.Run(s.url, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + for i, s := range scenarios { + prefix := fmt.Sprintf("[%s:%d] ", s.url, i+1) + rec := httptest.NewRecorder() req := httptest.NewRequest("GET", s.url, nil) if s.authenticated { auth, err := app.FindAuthRecordByEmail("users", "test@example.com") if err != nil { - t.Fatal(err) + t.Fatalf(prefix+"%v", err) } token, err := auth.NewAuthToken() if err != nil { - t.Fatal(err) + t.Fatalf(prefix+"%v", err) } req.Header.Add("Authorization", token) } if s.wait > 0 { - time.Sleep(time.Duration(s.wait) * time.Millisecond) + synctest.Sleep(time.Duration(s.wait) * time.Millisecond) } mux.ServeHTTP(rec, req) @@ -155,10 +159,10 @@ func TestDefaultRateLimitMiddleware(t *testing.T) { result := rec.Result() if result.StatusCode != s.expectedStatus { - t.Fatalf("Expected response status %d, got %d", s.expectedStatus, result.StatusCode) + t.Fatalf(prefix+"Expected response status %d, got %d", s.expectedStatus, result.StatusCode) } - }) - } + } + }) } func TestDefaultRateLimitMiddlewareSkipChecks(t *testing.T) { diff --git a/core/base_test.go b/core/base_test.go index 5555e020..0e383f8d 100644 --- a/core/base_test.go +++ b/core/base_test.go @@ -7,6 +7,7 @@ import ( "os" "slices" "testing" + "testing/synctest" "time" _ "unsafe" @@ -302,31 +303,34 @@ func TestBaseAppLoggerWrites(t *testing.T) { }) t.Run("test batch logs writes", func(t *testing.T) { - app.Settings().Logs.MaxDays = 1 + synctest.Test(t, func(t *testing.T) { + app.Settings().Logs.MaxDays = 1 - for i := 0; i < logsThreshold-1; i++ { + for i := 0; i < logsThreshold-1; i++ { + app.Logger().Error("test") + } + + if total := totalLogs(app, t); total != 0 { + t.Fatalf("Expected no logs, got %d", total) + } + + // should trigger batch write app.Logger().Error("test") - } - if total := totalLogs(app, t); total != 0 { - t.Fatalf("Expected no logs, got %d", total) - } + // should be added for the next batch write + app.Logger().Error("test") - // should trigger batch write - app.Logger().Error("test") + if total := totalLogs(app, t); total != logsThreshold { + t.Fatalf("Expected %d logs, got %d", logsThreshold, total) + } - // should be added for the next batch write - app.Logger().Error("test") + // wait for 3 secs to check the timer trigger + synctest.Sleep(3000 * time.Millisecond) - if total := totalLogs(app, t); total != logsThreshold { - t.Fatalf("Expected %d logs, got %d", logsThreshold, total) - } - - // wait for ~3 secs to check the timer trigger - time.Sleep(3200 * time.Millisecond) - if total := totalLogs(app, t); total != logsThreshold+1 { - t.Fatalf("Expected %d logs, got %d", logsThreshold+1, total) - } + if total := totalLogs(app, t); total != logsThreshold { + t.Fatalf("Expected %d logs, got %d", logsThreshold, total) + } + }) }) } diff --git a/core/settings_model.go b/core/settings_model.go index 435da103..1fb6033f 100644 --- a/core/settings_model.go +++ b/core/settings_model.go @@ -257,13 +257,6 @@ func (s *Settings) DBExport(app App) (map[string]any, error) { } result["updated"] = now - // @todo remove with encoding/json/2 - // serialize as empty array - //nolint:staticcheck - if s.settings.SuperuserIPs == nil { - s.settings.SuperuserIPs = []string{} - } - encoded, err := json.Marshal(s.settings, json.Deterministic(true)) if err != nil { return nil, err @@ -355,12 +348,6 @@ func (s *Settings) MarshalJSON() ([]byte, error) { } } - // @todo remove with encoding/json/2 - // serialize as empty array - if copy.SuperuserIPs == nil { - copy.SuperuserIPs = []string{} - } - return json.Marshal(copy, json.Deterministic(true)) } diff --git a/tests/api.go b/tests/api.go index 499a5447..f979fc17 100644 --- a/tests/api.go +++ b/tests/api.go @@ -249,6 +249,7 @@ func (scenario *ApiScenario) test(t testing.TB) { t.Errorf("Expected status code %d, got %d", scenario.ExpectedStatus, res.StatusCode) } + // @todo consider removing in favour of synctest.Wait() if scenario.Delay > 0 { time.Sleep(scenario.Delay) } diff --git a/tools/cron/cron_test.go b/tools/cron/cron_test.go index e08954fd..018e468d 100644 --- a/tools/cron/cron_test.go +++ b/tools/cron/cron_test.go @@ -5,6 +5,7 @@ import ( "slices" "sync" "testing" + "testing/synctest" "time" ) @@ -254,63 +255,65 @@ func TestCronJobs(t *testing.T) { func TestCronStartStop(t *testing.T) { t.Parallel() - var mu sync.Mutex + synctest.Test(t, func(t *testing.T) { + var mu sync.Mutex - test1 := 0 - test2 := 0 + test1 := 0 + test2 := 0 - c := New() + c := New() - c.SetInterval(250 * time.Millisecond) + c.SetInterval(250 * time.Millisecond) + + c.Add("test1", "* * * * *", func() { + mu.Lock() + defer mu.Unlock() + test1++ + }) + + c.Add("test2", "* * * * *", func() { + mu.Lock() + defer mu.Unlock() + test2++ + }) + + // call twice Start to check if the previous ticker will be reseted + c.Start() + c.Start() + + synctest.Sleep(500 * time.Millisecond) + + // call twice Stop to ensure that the second stop is no-op + c.Stop() + c.Stop() + + expectedCalls := 2 - c.Add("test1", "* * * * *", func() { mu.Lock() - defer mu.Unlock() - test1++ - }) + if test1 != expectedCalls { + t.Fatalf("Expected %d test1, got %d", expectedCalls, test1) + } + if test2 != expectedCalls { + t.Fatalf("Expected %d test2, got %d", expectedCalls, test2) + } + mu.Unlock() + + // resume for 1 seconds + c.Start() + + synctest.Sleep(1000 * time.Millisecond) + + c.Stop() + + expectedCalls += 4 - c.Add("test2", "* * * * *", func() { mu.Lock() - defer mu.Unlock() - test2++ + if test1 != expectedCalls { + t.Fatalf("Expected %d test1, got %d", expectedCalls, test1) + } + if test2 != expectedCalls { + t.Fatalf("Expected %d test2, got %d", expectedCalls, test2) + } + mu.Unlock() }) - - // call twice Start to check if the previous ticker will be reseted - c.Start() - c.Start() - - time.Sleep(505 * time.Millisecond) // slightly larger to minimize flakiness - - // call twice Stop to ensure that the second stop is no-op - c.Stop() - c.Stop() - - expectedCalls := 2 - - mu.Lock() - if test1 != expectedCalls { - t.Fatalf("Expected %d test1, got %d", expectedCalls, test1) - } - if test2 != expectedCalls { - t.Fatalf("Expected %d test2, got %d", expectedCalls, test2) - } - mu.Unlock() - - // resume for 1 seconds - c.Start() - - time.Sleep(1005 * time.Millisecond) // slightly larger to minimize flakiness - - c.Stop() - - expectedCalls += 4 - - mu.Lock() - if test1 != expectedCalls { - t.Fatalf("Expected %d test1, got %d", expectedCalls, test1) - } - if test2 != expectedCalls { - t.Fatalf("Expected %d test2, got %d", expectedCalls, test2) - } - mu.Unlock() } diff --git a/tools/filesystem/filesystem.go b/tools/filesystem/filesystem.go index 67c66035..32185e95 100644 --- a/tools/filesystem/filesystem.go +++ b/tools/filesystem/filesystem.go @@ -131,8 +131,8 @@ func (s *System) Close() error { return s.bucket.Close() } -// OnNewWriter is a low level hook that is triggered on every writer initialization -// (aka. when attempting to create a new file with [system.NewWriter], [system.Upload], etc.). +// OnNewWriter is a low level hook that is triggered on every new writer initialization +// (aka. when attempting to create a new file with [system.NewWriter] or [system.Upload]). // // Note that currently it doesn't trigger on [System.Copy] but this may change in future releases. func (s *System) OnNewWriter() *hook.Hook[*NewWriterEvent] { @@ -144,6 +144,9 @@ func (s *System) OnNewWriter() *hook.Hook[*NewWriterEvent] { } // OnDelete is a low level hook that is triggered on every [System.Delete] call. +// +// Note that the hook doesn't fire when a file is being overwritten +// by a new one, because in that case [System.Delete] is not invoked. func (s *System) OnDelete() *hook.Hook[*DeleteEvent] { if s.onDelete == nil { s.onDelete = &hook.Hook[*DeleteEvent]{}