Skip to content

feat(ci): run liquid tests in parallel, grouped per firm and template type #27

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

Open
Benjvandam wants to merge 5 commits into
base: main
Choose a base branch
from
+115 −49
Open

feat(ci): run liquid tests in parallel, grouped per firm and template type #27

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aa4eb23
feat(ci): run liquid tests in parallel grouped per firm
Benjvandam Jun 16, 2026
ddf487e
fix(ci): parse run-test output with [log] prefix and spinner noise
Benjvandam Jun 17, 2026
b2ddd3c
feat(ci): also test account templates, grouped per firm and type
Benjvandam Jun 17, 2026
21e646a
fix(ci): disable escape_json so the changed-files JSON is parseable
Benjvandam Jun 17, 2026
cbdd219
chore(ci): raise MAX_PARALLEL_TESTS from 5 to 10
Benjvandam Jun 17, 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
164 changes: 115 additions & 49 deletions .github/workflows/run_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,39 +31,33 @@ jobs:
since_last_remote_commit: false
dir_names: false
base_sha: 'main'

@michieldegezelle michieldegezelle Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟠 Major — a template whose folder name has a non-ASCII character is silently dropped by changed-files (untested, green CI).

be_market#2949 modifies account_templates/Op te stellen … creditnota’s /main.liquid, but that path never appears in all_changed_files (run 27957849400) — so the template was never queued or tested, while the run stayed green. Every other changed path that run was pure ASCII and came through fine.

Cause: the folder name contains a curly apostrophe ’ (U+2019). With git's default core.quotepath=true, git emits the path double-quoted and octal-escaped:

"account_templates/…creditnota\342\200\231s /main.liquid"

changed-files doesn't reconcile that quoted form against the **/**.{liquid,…} glob, so it's filtered out.

Fix — disable quotepath before the changed-files step so non-ASCII paths emit raw:

      - run: git config --global core.quotepath false

Defense-in-depth: that folder also ends in a trailing space (…creditnota’s before the /), which is fragile across the whole pipeline — worth renaming to drop the trailing space (and ideally the curly apostrophe) regardless.

# Emit the file list as a JSON array so paths containing spaces (e.g. account
# template names) survive the hand-off between steps and jobs intact.
# escape_json: false keeps it valid JSON for jq (the default double-escapes the quotes).
json: true
escape_json: false
Comment on lines +37 to +38

@michieldegezelle michieldegezelle Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟠 Major — every test in a PR is silently skipped (green CI) when any changed template path contains &, (, or ).

Confirmed live on the validation run be_market #2949 / run 27955521933: 13 templates changed (incl. account_templates/Voorraad), yet check-changed-templates printed No templates changed, emitted [], and test-templates was skipped — nothing was tested.

