Cover in the browser what jest was covering alone, and drop those cases

docs/backlog/jest-cases-the-browser-suite-could-take.md

@@ -9,34 +9,40 @@ Removing a jest case is only safe when a named e2e case asserts the same behavio
 assertion. Working through the suites on that rule, the cases that stay divide into a few recurring
 causes, and most of them are a gap in the browser suite rather than something it cannot reach.
 
-This is the list to work from. It is deliberately not acted on in the same change as the deletions:
-each item is an e2e case to write, and the jest cases it releases can then go with it.
+This was the list to work from. The items marked **done** below have their browser case and their
+jest cases have gone with them; what is left is what is still open, and the reasons it is open are
+worth more than the list itself.
 
 ## Selectors the browser cannot reach
 
 `tasks/babel-plugin-remove-test-id.js` removes `data-testid` from anything but a test build, so a
 browser case cannot select by it. Only one of the elements these jest cases assert through is
 actually stuck behind that:
 
-- `comments-counter` in `profile.spec.tsx`, asserted in four cases, is
+- **done.** `comments-counter` in `profile.spec.tsx` is
   `<div className={styles.container} data-testid="comments-counter">`. Its only class is hashed by
   the CSS modules, so nothing outside the bundle can name it. Giving it a stable class, the way
   `.auth-button`, `.auth-submit`, `.comment-actions` and `.sort-picker` are kept outside the
   modules for this reason, is what makes the counter assertable in a browser
 
-Three others are already reachable and simply have no browser case written yet, which is a smaller
+Two others are already reachable and simply have no browser case written yet, which is a smaller
 job than it looked:
 
-- `spinner` renders `clsx('spinner', styles.root, …)` with `role="presentation"`
-- `preloader` renders `clsx('preloader', className)` with `aria-label="Loading..."`
-- `comment-actions-additional` renders `clsx('comment-actions-additional', …)`, so the order of the
-  admin actions can be asserted from a browser case today
+- `spinner` renders `clsx('spinner', styles.root, …)` and is still open. It is the *between-pages*
+  indicator at `profile.tsx:162-165`, shown in place of the Load more button once a page is being
+  fetched, so it is reachable only when `comments` is not null. The profile's initial load and its
+  failure are a different element, the `Preloader` at `profile.tsx:221`, and that one is covered by
+  `TestProfile_LoadingAndFailureStates`; the two are easy to conflate and are not the same thing
+- **done for the telegram panel.** `preloader` renders `clsx('preloader', className)`, asserted
+  by `TestTelegramSub_ThePanelSaysItIsWorking`
+- **done.** `comment-actions-additional` carries a stable class, and the order is asserted by
+  `TestComment_AdminActionsKeepTheirOrder`
 
 ## Transient states nothing waits on
 
-- buttons disabled while a vote request is in flight (`comment-votes.spec.tsx`, three cases)
-- the loading indicator in the telegram subscription panel
-- the spinner between pages of the profile list
+- **done.** buttons disabled while a vote request is in flight, `TestVote_BothButtonsAreDisabledWhileTheVoteIsInFlight`
+- **done.** the loading indicator in the telegram subscription panel
+- **done.** the profile's loading and error states
 - the preloader that must not reappear after a load-more click
 
 Each needs a browser case that holds the request open, which the suite already knows how to do:
@@ -47,35 +53,47 @@ before releasing it.
 
 The browser suite asserts what appears far more readily than what does not. Cases kept for this:
 
-- the Reply action gone in a read-only thread. `TestComment_ReadOnlyThreadTakesTheFormAway`
-  asserts the comment form is gone and says nothing about the action
-- Hide absent on a reader's own comment, Delete absent on another reader's
-- the verification icon absent on an unverified user
+- **done.** the Reply action gone in a read-only thread, now asserted in
+  `TestComment_ReadOnlyThreadTakesTheFormAway`, which posts a comment first so the assertion has
+  something to be about
+- **done.** Hide absent on a reader's own comment, Delete absent on another reader's,
+  `TestComment_ActionsDependOnWhoseCommentItIs`
+- **done.** the verification icon absent before an admin verifies
 - the auth dropdown starting closed
 
 ## Configurations no instance runs
 
-- `email_notifications` and `telegram_notifications` off. The stack covers
-  `show_rss_subscription` and `show_email_subscription`, which are different settings
+- **done.** `email_notifications` and `telegram_notifications` off, each instance being the
+  other's negative case
 - upvote-only voting, and voting hidden altogether
 - the controversy tooltip, which needs a comment with controversy in it
 
 ## Values inside an element, where the browser case only waits for the element
 
