Merge fix/sdk-7493-queue-skip-reports-until-drain - #197
Merged
bsautomation merged 9 commits intoSep 11, 2026
Conversation
… lost (SDK-7493) Follow-up to #178, which fixed only the case where the skip is LAST. A skip in the middle of a spec still rendered "In Progress" on 9.35.3. wdio does not await onTestSkip, so emitting a skip's events inline let them interleave with a running test's. Both resolve through ONE per-worker tracked-instance slot, so the skip's INIT_TEST repointed that slot mid-test; the live test's afterTest then restored ITS uuid onto the skip's instance (service.ts _cliTestUuids), and from there both tests' TEST/POSTs collapsed onto a single uuid. One TestRunFinished was never sent -- that test stayed "In Progress" until Test Hub's ~60-min idle reap -- and the survivor was closed with the wrong result. reportSkippedTest now only QUEUES a descriptor; drainSkipReports() emits it. That drain runs from service.after(), where no test is in flight, so the tracked slot cannot be hijacked mid-test and _cliTestUuids cannot write onto a skip's instance. Also hardens the deferral itself: pendingTestFinish becomes a uuid-keyed map so a stash can never silently evict another test's pending finish, and the flush pins the uuid captured at defer time rather than re-reading a possibly-rewritten one at send time. Neither fixes this alone; they stop a future interleave degrading into lost data. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…DK-7493 review) Addresses PR review findings. [TESTING] The map and the defer-time uuid pin had no direct regression test — only an adapted assertion and the real-build wire evidence. Adds three focused cases: two finishes stashed against the SAME instance under different uuids both survive (the anti-eviction property the map exists for); a flush sends the defer-time uuid, not the instance uuid at send time; and a same-uuid re-stash replaces rather than duplicating (the LOG_REPORT/POST re-entry). All three fail against the pre-fix single-slot module. [GRACEFUL-DEGRADATION] Documents at the queue site that delivery now depends on service.after() running: on abnormal teardown queued skips are dropped with no send attempted, where the inline path would have tried. [MAINTAINABILITY] States explicitly that the queue-and-drain is the primary fix and the map/pin are retained defence-in-depth, so neither reads as load-bearing to a future maintainer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eview) Skipped tests are now reported at drain time, so they appear grouped at the end of the build rather than in source order. Customer-visible, so it belongs in the changeset and not only the internal notes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… (SDK-7493)
Two fixes from CI and review.
1. CI regression (Build & test, all node versions): queuing EVERY skip path broke the
bail cascade. `service.test.ts` asserts the cascade reports inline from afterTest
("expected spy to be called 10 times, but got 2"). Only the un-awaited onTestSkip
path has the interleave — wdio awaits both reportSuiteSkipped callers (the failed-hook
cascade in afterHook and the bail cascade in afterTest), so nothing can claim the
tracked slot underneath them. reportSuiteSkipped now reports immediately via a new
`{ immediate: true }` option; only the reporter's detached path queues. Restores the
previous behaviour for the awaited paths, which were never broken.
2. Review finding (CodeRabbit): emitSkipReport awaited each trackEvent in sequence, so a
rejection from TEST/PRE or LOG_REPORT/POST returned before TEST/POST -- leaving the
test started-but-never-finished, i.e. the exact "In Progress" outcome this ticket is
about. Each step is now attempted independently, the first error is retained and
rethrown for the caller's debug log, and a test asserts all four events fire in order
despite an intermediate rejection.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…evel field (SDK-7493)
The review finding was right and my earlier rebuttal was wrong.
The binary routes a mocha test_run on the uuid parsed OUT of event_json, not on
TestFrameworkEventRequest.uuid:
packages/@browserstack/observability/languages/webdriverio/index.js:83
const event = JSON.parse(eventJson)
.../webdriverio/index.js:111
testFrameworkModule.onTestRunFinished(context, event, request)
.../webdriverio/test-frameworks/mocha/index.js:159
uuid: event.test_uuid
So pinning only the top-level field left the field that actually decides closure still
read from the instance's live data at send time. A stale test_uuid there closes the wrong
run and leaves the deferred one open — the exact failure the pin exists to prevent.
sendTestFrameworkEvent now applies the pinned uuid to a copy of the serialized data as
well, leaving the instance untouched. Test asserts both the top-level field and
event_json.test_uuid carry the defer-time uuid after the instance uuid is rewritten.
My earlier evidence did not discriminate: in that build the top-level uuid and
event_json.test_uuid were both read from the same instance at send time, so they agreed
and the experiment could not tell which one routed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bsautomation
requested review from
shivam5643 and
yashdsaraf
and removed request for
a team
September 11, 2026 13:07
|
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: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge into sdk_v9_pre_prod :by automationbs