Disable accessibility when browserstackAccessibility plugin not loaded#1132
Disable accessibility when browserstackAccessibility plugin not loaded#1132pranay-v29 wants to merge 3 commits into
Conversation
When a build requests accessibility (browserstack.json/browser caps) but the browserstackAccessibility plugin is not wired into the cypress config, the CLI now detects this before the build start event, explicitly disables accessibility so the build is not counted as an a11y build, warns the user with the setup doc link, and instruments the end-of-session EDS event (accessibility_plugin_not_loaded) so such builds can be excluded from stability queries. Detection is code-based: the plugin sets BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED when its module is loaded/invoked by the cypress config; requireModule writes a flag file the parent process reads back. Falls back to a raw-source scan only when the config could not be required (e.g. TS before bstack packages are installed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-cli into disable-accessibility-when-plugin-not-loaded
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }; | ||
|
|
||
| const isBindingCalled = (content, binding) => { | ||
| const callRegex = new RegExp('\\b' + binding.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + '\\s*\\('); |
kamal-kaur04
left a comment
kamal-kaur04
left a comment
There was a problem hiding this comment.
Automated SDK-style review — accessibility-plugin-not-loaded detection.
Design is sound and degrades safely; nothing build-breaking. Inline notes below: 2 warnings worth resolving before merge (false-negative on unusual import syntax; redundant TS config compile on every a11y run) and 3 cleanup suggestions. Posted as comments — final approve/merge call is the human reviewer's.
| // If accessibility is requested but the BrowserStack accessibility plugin is | ||
| // not loaded in the cypress config, explicitly disable accessibility before | ||
| // the build start event so the build is not treated as an accessibility build. | ||
| if (isAccessibilitySession && isBrowserstackInfra && !isAccessibilityPluginLoaded(bsConfig)) { |
There was a problem hiding this comment.
isAccessibilityPluginLoaded(bsConfig) calls readCypressConfigFile, which is already invoked at capabilityHelper.js:251 and helper.js:378 during a normal run. This adds a third call. For a TS config each call runs convertTsConfig → cp.execSync(tsc …) (a fresh sync TypeScript compile) plus a child-process require that re-executes the user config's top-level side-effects.
Consider reading the config once and threading the detection signal through, or memoizing on BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED once an earlier read in the same run has populated it, only forcing a fresh read when it's still undefined.
|
|
||
| // Finds the symbol the accessibility plugin is imported as, via require() or | ||
| // import, regardless of path style. Returns the binding name or null. | ||
| const getAccessibilityPluginBinding = (content) => { |
There was a problem hiding this comment.
These regexes only match const/let/var X = require('…/plugin') and import X from '…/plugin'. They do not match import * as X from …, import { X } from …, or dynamic await import(…).
On the require-succeeded path this is benign (the invocation scan is lenient). But on the inconclusive path (TS config before bstack packages are installed), isAccessibilityPluginImportedAndCalledInSource is strict and returns false when the binding isn't found — so a correctly-wired config using namespace/dynamic import gets accessibility silently disabled (a billed feature).
Suggest widening the regex to cover import * as X / import { X }, or biasing the strict fallback toward keeping a11y on when the plugin path is present in source at all.
| // Strip JS/TS comments so that commented-out plugin imports/calls are ignored | ||
| // by the static scans below. Best-effort: handles block and line comments while | ||
| // avoiding `://` in URLs. | ||
| const stripComments = (src) => { |
There was a problem hiding this comment.
💡 Suggestion — stripComments can corrupt string literals.
The :// guard protects URLs, but a // inside a non-URL string (e.g. const re = "a//b") is still stripped, and the block-comment pass strips /* */ inside strings too. ^ isn't multiline-anchored (a preceding \n satisfies [^:], so line-start comments are still handled). Impact is low since the require-load marker is the primary signal — worth a comment noting the scan is intentionally lossy.
| // sending the build start event, and uses this marker to determine whether the | ||
| // accessibility plugin is actually wired in. Unlike a static text scan of the | ||
| // config file, this does NOT false-positive on commented-out requires. | ||
| process.env.BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED = 'true'; |
There was a problem hiding this comment.
💡 Suggestion — load-bearing process.env mutation as an import side-effect.
Setting BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED='true' at module top level is the intended detection mechanism, but a global mutation on require is easy to miss. Since the whole feature depends on it, consider making the invariant unmissable so anyone refactoring this module preserves it.
| try { | ||
| const accessibilityPluginLoaded = process.env.BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED === 'true'; | ||
| fs.writeFileSync( | ||
| config.accessibilityPluginFlagFileName, |
There was a problem hiding this comment.
💡 Suggestion — temp flag file can be left behind.
tmpA11yPluginLoaded.json is unlinked only inside loadJsFile after a successful read. If loadJsFile returns early (e.g. the configJsonFileName read throws before the propagation block), the file lingers, and a stale file from a crashed prior run could be read on a path that bypasses a fresh write. The child overwrites it on the normal path, so risk is low — but a guaranteed cleanup (or writing under an os-temp dir) would be safer.
3 participants
When a build requests accessibility (browserstack.json/browser caps) but the browserstackAccessibility plugin is not wired into the cypress config, the CLI now detects this before the build start event, explicitly disables accessibility so the build is not counted as an a11y build, warns the user with the setup doc link, and instruments the end-of-session EDS event (accessibility_plugin_not_loaded) so such builds can be excluded from stability queries.
Detection is code-based: the plugin sets BROWSERSTACK_ACCESSIBILITY_PLUGIN_LOADED when its module is loaded/invoked by the cypress config; requireModule writes a flag file the parent process reads back. Falls back to a raw-source scan only when the config could not be required (e.g. TS before bstack packages are installed).
Detection is also done at import and invocation of plugin.