fix(cli): drop a log written before the first mocha hook instead of logging an ERROR (SDK-7843) [v8] - #265
fix(cli): drop a log written before the first mocha hook instead of logging an ERROR (SDK-7843) [v8]#265AakashHotchandani wants to merge 3 commits into
Conversation
…ogging an ERROR (SDK-7843) Console output from wdio's `before`/`beforeSession` hooks reaches trackEvent(LOG, POST) before mocha's first hook, so no TestFramework instance exists and resolveInstance cannot create one for LOG. Since 9.30.0 that printed "resolveInstance: unable to resolve/create instance" and "trackEvent: instance not found" at ERROR level on every worker. Drop it at debug level instead, matching the classic path, which already ignores a log that has no hook or test uuid. 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 configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
AakashHotchandani
left a comment
AakashHotchandani
left a comment
There was a problem hiding this comment.
Automated SDK PR Review
Summary
Intent: Stop WdioMochaTestFramework.trackEvent from logging the resolveInstance: unable to resolve/create instance / trackEvent: instance not found ERROR pair when console output from a wdio before hook (after the service patched console) arrives as TestFrameworkState.LOG before any mocha hook has created a TestFramework instance (SDK-7843; regression from 7d6f822, first shipped in 9.30.0). When no instance is tracked for the worker, a LOG event is now dropped at debug level, matching the classic path and the CLI cucumber framework. Non-LOG events with no instance still log ERROR. Same fix as #264 on main.
Risk: Low
0 critical · 0 warnings · 2 suggestions | Files reviewed: 3
The guard is exactly as wide as the failure it silences. For LOG, resolveInstance returns null only when TestFramework.getTrackedInstance() (key ${pid}:${threadId}) is null. TestFramework.instances is never cleared, so the drop can only fire before the first mocha hook on each worker. after/afterSession, between-test and late-async logs still resolve as before, and nothing sent to TRA changes. The fix matches the existing no-instance-LOG handling in WdioCucumberTestFramework and the classic path. Same fix as #264 on main: the src/test patches and changeset are identical, and trackEvent/resolveInstance/TestFramework are identical on both lines. No proto/binary change.
See inline comments below for full detail on each finding.
Verdict
✅ Good to go — all 3 files are 🟢. The suggestions are optional polish (code-comment accuracy; test placement on v8). Same fix as #264 on main.
Rovo enrichment unavailable — review based on local docs only.
Generated by Automated SDK PR review.
| logger.info(`trackEvent: testFrameworkState=${testFrameworkState} hookState=${hookState}`) | ||
| await super.trackEvent(testFrameworkState, hookState, args) | ||
|
|
||
| // Console output from wdio's `before`/`beforeSession` hooks arrives before mocha's first |
There was a problem hiding this comment.
💡 Suggestion — [DOCS] Code comment names beforeSession, which can never reach this path
Problem
The new comment says console output from wdio's before/beforeSession hooks lands here. That is only true for before. Console is patched by patchConsoleLogs(), which the service calls inside its own before() (src/service.ts:262). A user's beforeSession always runs earlier, so its console.* output goes to the original methods and never reaches appendTestItemLog. The PR body says the same thing in its "Timing matters" note, and the changeset correctly mentions only before. A reader debugging a later attribution issue could be sent looking at beforeSession for no reason.
Suggested Fix
Name only the hook that can actually hit the guard:
// Console output from wdio's `before` hook (after the service has patched console) arrives
// before mocha's first hook, so there is no test or hook to attach it to yet ...Confidence: 🟢 Objectively verifiable: patchConsoleLogs() is only called from the service's before(), which wdio runs after beforeSession.
| @@ -0,0 +1,56 @@ | |||
| import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' | |||
There was a problem hiding this comment.
💡 Suggestion — [TESTING] New test sits outside v8's tests/cli/frameworks/ home for mocha framework tests
Problem
On v8, the existing WdioMochaTestFramework tests live in tests/cli/frameworks/wdioMochaTestFramework.test.ts, which uses ../../../src imports. This file is placed at tests/cli/ (the main layout, where wdioMochaTestFramework.*.test.ts siblings live at that level). It works either way. Moving it to tests/cli/frameworks/ on v8, or folding the 3 cases into the existing file, keeps the v8 tree consistent. Keeping it identical to #264 for cherry-pick symmetry is also a reasonable choice.
Suggested Fix
Move the file to packages/browserstack-service/tests/cli/frameworks/wdioMochaTestFramework.preTestLog.test.ts and change the imports from ../../src/... to ../../../src/.... Alternatively, append the describe block to the existing tests/cli/frameworks/wdioMochaTestFramework.test.ts.
There was a problem hiding this comment.
Not changing this one. I'm keeping the file identical to #264 (same path, same ../../src imports) so the test stays a straight cherry-pick between main and v8. Splitting the layout would make the next port of this file diverge. It runs fine where it is: 12/12 together with the existing tests/cli/frameworks/wdioMochaTestFramework.test.ts. As the suggestion itself notes, symmetry with #264 is a reasonable choice here.
… comment (SDK-7843) `beforeSession` runs before the service's own before() patches console, so its output never reaches appendTestItemLog. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
1 participant
What is this about?
Any console output from wdio's
before/beforeSessionhooks makes every worker print two ERROR lines when the BrowserStack CLI is running:A customer hit this (SDK-7843). Their
beforehook awaitsdriver.setGeoLocation()and then logs[SelfHealer] Installed ....Why it happens:
beforeruns before mocha's first hook. The service has already patchedconsoleby then, so the line goes toappendTestItemLog→trackEvent(LOG, POST).TestFrameworkinstance exists yet.resolveInstanceonly creates one forNONE,INIT_TESTand hookPRE, so it returnsnull.Customer SDK log, worker 0-0:
trackEvent: testFrameworkState=TestFrameworkState.LOG hookState=HookState.POSTERROR resolveInstance: unable to resolve/create instance ...trackWdioMochaInstance: created instance ... state=TestFrameworkState.BEFORE_ALL(the first instance)Impact is cosmetic. Only that one pre-test line can't be attributed. The classic path drops it silently as well, because the line has no hook or test uuid. Customers still read the ERROR as a broken run.
The fix: in
WdioMochaTestFramework.trackEvent, if aLOGevent arrives while no instance is tracked, log at debug and return. This matches the classic path. Everything else is unchanged:Verification
Reproduced on a real Automate session (chrome/Win11, WDIO 8 + mocha, Test Reporting on). The repro is a
beforehook that awaits one driver command and then callsconsole.log, the same shape as the customer's config.[SelfHealer]lineLogCreatedin sdk-cli.log)smosjpekx4gxcihtk0v7vwmtj6fcxpqloozfam0fstock 8.53.1djju5rlyfb0sd7n7yjagpyucu7rljbszifpx5hbmthis PRTests: new
tests/cli/wdioMochaTestFramework.preTestLog.test.tshas 3 cases:resolveInstancecall;The first case fails on the unfixed source.
Checks: the new file and the existing
tests/cli/frameworks/wdioMochaTestFramework.test.tspass (12/12).tsc -p tsconfig.prod.jsonis clean. The v8 branch has no eslint config.v8 port of #264 (same change, same test).
Related Jira task/s
SDK-7843
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)
resolveInstance: unable to resolve/create instance ... TestFrameworkState.LOG/trackEvent: instance not founderror messages. They were printed when a wdiobeforehook writes to the console. Test execution and reporting were never affected.Release notes (internal): (required — engineer-facing; what actually changed / why)
WdioMochaTestFramework.trackEventnow returns early, at debug level, forTestFrameworkState.LOGwhenTestFramework.getTrackedInstance()is empty. Console output from wdio'sbefore/beforeSessionhooks arrives before mocha's first hook, soresolveInstancecannot create an instance for it. Since 9.30.0 that printed two ERROR lines per worker. The classic path already drops such a log (no hook/test uuid), so nothing reported to TRA changes.Checklist
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.
🤖 Generated with Claude Code