Skip to content

Run multiple handles in parallel #239

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
michieldegezelle merged 3 commits into from
Jan 12, 2026
Merged

Run multiple handles in parallel #239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4178f1e
Revert "Revert "Run multiple handles in parallel (#233)" (#238)"
michieldegezelle Jan 12, 2026
261fe7c
Remove the logic to accept pipes in between handles
michieldegezelle Jan 12, 2026
e37358c
Make sure status is FAILED if there is no test result for a handle
michieldegezelle Jan 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

All notable changes to this project will be documented in this file.

## [1.52.0] (12/01/2026)
This update improves test execution performance when running tests with status checks across multiple template handles.
Tests are now run in parallel for multiple handles when using the `--status` flag, significantly reducing the overall execution time. Previously, tests with status checks for multiple handles would run sequentially, but now they leverage parallel processing for better efficiency. This change only affects the `silverfin run-test` command when both multiple handles and the status flag are used together.

Comment on lines +5 to +8
Copy link

@coderabbitai coderabbitai bot Jan 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Use an unambiguous date format in the changelog.

(12/01/2026) is locale-ambiguous; consider 2026-01-12 (ISO-8601) for consistency and clarity.

🤖 Prompt for AI Agents 
In @CHANGELOG.md around lines 5 - 8, The changelog uses a locale-ambiguous date
format in the release header "## [1.52.0] (12/01/2026)"; update that header to
use an unambiguous ISO-8601 date like "## [1.52.0] (2026-01-12)" so the date
format is consistent and clear across locales.

## [1.51.0] (08/01/2026)

This update should have no user impact whatsoever.
Expand Down
46 changes: 36 additions & 10 deletions bin/cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,16 +453,16 @@ program
.command("run-test")
.description("Run Liquid Tests for a reconciliation template from a YAML file")
.requiredOption("-f, --firm <firm-id>", "Specify the firm to be used", firmIdDefault)
.option("-h, --handle <handle>", "Specify the reconciliation to be used (mandatory)")
.option("-at, --account-template <name>", "Specify the account template to be used (mandatory)")
.option("-h, --handle <handle...>", "Specify one or more reconciliations to be used (mandatory)")
.option("-at, --account-template <name...>", "Specify one or more account templates to be used (mandatory)")
.option("-t, --test <test-name>", "Specify the name of the test to be run (optional)", "")
.option("--html-input", "Get a static html of the input-view of the template generated with the Liquid Test data (optional)", false)
.option("--html-preview", "Get a static html of the export-view of the template generated with the Liquid Test data (optional)", false)
.option("--preview-only", "Skip the checking of the results of the Liquid Test in case you only want to generate a preview template (optional)", false)
.option("--status", "Only return the status of the test runs as PASSED/FAILED (optional)", false)
.option("-p, --pattern <pattern>", "Run all tests that match this pattern (optional)", "")

.action((options) => {
.action(async (options) => {
if (!options.handle && !options.accountTemplate) {
consola.error("You need to specify either a reconciliation handle or an account template");
process.exit(1);
Expand All @@ -476,15 +476,41 @@ program
const templateType = options.handle ? "reconciliationText" : "accountTemplate";
const templateName = options.handle ? options.handle : options.accountTemplate;
Comment on lines 476 to 477
Copy link

@coderabbitai coderabbitai bot Jan 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Missing mutual exclusivity check for handle and accountTemplate.

If both --handle and --account-template are provided, handle silently wins and accountTemplate is ignored. Other commands in this file use cliUtils.checkUniqueOption() to enforce mutual exclusivity (e.g., line 721 in development-mode).

🔧 Proposed fix 
   const templateType = options.handle ? "reconciliationText" : "accountTemplate";
   const templateName = options.handle ? options.handle : options.accountTemplate;

+  if (options.handle && options.accountTemplate) {
+    consola.error("You cannot use both --handle and --account-template options at the same time");
+    process.exit(1);
+  }
+
   if (!templateName || templateName.length === 0) {

Alternatively, use the existing utility:

cliUtils.checkUniqueOption(["handle", "accountTemplate"], options);
🤖 Prompt for AI Agents 
In @bin/cli.js around lines 476 - 477, The code silently prefers options.handle
over options.accountTemplate when both are provided; add a mutual-exclusivity
check before computing templateType/templateName by calling
cliUtils.checkUniqueOption(["handle", "accountTemplate"], options) (or
equivalent) to validate that only one of these options is set, then keep the
existing logic that sets templateType and templateName based on options.handle;
ensure you reference options, templateType, and templateName in the same scope
so the check runs first.


if (!templateName || templateName.length === 0) {
consola.error("You need to provide at least one handle or account template name");
process.exit(1);
}

// Block multiple handles/templates without --status
if (templateName.length > 1 && !options.status) {
consola.error("Multiple handles/templates are only allowed when used with the --status flag");
process.exit(1);
}

if (options.status) {
liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern);
} else {
if (options.previewOnly && !options.htmlInput && !options.htmlPreview) {
consola.info(`When using "--preview-only" you need to specify at least one of the following options: "--html-input", "--html-preview"`);
process.exit(1);
}
liquidTestRunner.runTestsWithOutput(options.firm, templateType, templateName, options.test, options.previewOnly, options.htmlInput, options.htmlPreview, options.pattern);
// Status mode: allow multiple, pass array of template names
await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern);
return;
}
Comment on lines 490 to +494
Copy link

@coderabbitai coderabbitai bot Jan 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's look at the structure of the repository
git ls-files | grep -E "(cli|test)" | head -20

Repository: silverfin/silverfin-cli

Length of output: 659

🏁 Script executed:

# Find the CLI file and examine the status mode code
fd cli.js

Repository: silverfin/silverfin-cli

Length of output: 77

🏁 Script executed:

# Let's read the relevant section of bin/cli.js
wc -l bin/cli.js

Repository: silverfin/silverfin-cli

Length of output: 81

🏁 Script executed:

# Read around lines 495-499 with more context
sed -n '480,520p' bin/cli.js

Repository: silverfin/silverfin-cli

Length of output: 2096

🏁 Script executed:

# Find where runTestsStatusOnly is defined
rg "runTestsStatusOnly" --type js -B 2 -A 5

Repository: silverfin/silverfin-cli

Length of output: 1298

🏁 Script executed:

# Check the return type/value of runTestsStatusOnly
ast-grep --pattern 'runTestsStatusOnly($$$) { $$$ }'

Repository: silverfin/silverfin-cli

Length of output: 49

🏁 Script executed:

# Let's also check how other test modes handle exit codes
sed -n '450,550p' bin/cli.js

Repository: silverfin/silverfin-cli

Length of output: 4967

🏁 Script executed:

# Get the full implementation of runTestsStatusOnly
sed -n '1,100p' lib/liquidTestRunner.js | grep -A 50 "async function runTestsStatusOnly"

Repository: silverfin/silverfin-cli

Length of output: 49

🏁 Script executed:

# Search for return statements in runTestsStatusOnly function
rg -A 100 "async function runTestsStatusOnly" lib/liquidTestRunner.js | head -120

Repository: silverfin/silverfin-cli

Length of output: 1910

Status mode should propagate failure via exit code (required for CI/CD workflows).

runTestsStatusOnly() returns "PASSED" or "FAILED", but the CLI ignores this return value and always exits with code 0, breaking CI/CD integrations that depend on non-zero exit codes to detect test failures.

Proposed fix 
     if (options.status) {
       // Status mode: allow multiple, pass array of template names
-      await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern);
-      return;
+      const overallStatus = await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern);
+      process.exitCode = overallStatus === "PASSED" ? 0 : 1;
+      return;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (options.status) {
liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern);
} else {
if (options.previewOnly && !options.htmlInput && !options.htmlPreview) {
consola.info(`When using "--preview-only" you need to specify at least one of the following options: "--html-input", "--html-preview"`);
process.exit(1);
}
liquidTestRunner.runTestsWithOutput(options.firm, templateType, templateName, options.test, options.previewOnly, options.htmlInput, options.htmlPreview, options.pattern);
// Status mode: allow multiple, pass array of template names
await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern);
return;
}
if (options.status) {
// Status mode: allow multiple, pass array of template names
const overallStatus = await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern);
process.exitCode = overallStatus === "PASSED" ? 0 : 1;
return;
}
🤖 Prompt for AI Agents 
In @bin/cli.js around lines 495 - 499, The CLI ignores the return value of
liquidTestRunner.runTestsStatusOnly in the options.status branch causing tests
to always exit 0; capture the result of liquidTestRunner.runTestsStatusOnly
(which returns "PASSED" or "FAILED") and call process.exit(1) when the result
indicates failure (e.g., result !== "PASSED"), otherwise exit 0 or simply
return; update the block handling options.status (where runTestsStatusOnly is
invoked) to check the returned status and propagate a non-zero exit code for
failures so CI/CD can detect test failures.


// Non-status mode: always run a single template, pass string handle/name
const singleTemplateName = templateName[0];

if (options.previewOnly && !options.htmlInput && !options.htmlPreview) {
consola.info(`When using "--preview-only" you need to specify at least one of the following options: "--html-input", "--html-preview"`);
process.exit(1);
}

await liquidTestRunner.runTestsWithOutput(
options.firm,
templateType,
singleTemplateName,
options.test,
options.previewOnly,
options.htmlInput,
options.htmlPreview,
options.pattern
);
});

// Create Liquid Test
Expand Down
63 changes: 44 additions & 19 deletions lib/liquidTestRunner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,33 +469,58 @@ async function runTestsWithOutput(firmId, templateType, handle, testName = "", p

// RETURN (AND LOG) ONLY PASSED OR FAILED
// CAN BE USED BY GITHUB ACTIONS
async function runTestsStatusOnly(firmId, templateType, handle, testName = "", pattern = "") {
async function runTestsStatusOnly(firmId, templateType, handles, testName = "", pattern = "") {
if (templateType !== "reconciliationText" && templateType !== "accountTemplate") {
consola.error(`Template type is missing or invalid`);
process.exit(1);
}
Comment on lines +472 to 476
Copy link

@coderabbitai coderabbitai bot Jan 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's examine the function implementation around lines 472-476
sed -n '472,530p' lib/liquidTestRunner.js | cat -n

Repository: silverfin/silverfin-cli

Length of output: 2354

🏁 Script executed:

#!/bin/bash
# Search for all calls to runTestsStatusOnly in the codebase
rg -n 'runTestsStatusOnly\s*\(' --type js -A 2

Repository: silverfin/silverfin-cli

Length of output: 579

Add input validation to guard against string being passed to handles parameter.

The function expects handles to be an array, but the only caller in bin/cli.js:492 passes templateName (a string). When a string is passed to .map(), it iterates over individual characters instead of array items, causing incorrect behavior.

Add defensive validation at the beginning of the function:

if (!Array.isArray(handles)) {
  consola.error('handles must be an array');
  process.exit(1);
}

Alternatively, update the caller to wrap templateName in an array: [templateName] or ensure it's always passed as an array.

🤖 Prompt for AI Agents 
In @lib/liquidTestRunner.js around lines 472 - 476, runTestsStatusOnly is
assuming handles is an array but can receive a string (which makes .map iterate
chars); add a defensive validation at the top of runTestsStatusOnly: check if
(!Array.isArray(handles)) then log an error via consola.error('handles must be
an array') and exit (process.exit(1)); alternatively, fix the caller that
invokes runTestsStatusOnly (the CLI invocation that passes templateName) to pass
an array (e.g., [templateName]) so handles is always an array.


let status = "FAILED";
const testResult = await runTests(firmId, templateType, handle, testName, false, "none", pattern);
const runSingleHandle = async (singleHandle) => {
let status = "FAILED";
const failedTestNames = [];
const testResult = await runTests(firmId, templateType, singleHandle, testName, false, "none", pattern);

if (!testResult) {
status = "FAILED";
consola.error(`Error running tests for ${singleHandle}`);
} else {
const testRun = testResult?.testRun;

if (testRun && testRun?.status === "completed") {
const errorsPresent = checkAllTestsErrorsPresent(testRun.tests);
if (errorsPresent === false) {
status = "PASSED";
} else {
// Extract failed test names
const testNames = Object.keys(testRun.tests).sort();
testNames.forEach((testName) => {
const testErrorsPresent = checkTestErrorsPresent(testName, testRun.tests);
if (testErrorsPresent) {
failedTestNames.push(testName);
}
});
}
}
}

if (!testResult) {
status = "PASSED";
consola.success(status);
return status;
}
if (status === "PASSED") {
consola.log(`${singleHandle}: ${status}`);
} else {
consola.log(`${singleHandle}: ${status}`);
// Display failed test names
failedTestNames.forEach((testName) => {
consola.log(` ${testName}: FAILED`);
});
}

const testRun = testResult?.testRun;
return { handle: singleHandle, status, failedTestNames };
};

if (testRun && testRun?.status === "completed") {
const errorsPresent = checkAllTestsErrorsPresent(testRun.tests);
if (errorsPresent === false) {
status = "PASSED";
consola.success(status);
return status;
}
}
consola.error(status);
return status;
const results = await Promise.all(handles.map(runSingleHandle));

const overallStatus = results.every((result) => result.status === "PASSED") ? "PASSED" : "FAILED";

return overallStatus;
}

module.exports = {
Expand Down
22 changes: 11 additions & 11 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "silverfin-cli",
"version": "1.51.0",
"version": "1.52.0",
Copy link

@coderabbitai coderabbitai bot Jan 12, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, find and read the package.json file
fd package.json -type f

Repository: silverfin/silverfin-cli

Length of output: 237

🏁 Script executed:

# Search for PR template files
fd -i pull_request_template -type f

Repository: silverfin/silverfin-cli

Length of output: 237

🏁 Script executed:

# Also check for common template locations
ls -la .github/ 2>/dev/null || echo "No .github directory"

Repository: silverfin/silverfin-cli

Length of output: 369

🏁 Script executed:

# Get git diff to see what actually changed in package.json
git diff HEAD -- package.json 2>/dev/null || echo "Git diff not available"

Repository: silverfin/silverfin-cli

Length of output: 49

🏁 Script executed:

# Read package.json
cat package.json | head -20

Repository: silverfin/silverfin-cli

Length of output: 585

🏁 Script executed:

# Read the PR template
cat .github/pull_request_template.md

Repository: silverfin/silverfin-cli

Length of output: 482

Confirm the version bump is intentional for this PR.

The PR template checklist includes "Skip bumping the CLI version", but package.json shows the version was updated to 1.52.0. Please clarify whether this version bump matches your release process.

🤖 Prompt for AI Agents 
In @package.json at line 3, The package.json "version" field was changed to
"1.52.0"—confirm whether this version bump is intentional per your release
process and the PR template checkbox "Skip bumping the CLI version"; if it was
unintentional, revert the change to the version field in package.json, otherwise
add a note in the PR description documenting that the bump is deliberate and
aligns with the release workflow.

"description": "Command line tool for Silverfin template development",
"main": "index.js",
"license": "MIT",
Expand Down