Fix for "skipped" tests getting marked as "failed"#6
Conversation
amaanbs
left a comment
amaanbs
left a comment
There was a problem hiding this comment.
Automated SDK PR Review
Verdict: src/cli/frameworks/ for the same !passed → 'failed' bug — if any exist without a skipped guard, they need the same fix applied before merge.
Summary: 0 critical · 1 warning · 2 suggestions across 3 files reviewed.
The fix is directionally correct and the Automate hub contract is preserved (hub only accepts passed/failed; collapsing skipped → 'passed' is correct). The one action item before merge: verify no sibling WDIO framework adapter in cli/frameworks/ has the same !passed → 'failed' bug that was just fixed in wdioMochaTestFramework.ts.
See inline comments below for full Problem and Suggested Fix detail on each finding.
Generated by Automated SDK PR review.
| let failure: Array<unknown>|null = null | ||
| let failureReason: string|null = null | ||
| let failureType: string|null = null | ||
| if (!passed) { |
There was a problem hiding this comment.
⚠️ Warning — [CROSS-FRAMEWORK IMPACT] Mocha adapter fixed — sibling WDIO adapters not audited
Problem
wdioMochaTestFramework.ts now correctly maps skipped → result: 'skipped' for the Observability path. But src/cli/frameworks/ likely contains adapters for other WDIO frameworks (e.g., wdioJasmineTestFramework.ts). If those adapters still use the old !passed → 'failed' logic without a skipped guard, the Observability bug persists for non-Mocha WDIO users.
This is a classic parallel-path issue: fixing one adapter does not propagate to its siblings.
Suggested Fix
Grep all sibling adapters for the same pattern and apply the same fix:
grep -n "passed" packages/wdio-browserstack-service/src/cli/frameworks/*.tsFor any adapter that has if (!passed) → result/status = 'failed' without checking skipped, apply the same fix:
if (skipped) {
result = 'skipped'
} else {
result = (!passed && error && ...) ? 'ignore' : 'failed'
}Confidence: 🟢 Grounded in SDK anti-pattern: fixing one code path without auditing parallel paths in the same module.
| } | ||
|
|
||
| const status = passed ? 'passed' : 'failed' | ||
| const status = passed || skipped ? 'passed' : 'failed' |
There was a problem hiding this comment.
💡 Suggestion — [CODE CLARITY] Automate → 'passed', Observability → 'skipped' divergence is undocumented
Suggestion
The Automate path maps skipped → 'passed' (because the hub only accepts 'passed'/'failed'). The Observability path in wdioMochaTestFramework.ts maps skipped → 'skipped'. Both are correct for their respective APIs, but the divergence looks like a bug to anyone reading this code without context.
Add an inline comment explaining the constraint:
// Automate hub only accepts 'passed'/'failed'; skipped tests are collapsed to 'passed'
const status = passed || skipped ? 'passed' : 'failed'This prevents future engineers from treating the Automate path as incorrect relative to the Observability path.
| const { error, passed } = results | ||
| if (!passed) { | ||
| const { error, passed, skipped } = results | ||
| if (!passed && !skipped) { |
There was a problem hiding this comment.
💡 Suggestion — [FEATURE CORRECTNESS] Verify full afterTest body — diff is truncated
Suggestion
The diff for service.ts shows only 3 changed lines: the skipped destructuring and the _failReasons.push guard. The rest of the afterTest method is not shown.
If there are additional state assignments downstream in the method — for example, a status variable or a session-state write derived from _failReasons.length — those paths might still reflect incorrect state for skipped tests without the skipped guard.
Scan the full afterTest method body to confirm:
- No other code path derives "this test failed" from
_failReasonswithout re-checkingskipped. - The
skippedflag is not needed for any session teardown or cleanup logic below the changed block.
3 participants
Proposed changes
Types of changes
Checklist
Backport Request
v9and doesn't need to be back-ported#XXXXXFurther comments
Reviewers: @webdriverio/project-committers