fix(observability): report Mocha/Jasmine test tags (SDK-7458) [v8] - #215
kamal-kaur04 wants to merge 3 commits into
Conversation
Port of the main-line fix to the v8 branch; the reported build pinned --wdio_versions v8. WDIO Mocha and Jasmine never populated a test-level tags field, so every Observability test arrived with tags == []. @tag tokens in the suite and test titles are the source, matching the node SDK's Jest/Playwright convention. The leading @ is kept so these match Cucumber pickle tags. Both event paths fixed: wdioMochaTestFramework#getTestData now sets KEY_TEST_TAGS (declared in the service's constants, never written, long consumed by the binary), and insights-handler#getRunData now sets tags on TestData. Inert for untagged suites -- a title with no @ token yields []. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
✅ Good to go
Change map (generated deterministically from the diff)graph LR
subgraph nnode_agent["node-agent"]
npackages_browserstack_service_src_util_ts["util.ts<br/>~38 lines"]
npackages_browserstack_service_tests_util_test_ts["util.test.ts<br/>~38 lines"]
npackages_browserstack_service_src_cli_frameworks_wdioMochaTestFramework_ts["wdioMochaTestFramework.ts<br/>~7 lines"]
npackages_browserstack_service_src_insights_handler_ts["insights-handler.ts<br/>~6 lines"]
n_changeset_pr_215_md["pr-215.md<br/>~5 lines"]
npackages_browserstack_service_src_reporter_ts["reporter.ts<br/>~4 lines"]
end
↻ This verdict comment is the review anchor — it's updated in place on each run (the gate posts its status separately). — SDK PR Review Agent |
Review findings (relay)The automated verdict above shows 1. Critical — the changeset claims Jasmine is fixed, but the Jasmine event producer was never touched
The changeset, the Jasmine's test and hook events are emitted by The same untouched path also emits Mocha skipped tests ( Fix — either add the same extraction to 2. Critical — the PR body says no manual changeset was added, but the diff adds one
The PR description states verbatim "No manual changeset added — per this template the changeset is generated from this section", while the diff adds Fix — decide which mechanism owns the changeset here: drop the file if the template section generates it, otherwise correct that line in the description. 3. Warning — tag tokens keep the leading
|
There was a problem hiding this comment.
Critical — the changeset claims Jasmine is fixed, but the Jasmine event producer was never touched.
File-level comment: the code at fault is in reporter.ts, which this PR doesn't modify, so there's no changed line to anchor to.
The changeset, the util.ts doc comment and the new @jasmine test all claim tags are now reported for Mocha and Jasmine. But the only Direct-flow change is getRunData here, which Jasmine never reaches — beforeTest (:463) and afterTest (:481) both early-return unless this._framework === 'mocha'.
Jasmine's test and hook events come from reporter.ts#getRunData (reporter.ts:272, untouched), reached because needToSendData returns event !== 'skip' for jasmine (reporter.ts:144). That TestData literal sets scopes but no tags. So Jasmine builds keep arriving with tags == [] and the customer-facing release note is wrong.
The same untouched path also emits Mocha skipped tests (needToSendData, mocha ⇒ event === 'skip'), so it.skip rows stay untagged too.
Fix — either add the extraction to reporter.ts#getRunData (its scopes is already this._suites.map(s => s.title), so tags: getTestTags(testStats as any, scopes) is close to a one-liner), or narrow the changeset and release notes to Mocha only and drop the Jasmine claim from the doc comment.
| "@wdio/browserstack-service": patch | ||
| --- | ||
|
|
||
| - Fixed test tags not being reported to Test Observability for Mocha and Jasmine. Tags written as `@tag` tokens in suite or test titles are now sent with each test. |
There was a problem hiding this comment.
Critical — the PR body says no manual changeset was added, but this file adds one.
The PR description states verbatim "No manual changeset added — per this template the changeset is generated from this section", while the diff adds this file declaring a patch bump. One of the two is stale; if both land, the changelog carries a duplicated entry for the same fix.
Fix — decide which mechanism owns the changeset for this repo: drop this file if the template section generates it, otherwise correct that line in the PR description.
| return value.reverse() | ||
| } | ||
|
|
||
| const TEST_TAG_PATTERN = /@[\w-]+/g |
There was a problem hiding this comment.
Warning — tag tokens keep the leading @, unlike the node SDK path the comment below cites as the same convention.
The pattern captures the @ as part of the token (no slice(1)), so this emits @smoke. The doc comment justifies that as "the same convention the node SDK uses for Jest and Playwright" — that half doesn't hold. In browserstack-node-agent/src/bin/playwright/reporter/pwUtils.js the equivalent helper strips the prefix on both branches (title.match(/@(\w+)/g)?.map(tag => tag.slice(1)) at :25, and test.tags.map(t => t.startsWith('@') ? t.substr(1) : t) at :30), and that stripped value is what goes out as tags:.
Net effect: the same logical tag reaches the Observability tags field as @smoke from a WDIO run and smoke from a node-agent Playwright run, so a dashboard tag filter won't match across the two SDKs.
Why this is a divergence rather than a defect: within this repo the choice is self-consistent — the Cucumber path already forwards raw pickle names, which carry the @ (insights-handler.ts:1135), so the backend accepts both shapes and keeping @ matches the in-repo sibling flow. What's verifiably wrong is the stated cross-SDK parity; the next maintainer will read this comment and believe node-agent keeps the @ too. (Which shape the Observability/TRA consumer prefers for filtering wasn't decidable from this diff.)
Fix — pick one and make the comment true. Either keep the behaviour (reasonable — it matches this repo's Cucumber tags) and correct the comment to say the @ is kept to match the pickle tags in this service, dropping the node-agent parity claim; or normalize to tag.slice(1), in which case the Cucumber path in insights-handler.ts must be normalized in the same PR or the two WDIO flows disagree with each other instead.
| }) | ||
| }) | ||
|
|
||
| describe('getTestTags', () => { |
There was a problem hiding this comment.
Warning — every case here passes scopes explicitly, so the fallback branch the new default parameter creates is never exercised.
getTestTags(test, scopes?) has two input paths: caller-supplied scopes, and — when it's undefined — the internally derived getMochaTestHierarchy(test). The tagsFor helper defaults scopes to [] and always forwards it, so scopes ?? getMochaTestHierarchy(test) resolves to the left operand in all seven cases. (The case named "falls back to the Jasmine description" exercises the test.title || test.description fallback, not the hierarchy one.)
The derived branch is where the shape risk lives: getMochaTestHierarchy returns reversed parent titles for Mocha via test.ctx/test.parent, and for Jasmine pushes test.description then derives suite names from fullName. Neither shape is asserted in this PR, so a regression there — wrong ordering, an extra or absent title, a suite title never reaching the matcher — ships green.
Fix — add one case per hierarchy shape that omits scopes:
it('derives scopes from the mocha hierarchy when none are supplied', () => {
const test = { title: 'logs in @smoke', parent: { title: 'auth @regression', parent: { title: '' } } }
expect(utils.getTestTags(test as any)).toEqual(['@regression', '@smoke'])
})Set the expected values from what getMochaTestHierarchy actually returns, so the assertion pins the order the caller will see.
| return value.reverse() | ||
| } | ||
|
|
||
| const TEST_TAG_PATTERN = /@[\w-]+/g |
There was a problem hiding this comment.
Critical — the pattern has no left boundary, so an @ inside a larger token becomes a tag.
This is a second, independent defect in the same three lines as the @-prefix comment above — they don't overlap and fixing one doesn't fix the other.
/@[\w-]+/g anchors nothing to the left of the @, so every @ in a title or scope starts a match, including one glued to a preceding word character:
getTestTags({ title: 'sends the invite to user@example.com' } as any, [])
// → ['@example']
describe('@wdio/logger output') // → ['@wdio']Same for any name@host or pkg@1.2.3 fragment in a describe/it title. The doc comment just below says the source is "@tag tokens written into the suite and test titles" — an embedded fragment isn't a token, so the implementation is wider than its own stated contract.
Silent and data-only: the run still passes, and the spurious tag is attached by both new call sites (insights-handler.ts and cli/frameworks/wdioMochaTestFramework.ts), so it reaches the Observability tag facet and pollutes filtering with a value nobody can reproduce deliberately.
Fix — require the @ to start a token:
const TEST_TAG_PATTERN = /(?:^|\s)(@[\w-]+)/g
// ...
for (const [, tag] of title.matchAll(TEST_TAG_PATTERN)) { ... }(matchAll is needed — String.prototype.match with /g drops capture groups.) A /(?<![\w-])@[\w-]+/g lookbehind keeps the current .match() call and is supported on every Node version this package targets. Worth adding expect(tagsFor('sends the invite to user@example.com')).toEqual([]) to the new suite.
Surfaced by the review of the sibling PR (#214) — same change on main, identical regex. Recorded here so both copies carry both defects; they're independent and neither supersedes the other.
Port of the main-line review fixes (#214). - Tag pattern now requires the @ to start a token. /@[\w-]+/g matched any embedded @, so 'user@example.com' produced a bogus @example tag. - reporter.ts#getRunData now sets tags. insights-handler#getRunData is reached only for mocha, so jasmine events and mocha skipped tests went through the reporter, which set scopes but no tags -- the Jasmine claim was false without this. - Doc comment no longer claims node-agent parity; that SDK strips the @. - Tests: embedded-@ cases, plus one per hierarchy shape with scopes omitted, pinned to the order the helper actually returns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed in 1 · Tag pattern had no left boundary — CONFIRMED, fixed. Reproduced exactly as described: Took the lookbehind form, Worth flagging: neither your suggested form fixes the third example. 2 · Jasmine was never actually fixed — CONFIRMED, fixed. This was the most valuable finding; the release note was wrong. Verified: 3 · node-agent parity claim was false — CONFIRMED, comment corrected. 4 · Tests never exercised the derived-hierarchy branch — CONFIRMED, fixed. Added one case per shape with 5 · Duplicate changeset — respectfully, false positive. Verification: Still not run: the vitest suite (vitest is absent from the workspace install) and an end-to-end run. #215 remains uncompiled locally — v8 resolves a different |
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the latest SDK PR Review Agent run reports GTG on the current head commit. A native reviewer approval is separately required by branch protection before merge. |
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
|
RUN_TESTS |
|
Recurring 2 failures. Rest Good. |
What is this about?
WDIO Mocha and Jasmine never populated a test-level tags field, so every Observability test arrived with
tags == []. Build tags were unaffected — they travel a different path (getObservabilityBuildTags).Neither runner has a tag construct, so the source is the
@tagtoken convention in suite and test titles — the same mechanismbrowserstack-node-agentalready ships for Jest (getJestTestTags) and Playwright (getTagsFromTitle). The leading@is kept so these match the Cucumber runner's pickle tags, which already reach Observability with it intact.The contract was already modelled end to end — only the thin layer never filled it:
KEY_TEST_TAGS: 'test_tags'is declared atcli/frameworks/constants/testFrameworkConstants.ts:6and had exactly one reference in all ofsrc/— its own declaration.observability/languages/webdriverio/test-frameworks/mocha/index.jsdoestags: event.test_tags || [].TestData.tags?: string[]already existed intypes.ts:316.No proto change, no binary change, no cross-SDK coordination.
Both event paths were missing it, and both are fixed:
CLISupportedFrameworks = ['mocha'], so with the CLI running,service.ts#beforeTest/#afterTestreturn early into the CLI test framework and never reachInsightsHandler.cli/frameworks/wdioMochaTestFramework.ts#getTestDataomittedKEY_TEST_TAGS. This is the path App Automate runs — fixing only the Direct Flow would have left the reported build still emitting[].insights-handler.ts#getRunDatabuiltTestDatawith notagskey;tagswas set only in the Cucumber-specificgetTestRunDataForCucumberviaworld.pickle.tags.Inert for untagged suites. A title with no
@token yields[], byte-identical to today's payload, so no existing customer build changes shape.Related Jira task/s
This is the v8 port. The main-line (v9) change is #214 — the two diffs are line-for-line identical apart from v8's pre-existing
getGitMetaDatain one import.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)
@tagtokens in suite or test titles are now sent with each test.Release notes (internal): (required — engineer-facing; what actually changed / why)
getTestTags(test, scopes?)inutil.tsextracts@tagtokens from the suite hierarchy plus the test title, deduped,@retained to match Cucumber pickle tags.wdioMochaTestFramework#getTestDatanow setsKEY_TEST_TAGS. That key was declared in the service's own constants and never written, while the binary had long consumedevent.test_tags.insights-handler#getRunDatanow setstagsonTestData(previously Cucumber-only).Checklist
Verification performed — read this carefully, it is weaker than the v9 PR's:
getGitMetaDatain theutil.jsimport. fix(observability): report Mocha/Jasmine test tags (SDK-7458) #214 istsc --noEmitclean in the changed files andeslintclean.descriptionfallback) pass against the function body taken from the real source. The helper is identical on both branches.@wdio/typesline, so a freshnpm cifor v8 is needed to typecheck it honestly; that was not run. Reviewers should rely on CI here rather than on a local green.details.tags == ["@TestTag"]is raised separately.PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.