feat(app-a11y): scan the WDIO config-level hook window, with or without a parent uuid - #176
feat(app-a11y): scan the WDIO config-level hook window, with or without a parent uuid#176kamal-kaur04 wants to merge 3 commits into
Conversation
A driver command issued from a config-level before()/beforeSuite was never scanned. The scan gate opened at the first test, and WDIO runs those hooks before any test or framework hook exists, so setup screens went uncovered. The gate now opens as soon as the session is known a11y-capable. Every validation the per-test gate applies still applies here — an a11y-capable session, autoScanning, a supported framework (mocha, cucumber; jasmine untouched), a real session id, and non-multiremote. The include/exclude tag filter is the one exception: it matches on suite and test titles, and in this window neither exists yet. onBeforeTest/beforeScenario re-computes the per-test gate, tags included, so the window governs nothing beyond itself. Scans from the window carry no test run uuid. TEST_ANALYTICS_ID there holds a uuid the framework minted at instance creation — a test that has not started — so sending it attributed the scan to a test it did not come from. Two existing tests pinned the exact argument list of the scan helpers, so appending a parameter made them silently follow it; both now assert by position. SDK-7422
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited), Workspace UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
…he first test The window targets WDIO's own config hooks. Closing it at the first test swept in mocha's before all / before each #1, which run inside that span, and stripped their test run uuid — baseline carried it, the first cut showed none. That is a behaviour change to framework hooks, which are not this feature's business. It now closes the moment the framework signals a hook: onHookStart on the CLI path, beforeHook on the classic one. The CLI clear sits ahead of that method's own early returns — if a framework hook started at all, the config-level window is over, whether or not that hook goes on to scan. The existing clears at the first test/scenario stay as a backstop for a spec with no framework hooks. Four tests cover it, two per flow: a framework hook closes the window, and a framework-hook scan still carries its test run uuid. Reverting either clear fails exactly those four. Verified on device — repro-inline-hooks (framework hooks only) is now structurally identical to baseline, every scan carrying its test uuid, while repro.conf.ts still gains its 7 parentless config-level scans. SDK-7422
Correction pushed (
|
| conf | baseline (main) |
corrected branch | reading |
|---|---|---|---|
repro.conf.ts |
chnsj2hc… — 6 scans, 0 in window |
bfhusrbb… · 4fda1f2fe573fd326b09ec560c148588fe62aa33 — 13 |
7 new parentless window scans; before all + before each #1 restored to test uuid db285069 + hook uuid |
repro-cucumber.conf.ts |
zrlxhmjd… — 3 |
edsvapa0… · 86407fe669f17674685c03293866de9711cf6544 — 5 |
2 new parentless; steps keep 86bbd66b |
repro-inline-hooks.conf.ts |
5xslabfr… — 6, all with test uuid |
bqez3voc… · 4a1e06127c5d9c9d39610ac63d599dec1a8b0897 — 6, all with test uuid |
structurally identical to baseline — the control, since its only scans come from framework hooks |
Tests
Four added, two per flow: a framework hook closes the window, and a framework-hook scan still carries its test run uuid. They are real regression tests — reverting src/ to the previous commit fails exactly those four (4 failed, 92 passed). Full suite 1231 passed, 0 failed; tsc and lint clean.
The PR description is updated: new build ids, and the "before each #1 loses its test uuid" known limit is deleted rather than reworded — it no longer happens.
| // validation still applies — an a11y-capable session (returned above), autoScanning, a | ||
| // supported framework, a real session id. The include/exclude tag filter is the one | ||
| // exception: it matches on suite and test titles, and neither exists in this window. | ||
| if (this._autoScanning && this.supportsPreTestWindow() && sessionId) { |
There was a problem hiding this comment.
This gate has no app-accessibility check, so it opens on web (Automate) a11y sessions too — supportsPreTestWindow() checks framework and multiremote only, and the !this._accessibility return above is satisfied by validateCapsWithA11y as well as the App variant.
On the web path the isGlobalHook correction has no reach: performA11yScan takes the non-app branch and calls executeAccessibilityScript(browser, performScan, { method: commandName }) — isGlobalHook is never forwarded. The web payload carries no test identity at all; attribution happens later, when sendTestStopEvent flushes accumulated results via saveTestResults(dataForExtension) with thTestRunUuid = process.env.TEST_ANALYTICS_ID.
Evidence: so every scan fired in the config-level window is flushed into the first test's results, inflating its issue count with setup screens — the same misattribution this PR removes on the app path, reintroduced where the fix cannot apply. The PR title, the changeset, and all three verification rows are app_automate; no web run appears in the table.
Fix:
if (this._autoScanning
&& isAppAccessibilityAutomationSession(this._accessibility, this.isAppAutomate)
&& this.supportsPreTestWindow() && sessionId) { … }Question: was a web a11y session bench-tested? If web coverage is intended, what attributes a window scan there, given saveTestResults is the only thing that assigns a test uuid?
| // exception: it matches on suite and test titles, and neither exists in this window. | ||
| if (this._autoScanning && this.supportsPreTestWindow() && sessionId) { | ||
| AccessibilityHandler._a11yScanSessionMap[sessionId] = true | ||
| this._preTestWindowActive = true |
There was a problem hiding this comment.
The PR body and the changeset both promise coverage for config-level before() and beforeSuite, but only the first beforeSuite of a session falls inside this window.
Evidence: beforeSuite fires per suite (service.ts:825 sets _suiteTitle on each one). The window closes at the first framework hook, with the first test as backstop — so from the second top-level describe onward the window is long gone, and commands issued in that beforeSuite fall back to the previous test's state: _a11yScanSessionMap still holds the last test's shouldScanTest, and TEST_ANALYTICS_ID still holds the last test's uuid. Those scans are attributed to a test that has already finished — the same misattribution this PR exists to remove, shifted to the second suite onward. A spec file with two top-level describes reproduces it.
Fix: either reopen the window on beforeSuite (a suite start with no active test), or narrow the claim in the PR body and changeset to before() plus the first beforeSuite.
| // on suite and test titles, and in this window neither exists yet. onBeforeTest | ||
| // re-computes the per-test gate, tags included, so this only affects the window. | ||
| const preTestSessionId = this.currentSessionId() | ||
| if (this.autoScanning && preTestSessionId !== undefined && preTestSessionId !== null) { |
There was a problem hiding this comment.
Same concern as on the classic gate (accessibility-handler.ts): this gate has no app-accessibility check, so it opens on web (Automate) a11y sessions too — the !this.accessibility return above is satisfied by validateCapsWithA11y as well as validateCapsWithAppA11y.
performScanCli only forwards isGlobalHook on the isAppAccessibility branch; the web branch is executeAsync(performScan, { method: commandName }) and drops it. Attribution on that path happens in sendTestStopEvent → saveTestResults(dataForExtension), so window scans land in whichever test saves next — the first one.
Fix:
if (this.autoScanning && this.isAppAccessibility
&& preTestSessionId !== undefined && preTestSessionId !== null) { … }Question: is web coverage intended here, or should both gates be scoped to app accessibility to match the PR's title and verification?
| const preTestSessionId = this.currentSessionId() | ||
| if (this.autoScanning && preTestSessionId !== undefined && preTestSessionId !== null) { | ||
| this.accessibilityMap.set(preTestSessionId, true) | ||
| this.preTestWindowActive = true |
There was a problem hiding this comment.
This gate carries neither of the two guards its classic twin has: no framework allowlist (PRE_TEST_SCAN_FRAMEWORKS = ['mocha','cucumber'], jasmine deliberately excluded) and no multiremote exclusion. Nothing upstream supplies them — AccessibilityModule is constructed purely on startBinResponse.accessibility?.success (cli/index.ts:190), and _isCliAccessibilityFlow() is isBrowserstackSession && isRunning() (service.ts:904). Neither is framework-aware.
Evidence: the window's two close paths are onBeforeTest (TestFrameworkState.TEST PRE) and onHookStart (BEFORE_ALL/BEFORE_EACH/…) — both registered on TestFramework, and both only ever emitted by WdioMochaTestFramework, which setupTestFramework() instantiates only for webdriverio-mocha (cli/index.ts:549). service.ts:437 already documents getTestFramework() being null for non-mocha. Cucumber never reaches beforeTest at all — service.beforeScenario calls the classic handler only.
So on CLI + cucumber the window opens at driver creation and never closes for the life of the session: accessibilityMap stays true and every scan in every scenario goes out with isGlobalHook = true, dropping thTestRunUuid for the whole run. Jasmine — "untouched" per the PR body — is likewise unguarded on this flow. The cucumber verification row is the one without a "(CLI)" annotation, so this cell looks untested.
Fix: mirror the classic guard here (ideally reading the same PRE_TEST_SCAN_FRAMEWORKS constant plus the multiremote check), and add a close on the cucumber scenario path — or restrict the CLI window to mocha, where a close signal provably exists.
Question: is the CLI/binary flow reachable for cucumber and jasmine today? If it is, what closes preTestWindowActive there?
| await accessibilityModule.onBeforeExecute() | ||
| await accessibilityModule.onHookStart({ instance: mockTestInstance }) | ||
|
|
||
| await (accessibilityModule as any).performScanCli(mockBrowser, 'click', 'hook-uuid-1', accessibilityModule.preTestWindowActive) |
There was a problem hiding this comment.
This asserts the helper, not the wiring. The flag is hand-fed as the fourth argument, so the test says nothing about what the production call site passes.
The real call site is commandWrapper → performScanCli(browser, command.name, this.currentHookRunUuid, this.preTestWindowActive) (accessibilityModule.ts:308). Change that fourth argument to a literal true and this test still passes — the regression it is meant to catch slips through.
Evidence: the classic-flow twin gets this right. tests/accessibility-handler.test.ts → "leaves a framework-hook scan carrying its test run uuid" drives handler['commandWrapper'](…) and asserts lastCall[7], so the wiring is genuinely pinned.
Fix: go through the wrapper and assert what the spy observed:
const orig = vi.fn().mockResolvedValue('ok')
await (accessibilityModule as any).commandWrapper({ name: 'click', class: 'Element' }, orig, 'arg')
expect(_getParamsForAppAccessibility).toHaveBeenCalledWith('click', undefined, 'hook-uuid-1', false)| expect(_getParamsForAppAccessibility).toHaveBeenCalledWith('click', undefined, undefined, undefined) | ||
| }) | ||
| }) | ||
| }) No newline at end of file |
There was a problem hiding this comment.
This file has no trailing newline (\ No newline at end of file in the diff). Pre-existing on main rather than introduced here, but the PR already edits this file — worth closing while it is open.
07souravkunda
left a comment
There was a problem hiding this comment.
The mechanism is minimal and well-reasoned, and the 2a872db correction — closing the window at the first framework signal rather than the first test, with the framework-hooks-only conf as a control — is the right call and properly covered. Blocking on scope rather than on design: neither gate is conditioned on app accessibility, so web (Automate) a11y sessions are silently in scope on a path where isGlobalHook is never forwarded and attribution happens at saveTestResults — window scans land in the first test's results. And the CLI gate carries neither of the classic gate's guards (framework allowlist, multiremote), while its only close signals are emitted by WdioMochaTestFramework, which is wired for webdriverio-mocha alone — so on CLI + cucumber the window appears to open at driver creation and never close. Two smaller items: beforeSuite is covered only for the first suite in a session, and the CLI test for the uuid-preservation rule hand-feeds the flag instead of going through commandWrapper, so the :308 wiring is unpinned. Details in the inline threads.
What is this about?
A driver command issued from a WDIO config-level
before()orbeforeSuitewas never scanned for App Accessibility. The scan gate opened at the first test — and WDIO runs config-level hooks before any test or framework hook exists — so screens visited during setup went uncovered.The gate now opens as soon as the session is known a11y-capable, and the scan is sent whether or not a parent uuid exists: in that window there is no test and no framework hook, so the payload carries neither
thTestRunUuidnorthHookRunUuid.Every other validation still applies
Only the include/exclude tag filter is skipped, and only inside the window — it matches on suite and test titles, and neither exists yet. Everything else the per-test gate checks is unchanged: an a11y-capable session (caps-validated),
autoScanning, a supported framework (mocha + cucumber; jasmine untouched, where App Accessibility is not supported), a real session id, and non-multiremote.onBeforeTest/beforeScenariore-computes the per-test gate — tags included — so the window governs nothing beyond itself.No test run uuid for a window scan
TEST_ANALYTICS_IDin that window holds a uuid the framework minted at instance creation — a test that has not started. Sending it attributed the scan to a test it did not come from.Verification
Bench:
test-samples/app_automate-wdio_mocha-android/, phase boundaries annotated into the session log viabrowserstack_executorso each scan is attributed by position. Verdicts read from the App Automate session-logs API (work/SDK-7422-app-a11y-fixes/uuid-verdict.py).main)repro.conf.tschnsj2hcevtndlsq4q4fghp8b0zvnvo3ajx0zded·119316d3e3689f4921b9256ed9bb9b5a948704ef— 6 scans, 0 in the windowbfhusrbbjfyxlf4gbh0crdudvwsaup7wy5qi2gau·4fda1f2fe573fd326b09ec560c148588fe62aa33— 13 scansexecute,executeScript,click×3,execute,executeScript, each with no test uuid and no hook uuidrepro-cucumber.conf.tszrlxhmjdu5z2diuvqeyqtizwvclttuc4uoefa8jt·c2a250be4c351e5ddc63b57ead8ba752e3a00fe4— 3 scansedsvapa0evg18zt2ilbbjmd3t4l3s2fhuhhuccih·86407fe669f17674685c03293866de9711cf6544— 5 scansrepro-inline-hooks.conf.ts— framework hooks only5xslabfrn225yzhdlaivg1z2fqubny7lcdsntrbj·ccd994cd941375698e449cc18398d73def6b66e8— 6 scans, all carrying their test uuidbqez3voc1pj3kgqefduycmvjis1ksttcajmfojy7·4a1e06127c5d9c9d39610ac63d599dec1a8b0897— 6 scans, all carrying their test uuidbefore()touches no scannable command, so its only scans come from framework hooksPer-phase on the mocha hook-command conf, this branch: 7 scans in
config-level before()where the baseline had zero, thenmocha before all2,before each #11, test 1 body 3 — every one of those four framework-hook and test-body scans carrying test uuiddb285069, exactly as the baseline does.Framework hooks are deliberately untouched
The window closes the moment the framework signals a hook (
onHookStarton the CLI path,beforeHookon the classic one), not at the first test. Closing at the first test would sweep in mocha'sbefore allandbefore each #1— they run inside that span — and strip their test run uuid, which is a behaviour change to framework hooks that this feature has no business making. The third row above is the control: a conf whose only scans come from framework hooks is structurally identical to baseline.Unit: the params rule (window ⇒ no test uuid, hook uuid preserved; non-window ⇒ uuid sent), the CLI gate (opens at driver creation, respects
autoScanning, closes at the first test), and the classic gate (mocha, cucumber, jasmine unchanged, multiremote skipped, closes at the first test). Full suite 1231 passed, 0 failed;npm run lintclean.Known limits
Related Jira task/s
https://browserstack.atlassian.net/browse/SDK-7422
Release (mandatory for every PR — required for the
ready-for-reviewlabel)Version bump: (required — tick exactly one)
Release notes type: (optional)
Release notes (customer-facing): (optional but encouraged)
before()andbeforeSuitehooks, so screens visited during setup are covered.Release notes (internal): (required — engineer-facing; what actually changed / why)
thTestRunUuid—TEST_ANALYTICS_IDthere holds a uuid minted at instance creation for a test that has not started. Every validation except the include/exclude tag filter still applies; tags cannot be evaluated without a suite/test title.Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.