feat(ci): run liquid tests in parallel, grouped per firm and template type#27
Merged
Conversation
Replace the per-template sequential test loop with a two-phase approach: group changed reconciliations by their resolved firm, then run each firm's handles through a single `run-test --status` call that executes them concurrently (CLI Promise.all). Batches are capped at MAX_PARALLEL_TESTS (default 5) and run sequentially so the total number of in-flight tests against the live platform stays bounded, since the CLI has no client-side rate limiting. Also gate the reconciliation_texts check before firm resolution so shared-part changes (whose ".id" is a plain string) are skipped cleanly instead of aborting the step on a jq error, and drop the per-iteration CONFIG_JSON re-write (a no-op; token refresh is handled elsewhere).
In CI, consola prefixes every line with a '[log] ' tag and the test spinner interleaves ANSI/braille frames onto the same line, so the per-handle status is not at the start of a line. Normalise carriage returns and ANSI escapes and match '<handle>: PASSED/FAILED' on a left word boundary instead of anchoring to '^', which previously caused every handle to be reported as 'status could not be determined'.
WalkthroughThe workflow now detects changed template directories as JSON, batches template test runs by firm and template type, caps concurrent identifiers per invocation, and parses batched ChangesFirm-grouped batch test execution
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
.github/workflows/run_tests.yml (1)
209-218: 💤 Low valueHandle names with regex metacharacters may cause incorrect status matching.
The
HANDLEvariable is interpolated directly into the grep regex pattern without escaping. If a handle contains regex metacharacters (e.g.,.,+,*), the pattern could match unintended strings or fail to match correctly.For typical alphanumeric handles this is unlikely to be an issue, but for robustness you could escape the handle:
♻️ Optional: escape handle for regex
for HANDLE in "${BATCH[@]}"; do + ESCAPED_HANDLE=$(printf '%s' "${HANDLE}" | sed 's/[.[\*^$()+?{|\\]/\\&/g') - if printf '%s\n' "${CLEAN_OUTPUT}" | grep -qE "(^|[^[:alnum:]_])${HANDLE}: PASSED"; then + if printf '%s\n' "${CLEAN_OUTPUT}" | grep -qE "(^|[^[:alnum:]_])${ESCAPED_HANDLE}: PASSED"; then echo "${HANDLE}: passed" - elif printf '%s\n' "${CLEAN_OUTPUT}" | grep -qE "(^|[^[:alnum:]_])${HANDLE}: FAILED"; then + elif printf '%s\n' "${CLEAN_OUTPUT}" | grep -qE "(^|[^[:alnum:]_])${ESCAPED_HANDLE}: FAILED"; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/run_tests.yml around lines 209 - 218, The HANDLE variable is being used directly in the grep regex patterns without escaping special regex metacharacters, which could cause incorrect pattern matching if the handle contains characters like dot, plus, asterisk, or other regex special characters. Before using HANDLE in the grep patterns on the lines that check for PASSED and FAILED status, escape the variable by replacing regex metacharacters with their escaped equivalents. You can do this by creating an escaped version of HANDLE using sed or a similar method to replace characters like dot, asterisk, plus, question mark, brackets, pipes, and parentheses with their backslash-escaped versions, then use this escaped variable in the grep regex patterns instead of the raw HANDLE variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/run_tests.yml:
- Line 107: The unquoted `${{ env.CHANGED_TEMPLATES }}` variable expansion in
the for loop is vulnerable to command injection if directory names contain shell
metacharacters. Quote the variable expansion by wrapping it as `"${{
env.CHANGED_TEMPLATES }}"` in the for loop statement to prevent unintended shell
interpretation, or alternatively refactor to use a safer iteration method such
as passing the templates as a JSON array and parsing them with jq to avoid
direct bash expansion of potentially malicious directory names.
---
Nitpick comments:
In @.github/workflows/run_tests.yml:
- Around line 209-218: The HANDLE variable is being used directly in the grep
regex patterns without escaping special regex metacharacters, which could cause
incorrect pattern matching if the handle contains characters like dot, plus,
asterisk, or other regex special characters. Before using HANDLE in the grep
patterns on the lines that check for PASSED and FAILED status, escape the
variable by replacing regex metacharacters with their escaped equivalents. You
can do this by creating an escaped version of HANDLE using sed or a similar
method to replace characters like dot, asterisk, plus, question mark, brackets,
pipes, and parentheses with their backslash-escaped versions, then use this
escaped variable in the grep regex patterns instead of the raw HANDLE variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e87b6978-d9be-4112-9bf8-bae3ac6a0967
📒 Files selected for processing (1)
.github/workflows/run_tests.yml
The CLI runs liquid tests for both reconciliation_texts (--handle) and account_templates (--account-template). Group changed templates by '<firm>|<type>' so each bucket maps to one run-test call (the CLI takes a single firm and a single type per call) and pick the flag per bucket. Account template folder names contain spaces and characters like & ( ) ', so make the whole path space-safe: emit the changed-files list as JSON, hand it between steps/jobs as a JSON array, quote the config reads, and parse the run-test output by exact template name instead of a regex. Shared parts (and anything without liquid tests) are still skipped.
With json: true the changed-files action defaults to escape_json: true, which double-escapes the quotes ([\"a\"] instead of ["a"]). jq then fails to parse it, no templates are detected, and the test job is skipped. escape_json: false emits plain JSON that jq can read.
Benjvandam
changed the title
Collaborator
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/run_tests.yml:
- Line 231: The grep pattern on this line uses a strict `$` anchor immediately
after `(PASSED|FAILED)` which fails to match lines with trailing whitespace or
invisible characters. Modify the pattern to allow optional trailing whitespace
by inserting `[[:space:]]*` between the `(PASSED|FAILED)` group and the `$`
anchor. This will match test result lines regardless of whether they have
trailing spaces or other whitespace characters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37dcc4f2-491e-4166-9276-09aa873dc355
📒 Files selected for processing (1)
.github/workflows/run_tests.yml
michieldegezelle
left a comment
michieldegezelle
left a comment
Collaborator
There was a problem hiding this comment.
🟡 Minor / question — the per-template re-fetch of CONFIG_JSON from secrets was dropped; was that intentional?
The old test loop re-ran echo '${{ secrets.CONFIG_JSON }}' > $HOME/.silverfin/config.json before each reconciliation, with the comment "fetch the newest version of the tokens from the secrets, in case they were updated by the initiation of a concurrent workflow." This PR removes that and relies on the single load in the Load Silverfin config file from secrets step at job start.
Benign in the common case (the CLI refreshes the access token locally during the run). The only scenario it changes: if a concurrent workflow rotates the OAuth refresh token mid-run, this job can no longer pick up the new value and its remaining batches would fail to authenticate. Given the move to batched parallel runs the old per-template re-read wouldn't have helped mid-batch anyway, so this is likely fine — just flagging the dropped safeguard for a conscious sign-off, since it isn't mentioned in the PR description.
michieldegezelle
left a comment
michieldegezelle
left a comment
Collaborator
There was a problem hiding this comment.
Potential improvements — silverfin-cli follow-ups (separate repo, not this PR)
Two CLI changes would let this workflow drop its output-scrubbing and stop mis-reporting:
- Spinner in non-interactive CI.
lib/cli/spinner.jsspin()starts unconditionally; in CI it animates viareadline.clearLine/cursor escapes on a 120 ms interval — the interleaved noise this workflow strips withtr '\r' '\n'+ an ANSIsed. One-line guard makes it a no-op off-TTY:spin(text = "…") { if (this.running || !process.stdout.isTTY) return; /* … */ }
- Distinct status for test-less templates.
runTestsStatusOnlyreturnsFAILEDwhenbuildTestParamsbails on an empty YAML (there are no tests stored in the YAML file). EmittingSKIPPED/NO_TESTSinstead would let every consumer treat "no tests yet" as not-a-failure (see the inline empty-YAML note).
…s noise Four fixes from PR review: - safe_output: false on changed-files. The default backslash-escapes shell metacharacters (& ( ) ...) inside the JSON values, which is invalid JSON, so jq silently emptied the template list and every test was skipped while CI stayed green. Safe to disable: the list is passed via env and parsed with jq, never eval'd by the shell. - Fail loudly when the changed-files output is not valid JSON, so a parse problem can never silently skip all tests again. - Disable git core.quotepath before changed-files, so paths with non-ASCII characters (e.g. a curly apostrophe in an account template name) are not octal-escaped and silently dropped by the glob filter. - Skip templates whose test YAML is missing or has no real content instead of letting the CLI report them as FAILED - a template without tests should not fail CI. - Drop shared_parts from the detection pattern: it has no liquid tests, so a shared-parts-only PR now skips the test job entirely instead of spinning it up just to skip every template.
…whitespace
Logging improvements from PR review:
- Queue phase prints one line per template ('+ <name> -> firm <id>') and
keeps only the firm-fallback warnings; the 'Checking ...' and happy-path
'Using ... firm ID' lines are dropped.
- Each parallel batch is wrapped in a ::group:: log group, so the raw CLI
output (spinner frames included) folds away unless opened.
- A flat results summary at the end lists every template with its status
and firm, plus a failed/passed count.
- The status parser now tolerates trailing whitespace on the CLI's
'PASSED/FAILED' lines (grep anchor relaxed and the parsed status
trimmed), so an output formatting quirk can only cause a false failure,
never a wrong verdict.
The previous 'git config --global core.quotepath false' step was ignored: tj-actions/changed-files sets quotepath from its own input (default true) and overrides the global config, so a folder with a non-ASCII character (e.g. a curly apostrophe) was still octal-escaped and dropped. Confirmed live: the path appeared 0 times in all_changed_files. Set quotepath: false as an action input instead.
Contributor
Author
|
Thanks for the thorough review @michieldegezelle — all points addressed. Summary:
Dropped silverfin-cli follow-ups (separate repo) captured in the description: spinner no-op off-TTY, and a distinct Re-validated end to end on be_market run 28644775530: 15 changed templates across 4 firms — the |
michieldegezelle
approved these changes
Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Two things:
The silverfin-cli can already run several templates in a single
run-test --statuscall (it runs them at the same time viaPromise.all). The workflow wasn't using that - it called the CLI once per template in a bash loop. Now we group the changed templates and run each group in one call.Account templates (new)
Before this PR only
reconciliation_textswere tested. The CLI can also testaccount_templates(--account-template), and a repo likebe_markethas 20 of them that were never run in CI.This was confirmed on the current
@mainworkflow: see silverfin/be_market#2950, where a PR that changes only an account template skips the test job entirely - the template isn't even detected (the old filter only matchesreconciliation_texts|shared_parts, and the old test step only ranreconciliation_texts).This PR tests them the same way as reconciliations. The CLI takes one firm and one template type per call, so templates are grouped by
<firm>|<type>and each group is one call. Account template folder names contain spaces and characters like& ( ) ', so the whole path is made space-safe: the changed-files list is passed around as JSON, config reads are quoted, and the output is matched by exact template name instead of a regex.Bugs we found and fixed along the way
.idas a plain string (not a firm -> id map), sojqerrored and aborted the step. We now decide the template type first and skip anything without liquid tests.&,(or)silently skipped all tests.tj-actions/changed-filesdefaults (escape_json,safe_output) backslash-escape the JSON values, which makes them invalid JSON -jqfailed, the template list came out empty, and the job skipped while staying green. Fixed withescape_json: false+safe_output: false(safe: the list is parsed withjq, never eval'd by the shell), and the filter now fails loudly if the JSON can't be parsed, so this class of bug can't silently skip tests again....creditnota's(curly apostrophe) was emitted octal-escaped and never matched the changed-files glob - so the template was never tested. Fixed with the changed-files action'squotepath: falseinput (its own default overrides git's globalcore.quotepath, so the input is the knob that matters).Details
test_firm_id->SF_TEST_FIRM_ID-> first key of.id).MAX_PARALLEL_TESTS(default 10), batches run one after another, so no more than that many tests run at once (the CLI has no rate limiting).run-test --statusexits 0 even when a test fails, so we read the<name>: PASSED/FAILEDlines from stdout and fail the job if any failed. In CI consola prefixes those lines with[log]and the spinner interleaves frames, so the output is normalised before parsing (trailing whitespace is tolerated too).::group::log block, and the job ends with a flat summary: every template with its status and firm, plus a failed/passed count.Dropped: per-template re-read of CONFIG_JSON (intentional)
The old loop re-wrote
$HOME/.silverfin/config.jsonfrom the secret before every template, "in case the tokens were updated by a concurrent workflow". That was a no-op: secrets are resolved when the job starts, so it kept writing the same value. It also wouldn't help mid-batch now that templates run in parallel. The config is loaded once at job start; keeping tokens fresh is handled separately (scheduled refresh).How to test (for reviewers)
The reusable workflow can't run on its own - a template repo has to call it. To test this branch end to end:
be_market), point the ref at this branch in.github/workflows/run_tests.yml:uses: silverfin/bso_github_actions/.github/workflows/run_tests.yml@run_tests_in_parallel{% comment %} test {% endcomment %}line in theirmain.liquid. Touch both reconciliations and account templates to cover both types.no-test-requiredlabel so the unrelatedcheck_testscheck stays green.firm 542 · reconciliationText · 3 in parallel) and the job ends with a per-template summary.Before / after (live runs on be_market)
Before - current
@main: silverfin/be_market#2950 - changes only an account template. Thetest-templatesjob is skipped; account templates are not detected or tested today.After - this branch: silverfin/be_market#2949 - tests both types at once, grouped per firm, including the hard names (
Leningen & Leasings (42), the non-ASCIIcreditnota'sfolder) and an empty-YAML template that is now skipped instead of failing.Follow-ups (not in this PR)
SKIPPED/NO_TESTSstatus for templates with an empty test YAML instead ofFAILED;Promise.all, and isolating a single hard error instead ofprocess.exit(1)killing the whole batch.