support of setting metadata - #1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces support for attaching additional “central user”/LCNC metadata to TestHub events and extends build-start payloads with a grouping identifier, alongside some CLI/service wiring changes.
Changes:
- Add a new
BrowserStackSDK.setTestMetadata()entrypoint and an internalTestMetadatastore for per-test/fallback metadata. - Add central-user flags into
product_mapand support agrouping_identifierfield for build start. - Make GRR URL updating more defensive (validate presence, return boolean, warn on missing).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/wdio-browserstack-service/src/util.ts | Adds central-user helper and includes grouping identifier in build-start payload. |
| packages/wdio-browserstack-service/src/types.ts | Extends TestData with optional app_lcnc metadata payload. |
| packages/wdio-browserstack-service/src/testHub/utils.ts | Adds central-user flags into product_map generation. |
| packages/wdio-browserstack-service/src/metadata.ts | New metadata store keyed by test UUID (and fallback). |
| packages/wdio-browserstack-service/src/index.ts | Exports BrowserStackSDK from the package entrypoint. |
| packages/wdio-browserstack-service/src/constants.ts | Adds env var constants for central user mode and grouping identifier. |
| packages/wdio-browserstack-service/src/cli/modules/testHubModule.ts | Associates current test UUID and injects metadata into outgoing test framework events. |
| packages/wdio-browserstack-service/src/cli/index.ts | Makes GRR URL update conditional and adds CLI-session env cleanup. |
| packages/wdio-browserstack-service/src/cli/apiUtils.ts | Adds GRR URL validation/return status, but also changes default endpoints. |
| packages/wdio-browserstack-service/src/browserStackSdk.ts | New public SDK surface for setting metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onBeforeTest(args: Record<string, unknown>) { | ||
| this.logger.debug('onBeforeTest: Called after test hook from cli configured module!!!') | ||
| const instance = args.instance as TestFrameworkInstance | ||
| const testUuid = TestFramework.getState(instance, TestFrameworkConstants.KEY_TEST_UUID) as string | undefined |
There was a problem hiding this comment.
onBeforeTest sets the current UUID using only KEY_TEST_UUID, but later sendTestFrameworkEvent falls back to instance.getRef() when KEY_TEST_UUID is missing. If KEY_TEST_UUID isn’t populated yet at PRE time, metadata set via BrowserStackSDK.setTestMetadata() will go to the fallback bucket and may be attached to the wrong test. Consider computing the UUID the same way in both places (state value or instance.getRef()) before calling TestMetadata.setCurrentTestRunUuid.
| onBeforeTest(args: Record<string, unknown>) { | |
| this.logger.debug('onBeforeTest: Called after test hook from cli configured module!!!') | |
| const instance = args.instance as TestFrameworkInstance | |
| const testUuid = TestFramework.getState(instance, TestFrameworkConstants.KEY_TEST_UUID) as string | undefined | |
| private getCurrentTestRunUuid(instance: TestFrameworkInstance): string | undefined { | |
| return (TestFramework.getState(instance, TestFrameworkConstants.KEY_TEST_UUID) as string | undefined) ?? instance.getRef() | |
| } | |
| onBeforeTest(args: Record<string, unknown>) { | |
| this.logger.debug('onBeforeTest: Called after test hook from cli configured module!!!') | |
| const instance = args.instance as TestFrameworkInstance | |
| const testUuid = this.getCurrentTestRunUuid(instance) |
|
Please resolve Copilot comments. |
xxshubhamxx
left a comment
xxshubhamxx
left a comment
There was a problem hiding this comment.
Reviewed as part of the App LCA ↔ SDK integration (LCAM-1360).
The metadata wiring design is solid — the uuid-keyed + fallback approach for TestMetadata is well thought out, and the defensive fixes (finally block cleanup, this.config = {} guard, hasValidGRRUrls type guard) are all good. Two issues need to be fixed before this is safe for production:
- 🔴 All API URLs are hardcoded to staging (
bsstag.com) — must be reverted before any production publish. - 🔴
TestMetadata.reset()is defined but never called —fallbackMetadatawill contaminate subsequent test events in the same process.
Inline comments below on both critical issues and a few smaller things.
| static DATA_ENDPOINT = 'https://collector-observability.browserstack.com' | ||
| static UPLOAD_LOGS_ADDRESS = 'https://upload-observability.browserstack.com' | ||
| static EDS_URL = 'https://eds.browserstack.com' | ||
| static FUNNEL_INSTRUMENTATION_URL = 'https://api.devapplca.bsstag.com/sdk/v1/event' |
There was a problem hiding this comment.
🔴 Critical — Staging URLs hardcoded as production defaults
All 10 static URL properties are now pointing to devapplca.bsstag.com / bsstag.com. This file ships inside the published wdio-browserstack-service npm package. If merged and published as-is, every WDIO user globally — not just LCA users — will have their Automate sessions, Observability data, Percy scans, and accessibility results routed to the LCA staging environment instead of production.
Before merging toward any production-facing branch, restore all URLs to their original browserstack.com values:
static FUNNEL_INSTRUMENTATION_URL = 'https://api.browserstack.com/sdk/v1/event'
static BROWSERSTACK_AUTOMATE_API_URL = 'https://api.browserstack.com'
// ... (all 10 properties)If staging configurability is needed long-term, the right approach is to keep browserstack.com as the default and override via updateURLSForGRR() at runtime when a staging env var is detected — not hardcode staging as the class default.
There was a problem hiding this comment.
❌ Still outstanding in 10fe160e — all 10 URLs in apiUtils.ts are still pointing to devapplca.bsstag.com / bsstag.com. Copilot also flagged this same issue back in April (#discussion_r3136764048).
Just to clarify the deploy boundary: this fork (ashish0305/webdriveriolc) is fine for staging verification of LCAM-1360, but the URL changes must not flow into the upstream webdriverio/webdriverio (or wherever @wdio/browserstack-service is published from). Could you confirm one of the following?
- This branch will never be merged toward an npm-published source (i.e., it's a throwaway staging fork) — in which case ignore.
- The URLs will be reverted before this is back-ported / re-PR'd against the production source.
- The staging URLs are guarded by an env var and the production defaults will be restored before merge.
| // _tests[fullTitle].uuid vs testHubModule's KEY_TEST_UUID) still resolve. | ||
| TestMetadata.fallbackMetadata = metadata | ||
|
|
||
| if (TestMetadata.currentTestRunUuid) { |
There was a problem hiding this comment.
🔴 Critical — fallbackMetadata is written here but never cleared between tests
fallbackMetadata is always updated to the most recently set metadata. Since reset() is never called in any cleanup path (not in onAllTestEvents, not in clearCliSessionEnv, not in stop()), this means:
- Test A calls
BrowserStackSDK.setTestMetadata({identifier: 'stepA', ...}) - Test B runs without calling
setTestMetadata() TestMetadata.get(testBUuid)returns... Test A's data viafallbackMetadata- Test B's events are now tagged with Test A's LCA metadata
Fix: call TestMetadata.reset() after each test completes. The cleanest hook is onAllTestEvents in testHubModule.ts for TEST/POST:
if (testFrameworkState === TestFrameworkState.TEST && testHookState === HookState.POST) {
TestMetadata.reset()
}There was a problem hiding this comment.
✅ Addressed in 10fe160e — TestMetadata.reset() now called on TEST/POST in testHubModule.ts:106-108, exactly as suggested. Looks good.
| TestFramework.registerObserver(state, hook, this.onAllTestEvents.bind(this)) | ||
| }) | ||
| }) | ||
| TestFramework.registerObserver(TestFrameworkState.TEST, HookState.PRE, this.onBeforeTest.bind(this)) |
There was a problem hiding this comment.
TEST/PRE
Before this PR, onBeforeTest was registered first, so for TEST/PRE events the execution order was:
onBeforeTest→setCurrentTestRunUuid(testUuid)onAllTestEvents→sendTestFrameworkEvent()
After this PR the order is reversed — onAllTestEvents (and thus sendTestFrameworkEvent) fires before onBeforeTest for TEST/PRE.
Currently harmless because app_lcnc metadata isn't set at test-start time. But setCurrentTestRunUuid — which gates where user's setTestMetadata() calls get stored — now happens after the TEST/PRE event is already sent. If this ordering was intentional (e.g., to fix another race), please add a comment explaining why. If not, restoring the original order is safer.
There was a problem hiding this comment.
❌ Still outstanding in 10fe160e — the constructor order is unchanged (testHubModule.ts:38-43): onAllTestEvents is registered for TEST/PRE before onBeforeTest, so the TEST/PRE event still dispatches sendTestFrameworkEvent before setCurrentTestRunUuid.
Currently a no-op for app_lcnc payloads (since metadata isn't set at TEST/PRE), but if this was an intentional change to fix some other ordering issue, a one-line code comment would prevent someone restoring the original order during a future cleanup.
| return | ||
| } | ||
|
|
||
| const testRunIdentifier = metadata.identifier |
There was a problem hiding this comment.
💡 Suggestion — Document the identifier constraint for callers
The 40-character limit on identifier is opaque — the public BrowserStackSDK.setTestMetadata() API gives users no indication this field is required or constrained. When the check fails, the warning 'The metadata object is not valid.' gives no signal about which field or what the limit is.
Consider:
if (typeof testRunIdentifier !== 'string') {
BStackLogger.warn('setTestMetadata: metadata.identifier must be a string.')
return
}
if (testRunIdentifier.length > 40) {
BStackLogger.warn(`setTestMetadata: identifier "${testRunIdentifier}" exceeds the 40-character limit.`)
return
}And add a JSDoc comment to setTestMetadata() in browserStackSdk.ts documenting the requirement.
There was a problem hiding this comment.
✅ Addressed in 10fe160e — validation now split into two specific checks with field/value-aware messages, and browserStackSdk.ts has the JSDoc documenting the identifier requirement and 40-char limit. 👍
| testData.hook_type = testData.name?.toLowerCase() ? getHookType(testData.name.toLowerCase()) : 'undefined' | ||
| } | ||
|
|
||
| if (eventType === 'TestRunSkipped') { |
There was a problem hiding this comment.
💡 Suggestion — Merge the two TestRunSkipped checks
There are two separate if (eventType === 'TestRunSkipped') blocks — one at line ~290 (sets result = 'skipped') and this one (attaches app_lcnc metadata). These can be merged into one for clarity:
if (eventType === 'TestRunSkipped') {
testData.result = 'skipped'
const appLcncMetaData = TestMetadata.get(testData.uuid)
if (Object.keys(appLcncMetaData).length > 0) {
testData.app_lcnc = appLcncMetaData
}
}There was a problem hiding this comment.
✅ Addressed in 10fe160e — both blocks merged into one. Looks good.
| settings: options.accessibilityOptions | ||
| }, | ||
| browserstackAutomation: shouldAddServiceVersion(config, options.testObservability), | ||
| grouping_identifier: process.env[BROWSERSTACK_BUILD_GROUPING_IDENTIFIER], |
There was a problem hiding this comment.
💡 Suggestion — grouping_identifier default differs from the binary's mocha event payload
Here:
grouping_identifier: process.env[BROWSERSTACK_BUILD_GROUPING_IDENTIFIER], // undefined when unsetBut in the binary side (mocha test event payload, in browserstack-binary PR webdriverio#1123):
grouping_identifier: process.env.BROWSERSTACK_BUILD_GROUPING_IDENTIFIER || '', // '' when unsetSo for the same env-unset case:
- Build-start payload (this file): field is omitted from the wire payload (JSON.stringify drops undefined).
- Per-test event payload (binary mocha module): field is emitted as
"".
If the backend treats missing-field and empty-string equivalently, this is just inconsistency. If they're treated differently (e.g., empty == "explicitly ungrouped", missing == "unknown"), this could lead to subtle classification mismatches between the launch event and the test events for the same build. Suggest aligning on one convention across both:
grouping_identifier: process.env[BROWSERSTACK_BUILD_GROUPING_IDENTIFIER] || '',There was a problem hiding this comment.
✅ Addressed in 10fe160e — now process.env[BROWSERSTACK_BUILD_GROUPING_IDENTIFIER] || '', aligned with the binary's mocha event payload. 👍
xxshubhamxx
left a comment
xxshubhamxx
left a comment
There was a problem hiding this comment.
Re-review of 10fe160e
Thanks for the quick turnaround. Several of my comments are cleanly addressed in this commit:
✅ Resolved
TestMetadata.reset()now wired intoTEST/POST— exactly the fixidentifiervalidation split with field-specific messages + JSDoc on the public API- Two
TestRunSkippedblocks merged inreporter.ts grouping_identifierinutil.tsnow uses|| ''to align with the binary mocha payload
❌ Still outstanding
- 🔴 Critical — staging URLs in
apiUtils.tsare still pointing todevapplca.bsstag.com. Copilot also flagged this back in April. Needs explicit confirmation that this branch is staging-only and the URLs will be reverted before any merge toward an npm-published source. ⚠️ Warning — observer registration order intestHubModule.tsconstructor (onAllTestEventsbeforeonBeforeTestforTEST/PRE) is unchanged. Currently a no-op for app_lcnc, but worth either restoring the original order or adding a one-line comment if intentional.
Other still-open items worth addressing (mostly raised by Copilot earlier, ack'd "removed" but worth confirming):
getCentralUser()always spreadsapp_lcnc: falseinto product maps for non-LCA users (#discussion_r3136764119, r3136764151, r3136764172). Consider returning{}when the env var isn'tapp_lcncto avoid changing payload shape for everyone.- No unit tests for
BrowserStackSDK.setTestMetadata()/ per-test vs fallback metadata behavior (#discussion_r3136764316). central_scannerfield declared in theCentralUsertype but never populated — dead code (#discussion_r3136764119).
Keeping REQUEST_CHANGES until at least the staging-URL question is resolved.
Current review status —
|
xxshubhamxx
left a comment
xxshubhamxx
left a comment
There was a problem hiding this comment.
✅ All review items resolved — 0b122fd8
Every item from the original review and re-review is addressed:
| # | Item | Resolution |
|---|---|---|
| 1 | 🔴 Staging URLs in apiUtils.ts |
All 10 endpoints reverted to production (browserstack.com). Added hasValidGRRUrls() guard before updateURLSForGRR — good defensive addition. |
| 2 | Documented with inline comment explaining the intentional sequence — acceptable resolution. | |
| 3 | TestMetadata.reset() on POST/TEST |
Fixed in prior commit 10fe160e. |
| 4 | 💡 Identifier validation split | Fixed in prior commit 10fe160e. |
| 5 | 💡 Duplicate TestRunSkipped blocks |
Merged in prior commit 10fe160e. |
| 6 | 💡 JSDoc on setTestMetadata |
Added in prior commit 10fe160e. |
Staging URL support correctly moved to the Binary layer via BROWSERSTACK_ENV=app_lcnc_stag — this is the right pattern (Binary owns env switching, SDK stays prod-clean).
No further blockers from my side. 👍
xxshubhamxx
left a comment
xxshubhamxx
left a comment
There was a problem hiding this comment.
All review items addressed. LGTM 👍
Follow-up to the central-user port; keeps src byte-faithful to ashish0305/webdriveriolc#1 and confines the fixes to tests. - getProductMap / funnel instrumentation now emit app_lcnc: false for every user (getCentralUser defaults to { app_lcnc: false }), so add the key to the expected product maps in testHub/utils and funnelInstrumentation specs. - onBeforeTest now reads args.instance to resolve the test-run uuid; give the onBeforeTest spec a mock instance (with getRef), matching the real event args (instance is always present for test events, as onAllTestEvents already assumes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Proposed changes
Types of changes
Checklist
Backport Request
//: # (The current
mainbranch is the development branch for WebdriverIO v9. If your change should be released to the current major version of WebdriverIO (v8), please raise another PR with the same changes against thev8branch.)v9and doesn't need to be back-ported#XXXXXFurther comments
Reviewers: @webdriverio/project-committers