Skip to content

👷 Split unit-bs CI job per browser#4623

Open
mormubis wants to merge 1 commit into
mainfrom
adlrb/split-unit-bs-per-browser
Open

👷 Split unit-bs CI job per browser#4623
mormubis wants to merge 1 commit into
mainfrom
adlrb/split-unit-bs-per-browser

Conversation

@mormubis
Copy link
Copy Markdown
Contributor

@mormubis mormubis commented May 14, 2026

Motivation

The unit-bs job runs all 5 browsers in a single Karma process. If one browser is unavailable on BrowserStack, the whole job fails and you lose the results from the other 4.

Changes

Each browser now runs as a separate CI job using parallel: matrix. The --browser flag on ci-bs.ts filters the Karma config to a single browser. Resource groups are per-browser so they don't serialize against each other. JUnit reports are written to per-browser directories to avoid collisions. Without --browser, all 5 browsers run as before.

Test instructions

BS_BROWSER=edge yarn test:unit:bs        # Only Edge launches
BS_BROWSER=nonexistent yarn test:unit:bs  # Throws with available ids
yarn test:unit:bs                         # All 5 browsers (backwards compat)

Checklist

  • Tested locally
  • Tested on staging
  • Added unit tests for this change.
  • Added e2e/integration tests for this change.
  • Updated documentation and/or relevant AGENTS.md file

@cit-pr-commenter-54b7da
Copy link
Copy Markdown

cit-pr-commenter-54b7da Bot commented May 14, 2026

Bundles Sizes Evolution

📦 Bundle Name Base Size Local Size 𝚫 𝚫% Status
Rum 169.51 KiB 169.51 KiB 0 B 0.00%
Rum Profiler 5.97 KiB 5.97 KiB 0 B 0.00%
Rum Recorder 21.23 KiB 21.23 KiB 0 B 0.00%
Logs 54.70 KiB 54.70 KiB 0 B 0.00%
Rum Slim 127.85 KiB 127.85 KiB 0 B 0.00%
Worker 22.99 KiB 22.99 KiB 0 B 0.00%
🚀 CPU Performance
Action Name Base CPU Time (ms) Local CPU Time (ms) 𝚫%
RUM - add global context 0.0021 0.0029 +38.10%
RUM - add action 0.0105 0.0135 +28.57%
RUM - add error 0.0097 0.0125 +28.87%
RUM - add timing 0.0004 0.0008 +100.00%
RUM - start view 0.0092 0.0144 +56.52%
RUM - start/stop session replay recording 0.0007 0.0012 +71.43%
Logs - log message 0.0139 0.0279 +100.72%
🧠 Memory Performance
Action Name Base Memory Consumption Local Memory Consumption 𝚫
RUM - add global context 38.24 KiB 38.44 KiB +211 B
RUM - add action 64.64 KiB 64.57 KiB -67 B
RUM - add timing 36.73 KiB 38.47 KiB +1.74 KiB
RUM - add error 70.01 KiB 70.96 KiB +981 B
RUM - start/stop session replay recording 45.58 KiB 41.90 KiB -3.68 KiB
RUM - start view 483.88 KiB 486.22 KiB +2.34 KiB
Logs - log message 55.00 KiB 55.82 KiB +847 B

🔗 RealWorld

@datadog-official
Copy link
Copy Markdown

