Skip to content

fix(observability): report Mocha/Jasmine test tags (SDK-7458) [v8] - #215

Open
kamal-kaur04 wants to merge 3 commits into
v8from
fix/SDK-7458-mocha-jasmine-test-tags-v8
Open

kamal-kaur04 wants to merge 3 commits into
v8from
fix/SDK-7458-mocha-jasmine-test-tags-v8

Conversation

@kamal-kaur04

Copy link
Copy Markdown
Collaborator

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 @tag token convention in suite and test titles — the same mechanism browserstack-node-agent already 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 at cli/frameworks/constants/testFrameworkConstants.ts:6 and had exactly one reference in all of src/ — its own declaration.
  • The binary has long consumed it: observability/languages/webdriverio/test-frameworks/mocha/index.js does tags: event.test_tags || [].
  • TestData.tags?: string[] already existed in types.ts:316.

No proto change, no binary change, no cross-SDK coordination.

Both event paths were missing it, and both are fixed:

  1. Binary FlowCLISupportedFrameworks = ['mocha'], so with the CLI running, service.ts#beforeTest/#afterTest return early into the CLI test framework and never reach InsightsHandler. cli/frameworks/wdioMochaTestFramework.ts#getTestData omitted KEY_TEST_TAGS. This is the path App Automate runs — fixing only the Direct Flow would have left the reported build still emitting [].
  2. Direct Flow — Jasmine, and Mocha when the CLI is not running. insights-handler.ts#getRunData built TestData with no tags key; tags was set only in the Cucumber-specific getTestRunDataForCucumber via world.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 getGitMetaData in one import.

Release (mandatory for every PR — required for the ready-for-review label)

Version bump: (required — tick exactly one)

  • minor (backwards-compatible feature)
  • patch (bug fix or other small change)

Release notes type: (optional)

  • New Feature
  • Bug Fix
  • Other Improvement

Release notes (customer-facing): (optional but encouraged)

  • 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.

Release notes (internal): (required — engineer-facing; what actually changed / why)

  • New getTestTags(test, scopes?) in util.ts extracts @tag tokens from the suite hierarchy plus the test title, deduped, @ retained to match Cucumber pickle tags.
  • Binary Flow: wdioMochaTestFramework#getTestData now sets KEY_TEST_TAGS. That key was declared in the service's own constants and never written, while the binary had long consumed event.test_tags.
  • Direct Flow: insights-handler#getRunData now sets tags on TestData (previously Cucumber-only).
  • Both call sites hoist the scope hierarchy into a local so it is computed once instead of twice.
  • No manual changeset added — per this template the changeset is generated from this section.

Checklist

  • Ready to review
  • Has it been tested locally?

Verification performed — read this carefully, it is weaker than the v9 PR's:

  • The added lines here are byte-identical to those in fix(observability): report Mocha/Jasmine test tags (SDK-7458) #214 apart from v8's pre-existing getGitMetaData in the util.js import. fix(observability): report Mocha/Jasmine test tags (SDK-7458) #214 is tsc --noEmit clean in the changed files and eslint clean.
  • The 7 tag cases (title tags, scope tags, scope+title dedup, untagged, multi-tag, hyphenated, Jasmine description fallback) pass against the function body taken from the real source. The helper is identical on both branches.
  • This branch was NOT independently compiled or linted. v8 resolves a different @wdio/types line, so a fresh npm ci for v8 is needed to typecheck it honestly; that was not run. Reviewers should rely on CI here rather than on a local green.
  • Not run: the vitest suite and an end-to-end App Automate run. The BStackAutomation coverage change that proves details.tags == ["@TestTag"] is raised separately.

PR Validations

Run Tests: Comment RUN_TESTS to trigger sanity tests.

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>
@kamal-kaur04
kamal-kaur04 requested a review from a team as a code owner September 22, 2026 15:05
@kamal-kaur04
kamal-kaur04 requested review from AakashHotchandani and harshit-browserstack and removed request for a team September 22, 2026 15:05
@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited), Workspace UI (inherited)

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 5eb192e6-552a-477b-ae3e-2ab2411331cd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@kamal-kaur04

kamal-kaur04 commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

Good to go

File Status Reason
.changeset/pr-215.md ✅ All Clear Judged against the PR body's ## Release section; bump and notes match, provenance confirmed machine-generated
packages/browserstack-service/src/cli/frameworks/wdioMochaTestFramework.ts ✅ All Clear Covered by review-knowledge, no issues found
packages/browserstack-service/src/insights-handler.ts ✅ All Clear Covered by review-knowledge, no issues found
packages/browserstack-service/src/reporter.ts ✅ All Clear Covered by review-knowledge, no issues found
packages/browserstack-service/src/util.ts ✅ All Clear Covered by review-knowledge, no issues found
packages/browserstack-service/tests/util.test.ts ✅ All Clear Covered by review-knowledge, no issues found

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
Loading

↻ 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

@kamal-kaur04

Copy link
Copy Markdown
Collaborator Author

Review findings (relay)

The automated verdict above shows ⚠️ pending, which is a process artifact, not the review's conclusion — the gate held the marker amber over its own internal bookkeeping. The review itself computed 4 blocking findings, and the verdict comment doesn't carry them. Relaying here. All four survived the falsification pass; coverage was clean (9/9 regions judged).