Root cause: with safe_output: true (tj-actions/changed-files default), the JSON values are backslash-escaped for shell metacharacters, so account_templates/Leningen & Leasings (42)/main.liquid is emitted as …Leningen \& Leasings \(42\)…. \& / \( are invalid JSON escapes, so jq -r '.[]' aborts with a parse error:

$ printf '%s' "$ALL_CHANGED_FILES" | jq -r '.[]'
jq: parse error: Invalid escape at line 1, column 139

Because that jq runs inside the mapfile < <(… | jq … | grep …) process substitution, its non-zero exit doesn't trip set -e — jq just outputs nothing, the template list comes out empty, and the job skips while staying green. escape_json is not the relevant knob here; the backslashes come from safe_output.

Fix — disable safe_output so the array is valid JSON (the list is already passed via env: and parsed with jq, never eval'd, so this is safe):

          json: true
          escape_json: false
          safe_output: false
          files: |

Worth a follow-up so this can't fail silently again: have the filter step fail loudly if jq errors, e.g. jq -er '.[]' with an explicit non-zero check instead of swallowing it in the process substitution.

files: |
**/**.{liquid,yml,yaml,json}
- name: Filter templates changed
id: templates_changed
env:
# Passed via env (not inlined) so a path with quotes/spaces cannot break the script.
ALL_CHANGED_FILES: ${{ steps.changed-files.outputs.all_changed_files }}
run: |
pattern="(?:reconciliation_texts|shared_parts)/([^/]+)/"
changed_files="${{ steps.changed-files.outputs.all_changed_files }}"
if [ -n "$changed_files" ]; then
filtered_names=($(printf "%s\n" "$changed_files" | grep -oP "$pattern" || true))
if [ $? -ne 0 ]; then
echo "No files match the pattern"
changed_templates=()
else
# Remove the trailing "/" from the extracted names
filtered_names=("${filtered_names[@]%/}")
# Remove duplicates
changed_templates=($(printf "%s\n" "${filtered_names[@]}" | sort -u))
fi
else
echo "No changed files"
changed_templates=()
fi
# Store outputs
# all_changed_files is a JSON array (json: true). Extract the unique template
# directories (reconciliation_texts / account_templates / shared_parts), space-safely.
pattern='(reconciliation_texts|account_templates|shared_parts)/[^/]+'

@michieldegezelle michieldegezelle Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💡 Suggestion — shared_parts is extracted here only to be skipped later, which spins up the whole test-templates job for nothing on a shared-parts-only PR.

This pattern includes shared_parts, so a PR touching only shared parts yields a non-empty changed_templates (e.g. ["shared_parts/foo"]), the if: … != '[]' guard passes, and test-templates checks out + npm installs the CLI just to hit the "no liquid tests for this template type" skip and exit green. Since the "run reconciliations that use changed shared parts" feature is currently commented out, you could drop shared_parts from the pattern so such PRs short-circuit to [] and skip the job entirely:

          pattern='(reconciliation_texts|account_templates)/[^/]+'

(If you keep it for a future re-enable of the shared-parts test feature, no change needed — just flagging the wasted run.)

mapfile -t changed_templates < <(printf '%s' "${ALL_CHANGED_FILES:-[]}" | jq -r '.[]' | grep -oE "$pattern" | sort -u)
if [ ${#changed_templates[@]} -eq 0 ]; then
echo "changed_templates=[]" >> $GITHUB_OUTPUT
echo "No templates changed"
echo "changed_templates=[]" >> "$GITHUB_OUTPUT"
else
echo "changed_templates=${changed_templates[*]}" >> $GITHUB_OUTPUT
# Print the templates names
for name in "${changed_templates[@]}"; do
echo "$name"
done
echo "Changed templates:"
printf ' %s\n' "${changed_templates[@]}"
# Emit a JSON array so directory names containing spaces survive as single entries.
json=$(printf '%s\n' "${changed_templates[@]}" | jq -R . | jq -s -c .)
echo "changed_templates=${json}" >> "$GITHUB_OUTPUT"
fi
exit 0

test-templates:
runs-on: ubuntu-latest
Expand All @@ -72,6 +66,11 @@ jobs:
SF_API_SECRET: "${{ secrets.SF_API_SECRET }}"
SF_TEST_FIRM_ID: "${{ vars.SF_TEST_FIRM_ID }}"
CHANGED_TEMPLATES: "${{ needs.check-changed-templates.outputs.changed_templates }}"
# Maximum number of liquid tests run in parallel against the live platform per batch.
# A single `run-test --status` call fires all of its handles concurrently (Promise.all)
# with no client-side rate limiting, so we cap the batch size and run batches
# sequentially to keep the global number of in-flight tests bounded.
MAX_PARALLEL_TESTS: 10
if: ${{ needs.check-changed-templates.outputs.changed_templates != '[]' }}
needs: [check-auth, check-changed-templates]
steps:
Expand All @@ -93,24 +92,46 @@ jobs:
npm install https://github.com/silverfin/silverfin-cli.git
VERSION=$(./node_modules/silverfin-cli/bin/cli.js -V)
echo "CLI version: ${VERSION}"
- name: Run liquid tests for updated templates
- name: Run liquid tests for updated templates (grouped per firm, run in parallel)
run: |
MAX_PARALLEL="${MAX_PARALLEL_TESTS:-10}"
# Testable templates grouped by "<firm id>|<template type>". Each bucket maps to one
# `run-test` call, because the CLI takes a single firm and a single template type per
# invocation. Identifiers are stored newline-separated so account template names
# (which contain spaces and other characters) stay intact.
declare -A TEMPLATE_BUCKETS
declare -a ERRORS
for CURRENT_DIR in ${{ env.CHANGED_TEMPLATES }}; do
# CHANGED_TEMPLATES is a JSON array; read it space-safely. The process substitution
# keeps the loop in the current shell so the arrays above survive.
while IFS= read -r CURRENT_DIR; do
[[ -z "${CURRENT_DIR}" ]] && continue
echo "Checking ${CURRENT_DIR}"
while [[ "${CURRENT_DIR}" != "." ]]; do
if [[ -e "${CURRENT_DIR}/config.json" ]]; then
HANDLE=$(cat ${CURRENT_DIR}/config.json | jq -r ".handle // .name")
# Decide the template type from the path. Only reconciliation_texts and
# account_templates have liquid tests; skip anything else (e.g. shared_parts,
# whose ".id" is a plain string) before resolving a firm.
if [[ "${CURRENT_DIR}" == *reconciliation_texts* ]]; then
TEMPLATE_TYPE="reconciliationText"
IDENTIFIER=$(cat "${CURRENT_DIR}/config.json" | jq -r ".handle // .name")
elif [[ "${CURRENT_DIR}" == *account_templates* ]]; then
TEMPLATE_TYPE="accountTemplate"
# Account template configs often have a null handle/name, so use the folder name.
IDENTIFIER=$(basename "${CURRENT_DIR}")
else
echo "Skipping ${CURRENT_DIR} (no liquid tests for this template type)"
break
fi

# Initialize FIRM_ID
FIRM_ID=""

# Check if test_firm_id is present in config
TEST_FIRM_ID=$(cat ${CURRENT_DIR}/config.json | jq -r ".test_firm_id // empty")
TEST_FIRM_ID=$(cat "${CURRENT_DIR}/config.json" | jq -r ".test_firm_id // empty")

if [[ -n "$TEST_FIRM_ID" && "$TEST_FIRM_ID" != "null" ]]; then
# 1. Template-specific test_firm_id (highest priority)
AVAILABLE_FIRM_IDS=$(cat ${CURRENT_DIR}/config.json | jq -r ".id | keys[]" 2>/dev/null || echo "")
AVAILABLE_FIRM_IDS=$(cat "${CURRENT_DIR}/config.json" | jq -r ".id | keys[]" 2>/dev/null || echo "")

# Check for exact match by looping through available IDs
FOUND_MATCH=false
Expand All @@ -131,7 +152,7 @@ jobs:

if [[ -z "$FIRM_ID" && -n "$SF_TEST_FIRM_ID" ]]; then
# 2. Environment variable fallback
AVAILABLE_FIRM_IDS=$(cat ${CURRENT_DIR}/config.json | jq -r ".id | keys[]" 2>/dev/null || echo "")
AVAILABLE_FIRM_IDS=$(cat "${CURRENT_DIR}/config.json" | jq -r ".id | keys[]" 2>/dev/null || echo "")

# Check for exact match by looping through available IDs
FOUND_MATCH=false
Expand All @@ -152,33 +173,78 @@ jobs:

if [[ -z "$FIRM_ID" ]]; then
# 3. Default behavior - use first available firm ID
FIRM_ID=$(cat ${CURRENT_DIR}/config.json | jq -r ".id" | jq "keys_unsorted" | jq "first" | tr -d '"')
FIRM_ID=$(cat "${CURRENT_DIR}/config.json" | jq -r ".id" | jq "keys_unsorted" | jq "first" | tr -d '"')
echo "Using first available firm ID: ${FIRM_ID}"
fi

if [[ "${CURRENT_DIR}" == *reconciliation_texts* ]]; then
# FETCH THE NEWEST VERSION OF THE TOKENS FROM THE SECRETS, IN CASE THEY WERE UPDATED BY THE INITIATION OF A CONCURRENT WORKFLOW
echo '${{ secrets.CONFIG_JSON }}' > $HOME/.silverfin/config.json
# RUN TEST
echo "Running tests for ${HANDLE} in firm ${FIRM_ID}"
OUTPUT=$(node ./node_modules/silverfin-cli/bin/cli.js run-test --handle "${HANDLE}" --firm "${FIRM_ID}" --status 2>&1)
# CHECK OUTPUT
if [[ "$OUTPUT" =~ "PASSED" ]]; then
echo "${HANDLE}: passed"
elif [[ "$OUTPUT" =~ "FAILED" ]]; then
echo "${HANDLE}: failed"
ERRORS+=("${HANDLE}")
else
echo "${HANDLE}: other errors: ${OUTPUT}"
ERRORS+=("${OUTPUT}")
fi
fi
# Group this template under "<firm>|<type>" so all templates that share a firm
# and type run together in one parallel batch. Newline-separated keeps identifiers
# that contain spaces (account template names) intact.
TEMPLATE_BUCKETS["${FIRM_ID}|${TEMPLATE_TYPE}"]+="${IDENTIFIER}"$'\n'
echo "Queued ${IDENTIFIER} (${TEMPLATE_TYPE}) for firm ${FIRM_ID}"
Comment on lines +183 to +184

@michieldegezelle michieldegezelle Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟠 Major — templates with an empty test YAML are reported as FAILED, not skipped.

5 of be_market's 20 account templates currently have a placeholder-only test file (Verworpen uitgaven, Huurinkomsten (detail), Opsplitsing kosten wagenpark, Overlopende rekeningen, Standaardtoelichting). For these the CLI prints:

[info] <name>: there are no tests stored in the YAML file
[error] Error running tests for <name>
[log]  <name>: FAILED

runTestsStatusOnly returns FAILED whenever buildTestParams bails on an empty YAML (testContent.trim().split("\n").length <= 1), so this job will go red for templates that simply have no tests yet — which shouldn't be a CI failure.

The [log] <name>: FAILED line is identical to a real failure, so it can't be told apart at the case step. Cleanest fix is a pre-flight skip before queuing — replicate the CLI's own empty check so we never queue a test-less template:

                # Skip templates whose test YAML is empty (CLI would report these as FAILED).
                TEST_REL=$(cat "${CURRENT_DIR}/config.json" | jq -r '.test // empty')
                TEST_PATH="${CURRENT_DIR}/${TEST_REL}"
                if [[ -z "${TEST_REL}" || ! -s "${TEST_PATH}" ]] \
                   || [[ "$(grep -cvE '^[[:space:]]*(#|$)' "${TEST_PATH}")" -eq 0 ]]; then
                  echo "Skipping ${IDENTIFIER} (no liquid tests defined)"
                  break
                fi

(~6 lines, right before the TEMPLATE_BUCKETS[…]+=… queue line.) This is in scope for this PR and easy. The more correct long-term fix is CLI-side: have runTestsStatusOnly emit a distinct SKIPPED/NO_TESTS status instead of FAILED for the empty-YAML case, so every consumer benefits — worth a silverfin-cli follow-up.

@michieldegezelle michieldegezelle Jun 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

💡 Suggestion — tighten per-template logging and group each parallel batch (with a final summary).

Each template currently emits 3 lines during queueing (Checking … / Using first available firm ID: … / Queued …), so ~13 templates ≈ 40 lines before a single test runs — and the two things that matter (identifier + firm) get buried, then the spinner frames pile on top.

Proposed shape — collapsible group per parallel batch, plus a flat summary at the end:

Queue phase — drop Checking …, keep only the firm-fallback Warning: lines (the real diagnostics), collapse to one entry each:

+ Deelnemingen → firm 6056
+ expenses → firm 1355
…

Run phase — wrap each batch in an Actions log group so it folds up and reads as one parallel run:

echo "::group::firm ${FIRM_ID} · ${TEMPLATE_TYPE} · ${#BATCH[@]} in parallel"
# … run + per-template PASSED/FAILED …
echo "::endgroup::"

renders as:

▸ firm 6056 · accountTemplate · 1 in parallel
    Deelnemingen: PASSED
▸ firm 542 · reconciliationText · 4 in parallel
    2018_204_3_writedowns_provisions: PASSED
    2018_274_APT_8: PASSED
    liquidation_reserve: PASSED
    2018_275_A_liquidationreserve: FAILED

End — one aligned summary keyed on the two important columns:

─ Results ──────────────────────────────────
 PASS  Deelnemingen                  firm 6056
 PASS  vol_10                        firm 6056
 FAIL  vkt_5                         firm 6056
 FAIL  2018_275_A_liquidationreserve firm 542
─────────────────────────────────────────────
2 failed, 11 passed

The queue loop (firm resolution) and the run loop (results) are already separate, so this is mostly swapping echo lines, wrapping the batch loop in ::group::/::endgroup::, and adding a final loop over the collected results. (Spinner clutter is orthogonal — CLI-side, see the follow-up note.)

break
else
echo "Config file not found in ${CURRENT_DIR}"
CURRENT_DIR="$(dirname "${CURRENT_DIR}")"
fi
done
done < <(printf '%s' "${CHANGED_TEMPLATES}" | jq -r '.[]')
# Run each bucket. All identifiers passed to a single `run-test --status` call run
# concurrently in the CLI (Promise.all), so we cap each batch at MAX_PARALLEL and run
# batches sequentially to keep the number of in-flight tests against the platform bounded.
for KEY in "${!TEMPLATE_BUCKETS[@]}"; do
FIRM_ID="${KEY%%|*}"
TEMPLATE_TYPE="${KEY##*|}"
# The CLI uses a different flag per template type.
if [[ "${TEMPLATE_TYPE}" == "accountTemplate" ]]; then
CLI_FLAG="--account-template"
else
CLI_FLAG="--handle"
fi
# Deduplicate the identifiers for this bucket (newline-separated, so names with
# spaces stay intact).
mapfile -t IDENTIFIERS < <(printf '%s' "${TEMPLATE_BUCKETS[$KEY]}" | sort -u | grep -v '^$')
TOTAL=${#IDENTIFIERS[@]}
echo "Firm ${FIRM_ID} / ${TEMPLATE_TYPE}: ${TOTAL} template(s) to test"
for (( START=0; START<TOTAL; START+=MAX_PARALLEL )); do
BATCH=("${IDENTIFIERS[@]:START:MAX_PARALLEL}")
echo "Running batch for firm ${FIRM_ID} / ${TEMPLATE_TYPE} (${#BATCH[@]} in parallel): ${BATCH[*]}"
if OUTPUT=$(node ./node_modules/silverfin-cli/bin/cli.js run-test --firm "${FIRM_ID}" --status ${CLI_FLAG} "${BATCH[@]}" 2>&1); then
BATCH_EXIT=0
else
BATCH_EXIT=$?
fi
echo "${OUTPUT}"
# Parse the per-template status. In CI, consola prefixes every line with "[log] "
# and the test spinner interleaves frames via carriage returns, so split on carriage
# returns and strip ANSI escapes first. We then read the top-level
# "[log] <name>: PASSED|FAILED" lines (a leading space after "[log] " marks an
# indented sub-result, which is skipped) and match by EXACT name: account template
# names contain spaces and characters that are unsafe inside a regex.
CLEAN_OUTPUT=$(printf '%s\n' "${OUTPUT}" | tr '\r' '\n' | sed -E $'s/\033\\[[0-9;?]*[A-Za-z]//g')
declare -A RESULTS=()
while IFS= read -r LINE; do
STATUS="${LINE##*: }"
NAME="${LINE#\[log\] }"
NAME="${NAME%: *}"
RESULTS["${NAME}"]="${STATUS}"
done < <(printf '%s\n' "${CLEAN_OUTPUT}" | grep -oE '\[log\] [^[:space:]].*: (PASSED|FAILED)$')

@coderabbitai coderabbitai Bot Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Grep pattern may fail if CLI output has trailing whitespace.

The $ anchor requires the line to end exactly with PASSED or FAILED. If the CLI output has trailing whitespace or other invisible characters (which survive the ANSI stripping), the pattern won't match and all templates will show as "status could not be determined".

🛠️ Suggested fix to allow trailing whitespace 
-            done < <(printf '%s\n' "${CLEAN_OUTPUT}" | grep -oE '\[log\] [^[:space:]].*: (PASSED|FAILED)$')
+            done < <(printf '%s\n' "${CLEAN_OUTPUT}" | grep -oE '\[log\] [^[:space:]].*: (PASSED|FAILED)[[:space:]]*$')
📝 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
done < <(printf '%s\n' "${CLEAN_OUTPUT}" | grep -oE '\[log\] [^[:space:]].*: (PASSED|FAILED)$')
done < <(printf '%s\n' "${CLEAN_OUTPUT}" | grep -oE '\[log\] [^[:space:]].*: (PASSED|FAILED)[[:space:]]*$')
🤖 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 at 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.

for IDENTIFIER in "${BATCH[@]}"; do
case "${RESULTS[${IDENTIFIER}]:-}" in
PASSED)
echo "${IDENTIFIER}: passed"
;;
FAILED)
echo "${IDENTIFIER}: failed"
ERRORS+=("${IDENTIFIER}")
;;
*)
echo "${IDENTIFIER}: status could not be determined (batch exit code ${BATCH_EXIT})"
ERRORS+=("${IDENTIFIER}")
;;
esac
done
done
done
# CHECK ERRORS PRESENT
if [ ${#ERRORS[@]} -eq 0 ]; then
Expand Down
Loading