From 49bf83b09cb148e41d7674e6d7123ed517c7753c Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Sat, 22 Aug 2026 08:23:56 +0100 Subject: [PATCH] Address the review follow-ups from #2188, #2189 and #2190 (#2193) * Read the collapsed-threads key through getJsonItem `getFromLocalStorage` parsed the stored string directly, so anything malformed under `__remarkCollapsed` threw out of `restoreCollapsedThreads`. That call sits in `remark.tsx` ahead of the `render`, so the throw took the whole widget with it: the reader was left on the preloader, over a view preference. `getJsonItem` in `common/local-storage.ts` already wraps a parse of a localStorage key and returns null on failure, and null is a shape the check below already reads as empty. The rest of that function is total against whatever the browser holds, and the bare parse was the one way in. * Stop retrying a failed e2e test in CI The suite went in with one gotestsum rerun. It has no failures on record to justify that: 31 CI runs since it landed, all green, and no rerun report has ever been produced. A retry is what turns an intermittent regression into a green build, and while the suite is this young its own failures are the evidence worth keeping. `E2E_RUN_ID` stays. It stamps the threads a run works on with the CI run id, so a thread url in a trace or a log names the run it came from. It carries no data across: the stack is disposable, and a local run under the same id gets those urls on an empty database. * Stop two chooseUnusedPort comments claiming collisions cannot happen All four copies listen on :0, read the assigned port, close the listener and bind later, so nothing holds the number across that gap and another binary can take it. The copies in app/cmd and app/rest/api call a collision very unlikely, which is accurate; the ones in app and the example module said binaries never land on the same number, which is not, and a comment ruling out a port collision is what would send the next person chasing one somewhere else. All four now read the same. Closing the window rather than describing it means the server binding :0 itself and reporting the address it got, which is a larger change. --- .github/workflows/e2e-tests.yml | 38 +++++-------------- .../_example/memory_store/server/rpc_test.go | 4 +- backend/app/main_test.go | 4 +- e2e/README.md | 4 +- e2e/e2e_test.go | 10 ++--- .../remark42/app/store/thread/utils.test.ts | 14 +++++++ .../apps/remark42/app/store/thread/utils.ts | 7 +++- 7 files changed, 39 insertions(+), 42 deletions(-) diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index d83a81ba..d8af5b5b 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -59,8 +59,8 @@ jobs: tests: name: Tests needs: vet - # generous against the docker build plus two go test invocations: a job cancelled on - # timeout skips its own failure steps, so the run would end with neither logs nor traces + # generous against the docker build plus one 8m go test: a job cancelled on timeout skips + # its own failure steps, so the run would end with neither logs nor traces timeout-minutes: 45 runs-on: ubuntu-latest permissions: @@ -91,43 +91,25 @@ jobs: - name: Build & start the stack run: COMPOSE_DOCKER_CLI_BUILD=1 DOCKER_BUILDKIT=1 docker compose -f compose-e2e-test.yml up -d --build --quiet-pull --wait - - name: Install gotestsum - run: go install gotest.tools/gotestsum@v1.13.0 - - # each rerun is its own `go test` process carrying the same timeout, so the ceiling is - # 8m for the first attempt plus 4 x 8m of reruns, inside the job's 45. a job cancelled on - # timeout skips its own upload steps, which is the one outcome worth designing against. - # four failures covers one test failing across all three engines, counted as its three - # subtests plus their parent; more than that is a regression and should not be rerun. - # a browser suite has a flake floor no amount of care removes, and one flake failing the - # build is what makes people stop trusting it. one rerun, not two: a rerun stops at the - # first pass, so each further attempt only widens the window in which a real intermittent - # regression is absorbed. what needed a rerun is written to a report and uploaded, since - # a green job is a job nobody reads the log of + # no retry: a failure here is evidence about a suite too young to have a flake rate, + # and a rerun is how an intermittent regression becomes invisible. revisit when there + # are failures on record to look at - name: Run e2e - # shared across the rerun so it exercises the same threads as the attempt that failed + # stamps this run's comment threads with the CI run, so a thread url in a trace or a + # log names the run it came from env: E2E_RUN_ID: ${{ github.run_id }}-${{ github.run_attempt }} - run: | - cd e2e && gotestsum \ - --rerun-fails=1 \ - --rerun-fails-max-failures=4 \ - --rerun-fails-report=rerun-report.txt \ - --packages=./... \ - --format standard-verbose \ - -- -tags=e2e -count 1 -timeout 8m + run: cd e2e && go test -tags=e2e -count 1 -timeout 8m -v ./... - name: Server logs on failure if: failure() run: docker compose -f compose-e2e-test.yml logs --tail=200 - - name: Upload browser traces and any rerun report + - name: Upload browser traces if: always() uses: actions/upload-artifact@v7 with: name: playwright-traces - path: | - e2e/traces/ - e2e/rerun-report.txt + path: e2e/traces/ retention-days: 30 if-no-files-found: ignore diff --git a/backend/_example/memory_store/server/rpc_test.go b/backend/_example/memory_store/server/rpc_test.go index c9eee5e5..e16f8baa 100644 --- a/backend/_example/memory_store/server/rpc_test.go +++ b/backend/_example/memory_store/server/rpc_test.go @@ -19,8 +19,8 @@ import ( "github.com/umputun/remark42/memory_store/accessor" ) -// chooseUnusedPort asks the kernel for a free port from the ephemeral range, so concurrently -// running package test binaries never land on the same number +// 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") diff --git a/backend/app/main_test.go b/backend/app/main_test.go index d9ba1f8b..257b213c 100644 --- a/backend/app/main_test.go +++ b/backend/app/main_test.go @@ -128,8 +128,8 @@ func TestGetDump(t *testing.T) { t.Logf("\n dump: %s", dump) } -// chooseUnusedPort asks the kernel for a free port from the ephemeral range, so concurrently -// running package test binaries never land on the same number +// 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") diff --git a/e2e/README.md b/e2e/README.md index 3c39530a..c373ddaa 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -43,9 +43,9 @@ The build tag keeps these out of `go test ./...`; nothing runs without `-tags=e2 ## When something fails -A failed test writes a Playwright trace to `e2e/traces/`, which CI uploads as an artifact whether or not the job went green. Open one with `npx playwright show-trace e2e/traces/.zip`. +A failed test writes a Playwright trace to `e2e/traces/`, which CI uploads as an artifact. Nothing else writes one, so a run carrying the artifact is a run with a failure to look at. Open one with `npx playwright show-trace e2e/traces/.zip`. -CI runs the suite through `gotestsum` and gives a failing test one rerun, so a test that fails and then passes leaves the job green. That is the case worth looking at: it is named in `rerun-report.txt`, uploaded beside the traces. Only attempts that failed leave a trace, and they do not overwrite each other, so a flake leaves exactly one to open. `make e2e` locally does not rerun anything, so a test red on a laptop and green in CI is a flake with a report to read rather than a disagreement. +CI does not retry a failing test. The suite is young enough that a failure is evidence about the suite itself, and a retry is what would hide an intermittent regression. `E2E_RUN_ID` stamps the threads a run uses with the CI run id, so a thread url in a trace names the run it came from. It does not carry the data across: the stack is disposable, and a local run against the same id gets those urls on an empty database. Beyond that: `docker compose -f compose-e2e-test.yml logs` for the server side, and mailpit's web UI on for anything email. diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index a26a0ab6..682c109e 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -73,9 +73,8 @@ var ( extraBrowsersMu sync.Mutex // distinguishes this run's threads from those a previous run left in the database. - // a rerun is a fresh process, so without E2E_RUN_ID it would get its own threads and a - // failure caused by ordering or by state an earlier test left behind would pass on the - // second attempt whatever the code did + // E2E_RUN_ID pins it, so the urls a CI run works on name that run rather than the moment + // the process started runID = firstNonEmpty(os.Getenv("E2E_RUN_ID"), fmt.Sprintf("%d", time.Now().UnixNano())) authGate sync.Mutex @@ -271,9 +270,8 @@ func newPageOn(t *testing.T, b playwright.Browser) playwright.Page { } // say so rather than swallowing it: this runs only on a test that already failed, // and a silently missing trace is what the reader goes looking for - // the pid distinguishes attempts: a rerun is a fresh process that shares runID - // with the attempt it is retrying and restarts its own counter, so without it the - // rerun would overwrite the trace of the attempt that actually failed + // the pid keeps two processes sharing a run id, and so a trace directory, from + // overwriting each other: the counter restarts with every process name := strings.ReplaceAll(t.Name(), "/", "-") path := filepath.Join(traceDir, fmt.Sprintf("%s-%d-%d.zip", name, os.Getpid(), seq)) if serr := ctx.Tracing().Stop(path); serr != nil { diff --git a/frontend/apps/remark42/app/store/thread/utils.test.ts b/frontend/apps/remark42/app/store/thread/utils.test.ts index 3ed032a1..3b82ee2c 100644 --- a/frontend/apps/remark42/app/store/thread/utils.test.ts +++ b/frontend/apps/remark42/app/store/thread/utils.test.ts @@ -74,6 +74,20 @@ describe('collapsed comments storage', () => { expect(getCollapsedComments()).toEqual([]); }); + it('reads as empty when the stored value is not json', () => { + localStorage.setItem(LS_COLLAPSE_KEY, '{oops'); + + expect(getCollapsedComments()).toEqual([]); + }); + + it('starts a fresh entry when the stored value is not json', () => { + localStorage.setItem(LS_COLLAPSE_KEY, '{oops'); + + saveCollapsedComments(siteId, url, ['c1']); + + expect(getCollapsedComments()).toEqual(['c1']); + }); + it('reads as empty when the entry for the page is not a list of ids', () => { localStorage.setItem(LS_COLLAPSE_KEY, JSON.stringify({ [siteId]: { [url]: 'c1' } })); diff --git a/frontend/apps/remark42/app/store/thread/utils.ts b/frontend/apps/remark42/app/store/thread/utils.ts index b8bfe4c4..965fa02f 100644 --- a/frontend/apps/remark42/app/store/thread/utils.ts +++ b/frontend/apps/remark42/app/store/thread/utils.ts @@ -1,6 +1,6 @@ import { siteId, url } from 'common/settings'; import { LS_COLLAPSE_KEY } from 'common/constants'; -import { setItem as localStorageSetItem, getItem as localStorageGetItem } from 'common/local-storage'; +import { setItem as localStorageSetItem, getJsonItem } from 'common/local-storage'; import type { Comment } from 'common/types'; /** @@ -14,7 +14,10 @@ import type { Comment } from 'common/types'; type CollapsedComments = Record>; function getFromLocalStorage(): CollapsedComments { - const stored: unknown = JSON.parse(localStorageGetItem(LS_COLLAPSE_KEY) || '{}'); + // getJsonItem rather than a bare parse: the value is whatever is in the browser's storage, + // and a throw here would take down the restore this runs from, leaving the widget with no + // thread at all over a view preference + const stored = getJsonItem(LS_COLLAPSE_KEY); // anything of another shape, including the flat list this used to keep, reads as empty: // collapsed threads are a view preference, so re-expanding them once costs the reader