datadog-official Bot commented May 14, 2026

Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 76.96% (+0.00%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: a311531 | Docs | Datadog PR Page | Give us feedback!

@mormubis mormubis force-pushed the adlrb/split-unit-bs-per-browser branch from 533c184 to 61066db Compare May 14, 2026 11:04
Comment thread .gitlab-ci.yml Outdated
@mormubis mormubis force-pushed the adlrb/split-unit-bs-per-browser branch 7 times, most recently from 763ce7c to e0a2b44 Compare May 14, 2026 14:51
@mormubis mormubis force-pushed the adlrb/split-unit-bs-per-browser branch from e0a2b44 to a311531 Compare May 15, 2026 08:14
@mormubis mormubis marked this pull request as ready for review May 19, 2026 08:26
@mormubis mormubis requested a review from a team as a code owner May 19, 2026 08:26
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a311531a23

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

browsers: browserConfigurations.map((configuration) => configuration.sessionName),
concurrency: 5,
browsers: filteredConfigurations.map((configuration) => configuration.sessionName),
concurrency: 1,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Restore full-run BrowserStack concurrency

When BS_BROWSER is unset (the documented backward-compatible yarn test:unit:bs / ci-bs.ts test:unit path), filteredConfigurations still contains all five BrowserStack launchers but this now forces concurrency to 1. That makes the full BrowserStack run execute the five browsers serially instead of the previous concurrency: 5, so the fallback/local full run no longer behaves “as before” and takes roughly five times longer; keep 1 only for the single-browser filtered case.

Useful? React with 👍 / 👎.

import { browserConfigurations } from './browsers.conf.ts'
import karmaBaseConf from './karma.base.conf.js'

const selectedBrowser = process.env.BS_BROWSER
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

❓ question: ‏would it be possible to use parseArgs here, so we don't need to read it in ci-bs.ts and pass it here as env variable?

Comment thread scripts/test/ci-bs.ts
Comment on lines +52 to +54
// Override CI_JOB_NAME so getTestReportDirectory() produces a per-browser path
// (e.g. test-report/unit-bs-firefox/) instead of sharing test-report/unit-bs/ across all browsers.
environment.CI_JOB_NAME = `unit-bs-${browser}`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💬 suggestion: ‏I'd rather not mutate gitlab's build in environment variables.
Instead, and for consistency, I would do like for the e2e: keep the folder name unit-bs as normalized by getTestReportDirectory() and only change the filename of the report:

https://github.com/DataDog/browser-sdk/blob/f7daff3df9c57248f81aa8670cd6bea2b17cdfe8/test/unit/karma.base.conf.js#L77-L79C5

would become:

  junitReporter: {
    outputDir: testReportDirectory,
    outputFile: process.env.BS_BROWSER ? `results-${process.env.BS_BROWSER}.xml` : 'results.xml'
  },

Comment thread .gitlab-ci.yml
artifacts:
reports:
junit: test-report/unit-bs/*.xml
junit: test-report/unit-bs-$BS_BROWSER/*.xml
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
junit: test-report/unit-bs-$BS_BROWSER/*.xml
junit: test-report/unit-bs/*.xml

Comment thread .gitlab-ci.yml
- node scripts/test/ci-bs.ts test:unit --browser $BS_BROWSER
after_script:
- node ./scripts/test/export-test-result.ts unit-bs
- node ./scripts/test/export-test-result.ts unit-bs-$BS_BROWSER
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- node ./scripts/test/export-test-result.ts unit-bs-$BS_BROWSER
- node ./scripts/test/export-test-result.ts unit-bs

Copy link
Copy Markdown
Collaborator

@thomas-lebeau thomas-lebeau left a comment

Choose a reason for hiding this comment

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

hmm, bs-wrapper.ts also checks that there is only one BS job at a time running.

This can make these job wait longer than necessary: (example)

2026-05-15T12:43:32.881957Z 01O �[32;1mOther build running, waiting...�[0m
2026-05-15T12:44:02.930861Z 01O �[32;1mOther build running, waiting...�[0m
2026-05-15T12:44:32.951962Z 01O �[32;1mOther build running, waiting...�[0m
2026-05-15T12:45:03.482930Z 01O �[32;1mOther build running, waiting...�[0m
2026-05-15T12:45:33.517166Z 01O �[32;1mOther build running, waiting...�[0m
2026-05-15T12:46:03.548156Z 01O �[32;1mOther build running, waiting...�[0m
2026-05-15T12:46:33.586724Z 01O �[32;1mOther build running, waiting...�[0m
2026-05-15T12:47:03.618851Z 01O �[32;1mOther build running, waiting...�[0m
2026-05-15T12:47:33.653588Z 01O �[32;1mOther build running, waiting...�[0m
2026-05-15T12:48:03.683845Z 01O �[32;1mOther build running, waiting...�[0m
2026-05-15T12:48:33.716840Z 01O �[32;1mOther build running, waiting...�[0m
2026-05-15T12:49:03.758851Z 01O �[32;1mStarting BrowserStackLocal...�[0m

I think there is another BS api that can return you the number of seats occupied, maybe we should use that instead instead of just hasRunningBuild

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants