scout-best-practices-reviewer

name	scout-best-practices-reviewer
description	Review Scout UI/API tests (including Scout test migrations) for best practices, reuse, parity, and server config hygiene.

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 unless required); space-aware behavior covered or explicitly out of scope.

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 EuiXxxWrappers; 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.

Follow-up

Offer to generate the updated code, fully incorporating the suggested improvements and resolving any parity gaps.