Run multiple handles in parallel #233
Conversation
AgustinSilverfin
left a comment
AgustinSilverfin
left a comment
There was a problem hiding this comment.
I haven't tested it myself but in general it looks good 👍
|
Caution Review failedThe pull request is closed. WalkthroughThe CLI run-test command now accepts multiple template handles/names (variadic), enforces input normalization/validation, and restricts multi-handle use to --status. The test runner Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/liquidTestRunner.js (1)
457-465: Consider addressing previous review feedback on formatting and log level.Based on past review comments:
- Line 463 uses
consola.logfor failed test names, but a reviewer suggested usingconsola.errorfor consistency since these are failures.- A reviewer suggested formatting failed tests inline (e.g.,
handle_2: FAILED [unit_test_1, unit_test_2]) rather than on separate indented lines, which would make the output easier to parse programmatically.Option 1: Use consola.error for failures
} else { - consola.log(`${singleHandle}: ${status}`); + consola.error(`${singleHandle}: ${status}`); // Display failed test names failedTestNames.forEach((testName) => { - consola.log(` ${testName}: FAILED`); + consola.error(` ${testName}: FAILED`); }); }Option 2: Inline formatting for easier parsing
if (status === "PASSED") { consola.log(`${singleHandle}: ${status}`); } else { - consola.log(`${singleHandle}: ${status}`); - // Display failed test names - failedTestNames.forEach((testName) => { - consola.log(` ${testName}: FAILED`); - }); + const failedList = failedTestNames.length > 0 ? ` [${failedTestNames.join(', ')}]` : ''; + consola.error(`${singleHandle}: ${status}${failedList}`); }Based on learnings, addressing reviewer feedback is important for collaboration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/cli.js(1 hunks)lib/liquidTestRunner.js(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/liquidTestRunner.js (1)
bin/cli.js (2)
templateType(468-468)testName(502-502)
bin/cli.js (3)
lib/liquidTestRunner.js (1)
options(21-21)lib/cli/utils.js (1)
options(89-89)lib/cli/devMode.js (1)
liquidTestRunner(4-4)
🔇 Additional comments (4)
package.json (1)
3-3: LGTM! Version bump aligns with the new multi-handle feature.The version update to 1.48.0 is appropriate for this new functionality.
bin/cli.js (3)
454-455: LGTM! Variadic options correctly enable multiple handles/templates.The use of
<handle...>and<name...>with Commander.js properly enables multiple values while maintaining backward compatibility for single values.
476-480: Status mode correctly passes array to parallel processor.The implementation properly awaits
runTestsStatusOnlywith the fulltemplateNamearray, enabling parallel execution for multiple handles.
482-490: Non-status mode correctly extracts single template.The extraction of
singleTemplateNamefromtemplateName[0]is appropriate for backward compatibility with single-template execution, and the preview validation logic is preserved.
lib/liquidTestRunner.js
Outdated
| // RETURN (AND LOG) ONLY PASSED OR FAILED | ||
| // CAN BE USED BY GITHUB ACTIONS | ||
| async function runTestsStatusOnly(firmId, templateType, handle, testName = "") { | ||
| async function runTestsStatusOnly(firmId, templateType, handles, testName = "") { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add input validation for handles parameter.
The function now accepts handles as an array, but there's no validation to ensure it's actually an array or that it's not empty. If an invalid value is passed, it could cause runtime errors.
Apply this diff to add validation:
async function runTestsStatusOnly(firmId, templateType, handles, testName = "") {
+ if (!Array.isArray(handles) || handles.length === 0) {
+ consola.error(`At least one handle must be provided`);
+ process.exit(1);
+ }
+
if (templateType !== "reconciliationText" && templateType !== "accountTemplate") {
consola.error(`Template type is missing or invalid`);
process.exit(1);
}🤖 Prompt for AI Agents
In lib/liquidTestRunner.js around line 424, the runTestsStatusOnly function
accepts a handles parameter but lacks validation; add a check at the top that
ensures handles is an Array and not empty (e.g., if (!Array.isArray(handles) ||
handles.length === 0) throw new TypeError("handles must be a non-empty array")
or return a rejected Promise), so invalid inputs are rejected early with a clear
error; update any callers or tests if they rely on implicit behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @lib/liquidTestRunner.js:
- Around line 483-484: The code currently treats any falsy testResult as PASSED;
instead, update runTests and its caller to distinguish three cases: if
buildTestParams returns false (explicit "no tests" from an empty YAML) propagate
and set status = "SKIPPED" (or "NO_TESTS"); if runTests returns undefined
(indicates a config/file error) set status = "ERROR" and emit a clear error log;
otherwise handle the normal truthy testResult as before. Specifically, ensure
buildTestParams returns false for empty YAML and runTests forwards that false
rather than undefined, and change the caller logic around the status variable
(referencing runTests, buildTestParams, and status) to branch on === false, ===
undefined, and truthy results, logging errors on undefined.
🧹 Nitpick comments (3)
lib/liquidTestRunner.js (2)
505-512: Minor: Redundant conditional branches for logging.Both branches log
${singleHandle}: ${status}identically. Consider simplifying:Suggested simplification
- if (status === "PASSED") { - consola.log(`${singleHandle}: ${status}`); - } else { - consola.log(`${singleHandle}: ${status}`); - // Display failed test names - failedTestNames.forEach((testName) => { - consola.log(` ${testName}: FAILED`); - }); - } + consola.log(`${singleHandle}: ${status}`); + if (status !== "PASSED") { + // Display failed test names + failedTestNames.forEach((testName) => { + consola.log(` ${testName}: FAILED`); + }); + }
518-522: ConsiderPromise.allSettledfor more resilient parallel execution.Using
Promise.allmeans if any single handle throws an unhandled exception, the entire batch fails and results from other handles are lost. For CI/CD use cases, continuing to process remaining handles and reporting all failures would be more useful.Suggested improvement using Promise.allSettled
- const results = await Promise.all(handles.map(runSingleHandle)); + const settledResults = await Promise.allSettled(handles.map(runSingleHandle)); + const results = settledResults.map((result, index) => { + if (result.status === 'fulfilled') { + return result.value; + } + // Handle rejected promises + consola.error(`${handles[index]}: Error - ${result.reason?.message || 'Unknown error'}`); + return { handle: handles[index], status: 'FAILED', failedTestNames: [] }; + });bin/cli.js (1)
495-499: Consider setting exit code based on test status for CI/CD integration.The
runTestsStatusOnlyreturns"PASSED"or"FAILED", but the result is not used to set the process exit code. For GitHub Actions integration (mentioned in PR objectives), a non-zero exit code on failure would enable proper CI/CD workflow control.Suggested fix
if (options.status) { // Status mode: allow multiple, pass array of template names - await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern); + const overallStatus = await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern); + if (overallStatus === "FAILED") { + process.exit(1); + } return; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bin/cli.jslib/liquidTestRunner.js
🧰 Additional context used
🧬 Code graph analysis (2)
bin/cli.js (1)
lib/cli/devMode.js (3)
liquidTestRunner(4-4)liquidTestRunner(43-46)liquidTestRunner(35-38)
lib/liquidTestRunner.js (2)
bin/cli.js (2)
templateType(476-476)testName(521-521)lib/cli/devMode.js (1)
liquidTestRunner(43-46)
🔇 Additional comments (5)
lib/liquidTestRunner.js (1)
472-476: LGTM!The function signature update to accept
handles(array) is appropriate for the multi-handle support. The template type validation is preserved correctly.bin/cli.js (4)
456-457: LGTM!The variadic options (
<handle...>and<name...>) are correctly defined using Commander.js syntax to accept multiple values.
479-482: LGTM!The pipe-separation logic correctly handles the case where shell/GitHub Actions may pass multiple values as a single pipe-delimited string. The implementation properly trims whitespace and filters empty segments.
484-493: LGTM!The validation correctly enforces that at least one template is provided and that multiple handles/templates require the
--statusflag, aligning with the PR objectives.
501-509: LGTM!The non-status mode correctly extracts a single template name from the array and maintains backward compatibility with the existing
runTestsWithOutputsignature. The async/await pattern is properly applied.
| if (!testResult) { | ||
| status = "PASSED"; |
There was a problem hiding this comment.
Treating missing test results as PASSED may mask configuration errors.
When runTests returns falsy (undefined), this could indicate:
- Config file not found (error)
- Test file not found (error)
- Empty YAML file (info)
Setting status = "PASSED" for all these cases will silently pass handles with missing or misconfigured tests, potentially masking real issues in CI/CD pipelines.
Suggested fix: distinguish error cases from "no tests" cases
const runSingleHandle = async (singleHandle) => {
let status = "FAILED";
const failedTestNames = [];
const testResult = await runTests(firmId, templateType, singleHandle, testName, false, "none", pattern);
- if (!testResult) {
- status = "PASSED";
+ if (testResult === false) {
+ // Empty YAML file - no tests to run, consider as passed
+ status = "PASSED";
+ } else if (!testResult) {
+ // Config or test file not found - keep as FAILED
+ status = "FAILED";
} else {Note: This requires runTests to propagate the false return from buildTestParams for the empty YAML case, rather than returning undefined for all falsy inputs.
🤖 Prompt for AI Agents
In @lib/liquidTestRunner.js around lines 483 - 484, The code currently treats
any falsy testResult as PASSED; instead, update runTests and its caller to
distinguish three cases: if buildTestParams returns false (explicit "no tests"
from an empty YAML) propagate and set status = "SKIPPED" (or "NO_TESTS"); if
runTests returns undefined (indicates a config/file error) set status = "ERROR"
and emit a clear error log; otherwise handle the normal truthy testResult as
before. Specifically, ensure buildTestParams returns false for empty YAML and
runTests forwards that false rather than undefined, and change the caller logic
around the status variable (referencing runTests, buildTestParams, and status)
to branch on === false, === undefined, and truthy results, logging errors on
undefined.
This reverts commit ec3e88a.
Fixes # (link to the corresponding issue if applicable)
Description
Multiple handles/AT's can now be passed to the run-test command:
e.g.
silverfin run-test -f 200619 -h be_pit_wages_salaries_part_2 be_pit_wages_salaries --statusor
silverfin run-test -f 14400 -h 'Dubieuze debiteuren' 'Erelonen' --statusNote that I only allowed/activated it for the status flag since this will cover our usecase for now (Github actions).
This can be expanded later on if required.
Had some issues with the handle flag being rendered as an array in case there was only 1 element/handle, but some checks solved this issue.
Reporting is done per handle (and if differences, we show the units/tests as well).
QQ: in general, this is quicker than running them one by one, but in some cases, the API seems to be very slow. Is there a way in which we can avoid that?
Testing Instructions
Steps:
Author Checklist
Reviewer Checklist