* 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.
This commit is contained in:
Dmitry Verkhoturov
@@ -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
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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")
|
||||
|
||||
+2
-2
@@ -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/<name>.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/<name>.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 <http://127.0.0.1:8025> for anything email.
|
||||
|
||||
|
||||
+4
-6
@@ -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 {
|
||||
|
||||
@@ -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' } }));
|
||||
|
||||
|
||||
@@ -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<string, Record<string, Comment['id'][]>>;
|
||||
|
||||
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<unknown>(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
|
||||
|
||||
Reference in New Issue
Block a user