Skip to content

chore: Use CircleCI native timing-based test splitting for acceptance shards#6889

Open
danskmt wants to merge 1 commit into
mainfrom
chore/CLI-1482-circleci-native-test-splitting
Open

chore: Use CircleCI native timing-based test splitting for acceptance shards#6889
danskmt wants to merge 1 commit into
mainfrom
chore/CLI-1482-circleci-native-test-splitting

Conversation

@danskmt

@danskmt danskmt commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Pull Request Submission Checklist

  • Follows CONTRIBUTING guidelines
  • Commit messages are release-note ready, emphasizing what was changed, not how.
  • Includes detailed description of changes
  • Contains risk assessment (Low | Medium | High)
  • Highlights breaking API changes (if applicable)
  • Links to automated tests covering new functionality
  • Includes manual testing instructions (if necessary)
  • Updates relevant GitBook documentation (PR link: ___)
  • Includes product update to be announced in the next stable release notes

What does this PR do?

Replaces Jest's --shard with CircleCI's native circleci tests run --split-by=timings so acceptance test shards are automatically balanced by historic runtime data — no manually maintained weight files needed.

  • Replaces --shard=<index>/<total> with circleci tests glob | circleci tests run --split-by=timings --timings-type=file in the acceptance-tests job.
  • Adds addFileAttribute: true to jest-junit config so JUnit XML includes file="..." on each <testsuite>, which CircleCI needs for file-based timing matching.
  • Switches the Windows test step to bash.exe (from PowerShell) since the command uses xargs and Unix pipe syntax. Sources the bash-compatible env script (snyk-env.sh) instead of the PowerShell one.

Where should the reviewer start?

  • .circleci/config.yml — the acceptance-tests job definition (replaced shard_calc_cmd parameter with test_shell, new circleci tests run command)
  • package.jsonaddFileAttribute addition in jest-junit config

How should this be manually tested?

  1. Push branch and trigger CI.
  2. Check that acceptance tests run on all platforms (linux, macOS, Windows).
  3. Compare shard wall times — after 1-2 runs, CircleCI accumulates timing data and balances shards automatically.

What's the product update that needs to be communicated to CLI users?

None. CI-only change; no CLI behavior change for end users.

Risk assessment (Low | Medium | High)?

Medium — changes how tests are distributed across CI shards on all platforms including Windows (shell change). First run may use name-based splitting until CircleCI accumulates timing data from JUnit reports.

Any background context you want to provide?

CircleCI stores timing data per job name across all branches, so existing store_test_results uploads from main should provide immediate timing data. The addFileAttribute flag makes jest-junit emit the file attribute on each <testsuite>, enabling CircleCI to match timing history to spec files. See CircleCI docs: Test splitting and parallelism.

What are the relevant tickets?

CLI-1482

Screenshots (if appropriate)

Before (main) X Now (chore/CLI-1482-circleci-native-test-splitting):

imageimage

Before (feat/CLI-1482-balance-acceptance-test-shards) X Now (chore/CLI-1482-circleci-native-test-splitting)

image image

@snyk-io

snyk-io Bot commented Jun 9, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@danskmt danskmt force-pushed the chore/CLI-1482-circleci-native-test-splitting branch 9 times, most recently from be2b704 to cd2482b Compare June 11, 2026 07:31
@danskmt danskmt marked this pull request as ready for review June 11, 2026 08:20
@danskmt danskmt requested a review from a team as a code owner June 11, 2026 08:20
@snyk-pr-review-bot

This comment has been minimized.

Comment thread .circleci/config.yml
Comment thread scripts/windows/ensure-python-uv.ps1

@robertolopezlopez robertolopezlopez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Good job

