scout-best-practices-reviewer

# Scout Best Practices Reviewer ## Overview Perform a static PR review of Scout UI and API test files (`*.spec.ts`) against Scout best practices and existing Scout abstractions (fixtures, page objects, API helpers). Produce actionable, PR-review-ready feedback that pushes for reuse over one-off implementations. **Solution-specific skills may extend this skill** with additional review criteria. Check if one exists for your solution (e.g., Security Solution has one at `<plugin>/.agents/skills/scout-best-practices-reviewer/`). Run the general review first, then apply solution-specific checks. Important: Do not post GitHub comments unless explicitly stated. ### Inputs 1. Changed `*.spec.ts` files (and imported helpers/fixtures). - UI Tests: Use `test` / `spaceTest` (usually in `**/test/scout/ui/**`). - API Tests: Use `apiTest` (usually in `**/test/scout/api/**`). 2. Neighboring Scout code in the same plugin/solution (existing specs + `test/scout/**/fixtures/**`) to spot reuse opportunities and avoid duplicating helpers. 3. Removed/previous tests (if this is a migration) to verify behavior parity. 4. Scout docs (open only what you need — best practices are split by test type so you can skip the irrelevant half): - **General best practices** (always relevant): `docs/extend/testing/scout-best-practices.md` - **UI-only best practices** (open when reviewing UI tests): `docs/extend/testing/ui-best-practices.md` - **API-only best practices** (open when reviewing API tests): `docs/extend/testing/api-best-practices.md` - Core concepts & fixtures: `docs/extend/testing/scout.md`, `docs/extend/testing/fixtures.md` - Reuse surfaces: `docs/extend/testing/page-objects.md`, `docs/extend/testing/api-services.md` - Type-specific guides: `docs/extend/testing/write-ui-tests.md`, `docs/extend/testing/write-api-tests.md` - As needed: `docs/extend/testing/api-auth.md`, `docs/extend/testing/browser-auth.md`, `docs/extend/testing/parallelism.md`, `docs/extend/testing/deployment-tags.md`, `docs/extend/testing/a11y-checks.md`, `docs/extend/testing/debugging.md`, `docs/extend/testing/run-scout-tests.md` **Rule of thumb:** always read the general best practices, then open **only** the UI-specific file for UI reviews or the API-specific file for API reviews. If a PR mixes UI and API specs, open both. ## Critical checks (do these first, one by one) Work through these numbered checks **in order, one at a time**, before the general checklist — don't batch or skip. They're the highest-priority findings: a genuine hit almost always means the PR must change before merge. Each row links to the canonical public guidance — **read the linked section before flagging**, and cite it in the comment. | # | Critical check | Fires when the PR… | Flag / ask → see | |---|---|---|---| | 1 | **Custom server config earns its keep** | adds or updates a config set (files under `.../kbn-scout/.../config_sets/<name>/**`, a new `test/scout_<name>/` dir, or new `--serverConfigSet`) | Setting is runtime-toggleable → move it to `apiServices.core.settings(...)`. Duplicates an existing set's purpose/args → reuse that set, or ask its owners to extend it. → `docs/extend/testing/feature-flags.md`, `docs/extend/testing/scout-best-practices.md#prefer-runtime-feature-flags` | | 2 | **Spec lives where selective testing runs it** | adds or moves a spec that drives another plugin's app/UI (`page.gotoApp('<other>')`, another solution's `data-test-subj`, tags, or nav) | Host package doesn't own or `kbn_references`-depend on the code under test → relocate the spec to the owning plugin so PR CI actually runs it. → `docs/extend/testing/scout-best-practices.md#keep-tests-close-to-source-code` | | 3 | **Right test type, 100% justified** | adds or migrates a UI or API spec, or an FTR/Cypress→Scout migration | UI asserts data/logic only, or drives a pure local UI toggle already covered by a Jest unit/RTL test → move down the pyramid or remove. "It works / it's easier / that's how the FTR did it" isn't justification. → `docs/extend/testing/scout-best-practices.md#pick-the-right-test-type`, `docs/extend/testing/migrate-tests.md#dont-migrate-blindly` | | 4 | **Migration parity — no major coverage loss** | removes or replaces FTR/Cypress tests alongside new or changed Scout specs | Flag **only major** changes or drops: removed scenarios, dropped roles/error paths or classic/serverless coverage, weakened assertions or lost side-effect checks. Skip benign deltas — don't post an FYI parity map. Depth in **Migration parity analysis** below. → `docs/extend/testing/migrate-tests.md#dont-migrate-blindly` | ## Scope (be comprehensive) - Don’t limit the review to the diff. Look for duplication and missed reuse by scanning: - existing Scout specs in the same area (and similar suites elsewhere in the repo) - available fixtures (`docs/extend/testing/fixtures.md` + local `test/scout/**/fixtures`) - existing page objects, API services, and fixtures (in `@kbn/scout`, solution Scout packages, and plugin-local `test/scout/**`) before suggesting brand-new helpers ### Quick checklist Checklist items are tagged with the document they're detailed in: - **[general]** → `docs/extend/testing/scout-best-practices.md` (applies to both UI and API tests) - **[ui]** → `docs/extend/testing/ui-best-practices.md` - **[api]** → `docs/extend/testing/api-best-practices.md` Open only the docs relevant to the test type(s) under review. - **[general]** **Reuse-first**: prefer existing `pageObjects`, fixtures, and `apiServices`; if adding helpers/page objects, place them in the right scope (plugin vs solution vs `@kbn/scout`) and register via fixtures. - **[general]** **No unused constants**: flag constants that are unused or used in only one place — prefer inlining them. - **[general]** **Don't circumvent the linter**: flag attempts to silence a lint rule instead of fixing the root cause (`eslint-disable`, or swapping a flagged pattern for a hack that hides it). Sanctioned escape hatches (e.g. `dispatchEvent('click')` for a documented app bug) are fine only with an explicit documented justification. Don't re-flag plain lint violations — CI already gates those. - **[api]** **Fixture boundaries**: `apiClient` for the endpoint under test; `apiServices`/`kbnClient` for setup/teardown only; correct auth + common headers. - **[api]** **Correctness**: guardrail assertions before dereferencing response fields; validate contract + side effects; stable error assertions. - **[ui]** **UI scope**: keep UI specs focused on user interactions and rendering; for data-correctness assertions and choosing the right layer, see **Critical check 3 (right test type)**. - **[ui]** **Page objects**: Encapsulate multi-step interactions and reused sequences in page objects — specs should primarily hold assertions (`expect`), test flow (`test.step`), and page-object method calls. Short inline locator calls for simple one-off assertions (e.g. a single label or nav-link check) are acceptable. Flag raw locators when the interaction is complex enough to benefit from abstraction or is duplicated across specs. Extract all locators as `readonly` properties in the constructor; no inline locator creation inside methods. - **[general]** **Isolation**: parallel-safe data; resilient cleanup in `afterAll`/`afterEach`; defensive cleanup in `beforeAll` for failed-run leftovers; `scoutSpace.savedObjects.cleanStandardList()` as catch-all after domain-specific cleanup; no reliance on file ordering or shared mutable state. - **[general]** **RBAC / realism**: minimal permissions (avoid `admin` unless required); space-aware behavior covered or explicitly out of scope. - **[ui]** **Flake traps**: avoid `waitForTimeout()` and time-based assertions/retries; rely on auto-waiting + explicit readiness signals. Some locators are restricted by `@kbn/eslint/scout_no_locators` (e.g. `globalLoadingIndicator`). - **[general]** **Cost**: avoid repeating expensive setup; consider a global setup hook for shared one-time operations. - **[general]** **Global teardown** (when `global.teardown.ts` is present): cleanup must use `esClient`/`kbnClient`/`apiServices`. `esArchiver` isn't on the teardown fixture surface — Scout intentionally never exposed archive-unloading (slow and unnecessary; leftover indexes don't break tests with idempotent `loadIfNeeded`). Flag teardowns that try to use `esArchiver` at all, that **load** new data (teardown is for state reset only), or that duplicate work belonging in `afterAll`/per-test cleanup. - **[general]** **Tags / environment**: validate deployment tags and avoid assumptions that only hold in specific environments. ### Files to skip Do not review or comment on: - **`.meta` manifest files** (e.g., `**/.meta/**/*.json`): these are auto-generated for CI test planning and lane distribution. No manual regeneration is needed. ### Severity classification Use these definitions when assigning severity: - **Blocker**: Will cause test failures, breaks CI, missing required coverage (migration parity gaps), security or data leak risks - **Major**: Likely to cause flakiness, incorrect test coverage, permission/auth errors, violates core best practices in ways that affect correctness - **Minor**: Suboptimal patterns, missed reuse opportunities, efficiency improvements, style inconsistencies that don't affect correctness - **Nit**: Cosmetic issues, naming suggestions, optional improvements, "nice to have" changes When in doubt, prefer a lower severity. Optimization suggestions (efficiency improvements) should be `minor` or `nit`, not `major`. ### Migration parity analysis (Critical check 4 depth) Flag **only major** changes or drops — coverage that genuinely weakens. Skip benign deltas. - **Detect migration** when the PR removes/changes FTR tests (for example `test/functional/**`, `loadTestFile()`, FTR configs) alongside new/changed Scout specs. - **If migration is detected**: - Treat parity gaps as `blocker` unless explicitly de-scoped. - Confirm the suite is the right **test type** (UI vs API): if the old FTR suite is primarily “data correctness”, prefer migrating it to a Scout API test (or unit/integration) rather than a Scout UI test. - Build a parity map from old scenarios → new Scout coverage (roles, setup/teardown, assertions, cleanup). - Call out missing behaviors (including error paths) and recommend exactly where to add coverage. - Escalate meaningful **Scout vs FTR deltas** when they could change what’s actually being tested, weaken coverage, or increase flake risk. Treat these as parity issues that require action (code change or explicit de-scope/sign-off), and include them in the “Migration parity” output section. - auth/roles used (e.g., `admin` vs viewer), spaces behavior, and permission realism - headers/internal origin/REST versioning and any other request shaping differences - retries and error handling differences (e.g., helper methods with `ignoreErrors`, automatic retries) - parallelism/isolation differences (worker-scoped fixtures, shared state, cleanup semantics) - classic vs serverless coverage changes (suite removed from one environment but not the other) - assertion strength changes (weaker/stronger checks, removal of side-effect validation) - Verify suite wiring/discovery (new specs are picked up by Scout/Playwright config; no orphaned `loadTestFile()`). - Ensure any intentional de-scopes are explicit, and that tags/permissions remain equivalent and cloud/serverless compatible where applicable. - **Output**: include the “Migration parity” section only when action is required; otherwise omit it. ### Kibana / EUI component patterns (UI) These EUI/Kibana component behaviours are non-obvious and cannot be inferred from documentation alone. - **`QueryStringInput`**: `fill()` races with React prop sync; use `pressSequentially()` instead. - **`EuiBasicTable` empty state**: always renders a phantom "no items found" row — assert `toContainText('No items found')`, never `toHaveCount(0)`. - **EUI disabled button tooltip**: hover the `span:has([data-test-subj="..."])` wrapper, not the button itself. - **EUI CSS class selectors** (`.euiTableRow`, `.euiToolTipAnchor`, etc.): internal to EUI, change between versions — use `data-test-subj` or ARIA roles. - **DOM instability from app bugs**: use `dispatchEvent('click')` over `{ force: true }`; document the bug location in a comment. - **EUI Component Objects (`page.components.*`)**: prefer the published `@elastic/eui-test-helpers` (via `page.components.*`) over raw selectors or the old `EuiXxxWrapper`s; flag self-extension (local subclasses / one-off helper methods) — missing capabilities belong in the published helper via DevEx. See the `scout-ui-testing` skill for details. ## Output This skill does not prescribe an output format. The caller decides how findings are reported: - **Automation (macroscope, Bugbot, CI bots, etc.)**: follow the output instructions provided by the calling config. - **Local / direct invocation**: use the default format in [`OUTPUT.md`](./OUTPUT.md). ## Follow-up Offer to generate the updated code, fully incorporating the suggested improvements and resolving any parity gaps.