1. Critical — the changeset claims Jasmine is fixed, but the Jasmine event producer was never touched

packages/browserstack-service/src/insights-handler.ts · _InsightsHandler.getRunData

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 insights-handler#getRunData, which Jasmine never reaches — beforeTest (:463) and afterTest (:481) both early-return unless this._framework === 'mocha'.

Jasmine's test and hook events are emitted by reporter.ts#getRunData (reporter.ts:272, untouched by this PR), 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 same 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-line addition), or narrow the changeset and release notes to Mocha only and drop the Jasmine claim from the doc comment.


2. Critical — the PR body says no manual changeset was added, but the diff adds one

.changeset/pr-215.md

The PR description states verbatim "No manual changeset added — per this template the changeset is generated from this section", while the diff adds .changeset/pr-215.md 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 here: drop the file if the template section generates it, otherwise correct that line in the description.


3. Warning — tag tokens keep the leading @, unlike the node SDK path the comment cites

packages/browserstack-service/src/util.ts · getTestTags

The pattern captures the @ as part of the token (/@[\w-]+/g, 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)), and test.tags.map(t => t.startsWith('@') ? t.substr(1) : t)), 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.

Worth saying 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 that comment and believe node-agent keeps the @ too. (Which shape the Observability/TRA consumer actually 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 has to be normalized in the same PR or the two WDIO flows disagree with each other instead. Either way, state the convention once next to the pattern.


4. Warning — every new test passes scopes explicitly, so the fallback branch is untested

packages/browserstack-service/tests/util.test.ts · describe('getTestTags')

getTestTags(test, scopes?) has two input paths: caller-supplied scopes, and — when it's undefined — the internally derived getMochaTestHierarchy(test). The suite's local 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 actually 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 anywhere 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.


wdioMochaTestFramework.ts came back clean.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread .changeset/pr-215.md
"@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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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', () => {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@kamal-kaur04

Copy link
Copy Markdown
Collaborator Author

Addressed in f8c49f8 (#214) / efd87cf (#215). All four code findings verified against source before changing anything; one finding I am pushing back on with evidence.

1 · Tag pattern had no left boundary — CONFIRMED, fixed. Reproduced exactly as described:

"sends the invite to user@example.com" -> ["@example"]
"pkg@1.2.3"                            -> ["@1"]
"@wdio/logger output"                  -> ["@wdio"]

Took the lookbehind form, /(?<![\w-])@[\w-]+/g, which keeps the existing .match() call. The first two now yield [].

Worth flagging: neither your suggested form fixes the third example. @wdio/logger output still yields ["@wdio"] under both (?<![\w-]) and (?:^|\s)(@[\w-]+), because that @ genuinely does start a token. I tried a slash-guard ((?!\/)) and it is worse — backtracking makes it emit ["@wdi"]. Left as-is: a token-initial @ matching the stated contract is defensible, and I would rather not ship a regex that silently truncates. Happy to revisit if scoped-package names in describe titles are a real concern.

2 · Jasmine was never actually fixed — CONFIRMED, fixed. This was the most valuable finding; the release note was wrong. Verified: beforeTest (:461) and afterTest (:483) both return unless this._framework === 'mocha', and reporter.ts#getRunData had zero occurrences of tags. reporter.ts now sets tags: getTestTags(testStats as unknown as Frameworks.Test, scopes) — the cast matches the existing idiom at :244. That also covers the mocha it.skip rows you noted.

3 · node-agent parity claim was false — CONFIRMED, comment corrected. pwUtils.js does strip: .map(tag => tag.slice(1)) at :25 and .substr(1) at :30. Kept the @ (self-consistent with this service's Cucumber pickle tags, as you say) and rewrote the comment to state that, explicitly noting the node SDK strips it so the two SDKs differ. No silent parity claim left behind.

4 · Tests never exercised the derived-hierarchy branch — CONFIRMED, fixed. Added one case per shape with scopes omitted. Your instinct to pin the real ordering was right and it caught me out: I guessed ['@smoke', '@regression'] for the jasmine shape and the helper actually returns ['@regression', '@smoke']. Both assertions are now set from observed output, not assumption.

5 · Duplicate changeset — respectfully, false positive. .changeset/pr-215.md is not mine. git log --format='%an' on that path returns github-actions[bot], commit "chore(changeset): auto-generate from PR template (patch)". It is the file the template promises to generate. My manual .changeset/mocha-jasmine-test-tags.md was removed before the first push, so there is exactly one changeset and the PR body statement is accurate. No change made.

Verification: tsc --noEmit — zero errors in changed files on #214 (the 9 in the tree are pre-existing: src/grpc/generated/ absent without buf generate). eslint clean on all five files. 11 tag cases pass against the function body taken from real source, including the two new boundary cases and both hierarchy shapes.

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 @wdio/types, so its added lines are byte-identical to #214 apart from two pre-existing import neighbours, and CI is the real gate there.

@github-actions

Copy link
Copy Markdown
Contributor

🔴 SDK PR Review gate is red. Pending:

  • The SDK PR Review Agent reported blocking findings (🔴) on the current head commit — fix them and re-run the agent.

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.

@github-actions

Copy link
Copy Markdown
Contributor

🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: success).

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.

@kamal-kaur04

Copy link
Copy Markdown
Collaborator Author

RUN_TESTS

@kamal-kaur04

Copy link
Copy Markdown
Collaborator Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants