Repository navigation
fix(cli): send the per-test accessibility verdict on test events - #359
kamal-kaur04 wants to merge 4 commits into
Conversation
On the CLI flow the binary set product_map.accessibility from the platform check alone, so every test in a tag-scoped build was reported as an accessibility test. The accessibility module now sets test_accessibility (accessibility on, auto-scanning on, test in the include/exclude tag scope), matching the classic flow's setTestRunAccessibilityVar. Its TEST/PRE observer is prepended so the value is on the instance before TestRunStarted is sent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
|
RUN_TESTS |
|
[SDK Wdio Test] TRA build state: failed | Stability 48% — verdict: failure. Passed: 28, Failed: 30, Aggregate: 58. TRA: https://observability.browserstack.com/builds/rm4nr97feknfkgrnjdrggkk6mwbxwsjy3oobhtvu |
kamal-kaur04
left a comment
kamal-kaur04
left a comment
There was a problem hiding this comment.
Claude Code Review (automated) — 4 inline finding(s). Full report in the PR comment below. Verdict: Passed.
| const inScope = Boolean(this.accessibility && this.autoScanning && | ||
| shouldScanTestForAccessibility(suiteTitle, test.title || '', this.config.accessibilityOptions as Record<string, string> | undefined, world, Boolean(world))) |
There was a problem hiding this comment.
[Low] Verdict expression duplicated with the onBeforeTest scan gate
This expression copies the scan gate in onBeforeTest. One drives the reported product_map.accessibility and the other drives actual scanning. If one is edited without the other, the reported a11y-test flag and the scanning behaviour will drift apart.
Suggestion: Extract a single private helper and call it from both, or have onBeforeTest read KEY_TEST_ACCESSIBILITY back from the instance (the prepended observer has already set it).
Reviewer: stack:code-review
| shouldScanTestForAccessibility(suiteTitle, test.title || '', this.config.accessibilityOptions as Record<string, string> | undefined, world, Boolean(world))) | ||
| TestFramework.setState(testInstance, TestFrameworkConstants.KEY_TEST_ACCESSIBILITY, inScope) | ||
| } catch (error) { | ||
| this.logger.debug(`setTestAccessibility: ${error}`) |
There was a problem hiding this comment.
[Low] Failure logged at debug and silently falls back to the platform check
If this throws, no key is written, so the binary falls back to its platform check and marks the test as an accessibility test (the behaviour this PR fixes). Nothing in the logs explains why.
Suggestion: Log at error, as onBeforeTest and onHookStart do. Optionally also write false here so a failure stays in scope.
Reviewer: stack:code-review
| it('should be prepended to TEST/PRE so TestRunStarted carries the verdict', () => { | ||
| expect(TestFramework.registerObserver).toHaveBeenCalledWith( | ||
| TestFrameworkState.TEST, | ||
| HookState.PRE, | ||
| expect.any(Function), | ||
| true | ||
| ) | ||
| }) |
There was a problem hiding this comment.
[Low] Ordering is asserted only by the prepend flag
This test checks that prepend=true was passed. It does not check that test_accessibility is on the instance when an earlier-registered TEST/PRE observer (TestHub) reads it. The no-instance and exception paths are also untested.
Suggestion: Add a test against the real eventDispatcher: register a stub observer first that captures getAllData(), then register setTestAccessibility with prepend, notify, and assert the captured map contains test_accessibility. Also add tests for the missing-instance and throwing cases.
Reviewer: stack:code-review
| const suiteTitle = (typeof args.suiteTitle === 'string' ? args.suiteTitle : '') || '' | ||
| const test = (args.test && typeof args.test === 'object' ? args.test as { title?: string } : {}) || {} | ||
| const world = args.world as { [key: string]: unknown } | undefined | ||
| const inScope = Boolean(this.accessibility && this.autoScanning && |
There was a problem hiding this comment.
[Medium] Auto-scanning is part of the verdict (behaviour change on CLI flow)
With autoScanning: false, every test now sends product_map.accessibility: false, App Accessibility sessions included. Before this change the binary marked them true by platform. This matches the classic handler (setTestRunAccessibilityVar(this._accessibility && shouldScanTest)), but it differs from the behave precedent (999131df5 has no auto-scanning term), and manual performScan() results may no longer attach to an accessibility test row.
Suggestion: Confirm with the A11y backend owners that manual-scan results still attach when product_map.accessibility is false. If they don't, drop autoScanning from the verdict and keep it only on the scan gate. Add an App A11y case with autoScanning: false plus a manual performScan.
Reviewer: stack:workspace-pr-review (cross-repo)
Claude Code PR ReviewPR: #359 • Head: d83e8b1 • Reviewers: stack:code-review, stack:workspace-pr-review (cross-repo) SummaryCLI flow: Review Table
Correctness notes (Logic row):
Findings
Verdict: PASS |
Scenarios abandoned by a failed BeforeAll are built straight into TEST/POST, so the TEST/PRE verdict never ran for them and the binary fell back to its platform check. They never ran or scanned, so they now carry test_accessibility: false. The in-scope rule is computed by one helper for both the reported verdict and the scan gate, and a failure to set the verdict is logged at error level. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With autoScanning off, tests scanned manually with performScan() still need their accessibility test row, so test_accessibility is now accessibility on and test in the include/exclude tag scope. Auto-scanning still gates the automatic scans. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
✅ Good to go
Change map (generated deterministically from the diff)graph LR
subgraph nbinary["binary"]
ncli_test_modules_observability_languages_webdriverio_test_frameworks_cucumber_index_test_js["index.test.js<br/>~30 lines"]
ncli_test_modules_observability_languages_webdriverio_test_frameworks_mocha_index_test_js["index.test.js<br/>~13 lines"]
npackages__browserstack_observability_languages_webdriverio_test_frameworks_cucumber_index_js["index.js<br/>~9 lines"]
npackages__browserstack_observability_languages_webdriverio_test_frameworks_mocha_index_js["index.js<br/>~9 lines"]
end
subgraph nwdio_service["wdio-service"]
nwdio_browserstack_service_packages_browserstack_service_src_cli_modules_accessibilityModule_ts["accessibilityModule.ts<br/>~49 lines"]
nwdio_browserstack_service_packages_browserstack_service_tests_cli_modules_accessibilityModule_test_ts["accessibilityModule.test.ts<br/>~49 lines"]
nwdio_browserstack_service_packages_browserstack_service_src_cli_eventDispatcher_ts["⚠ eventDispatcher.ts<br/>~10 lines"]
nwdio_browserstack_service_packages_browserstack_service_tests_cli_eventDispatcher_test_ts["⚠ eventDispatcher.test.ts<br/>~10 lines"]
nwdio_browserstack_service__changeset_pr_359_md["pr-359.md<br/>~5 lines"]
nwdio_browserstack_service_packages_browserstack_service_src_cli_frameworks_testFramework_ts["testFramework.ts<br/>~5 lines"]
nwdio_browserstack_service_packages_browserstack_service_src_cli_frameworks_wdioCucumberTestFramework_ts["wdioCucumberTestFramework.ts<br/>~2 lines"]
nwdio_browserstack_service_packages_browserstack_service_src_cli_frameworks_constants_testFrameworkConstants_ts["testFrameworkConstants.ts<br/>~1 lines"]
nwdio_browserstack_service_packages_browserstack_service_tests_service_test_ts["service.test.ts<br/>~1 lines"]
end
nsvc_Observability_TRA(["Observability/TRA"])
nsvc_collector_observability_browserstack_com(["collector-observability.browserstack.com"])
nsvc_upload_observability_browserstack_com(["upload-observability.browserstack.com"])
npackages__browserstack_observability_languages_webdriverio_test_frameworks_cucumber_index_js --> nsvc_Observability_TRA
npackages__browserstack_observability_languages_webdriverio_test_frameworks_cucumber_index_js --> nsvc_collector_observability_browserstack_com
npackages__browserstack_observability_languages_webdriverio_test_frameworks_cucumber_index_js --> nsvc_upload_observability_browserstack_com
npackages__browserstack_observability_languages_webdriverio_test_frameworks_mocha_index_js --> nsvc_Observability_TRA
npackages__browserstack_observability_languages_webdriverio_test_frameworks_mocha_index_js --> nsvc_collector_observability_browserstack_com
npackages__browserstack_observability_languages_webdriverio_test_frameworks_mocha_index_js --> nsvc_upload_observability_browserstack_com
↻ This verdict comment is the review anchor — it's updated in place on each run (the gate posts its status separately). — SDK PR Review Agent |
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
What is this about?
On the CLI flow, every test in a build that uses
accessibilityOptions.includeTagsInTestingScope/excludeTagsInTestingScopeis reported as an accessibility test, including tests outside the tag scope. The binary sets each test'sproduct_map.accessibilityfrom the platform check alone (accessibility on + supported browser), so every test gets an accessibility test-execution record. The classic flow sends the per-test verdict (Listener.setTestRunAccessibilityVar), so only in-scope tests are recorded there.This PR sends that verdict on the CLI flow:
AccessibilityModule.setTestAccessibilitysetstest_accessibilityon the test instance: accessibility on and the test in the tag scope (sameshouldScanTestForAccessibilitycall the scan gate makes, so cucumber uses the gherkin tags and mocha the title match). Auto-scanning is deliberately left out, so tests scanned manually withperformScan()keep their accessibility row; it still gates the automatic scans.BeforeAllfailed are built straight into TEST/POST and never pass TEST/PRE, so they are created withtest_accessibility: false(they never ran or scanned).prependoption (EventDispatcher.registerObserver/TestFramework.registerObserver), so it runs beforeTestHubModuleserializes TestRunStarted. Without that, TestRunStarted would still carry the platform-only value.test_accessibilitywhen it is a boolean and keeps the platform check otherwise (paired binary PR). Older binaries ignore the extra key.Effect: in a tag-scoped build, out-of-scope tests no longer get accessibility test-execution records, matching the classic flow. Scanning behaviour is unchanged.
Backports: v9 and v8 (linked below).
Related Jira task/s
N/A (tracked internally)
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)
includeTagsInTestingScope/excludeTagsInTestingScopebeing reported as accessibility tests when using the CLI.Release notes (internal): (required — engineer-facing; what actually changed / why)
test_accessibilityverdict (a11y on and test in tag scope; auto-scanning excluded so manual-scan tests keep their row) on test events, set by a TEST/PRE observer prepended ahead of TestHubModule so TestRunStarted carries it; cucumber cascade-skipped scenarios send false. Restores tag scoping for product_map.accessibility; requires the paired binary change to take effect.Checklist
Unit tests:
accessibilityModule.test.tscovers the verdict (in scope / out of scope / auto-scanning off keeps an in-scope test / platform unsupported / mocha title match) and cascade-skipped scenarios carryingfalse, plus the prepended registration.eventDispatcher.test.tscovers prepend ordering (main and v9 only).tsc --noEmitis clean.End to end (WDIO 9 + cucumber, Chrome latest / Windows 11,
includeTagsInTestingScope: ['@accessibility'], CLI flow; 1 tagged scenario + 2 untagged),product_map.accessibilityon TestRunStarted / TestRunFinished:PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.