Give the sign-in field an accessible name and run the E2E suite that covers it - #180
Merged
Conversation
| expect(true).toBe(true); | ||
| // The seed gives this author an ORCID, so the link is not optional here. | ||
| await expect(orcidLink.first()).toBeVisible(); | ||
| await expect(orcidLink.first()).toHaveAttribute('href', /orcid\.org/); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
E2E has been commented out of CI since before this backlog was written. Turning it on found a real accessibility bug in the sign-in form, which is the argument for turning it on.
The sign-in field had no accessible name. shadcn's
FormControlclones its child to passid,aria-describedbyandaria-invalid;HandleInputaccepted none of them and forwarded none to its<input>. So the<label for>never matched, and a screen reader announced an unlabelled text box on the first field of the login page. The E2E page object had been working around it by matching the placeholder — which then changed frombsky.socialtoyourhandle.example.com, breaking eight tests and leaving the underlying bug in place. The component now accepts and forwards those props, and the test binds to the label, which is the name a screen reader actually announces.The landing-page tests described a page that no longer exists. They looked for a "Sign in with Bluesky" button and a Bluesky link; the button says "Sign in with ATProto" and the third link is ATProto. Naming one PDS operator was wrong about the product as well as the markup — Chive signs in any ATProto identity. Added coverage for the handle field itself, which nothing tested and which is the first thing a user touches.
Seventeen assertions of the form
expect(true).toBe(true)across four spec files now assert something. Each computed a visibility check and discarded the result, so the test passed whether the element rendered or not. Where the seed makes the answer deterministic they assert it —isCorrespondingAuthoris set on every seeded eprint, ORCID is seeded,isHighlightedandcontributionsandpaperDidare not, so those assert absence, which catches a page inventing data it was not given. Where the outcome genuinely depends on state the test does not control, they assert the state machine instead: after clicking vote the page must show either a confirmation or a sign-in prompt, and the approve/reject buttons must appear together or not at all. One case that cannot assert anything on/governancenow callstest.skipwith a reason, so the report says it did not run rather than that it passed.The five
test.skipcalls inhome.spec.tsstay skipped. Their reasons are correct — the browse button, submit CTA and features section genuinely are not on the alpha landing page. They were skipped via a comment above a silently-skipped test; they now calltest.skip(true, '<reason>')so the reason reaches the Playwright report instead of living in a comment nobody reads.CI runs the unauthenticated project. That is 24 tests over sign-in and the landing page, about two minutes, and it is green. The authenticated project is 487 tests taking hours on one worker; it is not enabled here and needs its own pass — a sharding decision and a triage of what currently fails. Running the part that is known good beats running nothing, which is what the commented-out job achieved.
Also here, since it is the same "config points somewhere nobody goes" problem:
main. Work reaches production throughstagingin this repository, so nine dependency PRs had accumulated against a branch the normal flow cannot merge them into. All four ecosystems now targetstaging.src/plugins/builtin/holds more than run.pnpm lexicons:generatemust run beforepnpm typecheck, because the generated directories are gitignored. Verified by movingsrc/lexicons/generatedaside and watchingtscfail withCannot find module, then restoring it.Related Issues
CI-15 (partial), CI-16, CI-20 in the 0.8.0 backlog, plus the Dependabot set (#125-#133) and CFG-16.
Type of Change
How Has This Been Tested?
Everything here was run, not reasoned about:
idforwarding again makeslogin form is accessiblefail. The test previously usedgetByText('Handle or DID'), which matched the label and the description both and would have passed while the input had no accessible name at all — it now usesgetByLabel, which only matches a real association.e2e-testsas a job.Checklist
General
npm run lintpasses)npm test)ATProto Compliance (required for data flow changes)
npm run test:compliance— 100% required)One component prop change and test/CI configuration; no data flow touched.
Breaking Changes
Still open
CI-15 is only half done. E2E still authenticates through
ENABLE_E2E_AUTH_BYPASSand hand-written localStorage, so no test exercises real OAuth. That needs a test PDS and credentials in CI, which is a separate piece of work, and I have not pretended otherwise by enabling something that only looks like it covers sign-in.