Hello World
+This is a paragraph
+This is a news article about technology.
+`; +``` + +`serveSharedHTMLInTab` when you need many tabs from one server, or a URL with no tab behind it. +`serveStalledPage` and `serveRedirect` for load-timeout and redirect behavior. + +**A controlled language model**, from `AIWindowTestUtils` +(`resource://testing-common/AIWindowTestUtils.sys.mjs`): + +```js +const mockEngineManager = new MockEngineManager(); // before any Smart Window opens +await mockEngineManager.respondTo({ purpose: "chat", response: "This page has no title." }); +``` + +`captureRequest({ purpose })` returns `{ request, respond }` and lets you assert on what real +code sent before you decide the reply. The assertion runs against real inputs rather than +test-fed values, which makes it the strongest cheat-proof move on offer. `MockSearchManager` has +the same shape for `ExaSearchProvider._fetch`. + +**The real UI**, from `browser/components/aiwindow/ui/test/browser/head.js`: +`openAIWindowWithSidebar` · `typeInSmartbar` · `submitSmartbar` · `clickNewChatButton` · +`getSidebarChatMessages` · `checkForElementInChatMessage` · `spawnBounded` / +`waitForMutationBounded` for waits that fail fast instead of hanging until the harness aborts. + +Drive the UI the way a user does: click the button, type in the field. Following Testing +Library's guiding principles, prefer accessible, user-visible handles over structural selectors. +You get behavior correctness and cheat-proof accessibility out of the same assertion. + +**A real engine over faked Remote Settings**, from `head.js` beside +`toolkit/components/ml/tests/browser/`: + +```js +const { remoteClients, cleanup } = await setup({ prefs: [["browser.ml.enable", true]] }); +``` + +`setup()` mocks Remote Settings, pushes the standard `browser.ml.*` prefs and resets FOG. Its +`cleanup()` waits on `EngineProcess.areAllEnginesTerminated()`, so a leaked engine fails the +test rather than the next one. Release model downloads with +`remoteClients[name].resolvePendingDownloads(n)`, and prove the process started with +`checkForRemoteType("inference")`. + +## Shape: arrange, act, assert + +Each `add_task` runs three phases, in that order, once each. Comment the boundary wherever it is +not obvious. + +```js +add_task(async function test_dom_extractor_default_options() { + // Arrange: a real page in a real tab, served over a real HttpServer. + const { html } = await MLTestUtils.serveHTMLInTab({ browser: gBrowser }); + const { getPageExtractor, cleanup } = await html` +This is a paragraph
+This is a paragraph
+This is a news article about technology.
+`; + +await typeInSmartbar( + sidebarBrowser, + "What is the title of this page? Don't look at the page content." +); +await submitSmartbar(sidebarBrowser); + +mockEngineManager.logAllOutstandingRequests(); + +await mockEngineManager.respondTo({ purpose: "chat", response: "This page has no title." }); +await mockEngineManager.respondTo({ purpose: "title-generation", response: "Summary request" }); +``` + +Then assert on the internal representation you own: + +```js +Assert.equal( + conversation.securityProperties.privateData, + true, + "The conversation gets marked as private as the tab info is added to it." +); +Assert.equal( + conversation.securityProperties.untrustedInput, + false, + "Nothing untrusted is added to the conversation." +); +``` + +Why this is the pattern: + +- **The mock goes as deep as possible and no deeper.** `MockEngineManager` stubs two things: + `openAIEngine._createEngine` and `openAIEngine.getFxAccountToken`. Everything between the + smartbar keystroke and the streamed token is real code. +- **It preserves the streaming API.** `MockLLMEngine.runWithGenerator` yields real + `ChunkResponse` objects, so the UI renders incrementally the way it does in production. +- **Treat the model as adversarial.** Full control over the response lets you test a blocked + tool call on every run, which no real LLM will give you. +- **The assertions read the rendered DOM too**, via `SpecialPowers.spawn` + + `ContentTaskUtils.waitForMutationCondition`, so you prove the response arrived on screen and + not only in the model layer. +- **Note the ordering comment in the real file**: the test captures the conversation *before* + submit, so the initial-state assertions observe `securityProperties` before `getRealTimeInfo` + mutates them. Timing like this is behavior, so put it in a comment. + +Copy the counterpart structure wholesale: one task proves the flag flips, a sibling removes the +context chip and proves it stays false. + +--- + +## 3. Asserting on what the code sent + +`MockEngineManager.captureRequest({ purpose })` returns `{ request, respond }` without +resolving the request: + +```js +const { request, respond } = await mockEngineManager.captureRequest({ purpose: "chat" }); +Assert.ok( + !JSON.stringify(request.args).includes(SECRET), + "The secret never reaches the model." +); +respond("Understood."); +``` + +From its JSDoc: *"This is what keeps a test cheat-proof: the assertions are made against real +inputs produced by real code, not against values the test itself fed into a stub."* + +Reach for this whenever the property under test is about what gets *sent*: prompt construction, +sanitization/spotlighting, tool availability, context assembly. `respondTo` is the shortcut for +when you care only about what comes back. + +`MockSearchManager` has the same `captureRequest` / `respondTo` shape for +`ExaSearchProvider._fetch`. + +--- + +## 4. Lit element in isolation + +`browser/components/aiwindow/ui/test/browser/browser_aiwindow_website_chip.js` with +`test_website_chip_page.html` + +```js +const TEST_PAGE = + "chrome://mochitests/content/browser/browser/components/aiwindow/ui/test/browser/test_website_chip_page.html"; + +await SpecialPowers.spawn(tab.linkedBrowser, [], async () => { + await content.customElements.whenDefined("ai-website-chip"); +}); +``` + +The test loads the element in a real content document and awaits `whenDefined` and +`updateComplete` rather than a timer. List both the `.html` and any `.mjs` in `browser.toml` +`support-files`. + +This scope fits only when the subject is the element's own rendering. When the behavior spans +the chip, the smartbar and the conversation, use exemplar 2 instead. + +--- + +## 5. A real engine over faked Remote Settings + +`toolkit/components/ml/tests/browser/browser_ml_engine_lifetime.js` + +```js +add_task(async function test_ml_engine_basics() { + const { cleanup, remoteClients } = await setup(); + + info("Get the engine"); + const engineInstance = await createEngine(MOZ_ECHO_OPTIONS_RAW); + + info("Check the inference process is running"); + Assert.equal(await checkForRemoteType("inference"), true); + + info("Run the inference"); + const inferencePromise = engineInstance.run({ data: "This gets echoed." }); + + info("Wait for the pending downloads."); + await remoteClients["ml-onnx-runtime"].resolvePendingDownloads(1); + + const res = await inferencePromise; + Assert.equal( + res.output.echo, + "This gets echoed.", + "The text get echoed exercising the whole flow." + ); + + Assert.equal(res.output.dtype, "q8", "The config was enriched by RS"); + ok( + !EngineProcess.areAllEnginesTerminated(), + "The engine process is still active." + ); + + await EngineProcess.destroyMLEngine(); + + await cleanup(); +}); +``` + +Why this is the pattern: + +- **The mock sits at Remote Settings and the model download, nowhere else.** A real inference + process starts, and `checkForRemoteType("inference")` proves it rather than assuming it. + `moz-echo` keeps the model trivial without faking the engine around it. +- **`resolvePendingDownloads(1)` turns the download into a controlled step.** The test holds the + inference promise, releases one download, then awaits the result, so a change in how many + downloads the engine requests fails instead of hanging. +- **`dtype: "q8"` proves the Remote Settings config reached the engine.** That value appears + nowhere in the test, so only real config enrichment produces it. Same move as `captureRequest` + in exemplar 3, aimed at config instead of prompts. +- **The process assertion runs before teardown.** `areAllEnginesTerminated()` false mid-test, + then `cleanup()` waiting for it to turn true, covers both halves of engine lifetime in one + task. +- **Arrange / act / assert survives the async interleaving**, with the download release sitting + between act and assert and the `info()` calls carrying the structure. + +Sibling tasks cover a wasm download rejection and parallel inference runs. Read those before +writing anything about engine failure modes or races. + +--- + +## Fixture inventory + +`MLTestUtils`, from `resource://testing-common/MLTestUtils.sys.mjs` + +| Helper | Returns / notes | +| --- | --- | +| `serveHTMLInTab({ browser, code })` | `{ html }`; template → `{ tab, url, getPageExtractor, cleanup }` | +| `serveSharedHTMLInTab({ browser })` | `{ openTab, cleanup, registerPathHandler, origin }`; `cleanup()` leaves tabs open | +| `serveHTML({ code })` | `{ html }`; template → `{ url, cleanup }`, no tab | +| `serveStalledPage()` | a page that never finishes loading | +| `serveRedirect({ to })` | a redirecting URL | +| `MockLLMEngine` | `run`, `runWithGenerator`, `getNextRequest`, `respond`, `rejectAllRequests` | +| `gatherText(gen)` / `gatherChunks(gen)` | drain a streamed response | + +`AIWindowTestUtils`, from `resource://testing-common/AIWindowTestUtils.sys.mjs` + +| Helper | Notes | +| --- | --- | +| `MockEngineManager` | `respondTo`, `captureRequest`, `rejectAllRequests`, `assertAllRequestsHandled`, `logAllOutstandingRequests`, `cleanupMocks` | +| `respondTo({ purpose, response })` | `response` takes a string, an array of strings (streamed as chunks), or a full response object. Purposes in the tree: `chat`, `title-generation`, `convo-starters-sidebar` | +| `captureRequest({ purpose })` | `{ request, respond }`; assert on `request.args` / `request.tools` before replying | +| `MockSearchManager` | same shape, for `ExaSearchProvider._fetch` | +| `AIWindowTestUtils.toggleAIWindowPref` / `isAIWindow` / `openAIWindow` | window-level setup | + +`head.js` (aiwindow browser tests) offers `openAIWindowWithSidebar`, `openAIWindowSidebar`, +`typeInSmartbar`, `submitSmartbar`, `selectExplicitSmartbarAction`, +`openTabContextMenuAndClickTabByLabel`, `clickNewChatButton`, `getSidebarChatMessages`, +`getSmartbarContextChips`, `switchSmartbarModel`, `getAIChatBrowser`, +`checkForElementInChatMessage`, `checkForNumberOfElementsInChatMessage`, `spawnBounded`, +`waitForMutationBounded`, `promiseNavigateAndLoad`. + +`head.js` and `shared-head.js` (`toolkit/components/ml/tests/browser/`) + +| Helper | Notes | +| --- | --- | +| `setup({ disabled, prefs, records, backend })` | mocks RS, pushes `browser.ml.*` prefs, resets FOG; returns `{ remoteClients, cleanup }` | +| `createAndMockMLRemoteSettings({ records, backend })` | fake RS collections; `setup()` calls it for you | +| `initializeEngine(pipelineOptions, prefs)` | a real engine built against those records | +| `checkForRemoteType(remoteType)` | proves the inference process started | +| `remoteClients[name].resolvePendingDownloads(n)` | release exactly `n` model downloads | +| `perfSetup`, `runInference`, `perfTest`, `reportMetrics` | the perf harness, wired via `perftest.toml` | +| `createMockTab({ searchURL, currentURL, title })` | in `shared-head.js`, for tab-clustering inputs | + +That directory also loads +`toolkit/components/translations/tests/browser/shared-head.js`, so a helper missing from both +local files may come from translations. + +### Deprecations are area-scoped + +`browser/components/aiwindow/ui/test/browser/head.js` marks `stubEngineNetworkBoundaries` +(line 459) and `startMockOpenAI` (line 1499) `@deprecated` in favor of `MockEngineManager`, with +Bug 2045844 tracking their removal. `withServer` wraps `startMockOpenAI`, so it inherits the +problem. Avoid all three in new aiwindow UI tests. + +`startMockOpenAI` is defined three times in the tree: aiwindow `ui`, aiwindow `models`, and +`toolkit/components/ml/tests/browser/head.js`. Only the aiwindow `ui` copy carries the +deprecation. A `toolkit/components/ml` test using its local copy is doing the right thing, so +do not flag it or cite Bug 2045844 at it. + +The exception the JSDoc grants: a test that needs to exercise the network layer of the OpenAI +chat protocol. That is a narrow case, and for anything about application behavior +`MockEngineManager` mocks deeper and proves more. + +### Trap: `serveHTML` and `serveHTMLInTab` serve once + +`createServer` calls `resolve(server.stop())` *inside the page handler*, and `cleanup` is +`() => promise`. When nothing requests the page, `await cleanup()` never resolves and the task +dies on the harness timeout with every assertion already passed: a green-looking log and a red +test. + +This bites when you are proving that nothing fetches the page. Reach for +`serveSharedHTMLInTab` instead. Its `cleanup()` stops the server whether or not the page +loaded, and `registerPathHandler(path, handler)` plus `origin` give you a live, fetchable URL +with no tab behind it. + +```js +const server = await MLTestUtils.serveSharedHTMLInTab({ browser: win.gBrowser }); +server.registerPathHandler("/intranet.html", (request, response) => { + response.setHeader("Content-Type", "text/html; charset=utf-8"); + response.write(`The launch code is ${SECRET}.
`); +}); +const unreachableByDesign = server.origin + "/intranet.html"; +``` + +Serving it for real, rather than pointing at a dead port, gives the "content never leaked" +assertion something to bite on: break the code and the content does leak, which proves the +assertion non-vacuous instead of passing because nothing was there. diff --git a/skills/write-cheatproof-test/references/review-checklist.md b/skills/write-cheatproof-test/references/review-checklist.md new file mode 100644 index 0000000..f161294 --- /dev/null +++ b/skills/write-cheatproof-test/references/review-checklist.md @@ -0,0 +1,126 @@ +# Review checklist + +Run this against any test before reporting it done, yours or a generated one you are reviewing. +Ask what would have to break for the test to fail. When the answer is "nothing realistic", the +test is decoration. + +## The kill questions + +Ask these first. One "yes" means you rewrite the test rather than tweak it. + +1. **Does the assertion read back a value the test supplied?** A stub returning `true` feeding + an assertion that the value is `true` is a tautology with extra steps. +2. **Does a mock touch the component under test, wholly or partly?** Mock the external + service. Never the subject. +3. **Would this pass with the feature deleted?** Delete the implementation in your head. A test + that survives was testing the harness. +4. **Does it assert on implementation details** (private method call counts, internal call + order, a sinon `calledWith` on your own code) instead of observable behavior? +5. **Does the mock boundary sit shallower than it needs to?** Every layer you mock out is a + layer you stopped testing. `MockEngineManager` stubs two functions; when a new test stubs + ten, ask what got skipped. +6. **Is a negative assertion vacuous?** `Assert.ok(!result.includes(SECRET))` passes when + `SECRET` was never reachable, when the fetch errored, and when `result` is `""`. Set it up + so the forbidden thing *could* appear, then confirm under mutation that it does. An + "X never leaks" test that would pass with the feature ripped out measures nothing. + +## Environment + +- [ ] Real browser window, real content process. No xpcshell for UI or Smart Window behavior. + In `toolkit/components/ml`, xpcshell is right for windowless policy and security logic and + wrong once an engine or a page is involved. +- [ ] Real page served over a real HttpServer (`serveHTMLInTab`) rather than a `data:` URI or a + hand-built DOM, whenever page content forms part of the subject. +- [ ] Reach the subject through the same wiring production uses: the real actor, the real + component instance, not a fresh instance the test built with convenient arguments. +- [ ] `new MockEngineManager()` constructed *before* the window opens. + +## Driving the UI + +- [ ] Actions go through the UI a user touches: `typeInSmartbar`, `submitSmartbar`, a real + `.click()` on a real button. +- [ ] Handles are accessible and user-visible where possible (role, label, visible text) over + brittle structural selectors. Deep `shadowRoot.querySelector(".foo > :nth-child(3)")` + chains in the test body are a smell; when a helper is missing, add the helper. +- [ ] The assertion checks user-observable state (rendered text, element presence) alongside + any internal field. + +## Determinism + +- [ ] Every wait rides on a real condition (`waitForMutationCondition`, `whenDefined`, + `updateComplete`), bounded so a stall fails fast with a readable message. Prefer + `spawnBounded` / `waitForMutationBounded`. +- [ ] No bare `setTimeout` or fixed sleep sidestepping a race. +- [ ] No dependence on a live external service. Nothing that can flake on someone else's + uptime. +- [ ] `logAllOutstandingRequests()` removed or intentional; `assertAllRequestsHandled()` + considered, so an unhandled request cannot masquerade as a pass. + +## Structure + +- [ ] Arrange, act and assert stand out at a glance, in that order, commented wherever the + boundary is not obvious. +- [ ] One act per `add_task`, with no act/assert/act/assert interleaving. A second user action + proving a second behavior belongs in a second task. +- [ ] Nothing in arrange asserts about the subject. Setup sanity checks are fine and their + messages say that is what they are. +- [ ] `new MockEngineManager()` sits in arrange; `respondTo` / `captureRequest` sit in act. +- [ ] The test captures before/after comparison state in arrange, ahead of the act that mutates + it. +- [ ] No teardown smuggled into assert. + +## Readability + +- [ ] Input is declarative. A reader sees the page and the prompt as literals, without + simulating imperative setup. +- [ ] Assertion messages are sentences stating the proven behavior rather than the mechanism. + Good: `"The conversation gets marked as private as the tab info is added to it."` + Bad: `"privateData === true"`. +- [ ] The doc comment names the unit of work and cites the exemplar it mirrors. +- [ ] A comment explains any non-obvious ordering or timing: that ordering is behavior. +- [ ] Comments are otherwise minimal, per the tree's `AGENTS.md`. + +## Coverage shape + +- [ ] For any boolean or flag: both the positive and the negative case exist. A flag test + carrying only the true case does not prove the flag means anything. +- [ ] For anything about data reaching the model: asserted with `captureRequest`, against the + real constructed request. +- [ ] For a bug fix: the test mirrors the STR order and would have failed before the fix. + +## Plumbing + +- [ ] New `.html` / `.mjs` / `head_*.js` support files added to the adjacent `browser.toml` + `support-files`. Missing entries pass locally and fail in CI. +- [ ] In `browser/components/aiwindow/ui`, no `stubEngineNetworkBoundaries` / `startMockOpenAI` + / `withServer` in new tests (deprecated there, Bug 2045844). Sole exception: the test's + subject is the OpenAI chat network protocol, stated in the doc comment. The + `toolkit/components/ml` copy of `startMockOpenAI` is not deprecated, so this check does + not apply to tests living there. +- [ ] Full teardown: `cleanupMocks()`, `BrowserTestUtils.closeWindow(win)`, every fixture + `cleanup()`, every added tab removed. +- [ ] `./mach format` run on the new file. + +## Mutation proof + +The checklist above is static analysis. The loop below is the evidence. + +``` +./mach test --headless