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 9 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: Team 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.
07souravkunda
left a comment
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.
Review feedback: the command wrapper fires performScan for any wrapped command and never knows
which hook it is in, so a window state machine keyed on hook boundaries was the wrong shape for
the question being asked.
The question is whether anything can own the scan, which is a property of the moment:
hasNoParent = no framework hook run uuid && no test context
Framework hook runs are reported to TRA and keep their test uuid, so they are untouched without
needing a hook boundary to protect them. This deletes preTestWindowActive, its three clears, and
the onHookStart/beforeHook plumbing added for it.
Two things fall out. A beforeSuite between suites is now parentless instead of inheriting the
finished test's uuid — the reviewer's second finding, fixed without reopening anything. And the
CLI "window never closes" concern stops being expressible, there being no window to leave open.
_testIdentifier was set at beforeTest/beforeScenario and never cleared, so on the classic path it
meant "has any test started" rather than "is one running". It is now cleared at afterTest and
afterScenario, which also makes startA11yScanning's "cannot be started from outside the test"
guard effective between tests instead of only before the first one.
The framework allowlist stays on the gate: it governs SCANNING, not attribution, and App
Accessibility is unsupported on jasmine, which must gain no scans it did not have before.
Tests: the CLI cases now drive commandWrapper rather than hand-feeding the flag, so they pin the
production call site — the reviewer's fifth finding. Added the between-tests case. Missing
trailing newline restored. All 12 fail against origin/main src.
SDK-7422
…k hook or test The all-hooks E2E matrix showed the per-scan rule reaching further than intended. Scans in cfg afterHook, the head of cfg beforeTest, and the trailing cfg afterScenario carried a test uuid on main and lost it here, because at those moments no hook run is active and no test is registered — true of every gap between tests, not just the one before the first. The rule is now latched: _testContextSeen is set at the first framework hook or test of the session and never reset, so only scans that precede anything the framework does are parentless. Everything from the first framework hook onward is byte-for-byte main's behaviour. That also makes the earlier _testIdentifier clear unnecessary, so it is reverted along with its side effect on startA11yScanning's "cannot be started from outside the test" guard. The cost, accepted deliberately: a beforeSuite between suites goes back to inheriting the finished test's uuid, exactly as on main. Verified across the full matrix — app and web, mocha and cucumber, every session-scoped config hook, two describes so beforeSuite fires twice. App mocha: baseline 17 scans, branch 19 — the 17 identical, +2 parentless in cfg before / cfg beforeSuite. SDK-7422
Full E2E matrix: App-A11y × Web-A11y × mocha × cucumber, every WDIO config hookNew bench
What the matrix caught, and the fix (
|
onAfterTest deleted the session's entry from accessibilityMap, and only the next test's onBeforeTest re-created it. Nothing scanned in the gap: afterSuite and after went unscanned on this flow for the life of the session, and a driver command in either was invisible. The classic flow never deleted, so the same customer code scanned on cucumber and was silently skipped on mocha. This removes the delete rather than forcing the entry true, so a tag-excluded test and a user's stopA11yScanning() still decide what happens next. Post-test scans carry the last test's uuid, which is what the classic flow has always done for cucumber's after hooks. Verified on device: app mocha now scans cfg afterSuite and cfg after (21 scans, up from 19), both stamped with the finished test's uuid. Web is unchanged at 3 scans — back() is not a wrapped command there — so nothing is gained or lost on that path. The test drives onAfterTest to completion, stubbing the stop-event internals that throw against these mocks; without that the assertion passes with the delete restored and proves nothing. SDK-7422
|
| app · mocha | scans | tail of the census |
|---|---|---|
| before this commit | 19 | ends at cfg afterTest |
after (iufzakkr… · aa79b764…) |
21 | 20 cfg afterSuite b9ceb250, 21 cfg after b9ceb250 |
Post-test scans carry the last test's uuid. That is a deliberate choice, and it matches what the classic flow has always done for cucumber's after hooks rather than inventing a third behaviour. Web (1njykirx…) is unchanged at 3 scans — back() is not a wrapped command there — so nothing is gained or lost on that path.
Two corrections to my earlier comments
- My
beforeSuiteclaim was wrong. I said the two-describespec makesbeforeSuitefire twice. This census shows exactly onecfg beforeSuiteand onecfg afterSuitefor a two-describespec — WDIO fires them per spec file, not per top-leveldescribe. Reproducing @07souravkunda's C2 scenario needs two spec files. It does not change the outcome (with the latch, a laterbeforeSuitebehaves exactly as onmain), but the evidence I cited for it did not show what I said it showed. - The first version of this commit's test was decorative — it passed with the delete restored, because
onAfterTestthrows inside its stop-event internals against these mocks and the outercatchswallowed it before reaching the line under test. Now stubbed so the method completes; verified it fails with the delete put back.
Suite 1233 passed / 0 failed; tsc and lint clean.
Correction: my web numbers were wrong — the bench used a command web does not wrap@07souravkunda flagged that the web scans looked partial. They were, and the fault was in my bench, not the SDK. Correcting it publicly because the earlier matrix comment gave web figures that cannot be defended. What happened. The web confs issued So "baseline 2 → branch 3" measured the bench, not the coverage. Corrected web matrix
Web · mocha now shows scans in all eight config hooks. Both fixes in this PR are visible on it, and the earlier claim that web gains "one scan per session" was an understatement produced by the broken bench. No code changed for this — the correction is entirely in |
Correction 2: the spec and steps had the same flaw as the hooks@07souravkunda pointed at the cucumber baseline Every Web matrix, third and final measurement
The deltas are unchanged from correction 1 (+4 / +2) — the extra spec-level scans land on both sides, so only the absolute counts were understated. What this measurement adds is the part that was missing: everything between the new scans is now exercised and identical to baseline, so "nothing changes once the framework starts" is demonstrated on web, not just on app. Full branch census for web · mocha shows scans in all eight config hooks plus both framework-hook and test-body positions: Still no product code change from either correction — both were bench defects. Three measurements in, the rule I have written into the bench is: verify the command is wrapped on the path under test, at every layer that issues one — hooks, specs and steps each needed it independently. |
07souravkunda
left a comment
07souravkunda
left a comment
There was a problem hiding this comment.
Re-reviewed at 027b140. One of my four findings was wrong and I have withdrawn it: the CLI flow is not reachable for cucumber, jasmine or multiremote — CLISupportedFrameworks = ['mocha'] gates the binary bootstrap at both launcher.ts:343 and service.ts:175, which I had not traced. The web-scope decision is yours and I am not contesting it; the residual attribution question is genuinely open and correctly tracked. The test wiring and newline are fixed, and your correction on currentTestName was right. Replacing the window state machine with the latched per-scan rule is a better design than what I reviewed, and 5d7d6ac's all-hooks matrix is a stronger reason for it than my original finding was. Still holding the change request on two things. First, code: the new rule reaches the auto-scan path on both flows but only the CLI flow threads it into the manual browser.performScan() — the classic one still calls with four arguments, so a user-invoked scan from a config-level before() is stamped with the not-yet-started test's uuid on one flow and parentless on the other. Second, artefacts: three commits of design change have not reached the PR description, the ## Known limits section, or either release-note block — the description still documents the window that 5d7d6ac removed, still claims beforeSuite coverage that now holds only for the first suite, and nothing announces the between-test gate change from 027b140 that the commit measured at 19→21 scans. The commit messages are honest about all of this; what a releaser and the next reviewer read is not. Details in the threads.
Review finding: the auto path got the rule on both flows, but the classic user-facing browser.performScan() still called with four arguments, so hookRunUuid and isGlobalHook both arrived undefined and the scan carried TEST_ANALYTICS_ID. The CLI equivalent was threaded. Same customer code — performScan() from a config-level before() — was parentless on mocha and misattributed on cucumber, and the manual path is the only route to a config-hook scan for a setup that issues no wrapped command. The rule is now a single getter per flow, used at every scan site: the auto path, the manual performScan(), and the CLI's per-test re-patch, which passed the hook uuid but not the flag. That last one was harmless — it is installed after the latch sets, so undefined and false agreed — but it would have diverged silently the moment the rule changed. Threading _currentHookRunUuid into the classic manual path also closes a pre-existing gap the reviewer noted: a manual scan inside a framework hook used to land as a NULL hook row. Two tests, both failing if the manual path is un-threaded: a manual scan from a config hook is parentless, and one inside a framework hook carries that hook's uuid. SDK-7422
Reviewer question on the gate-teardown change: with the map entry outliving the last test, does a
wrapped command in teardown turn a silent no-op into a logged failure? It does. Reproduced with a
driver command in afterSession, which runs after deleteSession:
ERROR @wdio/browserstack-service/cli: Accessibility Scan could not be performed :
Error: A sessionId is required for this command
The customer's own command fails identically a moment later, so nothing breaks — but main was
silent there and this is an error-level line in ordinary teardown, which is support-ticket
material.
Both wrappers now check the one thing that actually knows whether the session is alive — the
driver's sessionId — and skip the scan without touching the command. afterSuite and after still
scan, since those run before the session is deleted.
Verified on device: the error line is gone, the customer command still throws as it does on main,
and the all-hooks matrix is unchanged at 21 scans with cfg afterSuite and cfg after present.
The CLI mock browser gained a sessionId, since it was modelling a dead session and three tests
were passing for the wrong reason.
SDK-7422
|
RUN_TESTS |
07souravkunda
left a comment
07souravkunda
left a comment
There was a problem hiding this comment.
Lifting the change request at 64e8682 — every code finding is resolved, and the two rounds of self-correction (5d7d6ac bounding the rule to a latch, 64e8682 guarding the dead session) landed in better shape than what I asked for. Notably you found two things I had not: the CLI's per-test re-patch of performScan at :383, and the three CLI tests that were passing because their mock browser modelled a dead session. Reproducing the afterSession case with the actual error line rather than reasoning about it is the right instinct. Verified myself: the guard's early return in the CLI wrapper matches the normal tail so nothing is skipped or double-invoked, the classic one falls through correctly, and multiremote is unaffected since service.ts:277 seeds the gate from the same sessionId. Two non-blocking items left, both in threads. The classic commandWrapper guard has no test — the CLI side has one, and the shared classic mock carries sessionId: 'session123', so a revert there would go uncaught. And the description body plus verification table are still the pre-2b81d10 ones: they describe a window that no longer exists, claim the gate governs nothing beyond itself (which 027b140 deliberately reversed), and cite three superseded commits — while ## Known limits and both release blocks are now correct. Not blocking either; the second one only costs the next reader, and the numbers from 5d7d6ac and 027b140 are stronger evidence than the table currently carries. Approving on the code.
07souravkunda
left a comment
07souravkunda
left a comment
There was a problem hiding this comment.
Approving to actually clear the earlier change request — my previous comment said I was lifting it, but a comment-level review does not dismiss CHANGES_REQUESTED, so the PR was still showing blocked. Assessment is unchanged from that comment: all code findings resolved at 64e8682; the two remaining items (no test on the classic dead-session guard, stale description body and verification table) are non-blocking and live in the threads.
Full E2E re-run on the final build (
|
| combo | baseline (main) |
branch | delta | scans lost |
|---|---|---|---|---|
| app · mocha (CLI) | dtehwakw… · c6e142e3… — 17 |
rpzi0bbp… · aa5af9b4… — 21 |
+4 cfg before, cfg beforeSuite, cfg afterSuite, cfg after |
none |
| app · cucumber (classic) | 7cbxbazs… · dee5ef98… — 20 |
we7sj8hf… · 102d47cb… — 22 |
+2 cfg before, cfg beforeFeature |
none |
| web · mocha (CLI) | sje6k34f… · b7542d61… — 17 |
cxafmlvp… · 6a23fa4f… — 21 |
+4 cfg before, cfg beforeSuite, cfg afterSuite, cfg after |
none |
| web · cucumber (classic) | 2bewlu4o… · 8cecbde9… — 20 |
4dcjtuhu… · dfe3a3ac… — 22 |
+2 cfg before, cfg beforeFeature |
none |
Deltas computed as a multiset difference over scan phases, so "scans lost: none" is a measured result, not an eyeball: no scan present on main is missing on the branch, on any of the four combinations.
What the shape tells you
- +4 on both mocha rows, +2 on both cucumber rows — the gain tracks the flow, not the product. Mocha runs the CLI flow, which gains the two pre-test hooks and the two teardown hooks (the between-test gate was deleted there). Cucumber runs the classic flow, which never deleted the gate, so its teardown hooks already scanned and it gains only the two pre-test ones.
- App and web are now identical in shape, which is the part the earlier bench defects hid.
- Attribution unchanged after the first framework hook. On app · mocha the 17 baseline scans keep their uuids exactly; the 4 additions are parentless before the first test, and the two teardown scans carry the finished test's uuid as documented.
Teardown, re-confirmed
The afterSession probe still fires a driver command against a deleted session, and with the liveness guard the run is clean — no Accessibility Scan could not be performed line. The customer's own command throws exactly as on main.
Suite 1236 passed / 0 failed; tsc and lint clean.
2 participants
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
afterSuite/afteris stamped with the last test that ran. Deliberate: it is what the classic flow has always done for cucumber's after-hooks, and inventing a third behaviour for mocha would be worse than matching it.saveTestResultsflushes withthTestRunUuid. Scans from config hooks before the first test are therefore flushed into the first test's results.isGlobalHookcannot reach that path; fixing it needs a uuid-less flush, which depends on an extension contract this SDK cannot see.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()andbeforeSuitebefore the run starts, andafterSuite/afterduring teardown — so screens visited outside your tests are covered.browser.performScan()called from a config hook is no longer attributed to a test that has not started.Release notes (internal): (required — engineer-facing; what actually changed / why)
onAfterTestno longer deletes the session's entry fromaccessibilityMap. That teardown silenced every scan between tests and after the last one on the CLI flow —afterSuiteandafternever scanned there, while the classic flow (which never deleted) scanned them. Removing it rather than forcing the entry true keeps a tag-excluded test and a user'sstopA11yScanning()in effect.thTestRunUuid, becauseTEST_ANALYTICS_IDthere holds a uuid minted at instance creation for a test that has not started. The rule is latched at the first framework hook or test, so everything after that point behaves exactly as before. It is applied at every scan site on both flows, including the user-facingbrowser.performScan().Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.