Enable liquid batch testing #229
Conversation
delphine-demeulenaere
force-pushed
the
liquid-batch-testing
branch
from
76fd7c2 to
baa5328
Compare
AgustinSilverfin
requested review from
michieldegezelle
and removed request for
AgustinSilverfin
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bin/cli.js (1)
449-485: CLI UX mismatch: PR saysrun-test -b, code implementsrun-test -p(and dev-mode uses-b).Given the PR objective/example, I’d align the short flag across commands (or update docs). If you want
-beverywhere:- .option("-p, --pattern <pattern>", "Run all tests that match this pattern (optional)", "") + .option("-b, --pattern <pattern>", "Run all tests that match this pattern (optional)", "")Also: consider making the action
asyncandawaitthe runner calls to avoid unhandled rejections / edge-case early exit:- .action((options) => { + .action(async (options) => { @@ - liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern); + await liquidTestRunner.runTestsStatusOnly(options.firm, templateType, templateName, options.test, options.pattern); @@ - liquidTestRunner.runTestsWithOutput(options.firm, templateType, templateName, options.test, options.previewOnly, options.htmlInput, options.htmlPreview, options.pattern); + await liquidTestRunner.runTestsWithOutput(options.firm, templateType, templateName, options.test, options.previewOnly, options.htmlInput, options.htmlPreview, options.pattern); } });lib/liquidTestRunner.js (1)
103-193: BlocktestName+patternat the library boundary (not just CLI).If a non-CLI caller passes both, you’ll set
tests: filteredContentbut computetest_linefrom the unfilteredtestIndexes, which can point to the wrong line. Add a guard:function buildTestParams(firmId, templateType, handle, testName = "", renderMode, pattern = "") { + if (testName && pattern) { + consola.error("You cannot use both testName and pattern at the same time"); + process.exit(1); + } let relativePath = `./reconciliation_texts/${handle}`;
🧹 Nitpick comments (3)
lib/cli/devMode.js (1)
34-47: Guard against overlapping runs + unhandled rejections in chokidar callbacks.
chokidar.watch(...).on("change", async () => { await ... })can (a) trigger multiple times per save and (b) drop errors as unhandled promise rejections. Consider debouncing and wrapping the body intry/catch(or.catch(...)) and optionally serializing runs.lib/liquidTestRunner.js (2)
34-101:findTestRows()-based indexing is too loose for reliable filtering/line-adjustment.
element.includes(testName)can match values/comments and skewindexes→ wrong slicing and wronglineAdjustments. Prefer matching actual YAML top-level keys (e.g.,^\s*<escapedName>\s*:).
461-555: Pattern propagation throughrunTests*is good; watchrunTestsStatusOnly“undefined => PASSED” behavior.Because
runTests(...)returnsundefinedon various early-exit paths,runTestsStatusOnlycurrently treats that as PASSED. If that’s only intended for the “empty YAML” case, consider returning a sentinel/reason (orfalse) explicitly fromrunTests/buildTestParamsso status-only can distinguish “no tests” from “couldn’t run”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/cli.js(2 hunks)lib/cli/devMode.js(2 hunks)lib/liquidTestRunner.js(10 hunks)
🔇 Additional comments (4)
lib/cli/devMode.js (1)
9-22:watchLiquidTest(..., pattern = "")plumbing looks correct.Signature + JSDoc update is consistent, and defaulting
patternto""keeps backward behavior.bin/cli.js (1)
675-710:development-mode --patternis wired correctly; consider awaiting + enforce “pattern requires handle/account-template”.Plumbing into
devMode.watchLiquidTest(..., options.pattern)is consistent. Two follow-ups:
- Make the action
asyncandawait devMode.watchLiquidTest(...)to surface errors deterministically.- Verify
cliUtils.checkUniqueOption(["handle","updateTemplates","accountTemplate"], options)also enforces at least one of these; otherwise users could pass--patternwithout--handle/--account-templatedespite the help text.lib/liquidTestRunner.js (2)
220-270: Line adjustment hook inlistErrors(...)looks consistent.Passing
lineAdjustmentthrough and applying it only whenline_numberis numeric is clean.
308-382: Per-test line adjustment application is consistent; please validate API line-number semantics.Using
const lineAdjustment = lineAdjustments[testName] || 0;and applying it to reconciled/results/rollforwards is coherent ifline_numberis relative to the (possibly filtered) YAML file. Worth double-checking with a failing test in filtered mode to ensure numbers line up.
delphine-demeulenaere
force-pushed
the
liquid-batch-testing
branch
from
e944064 to
fdc8386
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/liquidTestRunner.js (2)
20-32:findTestRows()key matching is unsafe (substring collisions) → can select wrong tests / wrong line numbers.
findTestRows()useselement.includes(testName)(Line 28). This will mis-detect YAML key lines when:
- one test name is a substring of another (
unit_1matchesunit_10:),- the test name appears in a value or comment before the key,
- quoting/formatting varies.
That breaks
filterTestsByPattern()segmentation (Line 44-79),test_linecomputation (Line 193), andlineAdjustments(Line 87-93).Suggested fix: match YAML keys at line start (optionally quoted) instead of substring search.
function findTestRows(testContent) { const options = { maxAliasCount: 10000 }; const testYAML = yaml.parse(testContent, options); const testNames = Object.keys(testYAML); const testRows = testContent.split("\n"); const indexes = {}; + + const escapeRegExp = (s) => s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); testNames.forEach((testName) => { - const index = testRows.findIndex((element) => element.includes(testName)); + const keyRe = new RegExp(`^\\s*["']?${escapeRegExp(testName)}["']?\\s*:\\s*(#.*)?$`); + const index = testRows.findIndex((line) => keyRe.test(line)); indexes[testName] = index; }); return indexes; }(If you need to support
testNamefollowed by inline YAML (key: { ... }), the regex still works.)Also applies to: 34-101
103-196: AddtestNameXORpatternguard tobuildTestParams()While the CLI prevents using both
--testand--patterntogether, the constraint should be enforced at the function level rather than relying solely on CLI validation. IfbuildTestParams()is called with bothtestNameandpattern,finalTestsgets filtered by pattern (lines 133–145) buttestParams.test_linereferences the originaltestIndexes(line 193), causing a mismatch where the line number may target the wrong test.Add this guard at the start of
buildTestParams():function buildTestParams(firmId, templateType, handle, testName = "", renderMode, pattern = "") { + if (testName && pattern) { + consola.error("You cannot use both testName and pattern at the same time"); + process.exit(1); + } let relativePath = `./reconciliation_texts/${handle}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
bin/cli.js(2 hunks)lib/cli/devMode.js(2 hunks)lib/liquidTestRunner.js(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/cli/devMode.js
🔇 Additional comments (3)
lib/liquidTestRunner.js (2)
34-101: Line-adjustment plumbing looks consistent; please validate against APIline_numbersemantics.The
lineAdjustments[testName] = originalIndex - filteredIndexapproach (Line 87-93) and addinglineAdjustmentwhen printingline_number(Line 229, 366, 374, 379) is coherent if the API returns line numbers relative to the submitted YAML file.Request verification: run 1–2 failing expectations with and without
--patternand confirm the printed line points to the correct line in the original YAML.Also applies to: 135-156, 223-273, 311-385
135-156: Nice UX improvement: matched tests are clearly listed.bin/cli.js (1)
462-486:run-testpattern flag + mutual exclusivity check look good.
| .option("-b, --pattern <pattern>", `Run all tests that match this pattern (optional). It has to be used together with "--handle" or "--account-template"`, "") | ||
| .action((options) => { | ||
| cliUtils.checkDefaultFirm(options.firm, firmIdDefault); | ||
| cliUtils.checkUniqueOption(["handle", "updateTemplates", "accountTemplate"], options); | ||
|
|
||
| if (options.test && options.pattern) { | ||
| consola.error("You cannot use both --test and --pattern options at the same time"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (options.updateTemplates && !options.yes) { | ||
| cliUtils.promptConfirmation(); | ||
| } | ||
|
|
||
| if (options.accountTemplate) { | ||
| devMode.watchLiquidTest(options.firm, options.accountTemplate, options.test, options.html, "accountTemplate"); | ||
| devMode.watchLiquidTest(options.firm, options.accountTemplate, options.test, options.html, "accountTemplate", options.pattern); | ||
| } | ||
|
|
||
| if (options.handle) { | ||
| devMode.watchLiquidTest(options.firm, options.handle, options.test, options.html, "reconciliationText"); | ||
| devMode.watchLiquidTest(options.firm, options.handle, options.test, options.html, "reconciliationText", options.pattern); | ||
| } |
There was a problem hiding this comment.
Consider consistent short option for --pattern across commands (-p vs -b).
run-test uses -p (Line 462) but development-mode uses -b (Line 691) for the same concept. If there’s no conflict, aligning them reduces muscle-memory friction; otherwise, a short note in help text/docs would help.
🤖 Prompt for AI Agents
In bin/cli.js around lines 691 to 711 the short flag for --pattern is -b which
is inconsistent with the run-test command using -p (line ~462); change the short
option to -p to align both commands if no other option currently uses -p, and if
-p is already taken either choose a consistent alternative across commands or
add an explicit note in the command help text documenting the difference; after
changing, run a quick scan of the CLI options to ensure no flag collisions and
update any usage/help strings or tests that reference -b to use -p.
michieldegezelle
left a comment
michieldegezelle
left a comment
There was a problem hiding this comment.
Looks good! 1 remaining comment (but approved already). Don't forget to update the CLI version number still
| .option("-t, --test <test-name>", `Specify the name of the test to be run (optional). It has to be used together with "--handle"`, "") | ||
| .option("--html", `Get a html file of the template's input-view generated with the Liquid Test information (optional). It has to be used together with "--handle"`, false) | ||
| .option("--yes", "Skip the prompt confirmation (optional)") | ||
| .option("-b, --pattern <pattern>", `Run all tests that match this pattern (optional). It has to be used together with "--handle" or "--account-template"`, "") |
There was a problem hiding this comment.
Can you change the -b into -p?
Description
While there is currently the ability to run single tests and all tests there is no functionality to run a batch of related tests on a template.
I added an extra option to the run-test command to run all tests which contain a common string.
Overall, I understand the changes made and the steps taken to implement this fix. I should note, however, that most of the actual code modifications were applied via cursor. That said, testing indicates that these changes achieve the desired results.
Testing Instructions
silverfin run-test -b "string pattern" -h <template name>f.e.:
silverfin run-test -b "unit_3" -h be_pit_legal_structuresAuthor Checklist
Reviewer Checklist