Revert "fix(browserstack-service): omit unset accessibility config"#15252
Revert "fix(browserstack-service): omit unset accessibility config"#15252rahulpsq wants to merge 1 commit into
Conversation
…ebdriverio#15215)" This reverts commit b4ba956.
|
Greptile SummaryThis PR reverts #15215 ("fix(browserstack-service): omit unset accessibility config") but instead of a clean revert it introduces a new
Confidence Score: 3/5The revert changes how an unset accessibility flag is represented in the BrowserStack API payload, replacing an omitted key with an explicit null value, which alters the contract with the remote service. Three distinct defects exist in the changed files: the API build-start payload now sends accessibility null instead of omitting the key; getProductMap returns a runtime null through a type-only cast when callers expect a boolean; and DEFAULT_OPTIONS adds accessibility false which undermines the null sentinel relied on by the launcher. packages/wdio-browserstack-service/src/testHub/utils.ts requires the most attention — both the return type of getProductMapForBuildStartCall and the as boolean cast in getProductMap need to be addressed to avoid incorrect API payloads.
|
| Filename | Overview |
|---|---|
| packages/wdio-browserstack-service/src/constants.ts | Adds accessibility: false to DEFAULT_OPTIONS, which is semantically inconsistent with the new null-based sentinel introduced elsewhere in this revert. |
| packages/wdio-browserstack-service/src/config.ts | Changes accessibility field from `boolean |
| packages/wdio-browserstack-service/src/launcher.ts | Replaces undefined sentinel with null for _accessibilityAutomation. The null check replacing isUndefined is logically correct, but a null as boolean cast on line 438 silently propagates null into browserStackConfig.accessibility. |
| packages/wdio-browserstack-service/src/testHub/utils.ts | Two issues: getProductMap claims boolean return type but returns null at runtime via a type-only cast; getProductMapForBuildStartCall now serializes null to JSON instead of omitting the key, changing API payload semantics. |
Reviews (1): Last reviewed commit: "Revert "fix(browserstack-service): omit ..." | Re-trigger Greptile
| return { | ||
| observability: config.testObservability.enabled, | ||
| accessibility: config.accessibility, | ||
| accessibility: config.accessibility as boolean, |
There was a problem hiding this comment.
The
as boolean cast on config.accessibility tells TypeScript the value is a boolean, but when accessibility is not configured the runtime value is null. This silently puts null into the buildProductMap capability sent to BrowserStack rather than false, which may cause unexpected server-side behavior. The cast should be replaced with an explicit coercion.
| accessibility: config.accessibility as boolean, | |
| accessibility: config.accessibility ?? false, |
| export const getProductMapForBuildStartCall = (config: BrowserStackConfig, accessibilityAutomation: boolean | null): { [key: string]: boolean | null } => { | ||
| return { | ||
| observability: config.testObservability.enabled, | ||
| accessibility: accessibilityAutomation, |
There was a problem hiding this comment.
When
accessibilityAutomation is null (the new "not configured" sentinel), JSON.stringify serializes the value as "accessibility": null in the product_map payload. Before PR #15215 the field was undefined, which JSON.stringify omits entirely. Sending an explicit null is a behavioural change for the BrowserStack build-start API that this revert is supposed to undo. The return type should remain boolean | undefined with null converted or omitted before the object is built.
| export const getProductMapForBuildStartCall = (config: BrowserStackConfig, accessibilityAutomation: boolean | null): { [key: string]: boolean | null } => { | |
| return { | |
| observability: config.testObservability.enabled, | |
| accessibility: accessibilityAutomation, | |
| export const getProductMapForBuildStartCall = (config: BrowserStackConfig, accessibilityAutomation: boolean | null): { [key: string]: boolean | undefined } => { | |
| return { | |
| observability: config.testObservability.enabled, | |
| accessibility: accessibilityAutomation ?? undefined, |
| export const DEFAULT_OPTIONS: Partial<BrowserstackConfig> = { | ||
| setSessionName: true, | ||
| setSessionStatus: true, | ||
| testObservability: true | ||
| testObservability: true, | ||
| accessibility: false | ||
| } |
There was a problem hiding this comment.
DEFAULT_OPTIONS now always supplies accessibility: false. The BrowserStackConfig constructor and launcher.ts now use null to mean "user did not configure accessibility". Any code path that initialises BrowserStackConfig or the launcher options from DEFAULT_OPTIONS-merged options will see false instead of null, silently bypassing the null-based sentinel check at launcher.ts line 428 (=== null).
| export const DEFAULT_OPTIONS: Partial<BrowserstackConfig> = { | |
| setSessionName: true, | |
| setSessionStatus: true, | |
| testObservability: true | |
| testObservability: true, | |
| accessibility: false | |
| } | |
| export const DEFAULT_OPTIONS: Partial<BrowserstackConfig> = { | |
| setSessionName: true, | |
| setSessionStatus: true, | |
| testObservability: true | |
| } |
Reverts #15215