fix(service): report screenshot logs on the binary flow (SDK-4177)

.changeset/pr-190.md

@@ -0,0 +1,5 @@
+---
+"@wdio/browserstack-service": patch
+---
+
+- Fixed screenshots taken during a Mocha test not appearing in Test Reporting's consolidated logs.

packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts

@@ -266,12 +266,14 @@ export default class WdioMochaTestFramework extends TestFramework {
      */
     loadLogEntries(instance: TestFrameworkInstance, testFrameworkState: State, hookState: State, logEntry: Record<string, unknown>) {
         const logRecord: Record<string, unknown> = {}
-        const { level, message, timestamp } = logEntry
+        const { level, message, timestamp, kind } = logEntry
 
         if (CLIUtils.matchHookRegex(instance.getCurrentTestState().toString().split('.')[1])) {
             logRecord[TestFrameworkConstants.KEY_HOOK_ID] = TestFramework.getState(instance, TestFrameworkConstants.KEY_HOOK_ID)
         }
-        logRecord.kind = TestFrameworkConstants.KIND_LOG
+        // Console logs carry no kind and stay KIND_LOG; a producer that sets one (a screenshot,
+        // say) keeps it, or the entry would reach Observability labelled as a console log.
+        logRecord.kind = kind ?? TestFrameworkConstants.KIND_LOG
         logRecord.message = Buffer.from(message as string)
         logRecord.level = level
         logRecord.timestamp = timestamp

packages/browserstack-service/src/insights-handler.ts

@@ -27,7 +27,8 @@ import {
     removeAnsiColors,
     getObservabilityProduct,
     generateHashCodeFromFields,
-    isTrue
+    isTrue,
+    isFalse
 } from './util.js'
 import type {
     TestData,
@@ -45,6 +46,7 @@ import { TESTOPS_SCREENSHOT_ENV } from './constants.js'
 import { BrowserstackCLI } from './cli/index.js'
 import { TestFrameworkState } from './cli/states/testFrameworkState.js'
 import { HookState } from './cli/states/hookState.js'
+import { TestFrameworkConstants } from './cli/frameworks/constants/testFrameworkConstants.js'
 import PerformanceTester from './instrumentation/performance/performance-tester.js'
 import * as PERFORMANCE_SDK_EVENTS from './instrumentation/performance/constants.js'
 import CustomTagsHandler from './custom-tags-handler.js'
@@ -790,13 +792,34 @@ class _InsightsHandler {
         // log screenshot
         const body = 'body' in args ? args.body : undefined
         const result = 'result' in args ? args.result as { value: string } : undefined
-        if (Boolean(process.env[TESTOPS_SCREENSHOT_ENV]) && isScreenshotCommand(args) && result?.value) {
-            await this.listener.onScreenshot([{
-                test_run_uuid: testMeta.uuid,
-                timestamp: new Date().toISOString(),
-                message: result.value,
-                kind: 'TEST_SCREENSHOT'
-            }])
+        // `allow_screenshots` is an optional *string* on the wire, so a denial arrives as the
+        // string 'false' — which Boolean() reads as permission granted. Honour it explicitly.
+        const allowScreenshots = process.env[TESTOPS_SCREENSHOT_ENV]
+        if (Boolean(allowScreenshots) && !isFalse(allowScreenshots) && isScreenshotCommand(args) && result?.value) {
+            // On the binary path the direct screenshot endpoint answers 401 to the binary's JWT,
+            // so ride the same LOG rail appendTestItemLog uses: the CLI stamps the test uuid and
+            // forwards the entry over gRPC, where the binary owns reporting. The framework can be
+            // unset while isRunning() is true — the dev-env short-circuit returns true before
+            // setupTestFramework() has run, and it only assigns for webdriverio-mocha — so resolve
+            // it rather than assert, and let an untracked framework fall through to the direct
+            // upload instead of throwing the screenshot away.
+            const cliTestFramework = BrowserstackCLI.getInstance().isRunning()
+                ? BrowserstackCLI.getInstance().getTestFramework()
+                : undefined
+            await (cliTestFramework
+                ? cliTestFramework.trackEvent(TestFrameworkState.LOG, HookState.POST, {
+                    logEntry: {
+                        kind: TestFrameworkConstants.KIND_SCREENSHOT,
+                        message: result.value,
+                        timestamp: new Date().toISOString()
+                    }
+                })
+                : this.listener.onScreenshot([{
+                    test_run_uuid: testMeta.uuid,
+                    timestamp: new Date().toISOString(),
+                    message: result.value,
+                    kind: 'TEST_SCREENSHOT'
+                }]))
         }
 
         const requestData = this._commands[dataKey]

packages/browserstack-service/src/service.ts

@@ -314,6 +314,25 @@ export default class BrowserstackService implements Services.ServiceInstance {
                         BStackLogger.info(`CLI is running, tracking insights event for before: ${sessionId}`)
                         await BrowserstackCLI.getInstance().getAutomationFramework()!.trackEvent(AutomationFrameworkState.CREATE, HookState.POST, { browser: this._browser, hubUrl: this._config.hostname })
                         this._insightsHandler.setGitConfigPath()
+                        /**
+                         * `browserCommand` is the only producer of TEST_SCREENSHOT logs — the binary
+                         * has no screenshot producer of its own — so the result event has to be
+                         * registered on this path too, or a screenshot taken mid-test never reaches
+                         * Observability (SDK-4177). The `command` (beforeCommand) event is
+                         * deliberately NOT registered: it only fills the map that browserCommand's
+                         * HTTP-log half reads, and that half emits on the JS listener pipeline the
+                         * binary owns here. Leaving it unregistered keeps the screenshot upload —
+                         * which rides its own JWT-authenticated endpoint — as the single effect.
+                         */
+                        this._browser.on('result', (result) => {
+                            if (shouldProcessEventForTesthub('')) {
+                                this._insightsHandler?.browserCommand(
+                                    'client:afterCommand',
+                                    Object.assign(result, { sessionId }),
+                                    this._currentTest
+                                )
+                            }
+                        })
                         PerformanceTester.end(PERFORMANCE_SDK_EVENTS.DRIVER_EVENT.PRE_INITIALIZE)
                         return
                     }

packages/browserstack-service/tests/cli/wdioMochaTestFramework.logKind.test.ts

@@ -0,0 +1,69 @@
+import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'
+import * as bstackLogger from '../../src/bstackLogger.js'
+
+import WdioMochaTestFramework from '../../src/cli/frameworks/wdioMochaTestFramework.js'
+import TestFramework from '../../src/cli/frameworks/testFramework.js'
+import { TestFrameworkConstants } from '../../src/cli/frameworks/constants/testFrameworkConstants.js'
+import { TestFrameworkState } from '../../src/cli/states/testFrameworkState.js'
+import { HookState } from '../../src/cli/states/hookState.js'
+
+vi.spyOn(bstackLogger.BStackLogger, 'logToFile').mockImplementation(() => {})
+
+describe('SDK-4177 — loadLogEntries must not relabel a log that carries its own kind', () => {
+    let entries: unknown[]
+    let instance: any
+
+    beforeEach(() => {
+        entries = []
+        instance = {
+            getCurrentTestState: () => TestFrameworkState.TEST,
+            updateMultipleEntries: vi.fn(),
+        }
+        vi.spyOn(TestFramework, 'getState').mockReturnValue(entries)
+        vi.spyOn(WdioMochaTestFramework, 'lastActiveHook').mockReturnValue(null)
+    })
+
+    afterEach(() => {
+        vi.restoreAllMocks()
+    })
+
+    const load = (logEntry: Record<string, unknown>) => {
+        WdioMochaTestFramework.prototype.loadLogEntries.call(
+            WdioMochaTestFramework.prototype,
+            instance,
+            TestFrameworkState.LOG,
+            HookState.POST,
+            logEntry
+        )
+        return entries[0] as Record<string, unknown>
+    }
+
+    it('keeps TEST_SCREENSHOT, which the kind was previously hardcoded over', () => {
+        // Hardcoding KIND_LOG here meant a screenshot reached Observability labelled as a
+        // console log, so no screenshots manifest was ever built for the test run.
+        const record = load({
+            kind: TestFrameworkConstants.KIND_SCREENSHOT,
+            message: 'aBase64Screenshot',
+            timestamp: '2020-01-01T00:00:00.000Z',
+        })
+
+        expect(record.kind).toBe('TEST_SCREENSHOT')
+        expect(record.message).toEqual(Buffer.from('aBase64Screenshot'))
+    })
+
+    it('still labels a console log TEST_LOG', () => {
+        const record = load({
+            kind: TestFrameworkConstants.KIND_LOG,
+            message: 'hello',
+            level: 'info',
+            timestamp: '2020-01-01T00:00:00.000Z',
+        })
+
+        expect(record.kind).toBe('TEST_LOG')
+        expect(record.level).toBe('info')
+    })
+
+    it('falls back to TEST_LOG for an entry with no kind', () => {
+        expect(load({ message: 'hello', timestamp: '2020-01-01T00:00:00.000Z' }).kind).toBe('TEST_LOG')
+    })
+})

packages/browserstack-service/tests/insights-handler.test.ts

@@ -9,6 +9,7 @@ import InsightsHandler from '../src/insights-handler.js'
 import * as utils from '../src/util.js'
 import * as bstackLogger from '../src/bstackLogger.js'
 import { TESTOPS_SCREENSHOT_ENV } from '../src/constants.js'
+import { BrowserstackCLI } from '../src/cli/index.js'
 
 const log = logger('test')
 let insightsHandler: InsightsHandler
@@ -697,6 +698,33 @@ describe('browserCommand', () => {
         delete process.env[TESTOPS_SCREENSHOT_ENV]
     })
 
+    it('client:afterCommand - falls back to the direct upload when the CLI has no test framework', async () => {
+        // isRunning() can be true before setupTestFramework() has assigned one (dev-env
+        // short-circuit, or a framework the CLI doesn't track). Asserting non-null there threw the
+        // screenshot away; resolving it keeps the direct upload as the fallback.
+        process.env[TESTOPS_SCREENSHOT_ENV] = 'true'
+        commandSpy.mockImplementation(() => { return true })
+        const getInstanceSpy = vi.spyOn(BrowserstackCLI, 'getInstance').mockReturnValue({
+            isRunning: () => true,
+            getTestFramework: () => undefined
+        } as any)
+
+        await insightsHandler.browserCommand('client:afterCommand', { sessionId: 's', method: 'm', endpoint: 'e', result: { value: 'random' } } as any, {} as any)
+
+        expect(uploadEventDataSpy).toBeCalled()
+        getInstanceSpy.mockRestore()
+        delete process.env[TESTOPS_SCREENSHOT_ENV]
+    })
+
+    it('client:afterCommand - screenshot not uploaded when screenshots are denied', () => {
+        // arrives as the string 'false' off the wire, which Boolean() would read as granted
+        process.env[TESTOPS_SCREENSHOT_ENV] = 'false'
+        commandSpy.mockImplementation(() => { return true })
+        insightsHandler.browserCommand('client:afterCommand', { sessionId: 's', method: 'm', endpoint: 'e', result: { value: 'random' } } as any, {} as any)
+        expect(uploadEventDataSpy).toBeCalledTimes(0)
+        delete process.env[TESTOPS_SCREENSHOT_ENV]
+    })
+
     it('return if test not in _tests', () => {
         insightsHandler.browserCommand('client:afterCommand', { sessionId: 's', method: 'm', endpoint: 'e', result: { value: 'random' } } as any, {} as any)
         insightsHandler['_tests'] = { 'test title not there': { 'uuid': 'uuid' } }

packages/browserstack-service/tests/service.test.ts

@@ -580,6 +580,34 @@ describe('before', () => {
         expect(service['_sessionBaseUrl']).toEqual(sessionBaseUrl)
     })
 
+    it('registers the result event on the CLI path, and only that event', async () => {
+        // browserCommand is the only producer of TEST_SCREENSHOT logs, so the binary flow has to
+        // register the result event too (SDK-4177). `command` must stay unregistered — it only
+        // feeds browserCommand's HTTP-log half, which the binary owns on this path.
+        process.env.BROWSERSTACK_OBSERVABILITY = 'true'
+        const getInstanceSpy = vi.spyOn(BrowserstackCLI, 'getInstance').mockReturnValue({
+            isRunning: () => true,
+            getTestFramework: () => null,
+            getAutomationFramework: () => ({
+                trackEvent: vi.fn().mockResolvedValue(undefined)
+            })
+        } as any)
+        const service = new BrowserstackService({} as any, [{}] as any, {
+            user: 'foo',
+            key: 'bar',
+            capabilities: {}
+        })
+
+        await service.before(service['_config'] as any, [], browser)
+
+        const events = vi.mocked(browser.on).mock.calls.map(([event]) => event)
+        expect(events).toContain('result')
+        expect(events).not.toContain('command')
+
+        getInstanceSpy.mockRestore()
+        delete process.env.BROWSERSTACK_OBSERVABILITY
+    })
+
     it('should initialize correctly for multiremote', () => {
         const service = new BrowserstackService(
             {} as any,