chore: Use CircleCI native timing-based test splitting for acceptance shards#6889
chore: Use CircleCI native timing-based test splitting for acceptance shards#6889danskmt wants to merge 1 commit into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
be2b704 to
cd2482b
Compare
This comment has been minimized.
This comment has been minimized.
robertolopezlopez
left a comment
There was a problem hiding this comment.
LGTM! Good job
| "jest-junit": { | ||
| "reportTestSuiteErrors": "true" | ||
| "reportTestSuiteErrors": "true", | ||
| "addFileAttribute": "true" |
There was a problem hiding this comment.
We need this addFileAttribute: true to jest-junit config so JUnit XML includes file="..." on each , which CircleCI needs for file-based timing matching.
j-luong
left a comment
There was a problem hiding this comment.
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
9c558a5 to
1dc3634
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1dc3634 to
c84875a
Compare
This comment has been minimized.
This comment has been minimized.
c84875a to
08d6deb
Compare
This comment has been minimized.
This comment has been minimized.
08d6deb to
0099b5a
Compare
This comment has been minimized.
This comment has been minimized.
0099b5a to
e1176d3
Compare
This comment has been minimized.
This comment has been minimized.
| 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 |
There was a problem hiding this comment.
@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
e1176d3 to
035e2ad
Compare
This comment has been minimized.
This comment has been minimized.
| @@ -0,0 +1,45 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
nitpick: Would be nice to have this written in bash/PowerShell to remove the JavaScript dependency
There was a problem hiding this comment.
My vote for python, much easier to craft+read+review.
And multi platform
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
035e2ad to
29edbfc
Compare
This comment has been minimized.
This comment has been minimized.
29edbfc to
d46e183
Compare
This comment has been minimized.
This comment has been minimized.
d46e183 to
1c0c975
Compare
This comment has been minimized.
This comment has been minimized.
| 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); | ||
| } |
There was a problem hiding this comment.
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
}
}| 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
Is this helpful? React 👍 or 👎 to let us know.
1c0c975 to
5e613fb
Compare
PR Reviewer Guide 🔍
|
this seems a valid concern, and I'm not sure why it's not failing, but it's worth fixing |
Pull Request Submission Checklist
What does this PR do?
Replaces Jest's
--shardwith CircleCI's nativecircleci tests run --split-by=timingsso acceptance test shards are automatically balanced by historic runtime data — no manually maintained weight files needed.--shard=<index>/<total>withcircleci tests glob | circleci tests run --split-by=timings --timings-type=filein theacceptance-testsjob.addFileAttribute: truetojest-junitconfig so JUnit XML includesfile="..."on each<testsuite>, which CircleCI needs for file-based timing matching.bash.exe(from PowerShell) since the command usesxargsand 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— theacceptance-testsjob definition (replacedshard_calc_cmdparameter withtest_shell, newcircleci tests runcommand)package.json—addFileAttributeaddition injest-junitconfigHow should this be manually tested?
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_resultsuploads frommainshould provide immediate timing data. TheaddFileAttributeflag makesjest-junitemit thefileattribute 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):
Before (feat/CLI-1482-balance-acceptance-test-shards) X Now (chore/CLI-1482-circleci-native-test-splitting)