-- the edit countdown's remaining seconds. `TestComment_EditWithinTheDeadline` waits for the timer
-  and `TestComment_EditExpiresAfterTheDeadline` waits for it to go, so a blank timer passes both
+- **done.** the edit countdown's value, `TestComment_EditCountdownCountsDown`
 - the telegram link's full `https://t.me/<bot>/?start=<token>`, where the browser helper parses out
   the `start` parameter and never looks at the host or the bot name
 
 ## Error branches that are not the one the browser drives
 
 - the generic fallback message for an unrecognised code. The browser suite fulfils a 409 and
   asserts the catalogued string, which never enters that branch
-- a failed check or unsubscribe being cleared by a later success, in the telegram panel
+- **done.** a failed check or unsubscribe cleared by a later success, driven separately because
+  the two handlers clear their own error
 
 ## What is not worth moving
 
 Call counts and call arguments (`api.telegramSubscribe` called once, `getUserComments` called with
 a page size) assert how a client method was used, not what a reader gets. The browser suite asserts
 the request and the response instead, which is the better test of the same thing, so these stay in
 jest or go away on their own when the code changes.
+
+
+## What the static build would settle on its own
+
+Thirty-four of the cases still in jest exist because of the build rather than because of the
+behaviour: eighteen assert a hashed css-module class and sixteen select by a `data-testid` the
+production bundle strips. With static css there is no hash to pin and the class in the source is
+the class in the browser, so those assertions have nothing left to say and the browser can select
+what ships. Covering them now would mean writing browser cases whose purpose disappears with the
+build, which is why they are left here rather than done.

e2e/README.md

@@ -207,9 +207,11 @@ Everything else runs in Chromium alone, for the same reason inverted: those test
 
 ## Selectors
 
-The production bundle strips `data-testid`, so tests use what ships: the stable class hooks the widget keeps outside CSS modules (`.auth-button`, `.auth-submit`, `.comment-actions`, `.sort-picker`, `.preloader`), `title` attributes on icon-only controls, and visible text. Three shapes are worth knowing:
+The production bundle strips `data-testid`, so tests use what ships: the stable class hooks the widget keeps outside CSS modules (`.auth-button`, `.auth-submit`, `.comment-actions`, `.comment-actions-additional`, `.sort-picker`, `.preloader`, `.comments-counter`), `title` attributes on icon-only controls, and visible text. These shapes are worth knowing:
 
 - `.auth` only exists while signed out, so waiting on it hangs after sign-in. `widget()` waits on the comment form, which is present either way.
 - The production build hashes every css-module class name to a short opaque id, so a component's own class is not something a test can hold. `role` is: the footer is `[role="contentinfo"]` and the edit countdown is `[role="timer"]`.
 - Comments render through an IntersectionObserver, so one below the fold is an empty `article` with no text in it. That makes any absence assertion written as a text filter pass whether the comment is gone or merely off screen; count articles instead, which is what `articleCount` is for.
 - Collapsing a thread hides the comment text, so a locator filtered by that text stops matching the element under test. `TestThread_CollapsePersistsAcrossReload` anchors on the comment's id instead.
+- `text=Foo` matches a case-insensitive substring, so a comment whose own text contains the phrase satisfies a locator meant for the page's own copy, and two matches is a strict-mode failure. `text="Foo"` matches exactly. This surfaces late: a comment below the fold is empty under the IntersectionObserver, so the collision can pass locally and fail in CI.
+- `?` is a single-character wildcard in the glob `page.Route` takes, so a pattern written with a query string never matches the request it names, the route is never intercepted, and the case passes against an unmodified response. Use a regexp for anything with a query string, as `TestProfile_LoadingAndFailureStates` does.

e2e/comment_test.go

@@ -9,6 +9,8 @@ import (
 	neturl "net/url"
 	"os"
 	"path/filepath"
+	"regexp"
+	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -77,6 +79,10 @@ func TestComment_EditWithinTheDeadline(t *testing.T) {
 	// the countdown only renders while the comment is still editable
 	waitVisible(t, actions(frame, original).Locator(`[role="timer"]`))
 	require.NoError(t, actions(frame, original).Locator(`button:has-text("Edit")`).Click())
+	// the action becomes its own way out; leaving Edit in place strands a reader in the editor
+	waitVisible(t, actions(frame, original).Locator(`button:has-text("Cancel")`))
+	waitHidden(t, actions(frame, original).Locator(`button:has-text("Edit")`),
+		"the Edit action stayed beside Cancel while the comment was being edited")
 
 	edited := "after edit " + runID
 	submitForm(t, replyForm(t, frame), edited)
@@ -298,6 +304,9 @@ func TestComment_AdminPinsAndVerifies(t *testing.T) {
 	waitVisible(t, adminFrame.Locator(`[role="region"][aria-label="Pinned comments"]`))
 
 	// the verification toggle sits in the comment header beside the author, not in the action bar
+	// nothing marks an author verified until an admin says so, and the icon is what says it did
+	waitHidden(t, comment(adminFrame, text).Locator(`[title="Verified user"]`).First(),
+		"the author was shown as verified before anyone verified them")
 	require.NoError(t, comment(adminFrame, text).Locator(`[title="Toggle verification"]`).First().Click())
 	waitVisible(t, comment(adminFrame, text).Locator(`[title="Verified user"]`).First())
 
@@ -445,6 +454,12 @@ func TestComment_ReadOnlyThreadTakesTheFormAway(t *testing.T) {
 	frame := openURL(t, page, url)
 	signInDev(t, page, frame)
 
+	// a comment to hang the Reply action on. Without one the thread is empty and an assertion that
+	// Reply is gone passes whether or not read-only has anything to do with it
+	text := "locked thread reply " + runID
+	postComment(t, frame, text)
+	waitVisible(t, actions(frame, text).Locator(`button:has-text("Reply")`))
+
 	// the admin panel swaps its own button instead of showing the read-only notice, which is
 	// what an ordinary reader gets
 	require.NoError(t, frame.Locator(`button:has-text("Disable comments")`).Click())
@@ -457,10 +472,150 @@ func TestComment_ReadOnlyThreadTakesTheFormAway(t *testing.T) {
 	require.NoError(t, err)
 
 	readerFrame := reader.FrameLocator("#remark42 iframe")
-	waitVisible(t, readerFrame.Locator(`text=Read-only`))
+	// an exact match on the status: `text=` is a case-insensitive substring, so any comment whose
+	// own text contains the phrase would satisfy it too, and two matches is a strict-mode failure
+	waitVisible(t, readerFrame.Locator(`text="Read-only"`))
 	waitHidden(t, readerFrame.Locator(commentFormSel).First(),
 		"the thread is read-only but a reader is still shown a comment form")
+	// the form and the per-comment Reply action are separate controls, and a thread that takes one
+	// away has to take the other with it
+	waitVisible(t, comment(readerFrame, text))
+	waitHidden(t, readerFrame.Locator(`button:has-text("Reply")`).First(),
+		"the thread is read-only but a reader is still offered Reply on a comment")
 
 	require.NoError(t, frame.Locator(`button:has-text("Enable comments")`).Click())
 	waitVisible(t, frame.Locator(commentFormSel).First())
 }
+
+// TestComment_AdminActionsKeepTheirOrder covers the order of the moderation actions, which nothing
+// else asserts: every other case reaches one of them by name, so any arrangement passes.
+//
+// The order is what a moderator's hand learns, and Delete sits at the end of it deliberately. A
+// reshuffle that moved Delete next to Hide would pass every other case in this file while making
+// the destructive action the neighbor of a routine one.
+func TestComment_AdminActionsKeepTheirOrder(t *testing.T) {
+	t.Parallel()
+
+	text := "admin action order " + runID
+	author := newPage(t)
+	authorFrame := openThread(t, author)
+	signInAnon(t, author, authorFrame, anonName("actionorder"))
+	postComment(t, authorFrame, text)
+
+	admin := newPage(t)
+	adminFrame := openURL(t, admin, threadURL(t))
+	signInDev(t, admin, adminFrame)
+
+	// the class is kept outside the css modules for this: the production bundle hashes the rest
+	// and strips the data-testid the unit suite selects by
+	additional := comment(adminFrame, text).Locator(".comment-actions-additional").First()
+	waitVisible(t, additional)
+
+	labels, err := additional.Locator("> *").AllTextContents()
+	require.NoError(t, err)
+	require.Len(t, labels, 5, "the moderation menu no longer holds five actions: %v", labels)
+
+	want := []string{"Hide", "Copy", "Pin", "Block", "Delete"}
+	for i, expected := range want {
+		assert.Contains(t, labels[i], expected,
+			"the moderation actions are out of order, wanted %v, got %v", want, labels)
+	}
+
+	// the label switches, which is the render branch under test. It says nothing about the
+	// clipboard: copyComment sets isCopied outside its own catch, so the label appears whether the
+	// write succeeded or threw
+	require.NoError(t, additional.Locator(`button:has-text("Copy")`).Click())
+	waitVisible(t, additional.Locator(`button:has-text("Copied!")`))
+}
+
+// TestComment_EditCountdownCountsDown covers what the countdown says, which nothing else reads.
+// TestComment_EditWithinTheDeadline waits for the element and TestComment_EditExpiresAfterTheDeadline
+// waits for it to go, so a timer rendering blank, showing the wrong unit, or frozen at its starting
+// value passes both of them while telling the reader nothing about how long they have left.
+//
+// Runs against the short-edit instance so the window is small enough to watch a tick.
+func TestComment_EditCountdownCountsDown(t *testing.T) {
+	t.Parallel()
+
+	page := newPage(t)
+	frame := openURL(t, page, threadURLOn(t, shortEditURL))
+	signInAnon(t, page, frame, anonName("countdown"))
+
+	text := "countdown " + runID
+	postedAt := time.Now()
+	postComment(t, frame, text)
+
+	timer := actions(frame, text).Locator(`[role="timer"]`)
+	waitVisible(t, timer)
+
+	// the whole shape, not just the digits: trimming a suffix that is not there and parsing what
+	// is left accepts a bare number, and the unit is part of what the reader is being told
+	countdownShape := regexp.MustCompile(`^\d+s$`)
+	seconds := func() int {
+		t.Helper()
+		// pollText and not TextContent: this runs inside eventually, and a bare read carries
+		// playwright's own 30s default, which outlives the loop's budget and reports the wrong
+		// failure
+		shown, err := pollText(timer)
+		require.NoError(t, err)
+		trimmed := strings.TrimSpace(shown)
+		require.Truef(t, countdownShape.MatchString(trimmed),
+			"the countdown reads %q, which is not a number of seconds", shown)
+		n, convErr := strconv.Atoi(strings.TrimSuffix(trimmed, "s"))
+		require.NoError(t, convErr)
+		return n
+	}
+
+	first := seconds()
+	// bounded from below as well as above, or a countdown starting at 2s would satisfy the shape,
+	// stay under the window and still decrease while telling the reader something wrong. The floor
+	// comes from the time actually spent since the comment was posted rather than a fixed number,
+	// since under -parallel 4 the setup can take seconds the scheduler decides
+	spent := int(time.Since(postedAt).Seconds())
+	assert.GreaterOrEqual(t, first, int(editWindow.Seconds())-spent-1,
+		"the countdown started %ds below the edit window, having spent %ds getting there", int(editWindow.Seconds())-first, spent)
+	// the widget rounds the remaining time up, so a fifteen second window reads 16 at the moment
+	// the comment lands
+	assert.LessOrEqual(t, first, int(editWindow.Seconds())+1,
+		"the countdown starts above the edit window the instance was given")
+
+	// it has to move, or a value hard-coded at the window would satisfy everything above
+	eventually(t, waitTimeout, "the countdown never decreased", func() bool {
+		return seconds() < first
+	})
+}
+
+// TestComment_ActionsDependOnWhoseCommentItIs covers which moderation actions an ordinary reader is
+// offered, which the rest of the suite only ever exercises as an admin: TestThread_HideUserRemovesTheirCommentsOnly
+// clicks Hide from a reader signed in with signInDev, and the stack gives that user ADMIN_SHARED_ID,
+// so making either action admin-only would pass every other case here.
+//
+// One reader, two comments, so each half is the other's positive control: a rule that dropped both
+// actions everywhere would satisfy an absence assertion on its own.
+func TestComment_ActionsDependOnWhoseCommentItIs(t *testing.T) {
+	t.Parallel()
+
+	other := newPage(t)
+	otherFrame := openThread(t, other)
+	signInAnon(t, other, otherFrame, anonName("actorsforeign"))
+	foreign := "foreign comment " + runID
+	postComment(t, otherFrame, foreign)
+
+	reader := newPage(t)
+	readerFrame := openURL(t, reader, threadURL(t))
+	signInAnon(t, reader, readerFrame, anonName("actorsown"))
+	own := "own comment " + runID
+	postComment(t, readerFrame, own)
+
+	waitVisible(t, comment(readerFrame, foreign))
+
+	// on their own comment the reader may delete but has nobody to hide
+	waitVisible(t, actions(readerFrame, own).Locator(`button:has-text("Delete")`))
+	waitHidden(t, actions(readerFrame, own).Locator(`button:has-text("Hide")`),
+		"the widget offered to hide the reader's own author")
+
+	// on another reader's comment the reverse: hideable, not deletable
+	waitVisible(t, actions(readerFrame, foreign).Locator(`button:has-text("Hide")`))
+	waitHidden(t, actions(readerFrame, foreign).Locator(`button:has-text("Delete")`),
+		"the widget offered an ordinary reader the delete action on someone else's comment")
+}

e2e/config_test.go

@@ -102,6 +102,12 @@ func TestConfig_SubscriptionControlsCanBeHidden(t *testing.T) {
 			rss := frame.Locator(`[title="Subscribe by RSS"]`)
 			byMail := frame.Locator(`[title="Subscribe by Email"]`)
 
+			// this instance runs NOTIFY_USERS=email, so the telegram control has to be absent
+			// whatever the display settings say. The email control beside it is the positive
+			// control: a widget offering no subscriptions at all would satisfy the absence alone
+			waitHidden(t, frame.Locator(`[title="Subscribe by Telegram"]`),
+				"the telegram control was offered on an instance with telegram notifications off")
+
 			if tc.rss {
 				waitVisible(t, rss)
 			} else {