Fix E2E test quality issues: always-passing assertions, unawaited checks, and dead code

[voidmatcha]

Fix five E2E test quality issues across four spec files.

These issues were found using e2e-reviewer, a static analysis skill for Playwright/Cypress E2E tests that detects common anti-patterns.

---

crypto/migration.spec.ts — Remove vacuous toBeGreaterThanOrEqual(0)

// Before
expect(initialProgress).toBeGreaterThanOrEqual(0);
expect(initialProgress).toBeLessThanOrEqual(500);

// After
expect(initialProgress).toBeLessThanOrEqual(500);

toBeGreaterThanOrEqual(0) can never fail for a progress bar value. The adjacent toBeLessThanOrEqual(500) already bounds the valid range — and since NaN <= 500 evaluates to false, it also catches the parseFloat() returning NaN case. The >= 0 check is entirely redundant.

---

devtools/devtools.spec.ts / crypto/utils.ts — Fix unawaited isVisible() calls

// Before — Promise result discarded, check never runs
await page.getByText("Saved").isVisible();

// After
await expect(page.getByText("Saved")).toBeVisible();

isVisible() returns a Promise<boolean> without retrying or throwing. Without await, the expression resolves to a Promise object (always truthy), making the check a silent no-op. Playwright docs state: "Returns whether the element is visible. The method returns immediately (does not wait)" — web-first assertions like toBeVisible() retry automatically and throw on failure, which is the correct behaviour here.

---

spotlight/spotlight.spec.ts — Remove 1-hour waitForTimeout from a skipped test

// test.skip("should find unknown public rooms on other homeservers", ...)
- await page.waitForTimeout(3_600_000);
  await page.waitForTimeout(500); // wait for the dialog to settle

This line is inside a test.skip() block (the test requires federation which is unavailable in local e2e runs), so it never executes in CI. It was introduced in the original Playwright migration (matrix-react-sdk#12033, commit d1562bef, 2023-12-18) and appears to be a debugging artifact: a 1-hour pause so the developer could inspect browser state, left in before committing.

A possible alternative reading is that it was intentional — to make CI hang obviously if someone removes test.skip() prematurely. Either way, the // TODO comment directly above the test already communicates the intent clearly. A 1-hour hang is a poor signal; an explicit throw new Error("Requires federation — do not remove test.skip") would be clearer if a guard is desired.

---

threads/threads.spec.ts — Replace toBeAttached() with toBeVisible()

// Before — passes even if messages are hidden (display:none / visibility:hidden)
await expect(locator.locator(".mx_EventTile").first().getByText("Hello Mr. Bot")).toBeAttached();
await expect(locator.locator(".mx_EventTile").last().getByText("Hello Mr. User")).toBeAttached();

// After
await expect(locator.locator(".mx_EventTile").first().getByText("Hello Mr. Bot")).toBeVisible();
await expect(locator.locator(".mx_EventTile").last().getByText("Hello Mr. User")).toBeVisible();

The test verifies that thread messages are rendered after opening a thread. toBeAttached() only checks DOM presence — an element hidden with display:none or visibility:hidden would still pass. toBeVisible() is the correct assertion here.

Checklist

• I have read through review guidelines and CONTRIBUTING.md.
• Tests written for new code (and old code if feasible).
• New or updated public/exported symbols have accurate TSDoc documentation.
• Linter and other CI checks pass.
• I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

Notes: none

[florianduros]

Hi, thanks for the fixes!

[richvdh]

I'm failing to follow why you're removing this. Asserting that the progress is not negative seems useful to me?

[voidmatcha]

Good point. My original reasoning was that NaN <= 500 evaluates to false, so toBeLessThanOrEqual(500) already catches the parseFloat() returning NaN case. But you're right that it doesn't guard against negative values — >= 0 is not redundant for that. Restored.

[richvdh]

not that I object to this change, but it seems a bit odd to change this but leave the instance of toBeAttached at line 444 unchanged.

[voidmatcha]

Good catch, missed that one. Fixed to match the rest.

[richvdh]

Thanks!