Comment thread package.json
"jest-junit": {
"reportTestSuiteErrors": "true"
"reportTestSuiteErrors": "true",
"addFileAttribute": "true"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need this addFileAttribute: true to jest-junit config so JUnit XML includes file="..." on each , which CircleCI needs for file-based timing matching.

Comment thread .circleci/config.yml Outdated
Comment thread .circleci/config.yml Outdated
Comment thread .circleci/config.yml Outdated

@j-luong j-luong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think introducing bash as the shell for Windows causes more problems than it solves, let's see if we can do this without needing bash for Windows. That way we can clean up the extra scripts you introduced

@danskmt danskmt force-pushed the chore/CLI-1482-circleci-native-test-splitting branch 2 times, most recently from 9c558a5 to 1dc3634 Compare June 11, 2026 13:49
@snyk-pr-review-bot

This comment has been minimized.

@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1482-circleci-native-test-splitting branch from 1dc3634 to c84875a Compare June 11, 2026 15:31
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1482-circleci-native-test-splitting branch from c84875a to 08d6deb Compare June 11, 2026 16:46
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1482-circleci-native-test-splitting branch from 08d6deb to 0099b5a Compare June 12, 2026 12:55
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1482-circleci-native-test-splitting branch from 0099b5a to e1176d3 Compare June 12, 2026 14:21
@snyk-pr-review-bot

This comment has been minimized.

Comment thread .circleci/config.yml
Comment on lines +1005 to +1008
test_cmd: |
Get-ChildItem -Recurse -Path test/jest/acceptance,ts-binary-wrapper/test/acceptance -Filter *.spec.ts |
ForEach-Object { Resolve-Path -Relative $_.FullName } |
circleci tests run --command "xargs npx jest --maxWorkers=1 --selectProjects coreCli --reporters=jest-junit --reporters=default --" --split-by=timings --timings-type=file

@danskmt danskmt Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@j-luong it seems we do need the path thing... Needs both for saving as forward slash, and for normalizing when fetching
https://app.circleci.com/pipelines/gh/snyk/cli/37803/workflows/6ad62f25-703d-42a3-aad8-8401d73bb71d/jobs/595269

Image

@danskmt danskmt force-pushed the chore/CLI-1482-circleci-native-test-splitting branch from e1176d3 to 035e2ad Compare June 12, 2026 15:05
@snyk-pr-review-bot

This comment has been minimized.

@@ -0,0 +1,45 @@
#!/usr/bin/env node

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nitpick: Would be nice to have this written in bash/PowerShell to remove the JavaScript dependency

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My vote for python, much easier to craft+read+review.
And multi platform

j-luong
j-luong previously approved these changes Jun 15, 2026
@j-luong j-luong self-requested a review June 15, 2026 12:00
@j-luong j-luong dismissed their stale review June 15, 2026 12:01

issue: the windows acceptance tests seem to be taking longer than before (> 30mins) which means this PR actually degrades CI runtimes. Let's look into what's going on here

@danskmt danskmt force-pushed the chore/CLI-1482-circleci-native-test-splitting branch from 035e2ad to 29edbfc Compare June 15, 2026 12:24
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1482-circleci-native-test-splitting branch from 29edbfc to d46e183 Compare June 16, 2026 09:18
@snyk-pr-review-bot

This comment has been minimized.

@danskmt danskmt force-pushed the chore/CLI-1482-circleci-native-test-splitting branch from d46e183 to 1c0c975 Compare June 16, 2026 10:56
@snyk-pr-review-bot

This comment has been minimized.

Comment on lines +31 to +44
for (const entry of fs.readdirSync(reportsDir)) {
if (!entry.endsWith('.xml')) {
continue;
}

const filePath = path.join(reportsDir, entry);
const original = fs.readFileSync(filePath, 'utf8');
const normalized = original.replace(fileAttrPattern, (match) =>
match.split('\\').join('/'),
);

if (normalized !== original) {
fs.writeFileSync(filePath, normalized);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing error handling for file operations could fail the entire CI job even when tests pass. If fs.readFileSync() or fs.writeFileSync() throws an error (e.g., due to permissions or file locks), the unhandled exception will cause the normalization step to fail. Since this step runs with when: always in CircleCI, it will execute even after test failures, but any crash here will override the test results and fail the job.

Add try-catch error handling:

for (const entry of fs.readdirSync(reportsDir)) {
  if (!entry.endsWith('.xml')) {
    continue;
  }

  const filePath = path.join(reportsDir, entry);
  try {
    const original = fs.readFileSync(filePath, 'utf8');
    const normalized = original.replace(fileAttrPattern, (match) =>
      match.split('\\').join('/'),
    );

    if (normalized !== original) {
      fs.writeFileSync(filePath, normalized);
    }
  } catch (err) {
    console.error(`Failed to process ${filePath}:`, err.message);
    // Continue processing other files
  }
}
Suggested change
for (const entry of fs.readdirSync(reportsDir)) {
if (!entry.endsWith('.xml')) {
continue;
}
const filePath = path.join(reportsDir, entry);
const original = fs.readFileSync(filePath, 'utf8');
const normalized = original.replace(fileAttrPattern, (match) =>
match.split('\\').join('/'),
);
if (normalized !== original) {
fs.writeFileSync(filePath, normalized);
}
for (const entry of fs.readdirSync(reportsDir)) {
if (!entry.endsWith('.xml')) {
continue;
}
const filePath = path.join(reportsDir, entry);
try {
const original = fs.readFileSync(filePath, 'utf8');
const normalized = original.replace(fileAttrPattern, (match) =>
match.split('\\').join('/'),
);
if (normalized !== original) {
fs.writeFileSync(filePath, normalized);
}
} catch (err) {
console.error(`Failed to process ${filePath}:`, err.message);
// Continue processing other files
}

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@danskmt danskmt force-pushed the chore/CLI-1482-circleci-native-test-splitting branch from 1c0c975 to 5e613fb Compare June 16, 2026 12:20
@snyk-pr-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Environment Incompatibility 🟠 [major]

The new test_cmd for Windows acceptance tests uses a PowerShell pipe (|) to send file paths into circleci tests run, which then executes xargs. While Get-ChildItem is a PowerShell cmdlet, xargs is a Unix utility not natively available in PowerShell. If the win-server2022-amd64 executor does not have a Unix-like environment (like Git Bash) in its system PATH, this command will fail immediately. Additionally, the PR description mentions switching to bash.exe, but the code in the diff uses PowerShell syntax (Get-ChildItem, ForEach-Object, -replace).

test_cmd: |
  Get-ChildItem -Recurse -Path test/jest/acceptance,ts-binary-wrapper/test/acceptance -Filter *.spec.ts |
    ForEach-Object { (Resolve-Path -Relative $_.FullName) -replace '\\', '/' -replace '^\./', '' } |
    circleci tests run --command "xargs npx jest --maxWorkers=1 --selectProjects coreCli --reporters=jest-junit --reporters=default --" --split-by=timings --timings-type=file
Fragile Regex 🟡 [minor]

The fileAttrPattern regex /file="[^"]*"/g is used to find and replace paths in the JUnit XML. If a test failure message or a system-out block contains the string file="some\path", it will be incorrectly modified by this script. While acceptance tests typically don't output this specific pattern in logs, a more robust approach would be to use a proper XML parser to target only the file attribute on <testsuite> or <testcase> elements.

const fileAttrPattern = /file="[^"]*"/g;

for (const entry of fs.readdirSync(reportsDir)) {
  if (!entry.endsWith('.xml')) {
    continue;
  }

  const filePath = path.join(reportsDir, entry);
  const original = fs.readFileSync(filePath, 'utf8');
  const normalized = original.replace(fileAttrPattern, (match) =>
    match.split('\\').join('/'),
📚 Repository Context Analyzed

This review considered 7 relevant code sections from 6 files (average relevance: 0.81)

🤖 Repository instructions applied (from AGENTS.md)

@j-luong

j-luong commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

The new test_cmd for Windows acceptance tests uses a PowerShell pipe (|) to send file paths into circleci tests run, which then executes xargs. While Get-ChildItem is a PowerShell cmdlet, xargs is a Unix utility not natively available in PowerShell. If the win-server2022-amd64 executor does not have a Unix-like environment (like Git Bash) in its system PATH, this command will fail immediately. Additionally, the PR description mentions switching to bash.exe, but the code in the diff uses PowerShell syntax (Get-ChildItem, ForEach-Object, -replace).

this seems a valid concern, and I'm not sure why it's not failing, but it's worth fixing

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.

3 participants