tae-test-review

# Contributing to the TAE Test Framework ## How to use this document This one document serves two audiences: - **Test authors — before you submit.** Read "Principles", "The three test types", and "Anti-patterns", then self-check your test against the "Before you write" checklist and the severity tiers in "Conducting a review". If any of your findings would be *blocking* for a reviewer, fix them first. For temporary 1:1 smoke conversions, run each finding through the cost-of-fix gate in "Migration-era triage": fix the trivial ones yourself now, and flag the rest as known gaps so review is one pass, not a loop. - **Reviewers — during code review.** Work from "Conducting a review". It routes you to the relevant principles and anti-patterns and gives severity tiers so your feedback tells the author what blocks merge. For a conversion, start with the "Parity audit" — it is the one check a green test cannot substitute for. Both audiences share the same rules; the only difference is who applies them and when. ## Conducting a review This section is for the reviewer (and for the author doing a pre-submission self-check). The rest of the document defines what "good" looks like; this defines how to apply it to a diff. ### Scope first Identify what the diff touches, because different files get different scrutiny: - **Test file** (`tests/*.kt`) — apply test-type classification, three-phase structure, and the one-assertion rule (with the Behavior exception below). - **Page object** (`pageObjects/*.kt`) — check page-boundary discipline, whether new methods earn a `[STEP]`, and that navigation edges are registered in `init {}`. - **Selectors** (`selectors/*Selectors.kt`) — check anchor stability, group assignment, and that nothing is defined inline in a test. - **BasePage / helpers** — highest bar. A new primitive on BasePage is a blocking discussion, not a rubber stamp. A new `SelectorStrategy` has a specific two-path wiring trap — see anti-patterns. - **`devtools/*.kt`** — anything with an `@Test` in this package runs on Firebase, because the TAE flank config targets the whole package. Every dev-only method needs `assumeFalse("dev tool, not a CI test", isTestLab())`. ### Severity tiers Label each finding so the author knows what blocks merge. **Blocking** — merge only after this is fixed: - A legacy assertion is missing from a conversion and the gap isn't documented (see "Parity audit"). This one outranks everything else here. - Assertions interleaved with navigation in a Presence or Interaction test (see the Behavior exception below before flagging). - `Thread.sleep()` or a hand-rolled poll/wait loop. - Inline selector defined in a test file. - New method added to `BasePage` without a demonstrated missing *category*. - Page object method crossing page boundaries. - `@After` used for state that must be clean for the next test to be valid. - Hard-coded click sequence to reach a page instead of a `navigateToPage()` edge. - A new `SelectorStrategy` added to only one of the two resolution paths (`resolveComposeNode()`'s `candidates()` and `mozGetElement()`'s `when`). It compiles and fails at runtime for whichever verb you didn't wire. - An `@Test` under `devtools/` without an `isTestLab()` guard — it will consume a Firebase device slot on every TAE run. **Should-fix** — fix unless there's a stated reason: - Test can't be classified as one of the three types, or does too much to be one. - Setup done through the UI when a constructor flag or pre-seeded data exists. - `mozVerifyElementsByGroup` used for runtime/dynamic data. - Popup/dialog handling added inside a test instead of in a primitive/config. - A wrapper method whose name adds no clarity over `primitive + selector`. - A selector switched from text to a tag, leaving what the text was *asserting* unasserted — and often a now-dead parameter still passed by every caller. - A selector used by a class that sets `shouldUseExpandedToolbar = true` but only verified in the default layout. That flag relocates controls and changes the handles they expose. - A nav edge edited without grepping `to = "<TargetPage>"` across `pageObjects/` first. Edges are sometimes registered twice, and the loser is invisible. - Step-by-step narration comments in a converted test (see "Comments in converted tests"). **Nit** — note it, don't block: - Group name could be more meaningful. - A method that could be reused but is currently test-local. - Ordering/naming that reads slightly off-spec. ### Migration-era triage (temporary 1:1 conversions) Most tests under review right now are 1:1 conversions of legacy smoke tests. They are **temporary** — they exist until the factories generate the suite in CI — so the goal is to get correct coverage landed with minimal round-trips, not to make every conversion structurally perfect. The expensive thing is the feedback *loop*, not the feedback. Recalibrate accordingly. **Correctness is not a compromise variable.** Everything below relaxes structure, placement, and primitive design. None of it relaxes whether the test actually verifies its stated behavior. A green test that doesn't test the thing is *negative* value during a migration: it reads as coverage while hiding a gap, and if these hand-conversions seed factory templates, the wrong assertion propagates. So correctness findings stay land-blocking no matter how the gate below scores them. For a conversion, "correctness" means **assertion parity with the legacy test**, established by the diff in "Parity audit" — not by the test passing. A review of one 13-conversion stack found **17** legacy assertions silently missing, and none of them caused a failure. **Cost-of-fix gate.** For every non-correctness finding, before requesting the change, ask: 1. Does requesting it require more than ~1 day and another review round-trip? 2. Is the "better" way moderate+ complexity? 3. Is the preferred fix ambiguous because we have no existing example of it? 4. Does the fix require getting into the guts of BasePage primitives? 5. Is the root cause a missing harness/framework feature that should already exist? If all five are "no", it's a **fix-now** item — trivial, has precedent, no guts — and the author should just make it. If any is "yes", it's a **defer** item. **Three buckets** (use these instead of raw blocking/should-fix/nit for conversion patches): - **Land-blocking** — correctness only. The test must verify its premise. - **Fix-now** — trivial structural fixes that pass the gate (all five "no"): page-boundary swaps (`on.mainMenu` -> `on.downloads`), single-navigation assertion blocks, missing `all`-list registration, missing TestRail link. - **Defer** — anything that trips the gate: BasePage primitive surgery, no-precedent design calls, or work blocked on missing harness support. Let it land; file the item into the cleanup backlog (see guardrails). **Navigation and page-boundary findings are relax-by-default.** Do not block a conversion solely because it chains across pages or over-navigates — *unless* the fix is trivial-with-precedent (then it's fix-now). A test written awkwardly because the harness lacks a clean step is a signal of a **missing feature**, not an author error; record it as a gap rather than forcing the author to build the primitive. **Guardrails so "relaxed" doesn't become invisible debt:** - **Track every defer.** A deferral that lives only in a review comment evaporates when cleanup starts. Emit defer items into the actual cleanup backlog (meta-bug / tag) so "temporary and incomplete" stays visible and temporary. - **Watch what templates.** A one-off quirk in a throwaway test is fine; the same quirk in a pattern the factories will generalize from is not. That is the one structural class worth pushing on even when the fix is mildly non-trivial. - **Batch, don't iterate.** Deliver one consolidated pass ("N trivial fixes, M deferred to cleanup") rather than a multi-round loop. ### Parity audit (conversions only) **A dropped assertion does not fail — it passes for the wrong reason.** This is the single highest-yield check in a conversion review, and the only one that a green CI run actively disguises. `navigateToPage()` silently verifies every selector in the target page's `requiredForPage` group. That makes a legacy `verifyPageContent(...)` or `verifyUrl(...)` look redundant, so it gets dropped. What survives is the *navigation* assertion; what disappears is the *payload* assertion. The test still proves it reached a screen. It stops proving the screen is right. The audit: 1. Open the legacy test body and the port side by side. 2. List every legacy verification — `verify*` calls in the test **and** in the robot helper it delegates to. 3. Tick each one off against an explicit assertion in the port. 4. Anything unticked is either added back explicitly, or documented as a gap in **both** the test and the commit message. There is no third option. When the legacy test gets annotated (after the port is green and landed), that annotation is where a documented gap belongs permanently: ```kotlin @Converted( replacedBy = ["org.mozilla.fenix.ui.efficiency.tests.AutofillTest#verifyAddressAutofillTest"], bug = 2057958, since = "2026-07", notes = "Legacy also asserted X; not carried over — see bug NNNNN.", ) ``` In review, check that `replacedBy` points at a real non-`@Ignore`d `@Test` (the conversion lint check validates this) and that any parity gap you found in step 4 is recorded in `notes` rather than left in a review comment. Do not accept "`navigateToPage` already checks it" as a justification. An implicit assertion is invisible to the next person diffing the port against the legacy test, which makes the conversion impossible to audit. A faithful port now with duplicated-looking lines beats a tidy port that quietly lost coverage; house style gets settled in a later dedicated refactor pass. Two related traps: - **Tag swaps delete content assertions.** When a selector moves from text matching to tag-only (a legitimate fix for ambiguity — see the toolbar/address bar duplication), ask what the text was *asserting*. If it was content, match both with `COMPOSE_BY_TAG_AND_TEXT`. Otherwise the assertion degrades to "an element with this tag exists", which is nearly always true, and the parameters carrying the expected values become dead — passed by every caller, read by nothing. - **Report the inventory and the provenance.** When summarising a stack as "green", state which classes the count covers and *whose* runs it covers. An agent's verification and a human's are separate sets and neither is visible to the other unless recorded. "39 tests green across 6 classes" for a 7-class stack reads as full coverage with one class silently missing. ### Triage checklist (per changed test) 0. If it's a conversion, run the **Parity audit** first. Everything below is structure; this is whether the test still tests the thing. 1. Classify it: Presence, Interaction, or Behavior. If you can't, that's a should-fix — the author doesn't have a clear subject. 2. Strip the navigation mentally. Does the remaining body read as one coherent spec? If it reads as several, check the Behavior exception, then decide split vs. keep. 3. Find the single assertion (or, for Behavior, the ordered checkpoints) that states why the test exists. If it's buried or absent, flag it. 4. Trace setup: could any of it move to a constructor flag, intent extra, or pre-seeded data? Every UI setup step is a should-fix candidate. 5. Check every selector used: does it exist in the selectors file with a meaningful group? Any inline selector is blocking. 6. Check every new page-object method: does it earn a `[STEP]`, and does it stay on the page it operates on? 7. Check navigation: is every hop a registered edge? 8. For a conversion, read the legacy **robot** helper, not just the legacy test body. Retry/refresh semantics, per-assertion waits, and fallback behaviour live in the robot. Porting the body alone yields a test that looks correct and is flaky. 9. **Check the TestRail id against the legacy method it claims to replace.** It is the only link back to the case, nothing downstream validates it, and an id copied from a neighbouring test is invisible on review. Blocking if wrong. 10. **Ask what each assertion would do if the feature were broken.** Two shapes recur and both pass for the wrong reason: an assertion on system-UI text that is present in either state (an Android app-permissions row reads "Camera" whether allowed or denied), and a `Build.VERSION` branch whose modern arm asserts less than its legacy arm did. If a queryable state exists behind the UI — `checkSelfPermission`, a store field, a pref — assert that instead (A58). 11. **A new page object must be wired into `PageContext` in the same change** (A59). Edges register in the page's `init`, which never runs otherwise, so an unreferenced page object is dead code whose navigation has never executed — and its arrival anchor may not even match the screen its edge lands on. ### The Behavior-test exception (read before flagging "interleaved assertions") The "one test, one assertion" rule applies fully to **Presence** and **Interaction** tests. **Behavior** tests legitimately span surfaces and may contain *sequential checkpoints* — a verify after each state transition that a later step depends on. Distinguish a legitimate checkpoint from two tests glued together: - **Legitimate checkpoint** — the verified state is a *precondition for the next step*, and removing it would make a later failure impossible to localize (e.g. verify a bookmark exists before opening its menu to edit it). - **Glued-together tests** — the assertion blocks are independent; each could stand alone with its own setup, and neither depends on the other's state. Split these. If in doubt, ask: "does the second assertion require the first action to have happened?" Yes → checkpoint, allowed. No → split, blocking. ### Worked example **Rejected** — inline selector, UI setup, and two unrelated assertions glued together across navigation: @Test fun bookmarksTest() { // UI setup that a constructor flag / pre-seed could do on.browserPage.navigateToPage(url) on.browserPage.openMainMenu().mozClick(MainMenuSelectors.BOOKMARK_THIS) on.bookmarks.navigateToPage() .mozVerify(Selector(COMPOSE_BY_TEXT, "My Page", groups = listOf())) // inline selector .mozClick(BookmarksSelectors.THREE_DOT_MENU) // crosses into an action on.bookmarks.mozVerify(BookmarksSelectors.EDIT_BUTTON) // assertion #1: menu opens on.home.navigateToPage() // unrelated surface .mozVerifyElementsByGroup("jumpBackIn") // assertion #2: independent } Review comments: - **Blocking**: inline selector — move to `BookmarksSelectors` with a group. - **Blocking**: two independent assertions (bookmark menu, then Jump Back In) glued by navigation — the second doesn't depend on the first. Split into two tests. - **Should-fix**: the bookmark is created through the UI — pre-seed it with `createBookmarkItem(...)` so this test starts on the surface it's verifying. **Accepted** — one Presence test, setup pre-seeded, single assertion. Note the assertion uses `mozVerify` with a parameterized selector (`BOOKMARK_ITEM(title)`) rather than a group, because `title` is runtime data — this is correct, not a missing group: @Test fun bookmarksListShowsSavedItem() { // SETUP createBookmarkItem(url, title, null) // STEPS on.bookmarks.navigateToPage() // ASSERT on.bookmarks.mozVerify(BookmarksSelectors.BOOKMARK_ITEM(title)) } ## Principles ### Tests declare what, not how A test should read as a specification of behavior. Navigation mechanics, element resolution, retries, and waits belong in the framework. If someone reads your test and can't tell what feature it validates within 10 seconds, rewrite it. ### One test, one assertion (with a Behavior-test exception) For **Presence** and **Interaction** tests: if you have assertion blocks separated by navigation or interaction steps, you have written multiple tests glued together. Split them. Use `mozVerifyElementsByGroup` to assert multiple related elements as one logical check when they belong to the same verification. For **Behavior** tests: because they intentionally cross surfaces, a verify may appear after each state transition as a *sequential checkpoint* — but only when that verified state is a precondition for the next step and removing it would make a later failure impossible to localize. Independent assertion blocks that don't depend on each other's state are still two tests; split them. The distinguishing question: *does the next action require the previous assertion's state to have happened?* Yes → legitimate checkpoint. No → split. (See "Conducting a review > The Behavior-test exception" for how this is applied in review.) ### Three-phase structure Every test follows this pattern: ```kotlin // SETUP: what preconditions does this test need? // Prefer BaseTest constructor flags, pre-seeded data, or pre-runner state. // Reserve in-test setup for state that genuinely requires UI interaction. createBookmarkItem(url, title, null) // STEPS: navigation and page object interactions to reach the point of interest. on.bookmarks.navigateToPage() .openItemMenu(title) .mozClick(BookmarksSelectors.SHARE_BUTTON) // ASSERT: a single assertion block capturing the spirit of the test. // This is WHY the test exists and WHAT artifact is being verified. on.shareOverlay.mozVerifyElementsByGroup("shareTabLayout") ``` Push setup as early as possible. If state can be configured via feature flags in the `BaseTest` constructor, do that. If it can be set before the runner initializes (intent extras, shared prefs), do that. ## The three test types Before writing a test, identify which type it is. Each has a repeatable model: **Presence** - Navigate to a surface, verify elements render. No state changes. Answers: *"Does this page show what it should?"* ```kotlin on.home.navigateToPage() .mozVerifyElementsByGroup("requiredForPage") ``` **Interaction** - Navigate + perform an action + verify the immediate result. Modifies state on one surface. Answers: *"Does this control do what it should?"* ```kotlin on.home.navigateToPage() .mozClick(HomeSelectors.PRIVATE_BROWSING_BUTTON) on.home.mozVerifyElementsByGroup("privateBrowsing") ``` **Behavior** - One or more state changes across one or more pages. Answers: *"Does this feature work end-to-end?"* These compose presence and interaction primitives. ```kotlin on.browserPage.navigateToPage(url) on.home.navigateToPage() .mozVerifyElementsByGroup("jumpBackIn") ``` If you can't classify your test, you don't yet have a clear enough picture of what you're testing. ## Reuse over specificity ### Write for reuse by default Before adding a function, ask: "Would another test for a different feature need this?" If yes, it belongs in a page object or shared step. If no, reconsider whether you need a new function at all. ### Page objects are shared vocabulary If you add a method to a page object, it should be useful to any test touching that page. If only your test calls it, it doesn't belong there. ### Test steps belong on the page they operate on A page object method should only interact with the UI surface it represents. If a method on `BrowserPage` clicks through MainMenu and Collections selectors, it's crossing page boundaries -- put it on the page where the action starts, or split it across the relevant page objects. Similarly, don't chain primitives on one page object while interacting with another page's UI just because the return type allows it. ### Selectors are shared vocabulary too Define selectors once in the appropriate `selectors/*Selectors.kt` file. Assign meaningful groups. Don't create one-off selectors inline in tests. ## Primitives and test steps ### Use the existing primitives `mozClick`, `mozSwipeTo`, `mozVerify`, `mozVerifyElementsByGroup`, `mozEnterText`, `mozPressEnter` -- these are your building blocks. Compose tests from them. > Verified against `helpers/BasePage.kt` on 2026-07-29. This is a point-in-time > snapshot — new primitives are added over time, so treat the current BasePage > source as authoritative if it differs. `mozVerifyElement`, `mozGetElement`, and > `resolve` are **private** to BasePage; don't reach for them from tests or page > objects. ### Interaction primitives | Primitive | Purpose | |-----------|---------| | `mozClick(selector)` | Click an element | | `mozLongClick(selector)` | Long-click an element | | `mozClickIfPresent(selector)` | Click only if the element is present (no failure if absent) | | `mozClickFirstWithParentText(selector, parentText)` | Click the first match under a parent with given text | | `mozEnterText(text, selector)` | Enter text into a field | | `mozClear(selector)` | Clear a field | | `mozClearAndEnterText(text, selector)` | Clear then enter text | | `mozPressEnter(selector)` | Press the IME enter key on the field | | `mozSwipeTo(selector)` | Swipe until an element is visible |