diff --git a/.github/workflows/ci-backend.yml b/.github/workflows/ci-backend.yml
index e311c190..5a4c8401 100644
--- a/.github/workflows/ci-backend.yml
+++ b/.github/workflows/ci-backend.yml
@@ -48,7 +48,7 @@ jobs:
- name: test and build backend
run: |
- go test -race -timeout=60s -covermode=atomic -coverprofile=$GITHUB_WORKSPACE/profile.cov_tmp ./...
+ go test -race -timeout=300s -covermode=atomic -coverprofile=$GITHUB_WORKSPACE/profile.cov_tmp ./...
cat $GITHUB_WORKSPACE/profile.cov_tmp | grep -v "_mock.go" > $GITHUB_WORKSPACE/profile.cov
go build -race ./...
working-directory: backend/app
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
index fd433e4a..bd7a7908 100644
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -51,7 +51,7 @@ jobs:
- name: test and build backend
run: |
- go test -race -timeout=120s ./...
+ go test -race -timeout=300s ./...
go build -race ./...
working-directory: backend/app
env:
diff --git a/CLAUDE.md b/CLAUDE.md
index e3dd685d..08abb1f7 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -6,7 +6,7 @@
- Build: `make backend`
- Race test: `make race_test`
- **Backend Testing**:
- - Run all tests: `cd backend/app && go test -timeout=60s -count 1 ./...`
+ - Run all tests: `cd backend/app && go test -timeout=300s -count 1 ./...`
- Run single test: `cd backend/app && go test -run TestName ./path/to/package`
- **IMPORTANT**: Run example tests: `cd backend/_example/memory_store && go test -race ./... && go build -race ./...`
- **Frontend**:
@@ -25,6 +25,22 @@
- This applies to Dependabot pull requests too: the bot updates `backend/` only, so its Go module PRs need the example tidied before they can go green.
+## Backend Test Determinism
+
+Backend tests must never depend on how fast the machine is. CI runs them under `-race` with coverage on a shared runner, so any test that assumes an operation finishes within some duration eventually fails on a rerun-and-it-passes basis.
+
+- **Wait on a condition, never on a duration.** Use `require.Eventually` / `require.EventuallyWithT` to poll for the state the assertion needs, and `require.Never` when the point is that something did *not* happen. A bare `time.Sleep` before an assertion is a defect; sleeping until a deadline you computed, as `waitPastMillisecond` does, is not.
+- **Polling closures must not touch `*testing.T`.** testify runs them on a separate goroutine, where `t.FailNow` is undefined behaviour. Assert on the `*assert.CollectT` that `EventuallyWithT` hands the closure, so the real error also lands in the failure message.
+- **Mind the rate limiter when polling over HTTP.** Route groups are capped independently and most of the caps are hard-coded in `rest.go`, out of reach of a test: `/auth/` at 2 req/s and the admin, protected and image routes at 10 req/s. Only the open-route group is settable, via `openRouteLimiter` (100 in `startupT`). Poll with the existing constants rather than a new number, `httpPoll` for anything issuing an HTTP request and `pollInterval` only for in-process or filesystem checks, or the poll manufactures the 429s it then has to interpret.
+- **When a test needs time to have passed, pin the clock input rather than waiting for it:** `os.Chtimes` for file ages, an explicit `store.Comment.Timestamp` for anything that formats a timestamp.
+- **Prefer a `testing/synctest` bubble** where the code under test has no real I/O. Inside one the clock is fake, so `time.Sleep` is instant and deterministic. `app/notify`, `app/store/service`, `app/store/image`, `app/store/engine`, `app/providers`, `app/migrator` and `_example/memory_store/accessor` already use it, and most surviving `time.Sleep` calls live in them.
+- **Helpers fail loudly.** A wait that gives up must call `t.Fatal`/`require` naming what it was waiting for, never return silently and leave the next assertion to fail with something unrelated. Because these packages run `goleak.VerifyTestMain`, a failing helper also exits the test goroutine, so anything that started a server in a goroutine must `defer cancel()` or `defer srv.Shutdown()` right after launching it; otherwise a failed readiness wait is reported as a goroutine leak rather than the failure that caused it.
+- **Take ports and paths from outside the test.** Ports come from the kernel with `net.Listen("tcp", ":0")`, files from `t.TempDir()`. `go test ./...` runs package binaries concurrently, so a number out of a fixed range or a fixed name under `/tmp` lets two of them collide.
+- **Close idle connections before shutting a test server down.** Clients built as `http.Client{Timeout: x}` share `http.DefaultTransport`, and `Shutdown` waits on their keep-alive connections until its own deadline expires.
+- **Keep the test timeout budgets aligned.** `Makefile`, `ci-backend.yml`, `release.yml` and the command above all use `-timeout=300s`; the wait helpers allow 30s per condition, so a shorter per-package budget turns a slow runner into a timeout panic instead of a readable failure.
+
+`chooseUnusedPort` and the server-start wait helpers are duplicated in `app`, `app/cmd`, `app/rest/api` and `_example/memory_store/server`. Nothing shares them today; keep the copies in step when changing one.
+
## Release Procedure
Remark42 uses two tags for each release:
diff --git a/Makefile b/Makefile
index 937d5576..366b9073 100644
--- a/Makefile
+++ b/Makefile
@@ -26,7 +26,7 @@ release:
goreleaser release --snapshot --clean --skip=publish
race_test:
- cd backend/app && go test -race -timeout=60s -count 1 ./...
+ cd backend/app && go test -race -timeout=300s -count 1 ./...
backend:
docker compose -f compose-dev-backend.yml build
diff --git a/backend/_example/memory_store/server/image_test.go b/backend/_example/memory_store/server/image_test.go
index 79f133ea..3f114811 100644
--- a/backend/_example/memory_store/server/image_test.go
+++ b/backend/_example/memory_store/server/image_test.go
@@ -115,14 +115,17 @@ func TestRPC_imgCleanupHndl(t *testing.T) {
assert.Equal(t, 1462, len(img))
assert.Equal(t, gopherPNGBytes(), img)
- // wait for image to expire
- time.Sleep(time.Millisecond * 50)
- // reset the time to cleanup
+ // age the image past the ttl used below, so the reset that follows is what keeps it on
+ // staging rather than the image simply being young
+ const stagingTTL = 500 * time.Millisecond
+ time.Sleep(stagingTTL + 100*time.Millisecond)
+
+ // reset the time to cleanup, which leaves a full ttl before it could be collected again
err = ri.ResetCleanupTimer(id)
assert.NoError(t, err)
// cleanup, should not affect the new image
- err = ri.Cleanup(context.TODO(), time.Millisecond*45)
+ err = ri.Cleanup(context.TODO(), stagingTTL)
assert.NoError(t, err)
// load after cleanup should succeed
diff --git a/backend/_example/memory_store/server/rpc_test.go b/backend/_example/memory_store/server/rpc_test.go
index a5a0a2df..c9eee5e5 100644
--- a/backend/_example/memory_store/server/rpc_test.go
+++ b/backend/_example/memory_store/server/rpc_test.go
@@ -8,7 +8,6 @@ package server
import (
"fmt"
- "math/rand"
"net"
"net/http"
"testing"
@@ -20,27 +19,31 @@ import (
"github.com/umputun/remark42/memory_store/accessor"
)
-func chooseRandomUnusedPort() (port int) {
- for range 10 {
- port = 40000 + int(rand.Int31n(10000))
- if ln, err := net.Listen("tcp", fmt.Sprintf(":%d", port)); err == nil {
- _ = ln.Close()
- break
- }
- }
+// chooseUnusedPort asks the kernel for a free port from the ephemeral range, so concurrently
+// running package test binaries never land on the same number
+func chooseUnusedPort(t *testing.T) int {
+ t.Helper()
+ ln, err := net.Listen("tcp", ":0")
+ require.NoError(t, err, "no free port available")
+ port := ln.Addr().(*net.TCPAddr).Port
+ require.NoError(t, ln.Close())
return port
}
-func waitForHTTPServerStart(port int) {
- // wait for up to 3 seconds for server to start before returning it
+// waitForHTTPServerStart blocks until the server on port answers, failing the test naming the
+// port if it never does
+func waitForHTTPServerStart(t *testing.T, port int) {
+ t.Helper()
client := http.Client{Timeout: time.Second}
- for range 300 {
- time.Sleep(time.Millisecond * 10)
- if resp, err := client.Get(fmt.Sprintf("http://localhost:%d", port)); err == nil {
- _ = resp.Body.Close()
- return
+ defer client.CloseIdleConnections()
+ require.Eventually(t, func() bool {
+ resp, err := client.Get(fmt.Sprintf("http://localhost:%d", port))
+ if err != nil {
+ return false
}
- }
+ _ = resp.Body.Close()
+ return true
+ }, 30*time.Second, 10*time.Millisecond, "http server on port %d didn't start", port)
}
func prepTestStore(t *testing.T) (port int, teardown func()) {
@@ -61,14 +64,17 @@ func prepTestStore(t *testing.T) (port int, teardown func()) {
admRecDisabled.Enabled = false
adm.Set("test-site-disabled", admRecDisabled)
- port = chooseRandomUnusedPort()
+ port = chooseUnusedPort(t)
go func() {
_ = s.Run(port)
}()
- waitForHTTPServerStart(port)
+ waitForHTTPServerStart(t, port)
return port, func() {
+ // every test client here uses http.DefaultTransport, so their keep-alive connections
+ // sit in one shared pool; Shutdown waits on them and hits its own 5s deadline otherwise
+ http.DefaultTransport.(*http.Transport).CloseIdleConnections()
require.NoError(t, s.Shutdown())
}
}
diff --git a/backend/app/cmd/import_test.go b/backend/app/cmd/import_test.go
index c608459f..2708dc77 100644
--- a/backend/app/cmd/import_test.go
+++ b/backend/app/cmd/import_test.go
@@ -8,7 +8,6 @@ import (
"net/http/httptest"
"strings"
"testing"
- "time"
log "github.com/go-pkgz/lgr"
"github.com/jessevdk/go-flags"
@@ -133,15 +132,14 @@ func TestImport_ExecuteFailed(t *testing.T) {
}
func TestImport_ExecuteTimeout(t *testing.T) {
- ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ ts := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
assert.Equal(t, r.URL.Path, "/api/v1/admin/import")
assert.Equal(t, "POST", r.Method)
body, err := io.ReadAll(r.Body)
assert.NoError(t, err)
assert.Equal(t, "blah\nblah2\n12345678\n", string(body))
- time.Sleep(500 * time.Millisecond)
- fmt.Fprintln(w, "some response")
- fmt.Fprintln(w, string(body))
+ // hold the response until the client gives up on its own timeout
+ <-r.Context().Done()
}))
defer ts.Close()
diff --git a/backend/app/cmd/server_test.go b/backend/app/cmd/server_test.go
index cd8da5d9..7f216622 100644
--- a/backend/app/cmd/server_test.go
+++ b/backend/app/cmd/server_test.go
@@ -5,7 +5,6 @@ import (
"crypto/tls"
"fmt"
"io"
- "math/rand"
"net"
"net/http"
"os"
@@ -25,15 +24,33 @@ import (
"github.com/stretchr/testify/require"
)
+const (
+ // budget for a server to bind and answer, generous enough for a loaded CI runner
+ serverStartTimeout = 30 * time.Second
+ serverStartPoll = 10 * time.Millisecond
+
+ // budget for a server to stop once asked. tight enough to catch a shutdown that hangs,
+ // loose enough not to depend on how loaded the runner is
+ serverStopTimeout = 10 * time.Second
+
+ // connect budget for a single probe. kept off the poll interval so a slow loopback connect
+ // on a loaded runner does not look like a server that is not listening
+ probeDialTimeout = time.Second
+
+ // the /auth/ group is limited to 2 req/s, so retries sit at its refill interval rather than
+ // above it, which would only manufacture more 429s
+ authRetryPoll = 500 * time.Millisecond
+)
+
func TestServerApp(t *testing.T) {
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
app, ctx, cancel := prepServerApp(t, func(o ServerCommand) ServerCommand {
o.Port = port
return o
})
go func() { _ = app.run(ctx) }()
- waitForHTTPServerStart(port)
+ waitForHTTPServerStart(t, port)
// send ping
resp, err := http.Get(fmt.Sprintf("http://localhost:%d/api/v1/ping", port))
@@ -68,7 +85,7 @@ func TestServerApp(t *testing.T) {
}
func TestServerApp_DevMode(t *testing.T) {
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
app, ctx, cancel := prepServerApp(t, func(o ServerCommand) ServerCommand {
o.Port = port
o.AdminPasswd = "password"
@@ -77,7 +94,7 @@ func TestServerApp_DevMode(t *testing.T) {
})
go func() { _ = app.run(ctx) }()
- waitForHTTPServerStart(port)
+ waitForHTTPServerStart(t, port)
providers := app.restSrv.Authenticator.Providers()
require.Equal(t, 11+1, len(providers), "extra auth provider")
@@ -97,7 +114,7 @@ func TestServerApp_DevMode(t *testing.T) {
}
func TestServerApp_CustomOAuthProvider(t *testing.T) {
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
app, ctx, cancel := prepServerApp(t, func(o ServerCommand) ServerCommand {
o.Port = port
o.Auth.Custom.Name = "oidc"
@@ -110,7 +127,7 @@ func TestServerApp_CustomOAuthProvider(t *testing.T) {
})
go func() { _ = app.run(ctx) }()
- waitForHTTPServerStart(port)
+ waitForHTTPServerStart(t, port)
providers := app.restSrv.Authenticator.Providers()
require.Equal(t, 11+1, len(providers), "extra auth provider")
@@ -121,7 +138,7 @@ func TestServerApp_CustomOAuthProvider(t *testing.T) {
}
func TestServerApp_AnonMode(t *testing.T) {
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
app, ctx, cancel := prepServerApp(t, func(o ServerCommand) ServerCommand {
o.Port = port
o.Auth.Anonymous = true
@@ -129,7 +146,7 @@ func TestServerApp_AnonMode(t *testing.T) {
})
go func() { _ = app.run(ctx) }()
- waitForHTTPServerStart(port)
+ waitForHTTPServerStart(t, port)
providers := app.restSrv.Authenticator.Providers()
require.Equal(t, 11+1, len(providers), "extra auth provider for anon")
@@ -148,8 +165,7 @@ func TestServerApp_AnonMode(t *testing.T) {
assert.Equal(t, "pong", string(body))
// try to login with good name
- resp, err = client.Get(fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=blah123&aud=remark", port))
- require.NoError(t, err)
+ resp = getRetryThrottled(t, &client, fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=blah123&aud=remark", port))
defer resp.Body.Close()
assert.Equal(t, http.StatusOK, resp.StatusCode)
@@ -168,57 +184,43 @@ func TestServerApp_AnonMode(t *testing.T) {
assert.Equal(t, http.StatusCreated, resp.StatusCode)
// try to login with non-latin name
- time.Sleep(time.Second)
- resp, err = client.Get(fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=Раз_Два%20%20Три_34567&aud=remark", port))
- require.NoError(t, err)
+ nonLatin := fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=Раз_Два%20%20Три_34567&aud=remark", port)
+ resp = getRetryThrottled(t, &client, nonLatin)
defer resp.Body.Close()
assert.Equal(t, http.StatusOK, resp.StatusCode)
// try to login with bad name
- time.Sleep(time.Second)
- resp, err = client.Get(fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=**blah123&aud=remark", port))
- require.NoError(t, err)
+ resp = getRetryThrottled(t, &client, fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=**blah123&aud=remark", port))
defer resp.Body.Close()
assert.Equal(t, http.StatusForbidden, resp.StatusCode)
// try to login with short name
- time.Sleep(time.Second)
- resp, err = client.Get(fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=bl%%20%%20&aud=remark", port))
- require.NoError(t, err)
+ resp = getRetryThrottled(t, &client, fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=bl%%20%%20&aud=remark", port))
defer resp.Body.Close()
assert.Equal(t, http.StatusForbidden, resp.StatusCode)
// try to login with name what have space in prefix
- time.Sleep(time.Second)
- resp, err = client.Get(fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=%%20somebody&aud=remark", port))
- require.NoError(t, err)
+ resp = getRetryThrottled(t, &client, fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=%%20somebody&aud=remark", port))
defer resp.Body.Close()
assert.Equal(t, http.StatusForbidden, resp.StatusCode)
// try to login with name what have space in suffix
- time.Sleep(time.Second)
- resp, err = client.Get(fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=somebody%%20&aud=remark", port))
- require.NoError(t, err)
+ resp = getRetryThrottled(t, &client, fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=somebody%%20&aud=remark", port))
defer resp.Body.Close()
assert.Equal(t, http.StatusForbidden, resp.StatusCode)
// try to login with long name
- time.Sleep(time.Second)
ln := strings.Repeat("x", 65)
- resp, err = client.Get(fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=%s&aud=remark", port, ln))
- require.NoError(t, err)
+ resp = getRetryThrottled(t, &client, fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=%s&aud=remark", port, ln))
defer resp.Body.Close()
assert.Equal(t, http.StatusForbidden, resp.StatusCode)
// try to login with admin name
- time.Sleep(time.Second)
- resp, err = client.Get(fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=umpUtun&aud=remark", port))
- require.NoError(t, err)
+ resp = getRetryThrottled(t, &client, fmt.Sprintf("http://localhost:%d/auth/anonymous/login?user=umpUtun&aud=remark", port))
defer resp.Body.Close()
assert.Equal(t, http.StatusOK, resp.StatusCode)
// try to add a comment as anonymous with admin name
- time.Sleep(time.Second)
req, err = http.NewRequest("POST", fmt.Sprintf("http://localhost:%d/api/v1/comment?site=remark", port),
strings.NewReader(`{"text": "test 123", "locator":{"url": "https://radio-t.com/blah1", "site": "remark"}}`))
require.NoError(t, err)
@@ -250,12 +252,12 @@ func getAuthFromCookie(t *testing.T, app *serverApp, resp *http.Response) (tkn s
func TestServerApp_WithSSL(t *testing.T) {
opts := ServerCommand{}
- sslPort := chooseRandomUnusedPort()
+ sslPort := chooseUnusedPort(t)
opts.SetCommon(CommonOpts{RemarkURL: fmt.Sprintf("https://localhost:%d", sslPort), SharedSecret: "123456"})
// prepare options
p := flags.NewParser(&opts, flags.Default)
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
_, err := p.ParseArgs([]string{"--admin-passwd=password", "--port=" + strconv.Itoa(port), "--store.bolt.path=/tmp/xyz", "--backup=/tmp",
"--avatar.type=bolt", "--avatar.bolt.file=/tmp/ava-test.db",
"--ssl.type=static", "--ssl.cert=testdata/cert.pem", "--ssl.key=testdata/key.pem",
@@ -270,8 +272,9 @@ func TestServerApp_WithSSL(t *testing.T) {
require.NoError(t, err)
ctx, cancel := context.WithCancel(context.Background())
+ defer cancel() // this context is not the one createAppFromCmd registers for cleanup
go func() { _ = app.run(ctx) }()
- waitForHTTPSServerStart(sslPort)
+ waitForServerStart(t, sslPort, port) // the redirect check below uses the plain http port
client := http.Client{
// prevent http redirect
@@ -312,7 +315,7 @@ func TestServerApp_WithRemote(t *testing.T) {
// prepare options
p := flags.NewParser(&opts, flags.Default)
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
_, err := p.ParseArgs([]string{"--admin-passwd=password", "--cache.type=none",
"--store.type=rpc", "--store.rpc.api=http://127.0.0.1",
"--port=" + strconv.Itoa(port), "--avatar.fs.path=/tmp",
@@ -326,8 +329,9 @@ func TestServerApp_WithRemote(t *testing.T) {
require.NoError(t, err)
ctx, cancel := context.WithCancel(context.Background())
+ defer cancel() // this context is not the one createAppFromCmd registers for cleanup
go func() { _ = app.run(ctx) }()
- waitForHTTPServerStart(port)
+ waitForHTTPServerStart(t, port)
// send ping
resp, err := http.Get(fmt.Sprintf("http://localhost:%d/api/v1/ping", port))
@@ -516,34 +520,117 @@ func TestServerApp_InvalidCustomOAuthProviderName(t *testing.T) {
}
func TestServerApp_Shutdown(t *testing.T) {
+ port := chooseUnusedPort(t)
app, ctx, cancel := prepServerApp(t, func(o ServerCommand) ServerCommand {
- o.Port = chooseRandomUnusedPort()
+ o.Port = port
return o
})
- time.AfterFunc(100*time.Millisecond, func() {
- cancel()
- })
- st := time.Now()
- err := app.run(ctx)
- assert.NoError(t, err)
- assert.True(t, time.Since(st).Seconds() < 1, "should take about 100msec")
+
+ // cancel once the server actually answers, so the test measures shutdown and not startup.
+ // the deferred cancel also covers a failed wait, keeping app.run from racing the next test
+ errCh := make(chan error, 1)
+ go func() { errCh <- app.run(ctx) }()
+ defer cancel()
+ waitForHTTPServerStart(t, port)
+ cancel()
+
+ select {
+ case err := <-errCh:
+ assert.NoError(t, err)
+ case <-time.After(serverStopTimeout):
+ t.Fatal("server app did not stop after context cancel")
+ }
app.Wait()
}
-func TestServerApp_MainSignal(t *testing.T) {
- done := make(chan struct{})
- go func() {
- <-done
- time.Sleep(250 * time.Millisecond)
- err := syscall.Kill(syscall.Getpid(), syscall.SIGTERM)
+// TestServerApp_ClaimsUpd covers the hook the authenticator runs on every token mint, refresh
+// included: it stamps admin, blocked and email onto the claims and blocks impersonation of a
+// restricted name. Calling the updater directly keeps it independent of when a token expires.
+func TestServerApp_ClaimsUpd(t *testing.T) {
+ port := chooseUnusedPort(t)
+ app, ctx, cancel := prepServerApp(t, func(o ServerCommand) ServerCommand {
+ o.Port = port
+ return o
+ })
+
+ // the app owns stores and services that only run closes, so it goes through the usual
+ // lifecycle here rather than being built and abandoned
+ go func() { _ = app.run(ctx) }()
+ waitForHTTPServerStart(t, port)
+ defer app.Wait()
+ defer cancel()
+
+ upd := app.restSrv.Authenticator.TokenService().ClaimsUpd
+ require.NotNil(t, upd, "claims updater wired into the token service")
+
+ claimsFor := func(id, name string) token.Claims {
+ return token.Claims{
+ RegisteredClaims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"remark"}},
+ User: &token.User{ID: id, Name: name},
+ }
+ }
+
+ t.Run("plain user gets no attributes", func(t *testing.T) {
+ res := upd.Update(claimsFor("provider1_dev", "developer"))
+ assert.False(t, res.User.IsAdmin(), "not an admin")
+ assert.False(t, res.User.BoolAttr("blocked"), "not blocked")
+ assert.Empty(t, res.User.Email, "no email on file")
+ })
+
+ t.Run("admin from the admin store", func(t *testing.T) {
+ res := upd.Update(claimsFor("id1", "admin one"))
+ assert.True(t, res.User.IsAdmin(), "id1 is listed as admin")
+ })
+
+ t.Run("blocked user carries the blocked attribute", func(t *testing.T) {
+ require.NoError(t, app.restSrv.DataService.SetBlock("remark", "blocked_user", true, time.Hour))
+ res := upd.Update(claimsFor("blocked_user", "blocked"))
+ assert.True(t, res.User.BoolAttr("blocked"), "block is reflected on refresh")
+ })
+
+ t.Run("email is read from the store", func(t *testing.T) {
+ _, err := app.restSrv.DataService.SetUserEmail("remark", "with_email", "user@example.com")
require.NoError(t, err)
- }()
+ res := upd.Update(claimsFor("with_email", "someone"))
+ assert.Equal(t, "user@example.com", res.User.Email)
+ })
+
+ t.Run("anonymous impersonating a restricted name is blocked", func(t *testing.T) {
+ res := upd.Update(claimsFor("anonymous_x", " UmpUtun "))
+ assert.True(t, res.User.BoolAttr("blocked"), "restricted name matched case and space insensitively")
+ })
+
+ t.Run("email user impersonating a restricted name is blocked", func(t *testing.T) {
+ res := upd.Update(claimsFor("email_x", "bobuk"))
+ assert.True(t, res.User.BoolAttr("blocked"))
+ })
+
+ t.Run("regular user may carry a restricted name", func(t *testing.T) {
+ res := upd.Update(claimsFor("provider1_someone", "umputun"))
+ assert.False(t, res.User.BoolAttr("blocked"), "only anonymous and email logins are checked")
+ })
+
+ t.Run("claims without a user pass through", func(t *testing.T) {
+ res := upd.Update(token.Claims{RegisteredClaims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"remark"}}})
+ assert.Nil(t, res.User)
+ })
+
+ t.Run("claims without exactly one audience pass through", func(t *testing.T) {
+ c := claimsFor("id1", "admin one")
+ c.Audience = jwt.ClaimStrings{"remark", "second"}
+ res := upd.Update(c)
+ assert.False(t, res.User.IsAdmin(), "attributes need a single audience to resolve the site")
+ })
+}
+
+func TestServerApp_MainSignal(t *testing.T) {
+ sigErr := make(chan error, 1)
s := ServerCommand{}
s.SetCommon(CommonOpts{RemarkURL: "https://demo.remark42.com", SharedSecret: "123456"})
p := flags.NewParser(&s, flags.Default)
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
args := []string{"test", "--store.bolt.path=/tmp/xyz", "--backup=/tmp", "--avatar.type=bolt",
"--avatar.bolt.file=/tmp/ava-test.db", "--port=" + strconv.Itoa(port), "--image.fs.path=/tmp"}
defer os.Remove("/tmp/xyz")
@@ -551,15 +638,26 @@ func TestServerApp_MainSignal(t *testing.T) {
defer os.Remove("/tmp/ava-test.db")
_, err := p.ParseArgs(args)
require.NoError(t, err)
- st := time.Now()
- close(done)
+ // the signal goes out only once the server answers: SIGTERM landing before the handler is
+ // installed kills the test process, so a wait that timed out reports instead of sending it
+ go func() {
+ started := waitForServerPort(port, serverStartTimeout)
+ // signal either way: Execute blocks until it gets one, so bailing out here would hang
+ // the test until the package timeout instead of failing with the reason
+ killErr := syscall.Kill(syscall.Getpid(), syscall.SIGTERM)
+ if !started {
+ killErr = fmt.Errorf("server on port %d didn't start", port)
+ }
+ sigErr <- killErr
+ }()
+
err = s.Execute(args)
assert.NoError(t, err, "execute should be without errors")
- assert.True(t, time.Since(st).Seconds() < 5, "should take under five sec", time.Since(st).Seconds())
+ require.NoError(t, <-sigErr, "SIGTERM not delivered")
}
func TestServerApp_RunCanceledBeforeRESTStart(t *testing.T) {
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
app, ctx, cancel := prepServerApp(t, func(o ServerCommand) ServerCommand {
o.Port = port
return o
@@ -569,17 +667,19 @@ func TestServerApp_RunCanceledBeforeRESTStart(t *testing.T) {
errCh := make(chan error, 1)
go func() { errCh <- app.run(ctx) }()
+ // the budget is generous on purpose: the assertion is that run exits rather than hangs, and
+ // store construction can take a while on a loaded runner
select {
case err := <-errCh:
require.NoError(t, err)
app.Wait()
- case <-time.After(time.Second):
- waitForHTTPServerStart(port)
+ case <-time.After(serverStartTimeout):
+ waitForHTTPServerStart(t, port)
app.restSrv.Shutdown()
select {
case <-errCh:
app.Wait()
- case <-time.After(time.Second):
+ case <-time.After(serverStartTimeout):
t.Fatal("server app did not stop after forced REST shutdown")
}
t.Fatal("server app should exit when context is canceled before REST server starts")
@@ -747,24 +847,25 @@ func Test_ACMEEmail(t *testing.T) {
}
func TestServerAuthHooks(t *testing.T) {
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
app, ctx, cancel := prepServerApp(t, func(o ServerCommand) ServerCommand {
o.Port = port
return o
})
go func() { _ = app.run(ctx) }()
- waitForHTTPServerStart(port)
+ waitForHTTPServerStart(t, port)
- // make a token for user dev
+ // make a token for user dev. nothing here checks expiry, so the lifetime only has to
+ // outlast the whole test
tkService := app.restSrv.Authenticator.TokenService()
- tkService.TokenDuration = time.Second
+ tkService.TokenDuration = time.Hour
claims := token.Claims{
RegisteredClaims: jwt.RegisteredClaims{
Audience: jwt.ClaimStrings{"remark"},
Issuer: "remark",
- ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Second)),
+ ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour)),
NotBefore: jwt.NewNumericDate(time.Now().Add(-1 * time.Minute)),
},
User: &token.User{
@@ -867,8 +968,7 @@ func TestServerAuthHooks(t *testing.T) {
body, err = io.ReadAll(resp.Body)
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
- assert.True(t, resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusUnauthorized,
- "blocked user can't post, \n"+tk+"\n"+string(body))
+ assert.Equal(t, http.StatusForbidden, resp.StatusCode, "blocked user can't post, \n"+tk+"\n"+string(body))
cancel()
app.Wait()
@@ -968,40 +1068,79 @@ func Test_getAllowedRedirectHosts(t *testing.T) {
}
}
-func chooseRandomUnusedPort() (port int) {
- for range 10 {
- port = 40000 + int(rand.Int31n(10000))
- if ln, err := net.Listen("tcp", fmt.Sprintf(":%d", port)); err == nil {
- _ = ln.Close()
- break
- }
- }
+// chooseUnusedPort asks the kernel for a free port from the ephemeral range, which makes a
+// collision between concurrently running package test binaries very unlikely
+func chooseUnusedPort(t *testing.T) int {
+ t.Helper()
+ ln, err := net.Listen("tcp", ":0")
+ require.NoError(t, err, "no free port available")
+ port := ln.Addr().(*net.TCPAddr).Port
+ require.NoError(t, ln.Close())
return port
}
-func waitForHTTPServerStart(port int) {
- // wait for up to 3 seconds for server to start before returning it
+// waitForHTTPServerStart blocks until the server on port answers, failing the test naming the
+// port if it never does
+func waitForHTTPServerStart(t *testing.T, port int) {
+ t.Helper()
client := http.Client{Timeout: time.Second}
defer client.CloseIdleConnections()
- for range 300 {
- time.Sleep(time.Millisecond * 10)
- if resp, err := client.Get(fmt.Sprintf("http://localhost:%d", port)); err == nil {
- _ = resp.Body.Close()
- return
+ require.Eventually(t, func() bool {
+ resp, err := client.Get(fmt.Sprintf("http://localhost:%d", port))
+ if err != nil {
+ return false
}
+ _ = resp.Body.Close()
+ return true
+ }, serverStartTimeout, serverStartPoll, "http server on port %d didn't start", port)
+}
+
+// waitForServerStart blocks until something accepts on every listed port, failing the test
+// naming the port that never came up
+func waitForServerStart(t *testing.T, ports ...int) {
+ t.Helper()
+ for _, port := range ports {
+ require.True(t, waitForServerPort(port, serverStartTimeout), "server on port %d didn't start", port)
}
}
-func waitForHTTPSServerStart(port int) {
- // wait for up to 3 seconds for HTTPS server to start
- for range 300 {
- time.Sleep(time.Millisecond * 10)
- conn, _ := net.DialTimeout("tcp", fmt.Sprintf("localhost:%d", port), time.Millisecond*10)
- if conn != nil {
- _ = conn.Close()
- break
+// getRetryThrottled issues a GET and retries while the auth routes answer 429, since the /auth/
+// group is limited to 2 req/s and this test logs in more often than that. a transport error is
+// retried a couple of times and then reported as itself, so a dead server is not read as throttling
+func getRetryThrottled(t *testing.T, client *http.Client, url string) *http.Response {
+ t.Helper()
+ const transportRetries = 2
+ errCount := 0
+ for deadline := time.Now().Add(serverStartTimeout); time.Now().Before(deadline); time.Sleep(authRetryPoll) {
+ r, err := client.Get(url)
+ if err != nil {
+ errCount++
+ require.LessOrEqual(t, errCount, transportRetries, "request to %s failed: %v", url, err)
+ continue
}
+ if r.StatusCode == http.StatusTooManyRequests {
+ _ = r.Body.Close()
+ continue
+ }
+ return r
}
+ t.Fatalf("request to %s kept being rate limited", url)
+ return nil
+}
+
+// waitForServerPort blocks until something accepts on port, reporting whether it came up.
+// unlike the require-based helpers it is safe to call off the test goroutine.
+func waitForServerPort(port int, timeout time.Duration) bool {
+ deadline := time.Now().Add(timeout)
+ for time.Now().Before(deadline) {
+ conn, err := net.DialTimeout("tcp", fmt.Sprintf("localhost:%d", port), probeDialTimeout)
+ if err == nil {
+ _ = conn.Close()
+ return true
+ }
+ time.Sleep(serverStartPoll)
+ }
+ return false
}
func prepServerApp(t *testing.T, fn func(o ServerCommand) ServerCommand) (*serverApp, context.Context, context.CancelFunc) {
@@ -1064,6 +1203,9 @@ func prepServerApp(t *testing.T, fn func(o ServerCommand) ServerCommand) (*serve
func createAppFromCmd(t *testing.T, cmd ServerCommand) (*serverApp, context.Context, context.CancelFunc) {
ctx, cancel := context.WithCancel(context.Background())
+ // a require in a readiness wait exits the test goroutine, so without this an app started in
+ // a goroutine would never be stopped and goleak would report it instead of the failure
+ t.Cleanup(cancel)
app, err := cmd.newServerApp(ctx)
require.NoError(t, err)
return app, ctx, cancel
@@ -1073,8 +1215,14 @@ func TestMain(m *testing.M) {
// ignore is added only for GitHub Actions, can't reproduce locally
goleak.VerifyTestMain(
m,
+ // the shutdown goroutine in serverApp.run is not joined by Wait, and Rest.Shutdown gives
+ // httpServer.Shutdown a second, which can outlast goleak's retry budget on a loaded runner
goleak.IgnoreTopFunction("net/http.(*Server).Shutdown"),
// this will be fixed in https://github.com/hashicorp/golang-lru/issues/159
goleak.IgnoreTopFunction("github.com/hashicorp/golang-lru/v2/expirable.NewLRU[...].func1"),
+ // regexp2, pulled in by chroma for syntax highlighting, keeps one shared clock goroutine
+ // alive for up to a second after the last match with a timeout, sleeping in 100ms ticks.
+ // it ends on its own, but a binary that finishes inside that window is reported as leaking
+ goleak.IgnoreAnyFunction("github.com/dlclark/regexp2/v2.runClock"),
)
}
diff --git a/backend/app/main_test.go b/backend/app/main_test.go
index 9e968ff5..d9ba1f8b 100644
--- a/backend/app/main_test.go
+++ b/backend/app/main_test.go
@@ -3,7 +3,6 @@ package main
import (
"fmt"
"io"
- "math/rand"
"net"
"net/http"
"net/http/httptest"
@@ -25,7 +24,7 @@ func Test_Main(t *testing.T) {
require.NoError(t, err)
defer os.RemoveAll(dir)
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
os.Args = []string{"test", "server", "--secret=123456", "--store.bolt.path=" + dir, "--backup=/tmp",
"--avatar.fs.path=" + dir, "--port=" + strconv.Itoa(port), "--url=https://demo.remark42.com", "--dbg", "--notify.type=none"}
@@ -48,7 +47,7 @@ func Test_Main(t *testing.T) {
<-finished
}()
- waitForHTTPServerStart(port)
+ waitForHTTPServerStart(t, port)
resp, err := http.Get(fmt.Sprintf("http://localhost:%d/api/v1/ping", port))
require.NoError(t, err)
defer resp.Body.Close()
@@ -63,9 +62,9 @@ func TestMain_WithWebhook(t *testing.T) {
require.NoError(t, err)
defer os.RemoveAll(dir)
- var webhookSent int32
+ var webhookSent atomic.Int32
ts := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
- atomic.StoreInt32(&webhookSent, 1)
+ webhookSent.Store(1)
assert.Equal(t, "application/json", r.Header.Get("Content-Type"))
b, e := io.ReadAll(r.Body)
@@ -76,7 +75,7 @@ func TestMain_WithWebhook(t *testing.T) {
}))
defer ts.Close()
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
os.Args = []string{"test", "server", "--secret=123456", "--store.bolt.path=" + dir, "--backup=/tmp",
"--avatar.fs.path=" + dir, "--port=" + strconv.Itoa(port), "--url=https://demo.remark42.com", "--dbg",
"--admin-passwd=password", "--site=remark", "--notify.admins=webhook"}
@@ -107,7 +106,7 @@ func TestMain_WithWebhook(t *testing.T) {
<-finished
}()
- waitForHTTPServerStart(port)
+ waitForHTTPServerStart(t, port)
resp, err := http.Post(fmt.Sprintf("http://admin:password@localhost:%d/api/v1/comment", port), "",
strings.NewReader(`{"text": "env test", "locator":{"url": "https://radio-t.com", "site": "remark"}}`))
@@ -117,8 +116,8 @@ func TestMain_WithWebhook(t *testing.T) {
// wait for webhook to be sent before shutting down
assert.Eventually(t, func() bool {
- return atomic.LoadInt32(&webhookSent) == int32(1)
- }, time.Second, 100*time.Millisecond, "webhook was not sent")
+ return webhookSent.Load() == int32(1)
+ }, 30*time.Second, 10*time.Millisecond, "webhook was not sent")
}
func TestGetDump(t *testing.T) {
@@ -129,37 +128,46 @@ func TestGetDump(t *testing.T) {
t.Logf("\n dump: %s", dump)
}
-func chooseRandomUnusedPort() (port int) {
- for range 10 {
- port = 40000 + int(rand.Int31n(10000))
- if ln, err := net.Listen("tcp", fmt.Sprintf(":%d", port)); err == nil {
- _ = ln.Close()
- break
- }
- }
+// chooseUnusedPort asks the kernel for a free port from the ephemeral range, so concurrently
+// running package test binaries never land on the same number
+func chooseUnusedPort(t *testing.T) int {
+ t.Helper()
+ ln, err := net.Listen("tcp", ":0")
+ require.NoError(t, err, "no free port available")
+ port := ln.Addr().(*net.TCPAddr).Port
+ require.NoError(t, ln.Close())
return port
}
-func waitForHTTPServerStart(port int) {
- // wait for up to 10 seconds for server to start before returning it
+// waitForHTTPServerStart blocks until the server on port answers, failing the test naming the
+// port if it never does
+func waitForHTTPServerStart(t *testing.T, port int) {
+ t.Helper()
client := http.Client{Timeout: time.Second}
defer client.CloseIdleConnections()
- for range 100 {
- time.Sleep(time.Millisecond * 100)
- if resp, err := client.Get(fmt.Sprintf("http://localhost:%d", port)); err == nil {
- _ = resp.Body.Close()
- return
+ require.Eventually(t, func() bool {
+ resp, err := client.Get(fmt.Sprintf("http://localhost:%d", port))
+ if err != nil {
+ return false
}
- }
+ _ = resp.Body.Close()
+ return true
+ }, 30*time.Second, 10*time.Millisecond, "http server on port %d didn't start", port)
}
func TestMain(m *testing.M) {
// both ignores are for leaks which are detected locally
goleak.VerifyTestMain(
m,
- goleak.IgnoreTopFunction("github.com/umputun/remark42/backend/app.init.0.func1"),
+ // the shutdown goroutine in serverApp.run is not joined by Wait, and Rest.Shutdown gives
+ // httpServer.Shutdown a second, which can outlast goleak's retry budget on a loaded runner
goleak.IgnoreTopFunction("net/http.(*Server).Shutdown"),
+ goleak.IgnoreTopFunction("github.com/umputun/remark42/backend/app.init.0.func1"),
// this will be fixed in https://github.com/hashicorp/golang-lru/issues/159
goleak.IgnoreTopFunction("github.com/hashicorp/golang-lru/v2/expirable.NewLRU[...].func1"),
+ // regexp2, pulled in by chroma for syntax highlighting, keeps one shared clock goroutine
+ // alive for up to a second after the last match with a timeout, sleeping in 100ms ticks.
+ // it ends on its own, but a binary that finishes inside that window is reported as leaking
+ goleak.IgnoreAnyFunction("github.com/dlclark/regexp2/v2.runClock"),
)
}
diff --git a/backend/app/migrator/native.go b/backend/app/migrator/native.go
index 1fe81fcd..3f91776a 100644
--- a/backend/app/migrator/native.go
+++ b/backend/app/migrator/native.go
@@ -6,6 +6,7 @@ import (
"encoding/json"
"fmt"
"io"
+ "slices"
"sync/atomic"
log "github.com/go-pkgz/lgr"
@@ -46,8 +47,8 @@ func (n *Native) Export(w io.Writer, siteID string) (size int, err error) {
log.Printf("[DEBUG] exporting %d topics", len(topics))
commentsCount := 0
- for i := len(topics) - 1; i >= 0; i-- { // topics from List sorted in opposite direction
- topic := topics[i]
+ for _, topic := range slices.Backward(topics) { // topics from List sorted in opposite direction
+
comments, e := n.DataStore.Find(store.Locator{SiteID: siteID, URL: topic.URL}, "time", adminUser)
if e != nil {
return commentsCount, e
diff --git a/backend/app/notify/notify.go b/backend/app/notify/notify.go
index d8bdddbe..11473cdf 100644
--- a/backend/app/notify/notify.go
+++ b/backend/app/notify/notify.go
@@ -19,7 +19,7 @@ type Service struct {
queue chan Request
verificationQueue chan VerificationRequest
- closed uint32 // non-zero means closed. uses uint instead of bool for atomic
+ closed atomic.Uint32 // non-zero means closed. uses uint instead of bool for atomic
ctx context.Context
cancel context.CancelFunc
}
@@ -83,7 +83,7 @@ func NewService(dataService Store, size int, destinations ...Destination) *Servi
// Submit Request to internal channel if not busy, drop if can't send
func (s *Service) Submit(req Request) {
- if len(s.destinations) == 0 || atomic.LoadUint32(&s.closed) != 0 {
+ if len(s.destinations) == 0 || s.closed.Load() != 0 {
return
}
if s.dataService != nil && req.Comment.ParentID != "" {
@@ -130,7 +130,7 @@ func (s *Service) getNotificationTargets(
// SubmitVerification to internal channel if not busy, drop if can't send
func (s *Service) SubmitVerification(req VerificationRequest) {
- if len(s.destinations) == 0 || atomic.LoadUint32(&s.closed) != 0 {
+ if len(s.destinations) == 0 || s.closed.Load() != 0 {
return
}
select {
@@ -155,7 +155,7 @@ func (s *Service) Close() {
s.cancel()
<-s.ctx.Done()
}
- atomic.StoreUint32(&s.closed, 1)
+ s.closed.Store(1)
}
func (s *Service) do() {
diff --git a/backend/app/notify/notify_test.go b/backend/app/notify/notify_test.go
index 2a12fdd8..d0f162b9 100644
--- a/backend/app/notify/notify_test.go
+++ b/backend/app/notify/notify_test.go
@@ -2,7 +2,6 @@ package notify
import (
"fmt"
- "sync/atomic"
"testing"
"testing/synctest"
@@ -298,7 +297,7 @@ func TestService_Nop(t *testing.T) {
s := NopService
s.Submit(Request{Comment: store.Comment{}})
s.Close()
- assert.Equal(t, uint32(1), atomic.LoadUint32(&s.closed))
+ assert.Equal(t, uint32(1), s.closed.Load())
}
type mockStore struct {
diff --git a/backend/app/rest/api/admin_test.go b/backend/app/rest/api/admin_test.go
index 56ac5a26..ceac3f3c 100644
--- a/backend/app/rest/api/admin_test.go
+++ b/backend/app/rest/api/admin_test.go
@@ -62,7 +62,7 @@ func TestAdmin_Delete(t *testing.T) {
fmt.Sprintf("%s/api/v1/admin/comment/%s?site=remark42&url=https://radio-t.com/blah", ts.URL, id1), http.NoBody)
require.NoError(t, err)
requireAdminOnly(t, req)
- resp, err = sendReq(t, req, adminUmputunToken)
+ resp, err = sendReq(req, adminUmputunToken)
assert.NoError(t, err)
assert.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusOK, resp.StatusCode)
@@ -75,14 +75,22 @@ func TestAdmin_Delete(t *testing.T) {
assert.Equal(t, "", cr.Text)
assert.True(t, cr.Deleted)
- time.Sleep(250 * time.Millisecond)
- // check last comments updated
- res, code = get(t, ts.URL+"/api/v1/last/2?site=remark42")
- assert.Equal(t, http.StatusOK, code)
- comments = []store.Comment{}
- err = json.Unmarshal([]byte(res), &comments)
- assert.NoError(t, err)
- assert.Equal(t, 1, len(comments), "should have 1 comments")
+ // the last-comments list refreshes asynchronously after the delete. the polling closure runs
+ // off the test goroutine, so it asserts on the CollectT it is handed rather than on t, which
+ // also puts the real transport or decode error in the failure message
+ pollClient := http.Client{Timeout: waitTimeout}
+ defer pollClient.CloseIdleConnections()
+ require.EventuallyWithT(t, func(c *assert.CollectT) {
+ lastResp, gErr := pollClient.Get(ts.URL + "/api/v1/last/2?site=remark42")
+ if !assert.NoError(c, gErr) {
+ return
+ }
+ defer lastResp.Body.Close()
+ assert.Equal(c, http.StatusOK, lastResp.StatusCode)
+ last := []store.Comment{}
+ assert.NoError(c, json.NewDecoder(lastResp.Body).Decode(&last))
+ assert.Len(c, last, 1, "should have 1 comments")
+ }, waitTimeout, httpPoll)
// check count updated
res, code = get(t, ts.URL+"/api/v1/count?site=remark42&url=https://radio-t.com/blah")
@@ -139,7 +147,7 @@ func TestAdmin_Title(t *testing.T) {
fmt.Sprintf("%s/api/v1/admin/title/%s?site=remark42&url=%s/post1", ts.URL, id1, tss.URL), http.NoBody)
assert.NoError(t, err)
requireAdminOnly(t, req)
- resp, err := sendReq(t, req, adminUmputunToken)
+ resp, err := sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusOK, resp.StatusCode)
@@ -174,7 +182,7 @@ func TestAdmin_DeleteUser(t *testing.T) {
req, err := http.NewRequest(http.MethodDelete, fmt.Sprintf("%s/api/v1/admin/user/%s?site=remark42", ts.URL, "id2"), http.NoBody)
assert.NoError(t, err)
requireAdminOnly(t, req)
- resp, err := sendReq(t, req, adminUmputunToken)
+ resp, err := sendReq(req, adminUmputunToken)
assert.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusOK, resp.StatusCode)
@@ -275,7 +283,7 @@ func TestAdmin_Block(t *testing.T) {
req, err := http.NewRequest(http.MethodPut, url, http.NoBody)
assert.NoError(t, err)
requireAdminOnly(t, req)
- resp, err := sendReq(t, req, adminUmputunToken)
+ resp, err := sendReq(req, adminUmputunToken)
require.NoError(t, err)
body, err = io.ReadAll(resp.Body)
assert.NoError(t, err)
@@ -333,10 +341,12 @@ func TestAdmin_Block(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, false, j["block"])
- // block with ttl
+ // block with ttl, checked in place rather than through another admin request, which would
+ // push this test over the 10 req/s limit on that route
makeTwoComments()
- code, _ = block(1, "50ms")
+ code, _ = block(1, "500ms")
require.Equal(t, http.StatusOK, code)
+ require.True(t, srv.adminRest.dataService.IsBlocked("remark42", "user1"), "user1 blocked with ttl")
// get as regular user
res, code = get(t, ts.URL+"/api/v1/find?site=remark42&url=https://radio-t.com/blah&sort=+time")
@@ -350,7 +360,13 @@ func TestAdmin_Block(t *testing.T) {
srv.pubRest.cache = cache.NewScache[[]byte](cache.NewNopCache[[]byte]()) // TODO: with lru cache it won't be refreshed and invalidated for long
// time
- time.Sleep(50 * time.Millisecond)
+
+ // the ttl above is wide enough that the checks in between cannot outlast it, so reaching
+ // here still inside the block, and the wait below observes it lapse
+ require.Eventually(t, func() bool {
+ return !srv.adminRest.dataService.IsBlocked("remark42", "user1")
+ }, waitTimeout, pollInterval, "block with ttl did not expire")
+
res, code = get(t, ts.URL+"/api/v1/find?site=remark42&url=https://radio-t.com/blah&sort=+time")
assert.Equal(t, http.StatusOK, code)
comments = commentsWithInfo{}
@@ -383,23 +399,23 @@ func TestAdmin_BlockedList(t *testing.T) {
req, err := http.NewRequest(http.MethodPut,
fmt.Sprintf("%s/api/v1/admin/user/%s?site=remark42&block=%d", ts.URL, "user1", 1), http.NoBody)
assert.NoError(t, err)
- res, err := sendReq(t, req, adminUmputunToken)
+ res, err := sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.NoError(t, res.Body.Close())
assert.Equal(t, http.StatusOK, res.StatusCode)
- // block user2
+ // block user2 for long enough that the "two users blocked" check below cannot race the ttl
req, err = http.NewRequest(http.MethodPut,
- fmt.Sprintf("%s/api/v1/admin/user/%s?site=remark42&block=%d&ttl=150ms", ts.URL, "user2", 1), http.NoBody)
+ fmt.Sprintf("%s/api/v1/admin/user/%s?site=remark42&block=%d&ttl=1h", ts.URL, "user2", 1), http.NoBody)
assert.NoError(t, err)
- res, err = sendReq(t, req, adminUmputunToken)
+ res, err = sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.NoError(t, res.Body.Close())
assert.Equal(t, http.StatusOK, res.StatusCode)
req, err = http.NewRequest("GET", ts.URL+"/api/v1/admin/blocked?site=remark42", http.NoBody)
require.NoError(t, err)
- res, err = sendReq(t, req, adminUmputunToken)
+ res, err = sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.Equal(t, http.StatusOK, res.StatusCode)
users := []store.BlockedUser{}
@@ -412,18 +428,33 @@ func TestAdmin_BlockedList(t *testing.T) {
assert.Equal(t, "user2", users[1].ID)
assert.Equal(t, "user2 name", users[1].Name)
t.Logf("%+v", users)
- time.Sleep(150 * time.Millisecond)
- req, err = http.NewRequest("GET", ts.URL+"/api/v1/admin/blocked?site=remark42", http.NoBody)
+ // re-block user2 with a short ttl and wait for it to lapse, so the lapse is observed
+ // independently of the check above
+ req, err = http.NewRequest(http.MethodPut,
+ fmt.Sprintf("%s/api/v1/admin/user/%s?site=remark42&block=%d&ttl=150ms", ts.URL, "user2", 1), http.NoBody)
require.NoError(t, err)
- res, err = sendReq(t, req, adminUmputunToken)
+ res, err = sendReq(req, adminUmputunToken)
require.NoError(t, err)
- require.Equal(t, http.StatusOK, res.StatusCode)
- users = []store.BlockedUser{}
- err = json.NewDecoder(res.Body).Decode(&users)
- assert.NoError(t, err)
require.NoError(t, res.Body.Close())
- assert.Equal(t, 1, len(users), "one user left blocked")
+ require.Equal(t, http.StatusOK, res.StatusCode)
+
+ // the closure runs off the test goroutine and asserts on the CollectT it is handed, never on t
+ require.EventuallyWithT(t, func(c *assert.CollectT) {
+ blockedReq, reqErr := http.NewRequest("GET", ts.URL+"/api/v1/admin/blocked?site=remark42", http.NoBody)
+ if !assert.NoError(c, reqErr) {
+ return
+ }
+ blockedResp, sendErr := sendReq(blockedReq, adminUmputunToken)
+ if !assert.NoError(c, sendErr) {
+ return
+ }
+ defer blockedResp.Body.Close()
+ assert.Equal(c, http.StatusOK, blockedResp.StatusCode)
+ blocked := []store.BlockedUser{}
+ assert.NoError(c, json.NewDecoder(blockedResp.Body).Decode(&blocked))
+ assert.Len(c, blocked, 1, "one user left blocked")
+ }, waitTimeout, httpPoll)
}
func TestAdmin_ReadOnly(t *testing.T) {
@@ -448,11 +479,11 @@ func TestAdmin_ReadOnly(t *testing.T) {
req, err := http.NewRequest(http.MethodPut,
fmt.Sprintf("%s/api/v1/admin/readonly?site=remark42&url=https://radio-t.com/blah&ro=1", ts.URL), http.NoBody)
assert.NoError(t, err)
- resp, err := sendReq(t, req, "") // non-admin user
+ resp, err := sendReq(req, "") // non-admin user
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusUnauthorized, resp.StatusCode)
- resp, err = sendReq(t, req, adminUmputunToken)
+ resp, err = sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusOK, resp.StatusCode)
@@ -467,7 +498,7 @@ func TestAdmin_ReadOnly(t *testing.T) {
assert.NoError(t, err, "can't marshal comment %+v", c)
req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment?site=remark42", bytes.NewBuffer(b))
require.NoError(t, err)
- resp, err = sendReq(t, req, adminUmputunToken)
+ resp, err = sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusForbidden, resp.StatusCode)
@@ -476,7 +507,7 @@ func TestAdmin_ReadOnly(t *testing.T) {
req, err = http.NewRequest(http.MethodPut,
fmt.Sprintf("%s/api/v1/admin/readonly?site=remark42&url=https://radio-t.com/blah&ro=0", ts.URL), http.NoBody)
assert.NoError(t, err)
- resp, err = sendReq(t, req, adminUmputunToken)
+ resp, err = sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusOK, resp.StatusCode)
@@ -491,7 +522,7 @@ func TestAdmin_ReadOnly(t *testing.T) {
assert.NoError(t, err, "can't marshal comment %+v", c)
req, err = http.NewRequest("POST", ts.URL+"/api/v1/comment?site="+c.Locator.SiteID, bytes.NewBuffer(b))
require.NoError(t, err)
- resp, err = sendReq(t, req, adminUmputunToken)
+ resp, err = sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusCreated, resp.StatusCode)
@@ -506,7 +537,7 @@ func TestAdmin_ReadOnlyNoComments(t *testing.T) {
fmt.Sprintf("%s/api/v1/admin/readonly?site=remark42&url=https://radio-t.com/blah&ro=1", ts.URL), http.NoBody)
assert.NoError(t, err)
requireAdminOnly(t, req)
- resp, err := sendReq(t, req, adminUmputunToken)
+ resp, err := sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusOK, resp.StatusCode)
@@ -553,7 +584,7 @@ func TestAdmin_ReadOnlyWithAge(t *testing.T) {
fmt.Sprintf("%s/api/v1/admin/readonly?site=remark42&url=https://radio-t.com/blah&ro=1", ts.URL), http.NoBody)
assert.NoError(t, err)
requireAdminOnly(t, req)
- resp, err := sendReq(t, req, adminUmputunToken)
+ resp, err := sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusOK, resp.StatusCode)
@@ -565,7 +596,7 @@ func TestAdmin_ReadOnlyWithAge(t *testing.T) {
req, err = http.NewRequest(http.MethodPut,
fmt.Sprintf("%s/api/v1/admin/readonly?site=remark42&url=https://radio-t.com/blah&ro=0", ts.URL), http.NoBody)
assert.NoError(t, err)
- resp, err = sendReq(t, req, adminUmputunToken)
+ resp, err = sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusForbidden, resp.StatusCode)
@@ -594,7 +625,7 @@ func TestAdmin_Verify(t *testing.T) {
fmt.Sprintf("%s/api/v1/admin/verify/user1?site=remark42&verified=1", ts.URL), http.NoBody)
assert.NoError(t, err)
requireAdminOnly(t, req)
- resp, err := sendReq(t, req, adminUmputunToken)
+ resp, err := sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusOK, resp.StatusCode)
@@ -613,7 +644,7 @@ func TestAdmin_Verify(t *testing.T) {
req, err = http.NewRequest(http.MethodPut,
fmt.Sprintf("%s/api/v1/admin/verify/user1?site=remark42&verified=0", ts.URL), http.NoBody)
assert.NoError(t, err)
- resp, err = sendReq(t, req, adminUmputunToken)
+ resp, err = sendReq(req, adminUmputunToken)
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusOK, resp.StatusCode)
@@ -664,7 +695,7 @@ func TestAdmin_ExportFile(t *testing.T) {
req, err := http.NewRequest("GET", ts.URL+"/api/v1/admin/export?site=remark42&mode=file", http.NoBody)
require.NoError(t, err)
requireAdminOnly(t, req)
- resp, err := sendReq(t, req, adminUmputunToken)
+ resp, err := sendReq(req, adminUmputunToken)
require.NoError(t, err)
assert.Equal(t, http.StatusOK, resp.StatusCode)
diff --git a/backend/app/rest/api/middleware_test.go b/backend/app/rest/api/middleware_test.go
index 57b064a7..4e7fda35 100644
--- a/backend/app/rest/api/middleware_test.go
+++ b/backend/app/rest/api/middleware_test.go
@@ -50,6 +50,39 @@ func TestRouteTimeout(t *testing.T) {
assert.Equal(t, http.StatusOK, resp.StatusCode, "route without R.Timeout runs to completion")
}
+// TestRateLimiter covers the middleware guarding every route group: a burst past the per-second
+// allowance is refused with 429, and a client under the allowance is not. The limiter keys on
+// RemoteAddr, so the two cases use different ones rather than waiting for a bucket to refill.
+func TestRateLimiter(t *testing.T) {
+ router := routegroup.New(http.NewServeMux())
+ router.With(rateLimiter(1)).HandleFunc("GET /limited", func(http.ResponseWriter, *http.Request) {})
+ ts := httptest.NewServer(router)
+ defer ts.Close()
+
+ call := func(remoteAddr string) int {
+ req := httptest.NewRequest("GET", "http://example.com/limited", http.NoBody)
+ req.RemoteAddr = remoteAddr
+ w := httptest.NewRecorder()
+ router.ServeHTTP(w, req)
+ resp := w.Result()
+ assert.NoError(t, resp.Body.Close())
+ return resp.StatusCode
+ }
+
+ // one request a second is allowed, so the first of a burst passes and the rest are refused
+ assert.Equal(t, http.StatusOK, call("1.2.3.4:1000"), "first request within the allowance")
+ refused := 0
+ for range 5 {
+ if call("1.2.3.4:1000") == http.StatusTooManyRequests {
+ refused++
+ }
+ }
+ assert.Equal(t, 5, refused, "burst past the allowance is refused")
+
+ // a different client has its own bucket and is unaffected
+ assert.Equal(t, http.StatusOK, call("5.6.7.8:1000"), "limit is per client, not global")
+}
+
func TestRealIPMiddleware(t *testing.T) {
// call runs mw with the given peer and (optional) X-Real-IP header and returns what the
// downstream handler observes; state is per-call, so subtests don't share closure locals.
diff --git a/backend/app/rest/api/migrator_test.go b/backend/app/rest/api/migrator_test.go
index 21773c9d..734d227b 100644
--- a/backend/app/rest/api/migrator_test.go
+++ b/backend/app/rest/api/migrator_test.go
@@ -34,7 +34,7 @@ func TestMigrator_Import(t *testing.T) {
"ip":"ae12fe3b5f129b5cc4cdd2b136b7b7947c4d2741"},"locator":{"site":"remark42","url":"https://radio-t.com/blah2"},"score":0,
"votes":{},"time":"2018-04-30T01:37:00.861387771-05:00"}`)
- client := &http.Client{Timeout: 1 * time.Second}
+ client := &http.Client{Timeout: waitTimeout}
defer client.CloseIdleConnections()
req, err := http.NewRequest("POST", ts.URL+"/api/v1/admin/import?site=remark42&provider=native", r)
require.NoError(t, err)
@@ -125,7 +125,7 @@ func TestMigrator_ImportFromWP(t *testing.T) {
r := strings.NewReader(strings.ReplaceAll(xmlTestWP, "'", "`"))
- client := &http.Client{Timeout: 1 * time.Second}
+ client := &http.Client{Timeout: waitTimeout}
defer client.CloseIdleConnections()
req, err := http.NewRequest("POST", ts.URL+"/api/v1/admin/import?site=remark42&provider=wordpress", r)
assert.NoError(t, err)
@@ -170,7 +170,7 @@ func TestMigrator_ImportFromCommento(t *testing.T) {
"email":"somegreatmail@gmail.com","name":"User5276","link":"https://example.com/profile/257","photo":"https://secure.gravatar.com/avatar/8f279626d26175134b0d5c88648172f7",
"provider":"sso:example.com","joinDate":"2021-03-19T19:27:25.954285Z","isModerator":false}]}`)
- client := &http.Client{Timeout: 1 * time.Second}
+ client := &http.Client{Timeout: waitTimeout}
defer client.CloseIdleConnections()
req, err := http.NewRequest("POST", ts.URL+"/api/v1/admin/import?site=remark42&provider=commento", r)
assert.NoError(t, err)
@@ -211,7 +211,7 @@ func TestMigrator_ImportFromCommentoJSON(t *testing.T) {
r, err := os.Open("testdata/commento.json")
require.NoError(t, err)
- client := &http.Client{Timeout: 1 * time.Second}
+ client := &http.Client{Timeout: waitTimeout}
defer client.CloseIdleConnections()
req, err := http.NewRequest("POST", ts.URL+"/api/v1/admin/import?site=remark42&provider=commento", r)
assert.NoError(t, err)
@@ -258,7 +258,7 @@ func TestMigrator_ImportRejected(t *testing.T) {
"ip":"ae12fe3b5f129b5cc4cdd2b136b7b7947c4d2741"},"locator":{"site":"remark42","url":"https://radio-t.com/blah2"},"score":0,
"votes":{},"time":"2018-04-30T01:37:00.861387771-05:00"}`)
- client := &http.Client{Timeout: 1 * time.Second}
+ client := &http.Client{Timeout: waitTimeout}
defer client.CloseIdleConnections()
req, err := http.NewRequest("POST", ts.URL+"/api/v1/admin/import?site=remark42&provider=native&secret=XYZ", r)
assert.NoError(t, err)
@@ -281,7 +281,7 @@ func TestMigrator_ImportDouble(t *testing.T) {
recs = append(recs, fmt.Sprintf(tmpl, i))
}
r := strings.NewReader(`{"version":1}` + strings.Join(recs, "\n")) // reader with 10k records
- client := &http.Client{Timeout: 1 * time.Second}
+ client := &http.Client{Timeout: waitTimeout}
defer client.CloseIdleConnections()
req, err := http.NewRequest("POST", ts.URL+"/api/v1/admin/import?site=remark42&provider=native", r)
require.NoError(t, err)
@@ -380,7 +380,7 @@ func TestMigrator_Export(t *testing.T) {
"votes":{},"time":"2018-04-30T01:37:00.861387771-05:00"}`)
// import comments first
- client := &http.Client{Timeout: 1 * time.Second}
+ client := &http.Client{Timeout: waitTimeout}
defer client.CloseIdleConnections()
req, err := http.NewRequest("POST", ts.URL+"/api/v1/admin/import?site=remark42&provider=native", r)
require.NoError(t, err)
@@ -573,7 +573,7 @@ func TestMigrator_RemapReject(t *testing.T) {
defer teardown()
// without admin credentials
- client := &http.Client{Timeout: 1 * time.Second}
+ client := &http.Client{Timeout: waitTimeout}
defer client.CloseIdleConnections()
rules := strings.NewReader(`https://remark42.com/* https://www.remark42.com/*`)
req, err := http.NewRequest("POST", ts.URL+"/api/v1/admin/remap?site=remark42", rules)
diff --git a/backend/app/rest/api/rest_private_test.go b/backend/app/rest/api/rest_private_test.go
index fc6276d1..5e8ea4c9 100644
--- a/backend/app/rest/api/rest_private_test.go
+++ b/backend/app/rest/api/rest_private_test.go
@@ -144,7 +144,7 @@ func TestRest_CreateAndPreviewWithImage(t *testing.T) {
assert.Equal(t, false, pngRead, "original image is not yet accessed by server")
// retrieve the image from the cache
- imgURL := strings.Split(strings.Split(string(b), "src=\"")[1], "\"")[0]
+ imgURL, _, _ := strings.Cut(strings.Split(string(b), "src=\"")[1], "\"")
// replace srv.RemarkURL with ts.URL
imgURL = strings.ReplaceAll(imgURL, srv.RemarkURL, ts.URL)
resp, err = http.Get(imgURL)
@@ -432,6 +432,7 @@ func TestRest_Update(t *testing.T) {
strings.NewReader(`{"text":"updated text", "summary":"my edit"}`))
assert.NoError(t, err)
req.Header.Add("X-JWT", devToken)
+ beforeUpdate := time.Now()
b, err := client.Do(req)
assert.NoError(t, err)
body, err := io.ReadAll(b.Body)
@@ -447,7 +448,7 @@ func TestRest_Update(t *testing.T) {
assert.Equal(t, "updated text
\n", c2.Text)
assert.Equal(t, "updated text", c2.Orig)
assert.Equal(t, "my edit", c2.Edit.Summary)
- assert.True(t, time.Since(c2.Edit.Timestamp) < 1*time.Second)
+ assert.WithinRange(t, c2.Edit.Timestamp, beforeUpdate, time.Now(), "edit stamped during the update")
// read updated comment
res, code := getWithAdminAuth(t, fmt.Sprintf("%s/api/v1/id/%s?site=remark42&url=https://radio-t.com/blah1", ts.URL, id))
@@ -596,7 +597,7 @@ func TestRest_DeleteChildThenParent(t *testing.T) {
fmt.Sprintf("%s/api/v1/admin/comment/%s?site=remark42&url=https://radio-t.com/blah1", ts.URL, idC2), http.NoBody)
require.NoError(t, err)
requireAdminOnly(t, req)
- resp, err = sendReq(t, req, adminUmputunToken)
+ resp, err = sendReq(req, adminUmputunToken)
assert.NoError(t, err)
assert.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusOK, resp.StatusCode)
@@ -784,7 +785,7 @@ func TestRest_Vote(t *testing.T) {
req, err := http.NewRequest("GET",
fmt.Sprintf("%s/api/v1/id/%s?site=remark42&url=https://radio-t.com/blah", ts.URL, id1), http.NoBody)
assert.NoError(t, err)
- resp, err := sendReq(t, req, adminUmputunToken)
+ resp, err := sendReq(req, adminUmputunToken)
assert.NoError(t, err)
assert.Equal(t, http.StatusOK, resp.StatusCode)
cr = store.Comment{}
@@ -974,9 +975,7 @@ func TestRest_EmailNotification(t *testing.T) {
require.Equal(t, http.StatusCreated, resp.StatusCode, string(body))
parentComment := store.Comment{}
require.NoError(t, json.Unmarshal(body, &parentComment))
- // wait for mock notification Submit to kick off
- time.Sleep(time.Millisecond * 30)
- require.Equal(t, 1, len(mockDestination.Get()))
+ waitForCount(t, 1, func() int { return len(mockDestination.Get()) })
assert.Empty(t, mockDestination.Get()[0].Emails)
// create child comment from another user, email notification only to admin expected
@@ -994,9 +993,7 @@ func TestRest_EmailNotification(t *testing.T) {
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
require.Equal(t, http.StatusCreated, resp.StatusCode, string(body))
- // wait for mock notification Submit to kick off
- time.Sleep(time.Millisecond * 30)
- require.Equal(t, 2, len(mockDestination.Get()))
+ waitForCount(t, 2, func() int { return len(mockDestination.Get()) })
assert.Empty(t, mockDestination.Get()[1].Emails)
// send confirmation token for email
@@ -1013,9 +1010,7 @@ func TestRest_EmailNotification(t *testing.T) {
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
require.Equal(t, http.StatusOK, resp.StatusCode, string(body))
- // wait for mock notification Submit to kick off
- time.Sleep(time.Millisecond * 30)
- require.Equal(t, 1, len(mockDestination.GetVerify()))
+ waitForCount(t, 1, func() int { return len(mockDestination.GetVerify()) })
assert.Equal(t, "good@example.com", mockDestination.GetVerify()[0].Email)
verificationToken := mockDestination.GetVerify()[0].Token
@@ -1087,9 +1082,7 @@ func TestRest_EmailNotification(t *testing.T) {
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
require.Equal(t, http.StatusCreated, resp.StatusCode, string(body))
- // wait for mock notification Submit to kick off
- time.Sleep(time.Millisecond * 30)
- require.Equal(t, 3, len(mockDestination.Get()))
+ waitForCount(t, 3, func() int { return len(mockDestination.Get()) })
assert.Equal(t, []string{"good@example.com"}, mockDestination.Get()[2].Emails)
// delete user's email
@@ -1117,9 +1110,7 @@ func TestRest_EmailNotification(t *testing.T) {
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
require.Equal(t, http.StatusCreated, resp.StatusCode, string(body))
- // wait for mock notification Submit to kick off
- time.Sleep(time.Millisecond * 30)
- require.Equal(t, 4, len(mockDestination.Get()))
+ waitForCountSettled(t, 4, func() int { return len(mockDestination.Get()) })
assert.Empty(t, mockDestination.Get()[3].Emails)
// confirm email via subscribe call with query params, old behavior, email notification is expected
@@ -1136,9 +1127,7 @@ func TestRest_EmailNotification(t *testing.T) {
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
require.Equal(t, http.StatusOK, resp.StatusCode, string(body))
- // wait for mock notification Submit to kick off
- time.Sleep(time.Millisecond * 30)
- require.Equal(t, 2, len(mockDestination.GetVerify()), "verification email was sent")
+ waitForCount(t, 2, func() int { return len(mockDestination.GetVerify()) }, "verification email was sent")
// get email user information to verify there is no subscription yet
req, err = http.NewRequest(
@@ -1173,9 +1162,7 @@ func TestRest_EmailNotification(t *testing.T) {
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
require.Equal(t, http.StatusOK, resp.StatusCode, string(body))
- // wait for mock notification Submit to kick off
- time.Sleep(time.Millisecond * 30)
- require.Equal(t, 2, len(mockDestination.GetVerify()), "no new verification email was sent")
+ waitForCountSettled(t, 2, func() int { return len(mockDestination.GetVerify()) }, "no new verification email was sent")
// get email user information to verify the subscription happened without the confirmation call
req, err = http.NewRequest(
@@ -1224,9 +1211,7 @@ func TestRest_TelegramNotification(t *testing.T) {
require.Equal(t, http.StatusCreated, resp.StatusCode, string(body))
parentComment := store.Comment{}
require.NoError(t, json.Unmarshal(body, &parentComment))
- // wait for mock notification Submit to kick off
- time.Sleep(time.Millisecond * 30)
- require.Equal(t, 1, len(mockDestination.Get()))
+ waitForCount(t, 1, func() int { return len(mockDestination.Get()) })
assert.Empty(t, mockDestination.Get()[0].Telegrams)
// create child comment from another user, telegram notification only to admin expected
@@ -1244,9 +1229,7 @@ func TestRest_TelegramNotification(t *testing.T) {
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
require.Equal(t, http.StatusCreated, resp.StatusCode, string(body))
- // wait for mock notification Submit to kick off
- time.Sleep(time.Millisecond * 30)
- require.Equal(t, 2, len(mockDestination.Get()))
+ waitForCount(t, 2, func() int { return len(mockDestination.Get()) })
assert.Empty(t, mockDestination.Get()[1].Telegrams)
// subscribe to telegram while the telegram destination is absent
@@ -1357,9 +1340,7 @@ func TestRest_TelegramNotification(t *testing.T) {
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
require.Equal(t, http.StatusCreated, resp.StatusCode, string(body))
- // wait for mock notification Submit to kick off
- time.Sleep(time.Millisecond * 30)
- require.Equal(t, 3, len(mockDestination.Get()))
+ waitForCount(t, 3, func() int { return len(mockDestination.Get()) })
assert.Equal(t, []string{"good_telegram"}, mockDestination.Get()[2].Telegrams)
// delete user's telegram
@@ -1387,9 +1368,7 @@ func TestRest_TelegramNotification(t *testing.T) {
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
require.Equal(t, http.StatusCreated, resp.StatusCode, string(body))
- // wait for mock notification Submit to kick off
- time.Sleep(time.Millisecond * 30)
- require.Equal(t, 4, len(mockDestination.Get()))
+ waitForCountSettled(t, 4, func() int { return len(mockDestination.Get()) })
assert.Empty(t, mockDestination.Get()[3].Telegrams)
}
@@ -1412,7 +1391,7 @@ func TestRest_UserAllData(t *testing.T) {
_, err = srv.DataService.Create(c3)
require.NoError(t, err)
- client := &http.Client{Timeout: 1 * time.Second}
+ client := &http.Client{Timeout: waitTimeout}
defer client.CloseIdleConnections()
req, err := http.NewRequest("GET", ts.URL+"/api/v1/userdata?site=remark42", http.NoBody)
require.NoError(t, err)
@@ -1465,7 +1444,7 @@ func TestRest_UserAllDataManyComments(t *testing.T) {
_, err := srv.DataService.Create(c)
require.NoError(t, err)
}
- client := &http.Client{Timeout: 1 * time.Second}
+ client := &http.Client{Timeout: waitTimeout}
defer client.CloseIdleConnections()
req, err := http.NewRequest("GET", ts.URL+"/api/v1/userdata?site=remark42", http.NoBody)
require.NoError(t, err)
@@ -1608,7 +1587,9 @@ func TestRest_CreateWithPictures(t *testing.T) {
Staging: "/tmp/remark42/images.staging",
Location: "/tmp/remark42/images",
}, image.ServiceParams{
- EditDuration: 100 * time.Millisecond,
+ // the "not moved yet" checks below run right after the comment POST returns, so the
+ // commit window has to be wide enough that a stalled runner cannot close it first
+ EditDuration: 3 * time.Second,
MaxSize: 2000,
ImageAPI: svc.RemarkURL + "/api/v1/picture/",
ProxyAPI: svc.RemarkURL + "/api/v1/img",
@@ -1671,11 +1652,12 @@ func TestRest_CreateWithPictures(t *testing.T) {
assert.Error(t, err, "picture %d not moved from staging yet", i)
}
- time.Sleep(1500 * time.Millisecond)
-
+ // the commit runs once EditDuration expires
for i := range ids {
- _, err = os.Stat("/tmp/remark42/images/" + ids[i])
- assert.NoError(t, err, "picture %d moved from staging and available in permanent location", i)
+ require.Eventually(t, func() bool {
+ _, e := os.Stat("/tmp/remark42/images/" + ids[i])
+ return e == nil
+ }, waitTimeout, pollInterval, "picture %d moved from staging and available in permanent location", i)
}
}
diff --git a/backend/app/rest/api/rest_public_test.go b/backend/app/rest/api/rest_public_test.go
index a583a8e9..adff57f0 100644
--- a/backend/app/rest/api/rest_public_test.go
+++ b/backend/app/rest/api/rest_public_test.go
@@ -381,11 +381,12 @@ func TestRest_Last(t *testing.T) {
c2 := store.Comment{Text: "test test #2", ParentID: "p1",
Locator: store.Locator{SiteID: "remark42", URL: "https://radio-t.com/blah2"}}
- // add 3 comments
+ // add 3 comments, with the clock pushed past a millisecond boundary in between so the two
+ // "since" values below are distinct
ts1 := time.Now().UnixNano() / 1000000
addComment(t, c1, ts)
id1 := addComment(t, c1, ts)
- time.Sleep(10 * time.Millisecond)
+ waitPastMillisecond(time.Now())
ts2 := time.Now().UnixNano() / 1000000
id2 := addComment(t, c2, ts)
@@ -539,9 +540,17 @@ func TestRest_FindUserComments_CWE_918(t *testing.T) {
assert.Equal(t, arbitraryServer.URL, resp.Comments[0].Locator.URL, "arbitrary URL provided by the request")
}
+// waitPastMillisecond blocks until the wall clock moves past ts's millisecond, so whatever is
+// created next gets a distinct value for the millisecond-precision "since" filter
+func waitPastMillisecond(ts time.Time) {
+ next := ts.Truncate(time.Millisecond).Add(time.Millisecond)
+ time.Sleep(time.Until(next) + time.Microsecond) // a non-positive duration returns at once
+}
+
func TestPublic_FindCommentsCtrl_ConsistentCount(t *testing.T) {
// test that comment counting is consistent between tree and plain formats
- ts, srv, teardown := startupT(t)
+ // the open-route limit is lifted so the subtests below can run back to back
+ ts, srv, teardown := startupT(t, func(srv *Rest) { srv.openRouteLimiter = 100000 })
defer teardown()
commentLocator := store.Locator{URL: "test-url", SiteID: "remark42"}
@@ -567,55 +576,55 @@ func TestPublic_FindCommentsCtrl_ConsistentCount(t *testing.T) {
}
// adding initial comments (8 to test-url and 1 to another-url) and voting, and delete two of comments to the first post.
- // with sleep so that at least few millisecond pass between each comment
- // and later we would be able to use that in "since" filter with millisecond precision
+ // each comment waits for the clock to pass the previous one's millisecond so the "since"
+ // filter, which has millisecond precision, can tell them apart
ids := make([]string, 9)
timestamps := make([]time.Time, 9)
c1 := store.Comment{Text: "top-level comment 1", Locator: commentLocator}
ids[0], timestamps[0] = addCommentGetCreatedTime(t, c1, ts)
// #3 by score
setScore(commentLocator, ids[0], 1)
- time.Sleep(time.Millisecond * 5)
+ waitPastMillisecond(timestamps[0])
c2 := store.Comment{Text: "top-level comment 2", Locator: commentLocator}
ids[1], timestamps[1] = addCommentGetCreatedTime(t, c2, ts)
// #2 by score
setScore(commentLocator, ids[1], 2)
- time.Sleep(time.Millisecond * 5)
+ waitPastMillisecond(timestamps[1])
c3 := store.Comment{Text: "second-level comment 1", ParentID: ids[0], Locator: commentLocator}
ids[2], timestamps[2] = addCommentGetCreatedTime(t, c3, ts)
// #1 by score
setScore(commentLocator, ids[2], 10)
- time.Sleep(time.Millisecond * 5)
+ waitPastMillisecond(timestamps[2])
c4 := store.Comment{Text: "third-level comment 1", ParentID: ids[2], Locator: commentLocator}
ids[3], timestamps[3] = addCommentGetCreatedTime(t, c4, ts)
// #5 by score, #1 by controversy
setScore(commentLocator, ids[3], 4)
setScore(commentLocator, ids[3], -4)
- time.Sleep(time.Millisecond * 5)
+ waitPastMillisecond(timestamps[3])
c5 := store.Comment{Text: "second-level comment 2", ParentID: ids[1], Locator: commentLocator}
ids[4], timestamps[4] = addCommentGetCreatedTime(t, c5, ts)
// #5 by score, #2 by controversy
setScore(commentLocator, ids[4], 2)
setScore(commentLocator, ids[4], -3)
- time.Sleep(time.Millisecond * 5)
+ waitPastMillisecond(timestamps[4])
c6 := store.Comment{Text: "deleted third-level comment 2", ParentID: ids[4], Locator: commentLocator}
ids[5], timestamps[5] = addCommentGetCreatedTime(t, c6, ts)
// deleted later so not visible in site-wide requests
setScore(commentLocator, ids[5], 10)
setScore(commentLocator, ids[5], -10)
- time.Sleep(time.Millisecond * 5)
+ waitPastMillisecond(timestamps[5])
c7 := store.Comment{Text: "top-level comment 3", Locator: commentLocator}
ids[6], timestamps[6] = addCommentGetCreatedTime(t, c7, ts)
// #6 by score, #4 by controversy
setScore(commentLocator, ids[6], -3)
setScore(commentLocator, ids[6], 1)
- time.Sleep(time.Millisecond * 5)
+ waitPastMillisecond(timestamps[6])
c8 := store.Comment{Text: "deleted second-level comment 3", ParentID: ids[6], Locator: commentLocator}
ids[7], timestamps[7] = addCommentGetCreatedTime(t, c8, ts)
@@ -782,8 +791,6 @@ func TestPublic_FindCommentsCtrl_ConsistentCount(t *testing.T) {
assert.Equal(t, expectedStatus, code)
assert.Contains(t, body, tc.expectedBody)
t.Log(body)
- // prevent hit limiter from engaging
- time.Sleep(80 * time.Millisecond)
})
}
}
diff --git a/backend/app/rest/api/rest_test.go b/backend/app/rest/api/rest_test.go
index 2d2a6032..53148231 100644
--- a/backend/app/rest/api/rest_test.go
+++ b/backend/app/rest/api/rest_test.go
@@ -8,7 +8,6 @@ import (
"fmt"
"io"
"io/fs"
- "math/rand"
"net"
"net/http"
"net/http/httptest"
@@ -368,20 +367,25 @@ func TestRest_AvatarMounts(t *testing.T) {
func TestRest_Shutdown(t *testing.T) {
srv := Rest{Authenticator: &auth.Service{}, ImageProxy: &proxy.Image{}}
+ port := chooseUnusedPort(t)
done := make(chan bool)
// without waiting for channel close at the end goroutine will stay alive after test finish
// which would create data race with next test
go func() {
- time.Sleep(200 * time.Millisecond)
- srv.Shutdown()
+ srv.Run("127.0.0.1", port)
close(done)
}()
- st := time.Now()
- srv.Run("127.0.0.1", 0)
- assert.True(t, time.Since(st).Seconds() < 1, "should take about 100ms")
- <-done
+ defer srv.Shutdown() // a failed readiness wait must not leave srv.Run behind for goleak
+ waitForServerStart(t, port)
+ srv.Shutdown()
+
+ select {
+ case <-done:
+ case <-time.After(serverStopTimeout):
+ t.Fatal("rest server did not stop after Shutdown")
+ }
}
func TestRest_filterComments(t *testing.T) {
@@ -400,7 +404,7 @@ func TestRest_filterComments(t *testing.T) {
}
func TestRest_RunStaticSSLMode(t *testing.T) {
- sslPort := chooseRandomUnusedPort()
+ sslPort := chooseUnusedPort(t)
srv := Rest{
Authenticator: auth.NewService(auth.Opts{
AvatarStore: avatar.NewLocalFS("/tmp"),
@@ -417,12 +421,12 @@ func TestRest_RunStaticSSLMode(t *testing.T) {
RemarkURL: fmt.Sprintf("https://localhost:%d", sslPort),
}
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
go func() {
srv.Run("", port)
}()
- waitForHTTPSServerStart(sslPort)
+ waitForServerStart(t, sslPort, port)
client := http.Client{
// prevent http redirect
@@ -455,7 +459,7 @@ func TestRest_RunStaticSSLMode(t *testing.T) {
}
func TestRest_RunAutocertModeHTTPOnly(t *testing.T) {
- sslPort := chooseRandomUnusedPort()
+ sslPort := chooseUnusedPort(t)
srv := Rest{
Authenticator: &auth.Service{},
ImageProxy: &proxy.Image{},
@@ -466,13 +470,13 @@ func TestRest_RunAutocertModeHTTPOnly(t *testing.T) {
RemarkURL: fmt.Sprintf("https://localhost:%d", sslPort),
}
- port := chooseRandomUnusedPort()
+ port := chooseUnusedPort(t)
go func() {
// can't check https server locally, just only http server
srv.Run("", port)
}()
- waitForHTTPSServerStart(sslPort)
+ waitForServerStart(t, sslPort, port)
client := http.Client{
// prevent http redirect
@@ -586,25 +590,11 @@ func TestRest_frameAncestors(t *testing.T) {
assert.Contains(t, resp.Header.Get("Content-Security-Policy"), "frame-ancestors *;")
}
-// randomPath pick a file or folder name which is not in use for sure
-func randomPath(tempDir, basename, suffix string) (string, error) {
- for range 10 {
- fname := fmt.Sprintf("/%s/%s-%d%s", tempDir, basename, rand.Int31(), suffix)
- fmt.Printf("fname %q", fname)
- _, err := os.Stat(fname)
- if err != nil {
- return fname, nil
- }
- }
- return "", fmt.Errorf("cannot create temp file in %s", tempDir)
-}
-
// startupT runs fully configured testing server
// srvHook is an optional func to set some Rest param after the creation but prior to Run
func startupT(t *testing.T, srvHook ...func(srv *Rest)) (ts *httptest.Server, srv *Rest, teardown func()) {
tmp := os.TempDir()
- testDB, err := randomPath(tmp, "test-remark", ".db")
- require.NoError(t, err)
+ testDB := filepath.Join(t.TempDir(), "test-remark.db") // per-test dir, removed when the test ends
_ = os.RemoveAll(tmp + "/ava-remark42")
_ = os.RemoveAll(tmp + "/pics-remark42")
@@ -685,7 +675,6 @@ func startupT(t *testing.T, srvHook ...func(srv *Rest)) (ts *httptest.Server, sr
teardown = func() {
ts.Close()
require.NoError(t, srv.DataService.Close())
- _ = os.Remove(testDB)
_ = os.RemoveAll(tmp + "/ava-remark42")
_ = os.RemoveAll(tmp + "/pics-remark42")
}
@@ -693,6 +682,44 @@ func startupT(t *testing.T, srvHook ...func(srv *Rest)) (ts *httptest.Server, sr
return ts, srv, teardown
}
+const (
+ // outer bound before a wait is called a hang, generous enough for a loaded CI runner
+ waitTimeout = 30 * time.Second
+ pollInterval = 10 * time.Millisecond
+
+ // budget for a server to stop once asked, tight enough to catch a shutdown that hangs
+ serverStopTimeout = 10 * time.Second
+
+ // connect budget for a single probe, kept off the poll interval so a slow loopback connect
+ // on a loaded runner does not look like a server that is not listening
+ probeDialTimeout = time.Second
+
+ // window to prove something did not happen
+ notifySettle = 300 * time.Millisecond
+
+ // poll interval for waits that issue an HTTP request. the admin routes allow 10 req/s and
+ // the open ones 100 in tests, so this stays below the tighter of the two and the poll
+ // cannot manufacture the 429s it would then have to interpret
+ httpPoll = 150 * time.Millisecond
+)
+
+// waitForCount blocks until got reaches want, failing the test with the last value it saw.
+// for work that is delivered asynchronously, such as notifications reaching a mock destination
+func waitForCount(t *testing.T, want int, got func() int, msgAndArgs ...any) {
+ t.Helper()
+ require.EventuallyWithT(t, func(c *assert.CollectT) {
+ assert.Equal(c, want, got(), msgAndArgs...)
+ }, waitTimeout, pollInterval)
+}
+
+// waitForCountSettled waits for got to reach want and then holds it there, so a delivery
+// arriving late is caught rather than passing because the count was read the instant it matched
+func waitForCountSettled(t *testing.T, want int, got func() int, msgAndArgs ...any) {
+ t.Helper()
+ waitForCount(t, want, got, msgAndArgs...)
+ require.Never(t, func() bool { return got() != want }, notifySettle, pollInterval, msgAndArgs...)
+}
+
// fake auth middleware make user authenticated and uses query's fake_id for ID and fake_name for Name
func fakeAuth(next http.Handler) http.Handler {
fn := func(w http.ResponseWriter, r *http.Request) {
@@ -716,7 +743,7 @@ func get(t *testing.T, url string) (response string, statusCode int) {
return string(body), r.StatusCode
}
-func sendReq(_ *testing.T, r *http.Request, tkn string) (*http.Response, error) {
+func sendReq(r *http.Request, tkn string) (*http.Response, error) {
client := http.Client{Timeout: 5 * time.Second}
defer client.CloseIdleConnections()
if tkn != "" {
@@ -798,7 +825,6 @@ func addCommentGetCreatedTime(t *testing.T, c store.Comment, ts *httptest.Server
crResp := R.JSON{}
err = json.Unmarshal(b, &crResp)
require.NoError(t, err)
- time.Sleep(time.Nanosecond * 10)
created, err = time.Parse(time.RFC3339, crResp["time"].(string))
require.NoError(t, err)
return crResp["id"].(string), created
@@ -810,37 +836,41 @@ func addComment(t *testing.T, c store.Comment, ts *httptest.Server) string {
}
func requireAdminOnly(t *testing.T, req *http.Request) {
- resp, err := sendReq(t, req, "") // no-auth user
+ resp, err := sendReq(req, "") // no-auth user
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusUnauthorized, resp.StatusCode)
- resp, err = sendReq(t, req, devToken) // non-admin user
+ resp, err = sendReq(req, devToken) // non-admin user
require.NoError(t, err)
require.NoError(t, resp.Body.Close())
assert.Equal(t, http.StatusForbidden, resp.StatusCode)
}
-func chooseRandomUnusedPort() (port int) {
- for range 10 {
- port = 40000 + int(rand.Int31n(10000))
- if ln, err := net.Listen("tcp", fmt.Sprintf(":%d", port)); err == nil {
- _ = ln.Close()
- break
- }
- }
+// chooseUnusedPort asks the kernel for a free port from the ephemeral range, which makes a
+// collision between concurrently running package test binaries very unlikely
+func chooseUnusedPort(t *testing.T) int {
+ t.Helper()
+ ln, err := net.Listen("tcp", ":0")
+ require.NoError(t, err, "no free port available")
+ port := ln.Addr().(*net.TCPAddr).Port
+ require.NoError(t, ln.Close())
return port
}
-func waitForHTTPSServerStart(port int) {
- // wait for up to 3 seconds for HTTPS server to start
- for range 300 {
- time.Sleep(time.Millisecond * 10)
- conn, _ := net.DialTimeout("tcp", fmt.Sprintf("localhost:%d", port), time.Millisecond*10)
- if conn != nil {
+// waitForServerStart blocks until something accepts on every listed port, failing the test
+// naming the port that never came up
+func waitForServerStart(t *testing.T, ports ...int) {
+ t.Helper()
+ for _, port := range ports {
+ require.Eventually(t, func() bool {
+ conn, err := net.DialTimeout("tcp", fmt.Sprintf("localhost:%d", port), probeDialTimeout)
+ if err != nil {
+ return false
+ }
_ = conn.Close()
- break
- }
+ return true
+ }, waitTimeout, pollInterval, "server on port %d didn't start", port)
}
}
@@ -849,5 +879,9 @@ func TestMain(m *testing.M) {
m,
// this will be fixed in https://github.com/hashicorp/golang-lru/issues/159
goleak.IgnoreTopFunction("github.com/hashicorp/golang-lru/v2/expirable.NewLRU[...].func1"),
+ // regexp2, pulled in by chroma for syntax highlighting, keeps one shared clock goroutine
+ // alive for up to a second after the last match with a timeout, sleeping in 100ms ticks.
+ // it ends on its own, but a binary that finishes inside that window is reported as leaking
+ goleak.IgnoreAnyFunction("github.com/dlclark/regexp2/v2.runClock"),
)
}
diff --git a/backend/app/rest/api/rss_test.go b/backend/app/rest/api/rss_test.go
index 914270a6..fc583ab5 100644
--- a/backend/app/rest/api/rss_test.go
+++ b/backend/app/rest/api/rss_test.go
@@ -14,22 +14,29 @@ import (
"github.com/umputun/remark42/backend/app/store"
)
+// rssPubTime returns a second-aligned base timestamp and formats it the way the feed does, so
+// comment pubDates are pinned rather than dependent on when in the second the test runs.
+func rssPubTime() (base time.Time, pubDate string) {
+ base = time.Now().Truncate(time.Second)
+ return base, base.Format(time.RFC1123Z)
+}
+
func TestServer_RssPost(t *testing.T) {
ts, rst, teardown := startupT(t)
defer teardown()
- waitOnSecChange()
+ base, pubDate := rssPubTime()
c1 := store.Comment{
- ID: "1234567890",
- Text: "test 123",
- Locator: store.Locator{URL: "https://radio-t.com/blah1", SiteID: "remark42"},
- User: store.User{ID: "u1", Name: "developer one"},
+ ID: "1234567890",
+ Text: "test 123",
+ Timestamp: base,
+ Locator: store.Locator{URL: "https://radio-t.com/blah1", SiteID: "remark42"},
+ User: store.User{ID: "u1", Name: "developer one"},
}
id1, err := rst.DataService.Create(c1)
require.NoError(t, err)
assert.Equal(t, "1234567890", id1)
- pubDate := time.Now().Format(time.RFC1123Z)
res, code := get(t, ts.URL+"/api/v1/rss/post?site=remark42&url=https://radio-t.com/blah1")
assert.Equal(t, http.StatusOK, code)
@@ -63,21 +70,21 @@ func TestServer_RssSite(t *testing.T) {
ts, rst, teardown := startupT(t)
defer teardown()
- waitOnSecChange()
-
- pubDate := time.Now().Format(time.RFC1123Z)
+ base, pubDate := rssPubTime()
c1 := store.Comment{
- ID: "comment-id-1",
- Text: "test 123",
- Locator: store.Locator{URL: "https://radio-t.com/blah10", SiteID: "remark42"},
- User: store.User{ID: "u1", Name: "developer one"},
+ ID: "comment-id-1",
+ Text: "test 123",
+ Timestamp: base,
+ Locator: store.Locator{URL: "https://radio-t.com/blah10", SiteID: "remark42"},
+ User: store.User{ID: "u1", Name: "developer one"},
}
c2 := store.Comment{
- ID: "comment-id-2",
- Text: "xyz test",
- Locator: store.Locator{URL: "https://radio-t.com/blah11", SiteID: "remark42"},
- User: store.User{ID: "u1", Name: "developer one"},
+ ID: "comment-id-2",
+ Text: "xyz test",
+ Timestamp: base.Add(time.Millisecond),
+ Locator: store.Locator{URL: "https://radio-t.com/blah11", SiteID: "remark42"},
+ User: store.User{ID: "u1", Name: "developer one"},
}
_, err := rst.DataService.Create(c1)
@@ -126,22 +133,22 @@ func TestServer_RssWithReply(t *testing.T) {
ts, rst, teardown := startupT(t)
defer teardown()
- waitOnSecChange()
-
- pubDate := time.Now().Format(time.RFC1123Z)
+ base, pubDate := rssPubTime()
c1 := store.Comment{
- ID: "comment-id-1",
- Text: "test 123",
- Locator: store.Locator{URL: "https://radio-t.com/blah10", SiteID: "remark42"},
- User: store.User{ID: "u1", Name: "developer one"},
+ ID: "comment-id-1",
+ Text: "test 123",
+ Timestamp: base,
+ Locator: store.Locator{URL: "https://radio-t.com/blah10", SiteID: "remark42"},
+ User: store.User{ID: "u1", Name: "developer one"},
}
c2 := store.Comment{
- ID: "comment-id-2",
- ParentID: "comment-id-1",
- Text: "xyz test",
- Locator: store.Locator{URL: "https://radio-t.com/blah10", SiteID: "remark42"},
- User: store.User{ID: "u1", Name: "developer one"},
+ ID: "comment-id-2",
+ ParentID: "comment-id-1",
+ Text: "xyz test",
+ Timestamp: base.Add(time.Millisecond),
+ Locator: store.Locator{URL: "https://radio-t.com/blah10", SiteID: "remark42"},
+ User: store.User{ID: "u1", Name: "developer one"},
}
_, err := rst.DataService.Create(c1)
@@ -186,42 +193,45 @@ func TestServer_RssReplies(t *testing.T) {
ts, srv, teardown := startupT(t)
defer teardown()
- waitOnSecChange()
-
- pubDate := time.Now().Format(time.RFC1123Z)
+ base, pubDate := rssPubTime()
c1 := store.Comment{
- ID: "comment-1",
- Text: "c1",
- Locator: store.Locator{URL: "https://radio-t.com/blah1", SiteID: "remark42"},
- User: store.User{ID: "user1", Name: "user1"},
+ ID: "comment-1",
+ Text: "c1",
+ Timestamp: base,
+ Locator: store.Locator{URL: "https://radio-t.com/blah1", SiteID: "remark42"},
+ User: store.User{ID: "user1", Name: "user1"},
}
c2 := store.Comment{
- ID: "comment-2",
- Text: "reply to c1 from user2",
- ParentID: "comment-1",
- Locator: store.Locator{URL: "https://radio-t.com/blah1", SiteID: "remark42"},
- User: store.User{ID: "user2", Name: "user2"},
+ ID: "comment-2",
+ Text: "reply to c1 from user2",
+ ParentID: "comment-1",
+ Timestamp: base.Add(time.Millisecond),
+ Locator: store.Locator{URL: "https://radio-t.com/blah1", SiteID: "remark42"},
+ User: store.User{ID: "user2", Name: "user2"},
}
c3 := store.Comment{
- ID: "comment-3",
- Text: "reply to c1 from user3",
- ParentID: "comment-1",
- Locator: store.Locator{URL: "https://radio-t.com/blah1", SiteID: "remark42"},
- User: store.User{ID: "user3", Name: "user3"},
+ ID: "comment-3",
+ Text: "reply to c1 from user3",
+ ParentID: "comment-1",
+ Timestamp: base.Add(2 * time.Millisecond),
+ Locator: store.Locator{URL: "https://radio-t.com/blah1", SiteID: "remark42"},
+ User: store.User{ID: "user3", Name: "user3"},
}
c4 := store.Comment{
- ID: "comment-4",
- Text: "reply to c2 from developer one",
- ParentID: "comment-2",
- Locator: store.Locator{URL: "https://radio-t.com/blah1", SiteID: "remark42"},
- User: store.User{ID: "dev", Name: "developer one"},
+ ID: "comment-4",
+ Text: "reply to c2 from developer one",
+ ParentID: "comment-2",
+ Timestamp: base.Add(3 * time.Millisecond),
+ Locator: store.Locator{URL: "https://radio-t.com/blah1", SiteID: "remark42"},
+ User: store.User{ID: "dev", Name: "developer one"},
}
c5 := store.Comment{
- ID: "comment-5",
- Text: "developer one",
- Locator: store.Locator{URL: "https://radio-t.com/blah1", SiteID: "remark42"},
- User: store.User{ID: "dev", Name: "developer one"},
+ ID: "comment-5",
+ Text: "developer one",
+ Timestamp: base.Add(4 * time.Millisecond),
+ Locator: store.Locator{URL: "https://radio-t.com/blah1", SiteID: "remark42"},
+ User: store.User{ID: "dev", Name: "developer one"},
}
_, err := srv.DataService.Create(c1)
@@ -270,12 +280,6 @@ func TestServer_RssReplies(t *testing.T) {
assert.Equal(t, http.StatusBadRequest, code)
}
-func waitOnSecChange() {
- for time.Now().Nanosecond() >= 100000000 {
- time.Sleep(10 * time.Nanosecond)
- }
-}
-
// clean formatting, i.e. multiple spaces, \t, \n
func cleanRssFormatting(expected, actual string) (cleanExp, cleanAct string) {
reSpaces := regexp.MustCompile(`[\s\p{Zs}]{2,}`)
diff --git a/backend/app/rest/proxy/image_test.go b/backend/app/rest/proxy/image_test.go
index 7b02240a..a7f03971 100644
--- a/backend/app/rest/proxy/image_test.go
+++ b/backend/app/rest/proxy/image_test.go
@@ -813,8 +813,8 @@ func imgHTTPTestsServer(t *testing.T) *httptest.Server {
return
}
if r.URL.Path == "/image/img-slow.png" {
- time.Sleep(500 * time.Millisecond)
- w.WriteHeader(500)
+ // hold the response until the proxy gives up on its own timeout
+ <-r.Context().Done()
return
}
t.Log("http img request - not found", r.URL)
diff --git a/backend/app/store/comment.go b/backend/app/store/comment.go
index 108b4ce5..0622efd6 100644
--- a/backend/app/store/comment.go
+++ b/backend/app/store/comment.go
@@ -4,6 +4,7 @@ import (
"fmt"
"html/template"
"regexp"
+ "slices"
"strings"
"time"
@@ -151,8 +152,8 @@ func (c *Comment) Snippet(limit int) string {
}
snippet := []rune(cleanText)[:limit]
// go back in snippet and found the first space
- for i := len(snippet) - 1; i >= 0; i-- {
- if snippet[i] == ' ' {
+ for i, s := range slices.Backward(snippet) {
+ if s == ' ' {
snippet = snippet[:i]
break
}
diff --git a/backend/app/store/image/image.go b/backend/app/store/image/image.go
index 72ac500e..27cdba4f 100644
--- a/backend/app/store/image/image.go
+++ b/backend/app/store/image/image.go
@@ -43,8 +43,8 @@ type Service struct {
wg sync.WaitGroup
submitCh chan submitReq
once sync.Once
- term int32 // term value used atomically to detect emergency termination
- submitCount int32 // atomic increment for counting submitted images
+ term atomic.Int32 // term value used atomically to detect emergency termination
+ submitCount atomic.Int32 // atomic increment for counting submitted images
}
// ServiceParams contains externally adjustable parameters of Service
@@ -113,7 +113,7 @@ func (s *Service) Submit(idsFn func() []string) {
s.wg.Go(func() {
for req := range s.submitCh {
// wait for EditDuration expiration with emergency pass on term
- for atomic.LoadInt32(&s.term) == 0 && time.Since(req.TS) <= s.EditDuration {
+ for s.term.Load() == 0 && time.Since(req.TS) <= s.EditDuration {
time.Sleep(time.Millisecond * 10) // small sleep to relive busy wait but keep reactive for term (close)
}
err := s.Commit(req.idsFn)
@@ -121,13 +121,13 @@ func (s *Service) Submit(idsFn func() []string) {
log.Printf("[WARN] image commit error %v", err)
}
- atomic.AddInt32(&s.submitCount, -1)
+ s.submitCount.Add(-1)
}
log.Printf("[INFO] image submitter terminated")
})
})
- atomic.AddInt32(&s.submitCount, 1)
+ s.submitCount.Add(1)
// reset cleanup timer before submitting the images
// to prevent them from being cleaned up while waiting for EditDuration to expire
@@ -196,14 +196,14 @@ func (s *Service) Close(ctx context.Context) {
case <-ctx.Done():
return
case <-ticker.C:
- if atomic.LoadInt32(&s.submitCount) == 0 {
+ if s.submitCount.Load() == 0 {
return
}
}
}
}
- atomic.StoreInt32(&s.term, 1) // enforce non-delayed commits for all ids left in submitCh
+ s.term.Store(1) // enforce non-delayed commits for all ids left in submitCh
waitForTerm(ctx)
if s.submitCh != nil {
diff --git a/backend/app/store/service/service_test.go b/backend/app/store/service/service_test.go
index dbaf69da..dd9e4d9d 100644
--- a/backend/app/store/service/service_test.go
+++ b/backend/app/store/service/service_test.go
@@ -38,6 +38,7 @@ func TestService_CreateFromEmpty(t *testing.T) {
User: store.User{IP: "192.168.1.1", ID: "user", Name: "name"},
Locator: store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"},
}
+ beforeCreate := time.Now()
id, err := b.Create(comment)
assert.NoError(t, err)
assert.True(t, id != "", id)
@@ -46,7 +47,7 @@ func TestService_CreateFromEmpty(t *testing.T) {
assert.NoError(t, err)
t.Logf("%+v", res)
assert.Equal(t, "text", res.Text)
- assert.True(t, time.Since(res.Timestamp).Seconds() < 1)
+ assert.WithinRange(t, res.Timestamp, beforeCreate, time.Now(), "timestamp set during create")
assert.Equal(t, "user", res.User.ID)
assert.Equal(t, "name", res.User.Name)
assert.Equal(t, "23f97cf4d5c29ef788ca2bdd1c9e75656c0e4149", res.User.IP)
@@ -218,9 +219,9 @@ func TestService_Put(t *testing.T) {
}
func TestService_SetTitle(t *testing.T) {
- var titleEnable int32
+ var titleEnable atomic.Int32
tss := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- if atomic.LoadInt32(&titleEnable) == 0 {
+ if titleEnable.Load() == 0 {
w.WriteHeader(404)
}
if r.URL.String() == "/post1" {
@@ -262,7 +263,7 @@ func TestService_SetTitle(t *testing.T) {
b.TitleExtractor.cache.Purge()
- atomic.StoreInt32(&titleEnable, 1)
+ titleEnable.Store(1)
c, err := b.SetTitle(store.Locator{URL: tss.URL + "/post1", SiteID: "radio-t"}, id)
require.NoError(t, err)
assert.Equal(t, "post1 blah 123", c.PostTitle)
@@ -1885,8 +1886,8 @@ func TestService_alterCommentsFlagCaching(t *testing.T) {
}
svc := DataStore{Engine: &engineMock}
- var comments []store.Comment
- for i := 0; i < 5; i++ {
+ comments := make([]store.Comment, 0, 5)
+ for i := range 5 {
comments = append(comments, store.Comment{ID: fmt.Sprintf("c%d", i),
User: store.User{ID: "u1"}, Locator: store.Locator{SiteID: "site1"}})
}
diff --git a/backend/app/store/service/title_test.go b/backend/app/store/service/title_test.go
index 085ec8e0..501a7df5 100644
--- a/backend/app/store/service/title_test.go
+++ b/backend/app/store/service/title_test.go
@@ -44,10 +44,10 @@ func TestTitle_GetTitle(t *testing.T) {
func TestTitle_Get(t *testing.T) {
ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}, []string{"127.0.0.1"})
defer ex.Close()
- var hits int32
+ var hits atomic.Int32
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.String() == "/good" {
- atomic.AddInt32(&hits, 1)
+ hits.Add(1)
_, err := w.Write([]byte("\n\n blah 123\n 2222"))
assert.NoError(t, err)
return
@@ -68,7 +68,7 @@ func TestTitle_Get(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, "blah 123", r)
}
- assert.Equal(t, int32(1), atomic.LoadInt32(&hits))
+ assert.Equal(t, int32(1), hits.Load())
}
func TestTitle_GetConcurrent(t *testing.T) {
@@ -78,10 +78,10 @@ func TestTitle_GetConcurrent(t *testing.T) {
}
ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}, []string{"127.0.0.1"})
defer ex.Close()
- var hits int32
+ var hits atomic.Int32
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if strings.HasPrefix(r.URL.String(), "/good") {
- atomic.AddInt32(&hits, 1)
+ hits.Add(1)
_, err := fmt.Fprintf(w, "blah 123 %s%s", r.URL.String(), body.String())
assert.NoError(t, err)
return
@@ -100,15 +100,15 @@ func TestTitle_GetConcurrent(t *testing.T) {
})
}
g.Wait()
- assert.Equal(t, int32(100), atomic.LoadInt32(&hits))
+ assert.Equal(t, int32(100), hits.Load())
}
func TestTitle_GetFailed(t *testing.T) {
ex := NewTitleExtractor(http.Client{Timeout: 5 * time.Second}, []string{"127.0.0.1"})
defer ex.Close()
- var hits int32
+ var hits atomic.Int32
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
- atomic.AddInt32(&hits, 1)
+ hits.Add(1)
w.WriteHeader(404)
}))
defer ts.Close()
@@ -121,7 +121,7 @@ func TestTitle_GetFailed(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, "", r)
}
- assert.Equal(t, int32(1), atomic.LoadInt32(&hits), "hit once, errors cached")
+ assert.Equal(t, int32(1), hits.Load(), "hit once, errors cached")
}
func TestTitle_DoubleClosed(t *testing.T) {
diff --git a/docs/backlog/api-tests-deadlock-on-macos.md b/docs/backlog/api-tests-deadlock-on-macos.md
index 2da47a31..a8ed47da 100644
--- a/docs/backlog/api-tests-deadlock-on-macos.md
+++ b/docs/backlog/api-tests-deadlock-on-macos.md
@@ -5,13 +5,13 @@ added: 2026-08-18
---
# TestRest_securityHeaders deadlocks the whole api suite on macOS
-`cd backend/app && go test -timeout=60s -count 1 ./...` fails on macOS with the `app/rest/api` package
+`cd backend/app && go test -timeout=300s -count 1 ./...` fails on macOS with the `app/rest/api` package
timing out. It reproduced 3 times out of 3 in isolation with
`go test -run 'TestRest_securityHeaders$' -timeout=40s`, so it is deterministic rather than flaky, and it
takes the whole package down with it: 14 of 15 packages pass, that one never finishes. CI is unaffected,
the `Tests` job passes on Linux.
-The test blocks in `startupT`'s teardown (`rest_test.go:504`), inside `httptest.Server.Close()`, which
+The test blocks in `startupT`'s teardown (`rest_test.go:676`), inside `httptest.Server.Close()`, which
waits on the server's `WaitGroup` for handlers still in flight. The handler it is waiting on sits in IO
wait on a socket write:
diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go
index 9a20132d..a26a0ab6 100644
--- a/e2e/e2e_test.go
+++ b/e2e/e2e_test.go
@@ -538,7 +538,7 @@ func eventually(t *testing.T, timeout time.Duration, msg string, fn func() bool)
// pageFetch issues a request from the page's own session, so it carries the browser's cookies
// and the XSRF header the widget's own calls carry. body may be nil for a bodyless request
-func pageFetch(t *testing.T, page playwright.Page, method, url string, body interface{}) (status int, respBody string) {
+func pageFetch(t *testing.T, page playwright.Page, method, url string, body any) (status int, respBody string) {
t.Helper()
payload := ""
@@ -560,10 +560,10 @@ func pageFetch(t *testing.T, page playwright.Page, method, url string, body inte
body: payload === '' ? undefined : payload,
});
return {status: resp.status, body: await resp.text()};
- }`, map[string]interface{}{"method": method, "url": url, "payload": payload})
+ }`, map[string]any{"method": method, "url": url, "payload": payload})
require.NoError(t, err)
- out, ok := res.(map[string]interface{})
+ out, ok := res.(map[string]any)
require.True(t, ok, "unexpected shape from the page: %#v", res)
switch v := out["status"].(type) {
case int:
diff --git a/e2e/widgets_test.go b/e2e/widgets_test.go
index 07d27b13..ca58ccff 100644
--- a/e2e/widgets_test.go
+++ b/e2e/widgets_test.go
@@ -49,7 +49,7 @@ func TestWidgets_CounterFillsInTheCommentCount(t *testing.T) {
before := commentCount(t, poster, counted)
for i := range 2 {
- status, body := pageFetch(t, poster, "POST", baseURL+"/api/v1/comment?site=remark", map[string]interface{}{
+ status, body := pageFetch(t, poster, "POST", baseURL+"/api/v1/comment?site=remark", map[string]any{
"text": fmt.Sprintf("counted %d %s", i, runID),
"locator": map[string]string{"site": "remark", "url": counted},
})