From d799d0a042cf69693079d734f98cbe6c3b50f066 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Sun, 15 Feb 2026 21:10:28 +0000 Subject: [PATCH 01/24] Add workflow option to stats command --- bin/cli.js | 1 + 1 file changed, 1 insertion(+) diff --git a/bin/cli.js b/bin/cli.js index e3dfb6da..1c753034 100755 --- a/bin/cli.js +++ b/bin/cli.js @@ -626,6 +626,7 @@ program .command("stats") .description("Generate an overview with some statistics") .requiredOption("-s, --since , Specify the date which is going to be used to filter the data from (format: YYYY-MM-DD) (mandatory)") + .option("-w, --workflow [handle]", "Optionally filter test statistics by workflow (workflow handle is optional)") .action((options) => { stats.generateOverview(options.since); }); From 1f00cb44c6c5b2e6787b98283799ea7d484a44e4 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Sun, 15 Feb 2026 21:43:09 +0000 Subject: [PATCH 02/24] [241] Create new getWorkflowTemplateSummary function --- bin/cli.js | 4 +-- lib/cli/stats.js | 66 ++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/bin/cli.js b/bin/cli.js index 1c753034..06d60bff 100755 --- a/bin/cli.js +++ b/bin/cli.js @@ -626,9 +626,9 @@ program .command("stats") .description("Generate an overview with some statistics") .requiredOption("-s, --since , Specify the date which is going to be used to filter the data from (format: YYYY-MM-DD) (mandatory)") - .option("-w, --workflow [handle]", "Optionally filter test statistics by workflow (workflow handle is optional)") + .option("-w, --workflow [handle], Optionally filter test statistics by workflow (optional)") .action((options) => { - stats.generateOverview(options.since); + stats.generateOverview(options.since, options.workflow); }); // Set/Get FIRM ID diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 0aa4f942..74c472c8 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -6,15 +6,20 @@ const fsUtils = require("../utils/fsUtils"); const yaml = require("yaml"); const { consola } = require("consola"); -async function generateOverview(sinceDate) { +async function generateOverview(sinceDate, workflow) { const TODAY = new Date().toJSON().toString().slice(0, 10); - const templateSummary = await getTemplatesSummary(); - const yamlSummary = await getYamlSummary(sinceDate); - // Terminal - displayOverview(sinceDate, TODAY, templateSummary, yamlSummary); - // File - const row = createRow(sinceDate, TODAY, templateSummary, yamlSummary); - saveOverviewToFile(row); + if (workflow) { + const templateSummary = await getWorkflowTemplateSummary(workflow); + consola.log("Workflow summary test A") + } else { + const templateSummary = await getTemplatesSummary(); + const yamlSummary = await getYamlSummary(sinceDate); + // Terminal + displayOverview(sinceDate, TODAY, templateSummary, yamlSummary); + // File + const row = createRow(sinceDate, TODAY, templateSummary, yamlSummary); + saveOverviewToFile(row); + } } // Return an object with the count of activities by file and by type @@ -255,6 +260,51 @@ async function getTemplatesSummary() { return summary; } +async function getWorkflowTemplateSummary(workflowHandle) { + const summary = { + reconciliations: { + total: 0, + externallyManaged: 0, + externallyManagedPerc: 0, + yamlFiles: 0, + yamlFilesPerc: 0, + unitTests: 0, + yamlFilesWithAtLeastTwoTests: 0, + yamlFilesWithAtLeastTwoTestsPerc: 0, + }, + exportFiles: { + total: 0, + externallyManaged: 0, + externallyManagedPerc: 0, + }, + accountTemplates: { + total: 0, + externallyManaged: 0, + externallyManagedPerc: 0, + yamlFiles: 0, + yamlFilesPerc: 0, + unitTests: 0, + yamlFilesWithAtLeastTwoTests: 0, + yamlFilesWithAtLeastTwoTestsPerc: 0, + }, + all: { + total: 0, + externallyManaged: 0, + externallyManagedPerc: 0, + yamlFiles: 0, + yamlFilesPerc: 0, + unitTests: 0, + yamlFilesWithAtLeastTwoTests: 0, + yamlFilesWithAtLeastTwoTestsPerc: 0, + }, + }; + + // Reconciliations + // Assume no empty reconciliations within the workflow. + + return summary; +} + async function getYamlSummary(sinceDate) { const yamlActivity = await yamlFilesActivity(sinceDate); const summary = { created: 0, updated: 0 }; From dd2cfd3bbdabf5b8d968cd939e180dc934066deb Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Wed, 18 Mar 2026 15:39:41 +0000 Subject: [PATCH 03/24] Get template lists from the workflow --- lib/cli/stats.js | 11 ++++++++++- lib/utils/fsUtils.js | 15 +++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 74c472c8..59a34964 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -10,7 +10,6 @@ async function generateOverview(sinceDate, workflow) { const TODAY = new Date().toJSON().toString().slice(0, 10); if (workflow) { const templateSummary = await getWorkflowTemplateSummary(workflow); - consola.log("Workflow summary test A") } else { const templateSummary = await getTemplatesSummary(); const yamlSummary = await getYamlSummary(sinceDate); @@ -299,8 +298,18 @@ async function getWorkflowTemplateSummary(workflowHandle) { }, }; + // Fetch workflow + const workflow = await fsUtils.getWorkflow(workflowHandle); + // Reconciliations // Assume no empty reconciliations within the workflow. + const reconciliationsInWorkflow = workflow.templates.reconciliations; + + // Export Files + const exportFilesInWorkflow = workflow.templates.exportFiles; + + // Account Templates + const accountTemplatesInWorkflow = workflow.templates.accountTemplates; return summary; } diff --git a/lib/utils/fsUtils.js b/lib/utils/fsUtils.js index 87da3012..b138ff7b 100644 --- a/lib/utils/fsUtils.js +++ b/lib/utils/fsUtils.js @@ -483,6 +483,20 @@ function checkLiquidTestDependencies(targetHandle) { return dependentHandles; } +function getWorkflow(workflowHandle) { + try { + const workflowPath = path.join(process.cwd(), "workflows", `${workflowHandle}.json`); + if (!fs.existsSync(workflowPath)) { + throw new Error(`Workflow "${workflowHandle}" not found`); + } + return JSON.parse(fs.readFileSync(workflowPath).toString()); + } catch (error) { + consola.error(`An error occurred when trying to read the workflow "${workflowHandle}"`); + consola.error(error); + process.exit(1); + } +} + // Recursive option for fs.watch is not available in every OS (e.g. Linux) function recursiveInspectDirectory({ basePath, collection, pathsArray = [], typeCheck = "liquid" }) { collection.forEach((filePath) => { @@ -622,5 +636,6 @@ module.exports = { getTemplateId, setTemplateId, checkLiquidTestDependencies, + getWorkflow, scanTextParts, }; From 13bc804da530d5329a2ed22befc10dc9a2adca57 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Wed, 18 Mar 2026 16:24:18 +0000 Subject: [PATCH 04/24] Add filter for files in countYamlFiles and complete assignments --- lib/cli/stats.js | 56 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 9 deletions(-) diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 59a34964..35e37131 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -8,10 +8,8 @@ const { consola } = require("consola"); async function generateOverview(sinceDate, workflow) { const TODAY = new Date().toJSON().toString().slice(0, 10); - if (workflow) { - const templateSummary = await getWorkflowTemplateSummary(workflow); - } else { - const templateSummary = await getTemplatesSummary(); + const templateSummary = workflow ? await getWorkflowTemplateSummary(workflow) : await getTemplatesSummary(); + if (!workflow) { const yamlSummary = await getYamlSummary(sinceDate); // Terminal displayOverview(sinceDate, TODAY, templateSummary, yamlSummary); @@ -78,10 +76,12 @@ async function yamlFilesActivity(sinceDate) { // Count how many YAML files are stored. We base on the presence of a non empty file // Count how many unit tests are stored. We base on the presence of a title for each unit test // Count how many YAML files have at least two unit tests -async function countYamlFiles(templateType) { +async function countYamlFiles(templateType, templatesInWorkflow) { const files = fsUtils.listExistingFiles("yml"); const FOLDER = fsUtils.FOLDERS[templateType]; - const YAML_EXPRESSION = `.*${FOLDER}/.*/tests/.*_liquid_test.*.y(a)?ml`; + const TEMPLATE_PATTERN = templatesInWorkflow ? `(${templatesInWorkflow.join("|")})` : `.*`; + // Issue with counting multiple yml files in the same tests folder? + const YAML_EXPRESSION = `.*${FOLDER}/${TEMPLATE_PATTERN}/tests/.*_liquid_test.*.y(a)?ml`; const re = new RegExp(YAML_EXPRESSION, "g"); let countFiles = 0; let countFilesWithAtLeastTwoTests = 0; @@ -261,6 +261,7 @@ async function getTemplatesSummary() { async function getWorkflowTemplateSummary(workflowHandle) { const summary = { + template_name: "", reconciliations: { total: 0, externallyManaged: 0, @@ -299,18 +300,55 @@ async function getWorkflowTemplateSummary(workflowHandle) { }; // Fetch workflow + // Assume no empty items if present within the workflow. const workflow = await fsUtils.getWorkflow(workflowHandle); + summary.template_name = workflow.name; // Reconciliations - // Assume no empty reconciliations within the workflow. const reconciliationsInWorkflow = workflow.templates.reconciliations; + const reconciliationsExtMan = await listExternallyManagedTemplates("reconciliationText", reconciliationsInWorkflow); + const reconciliationsTests = await countYamlFiles("reconciliationText", reconciliationsInWorkflow); + summary.reconciliations.total = reconciliationsInWorkflow.length; + summary.reconciliations.externallyManaged = reconciliationsExtMan.length; + summary.reconciliations.externallyManagedPerc = percentageRoundTwo(summary.reconciliations.externallyManaged, summary.reconciliations.total); + summary.reconciliations.yamlFiles = reconciliationsTests.files; + summary.reconciliations.yamlFilesPerc = percentageRoundTwo(summary.reconciliations.yamlFiles, summary.reconciliations.total); + summary.reconciliations.unitTests = reconciliationsTests.tests; + summary.reconciliations.yamlFilesWithAtLeastTwoTests = reconciliationsTests.filesWithAtLeastTwoTests; + summary.reconciliations.yamlFilesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.reconciliations.yamlFilesWithAtLeastTwoTests, summary.reconciliations.total); // Export Files - const exportFilesInWorkflow = workflow.templates.exportFiles; + const exportFilesInWorkflow = workflow.templates.exports; + const exportFilesExtMan = await listExternallyManagedTemplates("exportFile", exportFilesInWorkflow); + summary.exportFiles.total = exportFilesInWorkflow.length; + summary.exportFiles.externallyManaged = exportFilesExtMan.length; + summary.exportFiles.externallyManagedPerc = percentageRoundTwo(summary.exportFiles.externallyManaged, summary.exportFiles.total); // Account Templates - const accountTemplatesInWorkflow = workflow.templates.accountTemplates; + const accountTemplatesInWorkflow = workflow.templates.accounts; + const accountTemplatesExtMan = await listExternallyManagedTemplates("accountTemplate", accountTemplatesInWorkflow); + const accountTemplatesTests = await countYamlFiles("accountTemplate", accountTemplatesInWorkflow); + summary.accountTemplates.total = accountTemplatesInWorkflow.length; + summary.accountTemplates.externallyManaged = accountTemplatesExtMan.length; + summary.accountTemplates.externallyManagedPerc = percentageRoundTwo(summary.accountTemplates.externallyManaged, summary.accountTemplates.total); + summary.accountTemplates.yamlFiles = accountTemplatesTests.files; + summary.accountTemplates.yamlFilesPerc = percentageRoundTwo(summary.accountTemplates.yamlFiles, summary.accountTemplates.total); + summary.accountTemplates.unitTests = accountTemplatesTests.tests; + summary.accountTemplates.yamlFilesWithAtLeastTwoTests = accountTemplatesTests.filesWithAtLeastTwoTests; + summary.accountTemplates.yamlFilesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.accountTemplates.yamlFilesWithAtLeastTwoTests, summary.accountTemplates.total); + + // All + summary.all.total = summary.reconciliations.total + summary.exportFiles.total + summary.accountTemplates.total; + summary.all.externallyManaged = + summary.reconciliations.externallyManaged + summary.exportFiles.externallyManaged + summary.accountTemplates.externallyManaged; + summary.all.externallyManagedPerc = percentageRoundTwo(summary.all.externallyManaged, summary.all.total); + summary.all.yamlFiles = summary.reconciliations.yamlFiles + summary.accountTemplates.yamlFiles; + summary.all.yamlFilesPerc = percentageRoundTwo(summary.all.yamlFiles, summary.reconciliations.total + summary.accountTemplates.total); + summary.all.unitTests = summary.reconciliations.unitTests + summary.accountTemplates.unitTests; + summary.all.yamlFilesWithAtLeastTwoTests = summary.reconciliations.yamlFilesWithAtLeastTwoTests + summary.accountTemplates.yamlFilesWithAtLeastTwoTests; + summary.all.yamlFilesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.all.yamlFilesWithAtLeastTwoTests, summary.reconciliations.total + summary.accountTemplates.total); + consola.log("TEST SUMMARY: ", summary); return summary; } From 175f7c90f5ac48c75b076599aac4e764ce222ce8 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 31 Mar 2026 12:26:18 +0100 Subject: [PATCH 05/24] [241] Update yamlFilesActivity function for workflows --- lib/cli/stats.js | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 35e37131..81d694f1 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -6,22 +6,25 @@ const fsUtils = require("../utils/fsUtils"); const yaml = require("yaml"); const { consola } = require("consola"); -async function generateOverview(sinceDate, workflow) { +async function generateOverview(sinceDate, workflowHandle) { + const workflow = workflowHandle ? await fsUtils.getWorkflow(workflowHandle) : null; const TODAY = new Date().toJSON().toString().slice(0, 10); const templateSummary = workflow ? await getWorkflowTemplateSummary(workflow) : await getTemplatesSummary(); - if (!workflow) { - const yamlSummary = await getYamlSummary(sinceDate); - // Terminal + const yamlSummary = workflow ? await getYamlSummary(sinceDate, workflow) : await getYamlSummary(sinceDate); + // Terminal + if (workflow) { + displayWorkflowOverview(sinceDate, TODAY, templateSummary, yamlSummary); + } else { displayOverview(sinceDate, TODAY, templateSummary, yamlSummary); - // File - const row = createRow(sinceDate, TODAY, templateSummary, yamlSummary); - saveOverviewToFile(row); } + // File + // const row = createRow(sinceDate, TODAY, templateSummary, yamlSummary); + // saveOverviewToFile(row); } // Return an object with the count of activities by file and by type // Type could be: A (added), M (modified), D (deleted) -async function yamlFilesActivity(sinceDate) { +async function yamlFilesActivity(sinceDate, workflow) { const countByType = {}; const filesChanged = exec.execSync(`git whatchanged --since="${sinceDate}" --name-status --pretty="format:"`); if (!filesChanged) { @@ -37,7 +40,9 @@ async function yamlFilesActivity(sinceDate) { } // Files to Search (YAML) - const YAML_EXPRESSION = `.*/.*/tests/.*_liquid_test.*.y(a)?ml`; + const templatesInWorkflow = workflow ? workflow.templates.reconciliations.concat(workflow.templates.accounts) : []; + const TEMPLATE_PATTERN = workflow ? `(${templatesInWorkflow.join("|")})` : `.*`; + const YAML_EXPRESSION = `.*/.${TEMPLATE_PATTERN}/tests/.*_liquid_test.*.y(a)?ml`; const fileTypeRegExp = RegExp(YAML_EXPRESSION, "g"); for (const row of nonEmptyRows) { @@ -259,7 +264,7 @@ async function getTemplatesSummary() { return summary; } -async function getWorkflowTemplateSummary(workflowHandle) { +async function getWorkflowTemplateSummary(workflow) { const summary = { template_name: "", reconciliations: { @@ -301,7 +306,6 @@ async function getWorkflowTemplateSummary(workflowHandle) { // Fetch workflow // Assume no empty items if present within the workflow. - const workflow = await fsUtils.getWorkflow(workflowHandle); summary.template_name = workflow.name; // Reconciliations @@ -348,12 +352,11 @@ async function getWorkflowTemplateSummary(workflowHandle) { summary.all.yamlFilesWithAtLeastTwoTests = summary.reconciliations.yamlFilesWithAtLeastTwoTests + summary.accountTemplates.yamlFilesWithAtLeastTwoTests; summary.all.yamlFilesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.all.yamlFilesWithAtLeastTwoTests, summary.reconciliations.total + summary.accountTemplates.total); - consola.log("TEST SUMMARY: ", summary); return summary; } -async function getYamlSummary(sinceDate) { - const yamlActivity = await yamlFilesActivity(sinceDate); +async function getYamlSummary(sinceDate, templatesInWorkflow) { + const yamlActivity = await yamlFilesActivity(sinceDate, templatesInWorkflow); const summary = { created: 0, updated: 0 }; summary.created = (yamlActivity["A"] || 0) - (yamlActivity["D"] || 0); summary.updated = yamlActivity["M"] || 0; @@ -406,6 +409,10 @@ function displayOverview(sinceDate, today, templateSummary, yamlSummary) { consola.log("------------------------------------"); } +function displayWorkflowOverview(sinceDate, today, templateSummary, yamlSummary) { + +} + function createRow(sinceDate, today, templateSummary, yamlSummary) { // Row to append to file const rowContent = [ From 5a2bd92255cd2a728060b1d3c25c4c23ad8d589c Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 31 Mar 2026 12:31:26 +0100 Subject: [PATCH 06/24] [241] Create displayWorkflowOverview function --- lib/cli/stats.js | 46 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 81d694f1..16f2a5a0 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -410,7 +410,51 @@ function displayOverview(sinceDate, today, templateSummary, yamlSummary) { } function displayWorkflowOverview(sinceDate, today, templateSummary, yamlSummary) { - + // Header + consola.log(""); + consola.info(`${chalk.bold(`Workflow Summary - ${templateSummary.template_name} ( ${sinceDate} - ${today} ):`)}`); + consola.log("------------------------------------"); + consola.log(""); + // YAML file changes + consola.log(`New YAML files created in the period: ${yamlSummary.created}`); + consola.log(`Updates to existing YAML files in the period: ${yamlSummary.updated}`); + consola.log(""); + consola.log("------------------------------------"); + consola.log(""); + // Reconciliations + consola.log(`${chalk.bold("Reconciliations:")}`); + consola.log(`Templates: ${templateSummary.reconciliations.total}`); + consola.log(`Externally Managed: ${templateSummary.reconciliations.externallyManaged} (${templateSummary.reconciliations.externallyManagedPerc}%)`); + consola.log(`YAML files: ${templateSummary.reconciliations.yamlFiles} (${templateSummary.reconciliations.yamlFilesPerc}%)`); + consola.log(`Unit Tests: ${templateSummary.reconciliations.unitTests}`); + consola.log( + `YAML files with at least two unit tests: ${templateSummary.reconciliations.yamlFilesWithAtLeastTwoTests} (${templateSummary.reconciliations.yamlFilesWithAtLeastTwoTestsPerc}%)` + ); + consola.log(""); + // Account Templates + consola.log(`${chalk.bold("Account Templates:")}`); + consola.log(`Templates: ${templateSummary.accountTemplates.total}`); + consola.log(`Externally Managed: ${templateSummary.accountTemplates.externallyManaged} (${templateSummary.accountTemplates.externallyManagedPerc}%)`); + consola.log(`YAML files: ${templateSummary.accountTemplates.yamlFiles} (${templateSummary.accountTemplates.yamlFilesPerc}%)`); + consola.log(`Unit Tests: ${templateSummary.accountTemplates.unitTests}`); + consola.log( + `YAML files with at least two unit tests: ${templateSummary.accountTemplates.yamlFilesWithAtLeastTwoTests} (${templateSummary.accountTemplates.yamlFilesWithAtLeastTwoTestsPerc}%)` + ); + consola.log(""); + // Export Files + consola.log(`${chalk.bold("Export Files:")}`); + consola.log(`Templates: ${templateSummary.exportFiles.total}`); + consola.log(`Externally Managed: ${templateSummary.exportFiles.externallyManaged} (${templateSummary.exportFiles.externallyManagedPerc}%)`); + consola.log(""); + // All + consola.log(`${chalk.bold("All:")}`); + consola.log(`Templates: ${templateSummary.all.total}`); + consola.log(`Externally Managed: ${templateSummary.all.externallyManaged} (${templateSummary.all.externallyManagedPerc}%)`); + consola.log(`YAML files: ${templateSummary.all.yamlFiles} (${templateSummary.all.yamlFilesPerc}%)`); + consola.log(`Unit Tests: ${templateSummary.all.unitTests}`); + consola.log(`YAML files with at least two unit tests: ${templateSummary.all.yamlFilesWithAtLeastTwoTests} (${templateSummary.all.yamlFilesWithAtLeastTwoTestsPerc}%)`); + consola.log(""); + consola.log("------------------------------------"); } function createRow(sinceDate, today, templateSummary, yamlSummary) { From c785f4ba93c33b2b36b65846f74a63cf6f4c7954 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 31 Mar 2026 12:46:44 +0100 Subject: [PATCH 07/24] fixup! [241] Update yamlFilesActivity function for workflows --- lib/cli/stats.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 16f2a5a0..8b68a100 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -42,7 +42,7 @@ async function yamlFilesActivity(sinceDate, workflow) { // Files to Search (YAML) const templatesInWorkflow = workflow ? workflow.templates.reconciliations.concat(workflow.templates.accounts) : []; const TEMPLATE_PATTERN = workflow ? `(${templatesInWorkflow.join("|")})` : `.*`; - const YAML_EXPRESSION = `.*/.${TEMPLATE_PATTERN}/tests/.*_liquid_test.*.y(a)?ml`; + const YAML_EXPRESSION = `.*/${TEMPLATE_PATTERN}/tests/.*_liquid_test.*.y(a)?ml`; const fileTypeRegExp = RegExp(YAML_EXPRESSION, "g"); for (const row of nonEmptyRows) { From c6942a69fcb2e1963d869c7d78bfda5f1581a1fc Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 31 Mar 2026 14:46:22 +0100 Subject: [PATCH 08/24] [241] Create a createWorkflowRow function --- lib/cli/stats.js | 43 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 8b68a100..2aa98fda 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -14,12 +14,12 @@ async function generateOverview(sinceDate, workflowHandle) { // Terminal if (workflow) { displayWorkflowOverview(sinceDate, TODAY, templateSummary, yamlSummary); + const row = createWorkflowRow(sinceDate, TODAY, templateSummary, yamlSummary); } else { displayOverview(sinceDate, TODAY, templateSummary, yamlSummary); + const row = createRow(sinceDate, TODAY, templateSummary, yamlSummary); + saveOverviewToFile(row); } - // File - // const row = createRow(sinceDate, TODAY, templateSummary, yamlSummary); - // saveOverviewToFile(row); } // Return an object with the count of activities by file and by type @@ -497,6 +497,43 @@ function createRow(sinceDate, today, templateSummary, yamlSummary) { return row; } +function createWorkflowRow(sinceDate, today, templateSummary, yamlSummary) { + const rowContent = [ + sinceDate, + today, + templateSummary.template_name, + yamlSummary.created, + yamlSummary.updated, + templateSummary.all.total, + templateSummary.all.externallyManaged, + templateSummary.all.yamlFiles, + templateSummary.all.unitTests, + templateSummary.reconciliations.total, + templateSummary.reconciliations.externallyManaged, + templateSummary.reconciliations.yamlFiles, + templateSummary.reconciliations.unitTests, + templateSummary.accountTemplates.total, + templateSummary.accountTemplates.externallyManaged, + templateSummary.accountTemplates.yamlFiles, + templateSummary.accountTemplates.unitTests, + templateSummary.exportFiles.total, + templateSummary.exportFiles.externallyManaged, + templateSummary.all.externallyManagedPerc, + templateSummary.reconciliations.externallyManagedPerc, + templateSummary.accountTemplates.externallyManagedPerc, + templateSummary.exportFiles.externallyManagedPerc, + templateSummary.all.yamlFilesPerc, + templateSummary.reconciliations.yamlFilesPerc, + templateSummary.accountTemplates.yamlFilesPerc, + templateSummary.reconciliations.yamlFilesWithAtLeastTwoTests, + templateSummary.reconciliations.yamlFilesWithAtLeastTwoTestsPerc, + templateSummary.accountTemplates.yamlFilesWithAtLeastTwoTests, + templateSummary.accountTemplates.yamlFilesWithAtLeastTwoTestsPerc, + ]; + const row = `\r\n${rowContent.join(";")}`; + return row; +} + // content row must be a string with each column separated by ";" function saveOverviewToFile(row) { const COLUMNS = [ From b0fe77eb26edac11a18c554305a3398bb8b9a3dd Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 31 Mar 2026 15:00:56 +0100 Subject: [PATCH 09/24] [241] Create a saveWorkflowOverviewToFile function --- lib/cli/stats.js | 58 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 4 deletions(-) diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 2aa98fda..8bc49a3d 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -15,6 +15,7 @@ async function generateOverview(sinceDate, workflowHandle) { if (workflow) { displayWorkflowOverview(sinceDate, TODAY, templateSummary, yamlSummary); const row = createWorkflowRow(sinceDate, TODAY, templateSummary, yamlSummary); + saveWorkflowOverviewToFile(row, workflowHandle); } else { displayOverview(sinceDate, TODAY, templateSummary, yamlSummary); const row = createRow(sinceDate, TODAY, templateSummary, yamlSummary); @@ -266,7 +267,7 @@ async function getTemplatesSummary() { async function getWorkflowTemplateSummary(workflow) { const summary = { - template_name: "", + workflow_name: "", reconciliations: { total: 0, externallyManaged: 0, @@ -306,7 +307,7 @@ async function getWorkflowTemplateSummary(workflow) { // Fetch workflow // Assume no empty items if present within the workflow. - summary.template_name = workflow.name; + summary.workflow_name = workflow.name; // Reconciliations const reconciliationsInWorkflow = workflow.templates.reconciliations; @@ -412,7 +413,7 @@ function displayOverview(sinceDate, today, templateSummary, yamlSummary) { function displayWorkflowOverview(sinceDate, today, templateSummary, yamlSummary) { // Header consola.log(""); - consola.info(`${chalk.bold(`Workflow Summary - ${templateSummary.template_name} ( ${sinceDate} - ${today} ):`)}`); + consola.info(`${chalk.bold(`Workflow Summary - ${templateSummary.workflow_name} ( ${sinceDate} - ${today} ):`)}`); consola.log("------------------------------------"); consola.log(""); // YAML file changes @@ -499,9 +500,9 @@ function createRow(sinceDate, today, templateSummary, yamlSummary) { function createWorkflowRow(sinceDate, today, templateSummary, yamlSummary) { const rowContent = [ + templateSummary.workflow_name, sinceDate, today, - templateSummary.template_name, yamlSummary.created, yamlSummary.updated, templateSummary.all.total, @@ -585,4 +586,53 @@ function saveOverviewToFile(row) { fs.appendFileSync(CSV_PATH, row); } +// content row must be a string with each column separated by ";" +function saveWorkflowOverviewToFile(row, workflowHandle) { + const COLUMNS = [ + "Workflow Name", + "Period - Start", + "Period - End", + "yaml files created in period", + "yaml files modified in period", + "All - templates", + "All - externally managed", + "All - yaml files", + "All - unit tests", + "Reconciliations - templates", + "Reconciliations - externally managed", + "Reconciliations - yaml files", + "Reconciliations - unit tests", + "Account Templates - templates", + "Account Templates - externally managed", + "Account Templates - yaml files", + "Account Templates - unit tests", + "Export Files - templates", + "Export Files - externally managed", + "All - externally managed (%)", + "Reconciliations - externally managed (%)", + "Account Templates - externally managed (%)", + "Export Files - externally managed (%)", + "All - yaml files (%)", + "Reconciliations - yaml files (%)", + "Account Templates - yaml files (%)", + "Reconciliations - yaml files with at least two tests", + "Reconciliations - yaml files with at least two tests (%)", + "Account Templates - yaml files with at least two tests", + "Account Templates - yaml files with at least two tests (%)", + ]; + const ROW_HEADER = `${COLUMNS.join(";")}`; + const CSV_PATH = `./stats/${workflowHandle}_stats.csv`; + // Create file and header columns + if (!fs.existsSync("./stats")) { + fs.mkdirSync("stats"); + } + if (!fs.existsSync(CSV_PATH)) { + fs.writeFileSync(CSV_PATH, ROW_HEADER, (err) => { + consola.error(err); + }); + } + // Append content + fs.appendFileSync(CSV_PATH, row); +} + module.exports = { generateOverview }; From ed902030e928cbcf5f4d617772966de0579f068a Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 31 Mar 2026 15:06:53 +0100 Subject: [PATCH 10/24] [241] Split generateOverview functions --- bin/cli.js | 7 ++++++- lib/cli/stats.js | 32 +++++++++++++++++--------------- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/bin/cli.js b/bin/cli.js index 06d60bff..a13543dd 100755 --- a/bin/cli.js +++ b/bin/cli.js @@ -628,7 +628,12 @@ program .requiredOption("-s, --since , Specify the date which is going to be used to filter the data from (format: YYYY-MM-DD) (mandatory)") .option("-w, --workflow [handle], Optionally filter test statistics by workflow (optional)") .action((options) => { - stats.generateOverview(options.since, options.workflow); + if (options.workflow) { + const workflowHandle = typeof options.workflow === "string" ? options.workflow : undefined; + stats.generateWorkflowOverview(options.since, workflowHandle); + } else { + stats.generateOverview(options.since); + } }); // Set/Get FIRM ID diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 8bc49a3d..731ce99a 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -6,21 +6,23 @@ const fsUtils = require("../utils/fsUtils"); const yaml = require("yaml"); const { consola } = require("consola"); -async function generateOverview(sinceDate, workflowHandle) { - const workflow = workflowHandle ? await fsUtils.getWorkflow(workflowHandle) : null; +async function generateOverview(sinceDate) { const TODAY = new Date().toJSON().toString().slice(0, 10); - const templateSummary = workflow ? await getWorkflowTemplateSummary(workflow) : await getTemplatesSummary(); - const yamlSummary = workflow ? await getYamlSummary(sinceDate, workflow) : await getYamlSummary(sinceDate); - // Terminal - if (workflow) { - displayWorkflowOverview(sinceDate, TODAY, templateSummary, yamlSummary); - const row = createWorkflowRow(sinceDate, TODAY, templateSummary, yamlSummary); - saveWorkflowOverviewToFile(row, workflowHandle); - } else { - displayOverview(sinceDate, TODAY, templateSummary, yamlSummary); - const row = createRow(sinceDate, TODAY, templateSummary, yamlSummary); - saveOverviewToFile(row); - } + const templateSummary = await getTemplatesSummary(); + const yamlSummary = await getYamlSummary(sinceDate); + displayOverview(sinceDate, TODAY, templateSummary, yamlSummary); + const row = createRow(sinceDate, TODAY, templateSummary, yamlSummary); + saveOverviewToFile(row); +} + +async function generateWorkflowOverview(sinceDate, workflowHandle) { + const TODAY = new Date().toJSON().toString().slice(0, 10); + const workflow = await fsUtils.getWorkflow(workflowHandle); + const templateSummary = await getWorkflowTemplateSummary(workflow); + const yamlSummary = await getYamlSummary(sinceDate, workflow); + displayWorkflowOverview(sinceDate, TODAY, templateSummary, yamlSummary); + const row = createWorkflowRow(sinceDate, TODAY, templateSummary, yamlSummary); + saveWorkflowOverviewToFile(row, workflowHandle); } // Return an object with the count of activities by file and by type @@ -635,4 +637,4 @@ function saveWorkflowOverviewToFile(row, workflowHandle) { fs.appendFileSync(CSV_PATH, row); } -module.exports = { generateOverview }; +module.exports = { generateOverview, generateWorkflowOverview }; From e1853da765e95c7fdfbc1edafca6ed5a1bb0a3e5 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 31 Mar 2026 15:14:35 +0100 Subject: [PATCH 11/24] Add review all workflows functionality --- lib/cli/stats.js | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 731ce99a..3e392697 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -17,12 +17,21 @@ async function generateOverview(sinceDate) { async function generateWorkflowOverview(sinceDate, workflowHandle) { const TODAY = new Date().toJSON().toString().slice(0, 10); - const workflow = await fsUtils.getWorkflow(workflowHandle); - const templateSummary = await getWorkflowTemplateSummary(workflow); - const yamlSummary = await getYamlSummary(sinceDate, workflow); - displayWorkflowOverview(sinceDate, TODAY, templateSummary, yamlSummary); - const row = createWorkflowRow(sinceDate, TODAY, templateSummary, yamlSummary); - saveWorkflowOverviewToFile(row, workflowHandle); + let workflowHandles; + if (workflowHandle) { + workflowHandles = [workflowHandle]; + } else { + const workflowsFolder = path.join(process.cwd(), "workflows"); + workflowHandles = fs.readdirSync(workflowsFolder).map((file) => path.basename(file, ".json")); + } + for (const handle of workflowHandles) { + const workflow = await fsUtils.getWorkflow(handle); + const templateSummary = await getWorkflowTemplateSummary(workflow); + const yamlSummary = await getYamlSummary(sinceDate, workflow); + displayWorkflowOverview(sinceDate, TODAY, templateSummary, yamlSummary); + const row = createWorkflowRow(sinceDate, TODAY, templateSummary, yamlSummary); + saveWorkflowOverviewToFile(row, handle); + } } // Return an object with the count of activities by file and by type From 84ae28171f23085820e2919fb18bba1fdd61a926 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Wed, 29 Jul 2026 17:22:29 +0100 Subject: [PATCH 12/24] Add sample workflows for tests --- fixtures/market-repo/workflows/workflow_1.json | 8 ++++++++ fixtures/market-repo/workflows/workflow_2.json | 8 ++++++++ fixtures/market-repo/workflows/workflow_empty.json | 8 ++++++++ fixtures/market-repo/workflows/workflow_invalid_json.json | 4 ++++ .../market-repo/workflows/workflow_missing_accounts.json | 7 +++++++ 5 files changed, 35 insertions(+) create mode 100644 fixtures/market-repo/workflows/workflow_1.json create mode 100644 fixtures/market-repo/workflows/workflow_2.json create mode 100644 fixtures/market-repo/workflows/workflow_empty.json create mode 100644 fixtures/market-repo/workflows/workflow_invalid_json.json create mode 100644 fixtures/market-repo/workflows/workflow_missing_accounts.json diff --git a/fixtures/market-repo/workflows/workflow_1.json b/fixtures/market-repo/workflows/workflow_1.json new file mode 100644 index 00000000..65d6d3c7 --- /dev/null +++ b/fixtures/market-repo/workflows/workflow_1.json @@ -0,0 +1,8 @@ +{ + "name": "Workflow 1", + "templates": { + "reconciliations": ["reconciliation_text_1", "reconciliation_text_2"], + "accounts": ["account_1"], + "exports": ["export_1"] + } +} diff --git a/fixtures/market-repo/workflows/workflow_2.json b/fixtures/market-repo/workflows/workflow_2.json new file mode 100644 index 00000000..aecc5241 --- /dev/null +++ b/fixtures/market-repo/workflows/workflow_2.json @@ -0,0 +1,8 @@ +{ + "name": "Workflow 2", + "templates": { + "reconciliations": ["reconciliation_text_3"], + "accounts": [], + "exports": [] + } +} diff --git a/fixtures/market-repo/workflows/workflow_empty.json b/fixtures/market-repo/workflows/workflow_empty.json new file mode 100644 index 00000000..46b4fede --- /dev/null +++ b/fixtures/market-repo/workflows/workflow_empty.json @@ -0,0 +1,8 @@ +{ + "name": "Workflow Without Templates", + "templates": { + "reconciliations": [], + "accounts": [], + "exports": [] + } +} diff --git a/fixtures/market-repo/workflows/workflow_invalid_json.json b/fixtures/market-repo/workflows/workflow_invalid_json.json new file mode 100644 index 00000000..5a760f4e --- /dev/null +++ b/fixtures/market-repo/workflows/workflow_invalid_json.json @@ -0,0 +1,4 @@ +{ + "name": "Workflow With Broken JSON", + "templates": { + "reconciliations": ["reconciliation_text_1",], diff --git a/fixtures/market-repo/workflows/workflow_missing_accounts.json b/fixtures/market-repo/workflows/workflow_missing_accounts.json new file mode 100644 index 00000000..c6421d32 --- /dev/null +++ b/fixtures/market-repo/workflows/workflow_missing_accounts.json @@ -0,0 +1,7 @@ +{ + "name": "Workflow Missing Accounts", + "templates": { + "reconciliations": ["reconciliation_text_1"], + "exports": [] + } +} From 7a4d8feb1a2cafa848ce6d5d39bad0008f3a576c Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Wed, 29 Jul 2026 17:35:20 +0100 Subject: [PATCH 13/24] Add date and workflow input checks to CLI command --- bin/cli.js | 24 ++++++++++++++++-------- lib/cli/utils.js | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/bin/cli.js b/bin/cli.js index a13543dd..3a764677 100755 --- a/bin/cli.js +++ b/bin/cli.js @@ -625,15 +625,23 @@ program program .command("stats") .description("Generate an overview with some statistics") - .requiredOption("-s, --since , Specify the date which is going to be used to filter the data from (format: YYYY-MM-DD) (mandatory)") - .option("-w, --workflow [handle], Optionally filter test statistics by workflow (optional)") - .action((options) => { - if (options.workflow) { - const workflowHandle = typeof options.workflow === "string" ? options.workflow : undefined; - stats.generateWorkflowOverview(options.since, workflowHandle); - } else { - stats.generateOverview(options.since); + .requiredOption("-s, --since ", "Specify the date which is going to be used to filter the data from (format: YYYY-MM-DD) (mandatory)") + .option("-w, --workflow [handle]", "Filter the statistics by workflow. Without a handle, every workflow stored in the workflows folder is used (optional)") + .action(async (options) => { + cliUtils.checkDateFormat(options.since); + // Commander sets workflow to true when the flag is used without a value + if (typeof options.workflow === "undefined") { + await stats.generateOverview(options.since); + return; + } + // A blank handle (e.g. an unset --workflow "$HANDLE") is falsy, so it would otherwise be read as "no handle given" and include every workflow + if (typeof options.workflow === "string" && options.workflow.trim() === "") { + consola.error(`An empty workflow handle was provided. Please pass a handle (--workflow ) or use --workflow on its own to include every workflow`); + process.exit(1); } + // When a handle was typed we pass it on without any surrounding spaces, otherwise the flag was used on its own (Commander gives true instead of text) and undefined means "every workflow" + const workflowHandle = typeof options.workflow === "string" ? options.workflow.trim() : undefined; + await stats.generateWorkflowOverview(options.since, workflowHandle); }); // Set/Get FIRM ID diff --git a/lib/cli/utils.js b/lib/cli/utils.js index 21b35206..7f83c5e6 100644 --- a/lib/cli/utils.js +++ b/lib/cli/utils.js @@ -58,6 +58,23 @@ function formatOption(inputString) { .join(""); } +// Check that a date is provided as YYYY-MM-DD and that it is a real calendar date +function checkDateFormat(dateString) { + if (typeof dateString !== "string" || !/^\d{4}-\d{2}-\d{2}$/.test(dateString)) { + consola.error(`Invalid date "${dateString}". Please provide a date using the format YYYY-MM-DD (e.g. 2024-01-31)`); + process.exit(1); + } + + // A regex match is not enough (e.g. 2024-02-31), so we check it round-trips + const parsedDate = new Date(`${dateString}T00:00:00Z`); + if (Number.isNaN(parsedDate.getTime()) || parsedDate.toISOString().slice(0, 10) !== dateString) { + consola.error(`Invalid date "${dateString}". This is not an existing calendar date`); + process.exit(1); + } + + return true; +} + // Check unique options function checkUniqueOption(uniqueParameters = [], options) { const optionsToCheck = Object.keys(options).filter((element) => { @@ -158,6 +175,7 @@ module.exports = { handleUncaughtErrors, promptConfirmation, formatOption, + checkDateFormat, checkUniqueOption, checkRequiredFirmOrPartner, getCommandSettings, From fd2a70e4b2b8d7812231f2163753b496c81fa62b Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Wed, 29 Jul 2026 17:37:37 +0100 Subject: [PATCH 14/24] Add guidance in README.md to stats and workflow folder? --- README.md | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/README.md b/README.md index 2662f843..6afeabf0 100644 --- a/README.md +++ b/README.md @@ -122,8 +122,30 @@ The CLI will stick to some conventions regarding the structure and organization /tests README.md [name_nl]_liquid_test.yml + /workflows + [workflow_handle].json ``` +### Workflows + +The optional `/workflows` directory groups templates so statistics can be reported per workflow (see [Statistics](#statistics)). Each workflow is a single JSON file, and the file name (without the `.json` extension) is the workflow handle used with `--workflow`. + +```json +{ + "name": "Workflow 1", + "templates": { + "reconciliations": ["reconciliation_text_1", "reconciliation_text_2"], + "accounts": ["account_1"], + "exports": ["export_1"] + } +} +``` + +- `name` is the label shown in the terminal output and stored in the CSV. +- `templates.reconciliations`, `templates.accounts` and `templates.exports` are all required, and each must be an array. Use an empty array when the workflow contains no template of that type. +- The values are the identifiers described in [Naming conventions](#naming-conventions): the `handle` for Reconciliation Texts, the `name_nl` for Account Templates and the `name` for Export Files. They must match the directory names in your repository. +- Shared Parts are not part of a workflow, so they are excluded from workflow statistics. + ### Naming conventions - As you can see in the previous diagram, we use `handle` for Reconciliation Texts, `name` for Shared Parts and Export Files and `name_nl` for Account Templates as their identifiers. We _strongly recommend to keep them unique_ since they will be used to name the directories, liquid files and yaml files and also to identify templates while running each of the commands. The only case where duplicate handles are supported in a single firm, are templates that are added from a marketplace package (so you could have one custom reconciliation text and one from the marketplace using the same handle, the CLI will skip the one from the marketplace and try to identify the custom one). @@ -297,6 +319,28 @@ The backend rejects or silently skips several situations. To avoid confusion: > **Note:** the Data Copier is deployed to specific environments. Point the CLI at the right host first with `silverfin config --set-host ` (or the `SF_HOST` env var). +### Statistics + +The `stats` command generates an overview of the templates and Liquid Tests in your repository, prints it to the terminal and appends a row to a CSV file inside `/stats`. The `--since` flag is mandatory and must use the `YYYY-MM-DD` format; it determines the period used to count created and updated YAML files (based on your git history). + +```bash +silverfin stats --since 2024-01-01 +``` + +This writes to `./stats/overview.csv` and covers every template in the repository, Shared Parts included. + +To report per workflow instead, use `--workflow`. See [Workflows](#workflows) for the expected file format. + +```bash +# A single workflow, from ./workflows/.json +silverfin stats --since 2024-01-01 --workflow + +# Every workflow stored in ./workflows +silverfin stats --since 2024-01-01 --workflow +``` + +Each workflow gets its own file, `./stats/_stats.csv`. When a handle is passed explicitly and it cannot be found or read, the command reports the problem and stops. When every workflow is included, a faulty workflow file is skipped with a warning and the remaining ones are still processed, followed by a summary of what was skipped. + ## Contributing If you find any bug or you have any suggestion, please feel free to open an issue in this repository. From fa5b3c8a583b5d04fb3bc83af1c839040327903e Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 11 Aug 2026 15:24:20 +0100 Subject: [PATCH 15/24] Add initial Claude harness --- .claude/hooks/skill-reminder.sh | 29 ++++ .claude/settings.json | 16 ++ .claude/skills/adding-methods/SKILL.md | 66 +++++++ .claude/skills/bumping-cli-version/SKILL.md | 62 +++++++ .claude/skills/writing-tests/SKILL.md | 76 ++++++++ CLAUDE.md | 41 +++++ docs/ARCHITECTURE.md | 182 ++++++++++++++++++++ docs/DEVELOPMENT.md | 21 +++ 8 files changed, 493 insertions(+) create mode 100755 .claude/hooks/skill-reminder.sh create mode 100644 .claude/settings.json create mode 100644 .claude/skills/adding-methods/SKILL.md create mode 100644 .claude/skills/bumping-cli-version/SKILL.md create mode 100644 .claude/skills/writing-tests/SKILL.md create mode 100644 CLAUDE.md create mode 100644 docs/ARCHITECTURE.md create mode 100644 docs/DEVELOPMENT.md diff --git a/.claude/hooks/skill-reminder.sh b/.claude/hooks/skill-reminder.sh new file mode 100755 index 00000000..39d6c06f --- /dev/null +++ b/.claude/hooks/skill-reminder.sh @@ -0,0 +1,29 @@ +#!/usr/bin/env bash +# PreToolUse hook: maps the file about to be written to the skill that governs it, +# and injects a reminder into the model's context. Silent for unmapped paths. +set -euo pipefail + +input=$(cat) +file=$(printf '%s' "$input" | jq -r '.tool_input.file_path // empty') +[ -z "$file" ] && exit 0 + +case "$file" in + */node_modules/*) exit 0 ;; +esac + +case "$file" in + */tests/*) skill="writing-tests" ;; + */package.json|*/package-lock.json|*/CHANGELOG.md) + skill="bumping-cli-version" ;; + */lib/*|*/bin/*) skill="adding-methods" ;; + *) exit 0 ;; +esac + +jq -n --arg s "$skill" '{ + hookSpecificOutput: { + hookEventName: "PreToolUse", + additionalContext: ("This file is governed by the \"" + $s + + "\" skill. If you have not already read .claude/skills/" + $s + + "/SKILL.md this session, read it now and follow it for this edit.") + } +}' diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..042b54da --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,16 @@ +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "Write|Edit", + "hooks": [ + { + "type": "command", + "command": "\"$CLAUDE_PROJECT_DIR/.claude/hooks/skill-reminder.sh\" 2>/dev/null || true", + "timeout": 10 + } + ] + } + ] + } +} diff --git a/.claude/skills/adding-methods/SKILL.md b/.claude/skills/adding-methods/SKILL.md new file mode 100644 index 00000000..cd9de968 --- /dev/null +++ b/.claude/skills/adding-methods/SKILL.md @@ -0,0 +1,66 @@ +--- +name: adding-methods +description: Use when adding a new function, method, or class to this repo, or when editing an existing one so that its behaviour, signature, or responsibilities change - including "add a helper", "extract this", "make X also do Y", and new CLI commands or API endpoints. +--- + +# Adding or changing a method + +Every method in this repo has one home and one job. Before you write it, decide the home; while you write it, hold the job to one. + +## Step 1 — Read the map + +Read [docs/ARCHITECTURE.md](../../../docs/ARCHITECTURE.md) before writing the method. It states what each file is for, which layer a method belongs in, and the naming conventions. Do not rely on the file you happen to have open. + +## Step 2 — Place it + +Name the method, then pick its home from its name and its dependencies: + +| The method... | Lives in | +|---|---| +| touches `fs` (read, write, scan, exists) | `lib/utils/fsUtils.js` | +| makes an HTTP request to Silverfin | `lib/api/sfApi.js` | +| knows a template type's config keys or folder layout | the matching `lib/templates/*.js` class | +| produces a user-facing error message | `lib/utils/errorUtils.js` | +| validates a CLI option or prompts the user | `lib/cli/utils.js` | +| sequences an API call plus a disk write | `index.js` | +| parses or transforms data with no I/O | the relevant `lib/utils/*.js` | + +Two checks before you commit to a location: + +- **Does it already exist?** Grep `lib/utils/` and the layer you picked. A near-duplicate helper is a call, not a new method. +- **Would it skip a layer?** `bin/cli.js` must not call `lib/api/`; `lib/api/` must not touch `fs`. If your method forces a skip, it is in the wrong place. + +If the method fits nowhere in the table, say so and propose where it should go before writing it. A new home is a decision for the user, not a default. + +## Step 3 — One responsibility + +The method does one thing, at one level of abstraction, for one reason to change. + +Three tests it must pass: + +1. **The name test.** You can name it without "and", "then", "Or", or a vague noun (`handle`, `process`, `manage`, `doStuff`). If the honest name needs "and", it is two methods. +2. **The layer test.** It does not both decide and perform I/O. Deciding *which* template to fetch and *fetching* it are separate methods. +3. **The reason test.** You can state one change to the product that would require editing it. Two unrelated reasons means split it. + +When you split, the caller keeps the sequencing and each new method keeps one step. Put each part in the home its own row of the table gives it — a split that leaves both halves in the same file has usually not split anything. + +Applies equally to edits: if you are asked to make an existing method "also" do something, the answer is a second method plus a caller, not a longer method. Say that in your response rather than silently growing the function. + +## Step 4 — Report + +State, in one line each: + +- where you put it and which row of the table put it there +- the one responsibility it has +- anything you split out, and where that went + +Then add the test in the mirrored `tests/` path, and run `npx jest ` before claiming it works. + +## Red flags + +- "I'll just add it to the file I'm already in" +- "It's only a few lines, no need to check the doc" +- A new method with `fs` and `axios` both in scope +- A parameter named `options` that switches behaviour between two unrelated jobs +- A boolean parameter that selects which of two things the method does — that is two methods +- Editing an exported method's signature without checking its callers diff --git a/.claude/skills/bumping-cli-version/SKILL.md b/.claude/skills/bumping-cli-version/SKILL.md new file mode 100644 index 00000000..1b2c7550 --- /dev/null +++ b/.claude/skills/bumping-cli-version/SKILL.md @@ -0,0 +1,62 @@ +--- +name: bumping-cli-version +description: Use when preparing a pull request, committing a user-facing change, releasing, or bumping the version - and whenever package.json, package-lock.json or CHANGELOG.md is about to be edited. Also use when the "CLI version check" GitHub action fails. +--- + +# Bumping the CLI version + +Every PR to `main` runs [.github/workflows/cli_version.yml](../../../.github/workflows/cli_version.yml), which fails unless the version was bumped correctly or the bump was explicitly skipped. There is no partial credit — miss one of the three and CI is red. + +## Does this change need a bump? + +Bump when the change affects what a user of the installed CLI gets: a command, its output, a fix, a dependency update. + +Skip only for changes with no effect on the published package — docs, tests, CI config, comments. To skip, the PR body must contain the checkbox ticked exactly: + +``` +- [x] Skip bumping the CLI version +``` + +That string is matched literally against the PR body. Ticking it in a commit message or a comment does nothing. + +## The three edits + +All three, or CI fails. + +1. **`package.json`** — `version` strictly greater than the version on `main`. Equal is a failure, not a pass. +2. **`package-lock.json`** — the top-level `version` set to the *same* string. The workflow compares them directly. `npm version` normally handles both; if the lockfile is stale, edit it rather than reinstalling — see the environment note in [docs/DEVELOPMENT.md](../../../docs/DEVELOPMENT.md). +3. **`CHANGELOG.md`** — a new entry at the top of the list whose heading contains the literal `## []`. + +Verify before pushing: + +```bash +jq -r .version package.json package-lock.json # must print the same string twice +grep -F "## [$(jq -r .version package.json)]" CHANGELOG.md +git fetch origin main && git show origin/main:package.json | jq -r .version # must be lower +``` + +## Changelog entry format + +Match the existing entries — heading, then one line of plain description, no bullet: + +```markdown +## [1.56.2] (11/08/2026) +Add workflow statistics to the stats command. +``` + +Date is `DD/MM/YYYY`. Write what changed for the user, not what changed in the code. + +`lib/cli/changelogReader.js` parses this file at runtime to show users what changed when they update, so the heading format is load-bearing. Do not reformat old entries, add sub-bullets under a version, or introduce `### ` levels. + +## Which number + +- **Patch** (`1.56.1` → `1.56.2`) — bug fix, dependency bump, no interface change. +- **Minor** (`1.56.1` → `1.57.0`) — new command, new option, new capability. +- **Major** — a breaking change to an existing command or its output. Ask before taking one; it is a release decision, not a code decision. + +## Red flags + +- Bumping `package.json` alone and assuming `npm install` will fix the lockfile — it fails with `EACCES` in this checkout +- A changelog entry written from the diff ("refactored `fsUtils`") rather than from the user's view +- Reformatting or re-dating existing changelog entries +- Assuming the skip checkbox applies because the change "feels internal" — if it ships in the package, it needs a bump diff --git a/.claude/skills/writing-tests/SKILL.md b/.claude/skills/writing-tests/SKILL.md new file mode 100644 index 00000000..6d7caed9 --- /dev/null +++ b/.claude/skills/writing-tests/SKILL.md @@ -0,0 +1,76 @@ +--- +name: writing-tests +description: Use when writing, adding, or fixing a Jest test in this repo - a new suite under tests/, a case for a new or changed function, a regression test for a bug, or a failing suite that needs diagnosing. Also use when deciding how to mock sfApi, axios, consola, or the filesystem. +--- + +# Writing tests + +Three harnesses live in this repo and they are not interchangeable. Pick by what the code under test touches, then follow that harness exactly — most test failures here come from using the wrong one, not from the assertion. + +## Pick the harness + +| Code under test | Harness | Lives in | +|---|---|---| +| A function in `lib/` with no HTTP and no disk | **Unit** | `tests/lib/**` | +| A function in `lib/api/sfApi.js` | **API** | `tests/lib/api/sfApi.test.js` | +| A `lib/templates/*.js` class, or anything that writes real files | **Temp-dir unit** | `tests/lib/templates/**`, `tests/lib/utils/**` | +| An `index.js` orchestration function, or a `bin/cli.js` command end to end | **E2E** | `tests/bin/cli/**` | + +The suite path mirrors the source path: `lib/utils/fsUtils.js` → `tests/lib/utils/fsUtils.test.js`. + +## Unit + +Mock every external dependency — `sfApi`, `consola`, and the filesystem if it is touched. Assert on the return value and on which mocked functions were called with what. + +## API + +Only for `sfApi.js`. Intercept a real axios instance with `axios-mock-adapter` rather than mocking axios itself: + +```js +jest.mock("../../../lib/utils/apiUtils", () => ({ + checkRequiredEnvVariables: jest.fn(), // module-load env check must not run + responseSuccessHandler: jest.fn(), + responseErrorHandler: jest.fn().mockResolvedValue(undefined), +})); +jest.mock("../../../lib/api/axiosFactory", () => ({ + AxiosFactory: { createInstance: jest.fn() }, // returns your controlled instance +})); +jest.mock("../../../lib/api/silverfinAuthorizer", () => ({ SilverfinAuthorizer: { /* ... */ } })); +``` + +Then have `AxiosFactory.createInstance` return the instance `AxiosMockAdapter` is attached to. Response bodies come from `fixtures/api-responses//single.json` or `list.json` — extend those rather than inlining a payload. + +## Temp-dir unit + +For template classes, where `save()`, `read()` and `updateTemplateId()` must write real files: `fs.mkdtempSync` plus `process.chdir` into it, inspect the files afterwards, and restore cwd in `afterEach`. + +## E2E + +Six steps, in this order, in every `tests/bin/cli/**` suite: + +1. `fsPromises.mkdtemp` in `os.tmpdir()`, save `process.cwd()`, `process.chdir()` into the temp dir. +2. Copy `fixtures/market-repo/` in when the test needs pre-existing local state. +3. `jest.mock("../../../lib/api/sfApi")` and `jest.mock("consola")`; assign `jest.fn()` to the `consola` methods the code path uses. +4. Replace `process.exit` with `jest.fn()`, keeping the original. **Error paths call `process.exit(1)` and will kill the runner otherwise.** +5. Call the toolkit function directly — `require("../../../index")` — not the CLI binary. +6. `afterEach`: restore `process.cwd()`, restore `process.exit`, `rm` the temp dir with `{ recursive: true, force: true }`. + +Start `beforeEach` with `jest.clearAllMocks()`. + +## Assert on both sides + +An E2E test that only checks `consola.success` was called has not tested anything. Assert on the filesystem result too — `config.json` contents, liquid files present with the right names — and on the API mock having been called with the expected arguments. + +## Before you finish + +- Run `npx jest ` on the suite and read the output. Do not report a test as passing without it. +- New exported function with no test? It is not done. +- Update the catalogue entry in [tests/TESTS.md](../../../tests/TESTS.md) for the suite you changed. + +## Red flags + +- Mocking `axios` directly instead of using `axios-mock-adapter` on a real instance +- An E2E test with no `process.exit` stub, or no `afterEach` cwd restore +- A large inline JSON payload that duplicates something in `fixtures/api-responses/` +- Writing test files anywhere other than the mirrored path +- `process.chdir` without a matching restore — it leaks into every later suite in the run diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 00000000..abcd72d4 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,41 @@ +# silverfin-cli + +Command line tool for Silverfin template development: imports, updates, and creates reconciliation texts, account templates, export files and shared parts against the Silverfin API, and runs liquid tests. + +## Commands + +```bash +npm test # full suite +npx jest # single suite +npm run lint # eslint +``` + +Run `npm run lint && npm test` after code changes. + +## Stack + +Node.js, CommonJS, Commander (CLI), axios (HTTP), Jest (tests), yaml, chokidar, consola. + +## Structure + +``` +bin/cli.js command definitions, option parsing, input validation +index.js orchestration: API call + file write, per template type +lib/api/ HTTP to Silverfin, OAuth, credentials +lib/templates/ serialise/deserialise a template to and from disk +lib/cli/ stats, dev mode, updater, spinner, command validation +lib/utils/ filesystem, parsing, errors, shared helpers +tests/ Jest suites, mirroring the source tree +fixtures/ test data +resources/ shipped assets: shell completion, liquid-test README +``` + +Layers run top to bottom. Code that skips one is wrong: `bin/cli.js` must not call the API directly, and `lib/api/` must not touch the filesystem. + +## Further Reading + +**IMPORTANT: Read the relevant docs below before starting any task.** + +- [docs/ARCHITECTURE.md](docs/ARCHITECTURE.md) — what each file is for, where a method belongs, naming conventions, review checklist. Read before adding, moving, or renaming anything. +- [docs/DEVELOPMENT.md](docs/DEVELOPMENT.md) — test conventions, fixtures, environment gotchas, `CHANGELOG.md` format. Read before writing tests or touching the release path. +- [tests/TESTS.md](tests/TESTS.md) — per-suite catalogue of what is already covered. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md new file mode 100644 index 00000000..ca70dd96 --- /dev/null +++ b/docs/ARCHITECTURE.md @@ -0,0 +1,182 @@ +# Architecture + +A map of what each file is for and what belongs in it. Use it to decide **where** a new method goes — and to judge whether a generated method has been put in the right place. + +## Rules of placement + +1. **One layer per concern.** CLI parsing → orchestration → API/filesystem. Never skip a layer: `bin/cli.js` must not call `lib/api/sfApi.js` directly, and `lib/api/` must not touch the filesystem. +2. **Anything reused twice belongs in `lib/utils/`**, not copied. +3. **Template-shape knowledge lives in `lib/templates/`.** If a method knows about `text_parts`, `name_nl`, or config layout for a specific template type, it belongs there. +4. **A new method should be findable from its name alone.** `fetch*`/`publish*`/`new*` → `index.js`; `read*`/`create*`/`update*`/`find*` (HTTP) → `sfApi.js`; `list*`/`create*File|Folder`/`read|writeConfig` → `fsUtils.js`. +5. **Every exported function gets a test** in the mirrored path under `tests/`. + +## Layers + +``` +bin/cli.js command definitions, option parsing, input validation + └─ index.js orchestration: API call + file write, per template type + ├─ lib/api/ HTTP to Silverfin, auth, credentials + ├─ lib/templates/ serialise/deserialise a template to/from disk + └─ lib/utils/ filesystem, parsing, errors, shared helpers + └─ lib/cli/ everything else a command needs (stats, dev mode, updater) +``` + +--- + +## `bin/cli.js` + +Commander program: one `.command()` block per CLI verb, each with `.option()`s, a short `.description()`, and an `.action()` that validates then delegates. Commands cover import/update/create per template type, `add`/`remove-shared-part`, `run-test`, `create-test`, `check-dependencies`, `authorize`, `stats`, `config`, `get-*-id`, `development-mode`, `generate-export-file`, `update`. + +**Contains:** command wiring only. Validation is delegated to `lib/cli/utils.js`; work is delegated to `index.js` or a `lib/cli/` module. +**Does not contain:** business logic, HTTP calls, `fs` calls, or more than a few lines inside an `.action()`. + +## `index.js` + +Public library surface (`main` in package.json) and the orchestration layer. Every function takes `(type, envId, …)` where `type` is `"firm"` or `"partner"`. + +Per template type (reconciliation text, export file, account template, shared part), the same quartet: + +- `fetchById` / `fetchByHandle|ByName` — pull one template from the API and write it to disk +- `fetchAlls` / `fetchExistings` — paginated pull; "existing" = only those already present locally +- `publishById` / `publishByName|ByHandle` / `publishAlls` — read from disk, push to the API +- `new` / `newAlls` — create a template that does not yet exist remotely + +Plus shared-part linkage (`addSharedPart`, `addAllSharedParts`, `removeSharedPart`), ID lookup (`getTemplateId`, `getAllTemplatesId`) and `updateFirmName`. + +**Contains:** the sequence "call the API → hand the payload to a `lib/templates/` class → report errors". New template-type operations go here, following the naming quartet above. +**Does not contain:** raw axios calls, `fs` calls, or `console.log` formatting beyond progress/error reporting. + +--- + +## `lib/api/` + +### `sfApi.js` +One thin function per Silverfin REST endpoint. Naming mirrors HTTP intent: `create*`, `read*`, `update*`, `find*ByName|ByHandle` (paginated search), plus `getPeriods`, `getCompanyDrop`, `getWorkflows`, `getAccountDetails`, `verifyLiquid`, `createTestRun`, `readTestRun`, `createPreviewRun`, `createExportFileInstance`, and the auth re-exports (`authorizeFirm`, `refreshFirmTokens`, `refreshPartnerToken`). + +**Contains:** URL, params, and a response/error pass-through. Each function is a handful of lines. +**Does not contain:** retries (the axios interceptors do that), filesystem access, or business decisions. A new endpoint = a new small function here. + +### `axiosFactory.js` — `AxiosFactory` +Builds configured axios instances. `createInstance(type, envId)`, `createAuthInstanceForFirm(envId)`; private statics build firm vs partner instances, attach token-refresh interceptors, and handle staging hosts / basic auth. +**Contains:** transport configuration and token-refresh-on-401 logic only. + +### `firmCredentials.js` — exported singleton `firmCredentials` +Reads/writes `~/.silverfin/config.json`. Token pairs (`storeNewTokenPair`, `getTokenPair`), firm names, default firm (`set/getDefaultFirmId`), `listAuthorizedFirms`, partner API keys (`storePartnerApiKey`, `getPartnerCredentials`, `listAuthorizedPartners`), host (`set/getHost`). +**Contains:** the only code that reads or writes the credentials file. Any new stored setting gets a getter/setter pair here. + +### `silverfinAuthorizer.js` — `SilverfinAuthorizer` +OAuth flow: `authorizeFirm`, `refreshFirm`, `refreshPartner`; private helpers prompt for the firm ID and auth code and open the browser. +**Contains:** the interactive authorisation dance. New auth flows go here, not in `sfApi.js`. + +--- + +## `lib/templates/` + +Four classes with an identical static shape — `ReconciliationText`, `AccountTemplate`, `ExportFile`, `SharedPart`. They are the only place that knows the on-disk layout of a template. + +Each exposes: +- `save(type, envId, template)` — API payload → folders, `main.liquid`, text parts, `config.json`, liquid-test stub +- `read(handle|name)` — disk → API payload +- `updateTemplateId(type, envId, handle|name, templateId)` — record the remote ID in `config.json` + +with private statics for config preparation, part filtering, locale defaults, folder-name validation, and main/parts liquid read+create. `SharedPart` additionally has `checkTemplateType` and `#processUsedIn` for the templates a shared part is linked to. + +**Contains:** template-type-specific config keys and folder conventions. +**Does not contain:** HTTP calls. A new field on a template type belongs in that class's `#prepareConfigDetails` / `#filterConfigItems`, not in `index.js`. + +--- + +## `lib/cli/` + +### `utils.js` +Pre-flight checks shared by commands: `loadDefaultFirmId`, `checkDefaultFirm`, `handleUncaughtErrors`, `promptConfirmation`, `formatOption`, `checkDateFormat`, `checkHandleFormat`, `checkUniqueOption`, `checkRequiredFirmOrPartner`, `getCommandSettings`, `runCommandChecks`, `logCurrentHost`, `checkPartnerSupport`. +**Contains:** validation that ends in a clear message + `process.exit` on failure. All new option validation goes here so `bin/cli.js` stays declarative. + +### `stats.js` +Coverage reporting over the local template repo. Entry points `generateOverview(sinceDate)` and `generateWorkflowOverview(sinceDate, workflowHandle)`; the rest are internal — counting templates and YAML tests (`getTemplatesSummary`, `getWorkflowTemplateSummary`, `yamlFilesActivity`, `countYamlFiles`), formatting (`displayOverview`, `createRow`, `percentageRoundTwo`) and CSV persistence (`saveOverviewToFile`, `saveWorkflowOverviewToFile`). +**Contains:** metrics and their presentation. Only the two `generate*` functions should be exported. + +### `devMode.js` +`watchLiquidTest(...)` and `watchLiquidFiles(firmId)` — chokidar watchers that re-run tests or re-push liquid on save. + +### `cliUpdater.js` — `CliUpdater` +`checkVersions()`, `performUpdate()`; privately fetches the latest npm version and compares semver. + +### `changelogReader.js` — `ChangelogReader` +`fetchChanges(userVersion, updateVersion)` — extracts the CHANGELOG entries between two versions. + +### `cwdValidator.js` — `CwdValidator` +`run()` — refuses to operate outside a templates repo. + +### `autoCompletions.js` — `AutoCompletions` +`set()` — installs the shell completion script from `resources/autoCompletion`. + +### `spinner.js` +Exported singleton `spinner` with `spin(text)`, `stop()`, `clear()`. The only terminal-animation code. + +--- + +## `lib/` (top level) + +### `liquidTestRunner.js` +Runs liquid tests against the API and renders the result. Public: `runTests`, `runTestsWithOutput`, `runTestsStatusOnly`, `getHTML`, `checkAllTestsErrorsPresent`, `checkTestErrorsPresent`. Internal: YAML scanning (`findTestRows`, `findAnchorDefinitions`, `findAliasReferences`, `extractAnchorBlocks`, `filterTestsByPattern`), request building (`buildTestParams`), polling (`fetchResult`), and output (`listErrors`, `processTestRunResponse`, `handleHTMLfiles`). +**Contains:** test-run lifecycle and terminal output for results. + +### `liquidTestGenerator.js` +`testGenerator(url, testName, reconciledStatus)` — builds a YAML liquid test from a live Silverfin URL, using `lib/utils/liquidTestUtils.js` for the pieces. + +### `exportFileInstanceGenerator.js` — `ExportFileInstanceGenerator` +`new ExportFileInstanceGenerator(firmId, companyId, periodId, exportFileId)` then `generateAndOpenFile()` — creates an export-file instance, polls it, opens the result. + +--- + +## `lib/utils/` + +### `fsUtils.js` +All filesystem access. Constants `FOLDERS`, `TEMPLATE_TYPES`, `WORKFLOWS_FOLDER`, `SILVERFIN_URL_PATHS`. Config I/O (`configExists`, `readConfig`, `writeConfig`, `createConfigIfMissing`, `getTemplateId`, `setTemplateId`); creation (`createFolder`, `createTemplateFolders`, `createSharedPartFolders`, `createTemplateFiles`, `createLiquidFile`, `createLiquidTestFiles`); discovery (`getAllTemplatesOfAType`, `findHandleByID`, `identifyTypeAndHandle`, `listExistingFiles`, `listExistingRelatedLiquidFiles`, `listSharedPartsUsedInTemplate`, `findTemplatesWithLiquidTests`, `scanTextParts`, `checkLiquidTestDependencies`); workflows (`getWorkflow`, `getAllWorkflowHandles`). +**Contains:** every `fs` call in the codebase. If a new method needs to read or write a file, it goes here and is called from elsewhere. + +### `templateUtils.js` +Template-type vocabulary and name validation: `TEMPLATES_NAME_ATTRIBUTE`, `TEMPLATE_TYPE_NAMES`, `TEMPLATE_MAP_TYPES`, `FILE_NAME_PROBLEMS`, `getTemplateName`, `checkValidName`, `fileNameProblem`, `isSafeName`, `filterParts`, `missingLiquidCode`, `missingNameNL`. +**Contains:** the mapping between API type names and internal type keys. New template types are registered here first. + +### `errorUtils.js` +`uncaughtErrors`, `errorHandler`, `missingConfig`, `missingId`, and `printBatchErrorSummary` for each of the four template types. +**Contains:** every user-facing error message. New failure modes get a named function here rather than an inline `console.error`. + +### `apiUtils.js` +`checkAuthorizePartners`, `checkRequiredEnvVariables`, `responseSuccessHandler`, `responseErrorHandler` — the axios interceptor callbacks. + +### `liquidTestUtils.js` +Pure helpers for generating a liquid test: `createBaseLiquidTest`, `extractURL`, `generateFileName`, `exportYAML`, `processCustom`, `getCompanyDependencies`, `searchForResultsFromDependenciesInLiquid`, `searchForCustomsFromDependenciesInLiquid`, `lookForSharedPartsInLiquid`, `lookForAccountsIDs`. +**Contains:** parsing/transformation only — no HTTP, no `fs`. + +### `runTestUtils.js` +`checkRenderMode(htmlInput, htmlPreview)` — resolves the render mode from CLI flags. + +### `urlHandler.js` — `UrlHandler` +`new UrlHandler(url, customFilename)` + `openFile()` — downloads or resolves a file and opens it, de-duplicating filenames. + +### `wslHandler.js` — `WSLHandler` +`isWSL()`, `open(filePath)` — opening files from WSL. All Windows/WSL-specific behaviour lives here. + +--- + +## Supporting directories + +| Path | Purpose | +|---|---| +| `tests/` | Jest suites mirroring `lib/` and `bin/`; see `tests/TESTS.md`. `tests/setup.js` holds global setup. | +| `fixtures/` | Test data: `api-responses/`, `market-repo/` (a fake templates repo), `silverfin/`. | +| `resources/` | Shipped assets: `autoCompletion/` shell script, `liquidTests/` README template. | +| `jest.config.js`, `eslint.config.js` | Test and lint configuration. | +| `CHANGELOG.md` | Read at runtime by `ChangelogReader` — keep the version-heading format intact. | + +## Review checklist for new code + +- Is the method in the layer that matches its name and its dependencies (`fs` → `fsUtils`, HTTP → `sfApi`)? +- Does it duplicate an existing helper in `lib/utils/`? +- If it is template-type-specific, does it live in the right `lib/templates/` class rather than in a `switch` in `index.js`? +- Is it exported only if callers outside the file need it? +- Does it have a test in the mirrored `tests/` path? +- Do error paths go through `errorUtils.js`? diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md new file mode 100644 index 00000000..f1c6486a --- /dev/null +++ b/docs/DEVELOPMENT.md @@ -0,0 +1,21 @@ +# Development + +Commands and stack are in [CLAUDE.md](../CLAUDE.md); this covers the detail behind them. + +## Tests + +Suites mirror the source tree: `lib/utils/fsUtils.js` → `tests/lib/utils/fsUtils.test.js`, `bin/cli.js` commands → `tests/bin/cli/.test.js`. Every exported function needs one. Global setup lives in `tests/setup.js`; [tests/TESTS.md](../tests/TESTS.md) catalogues what each suite already covers. + +How to write one — which of the three harnesses to use, and how to mock `sfApi`, axios and `consola` — is in the `writing-tests` skill. + +Test data lives in `fixtures/` — `api-responses/` for stubbed Silverfin payloads, `market-repo/` for a fake templates repo, `silverfin/` for credentials-file shapes. Prefer extending a fixture over inlining a large literal in a test. + +## Environment + +`node_modules` is root-owned in some checkouts, so `npm ci` and `npm install` fail with `EACCES`. Repair a single package with `npm pack` plus `tar` rather than reinstalling the tree. + +## Releasing + +Every PR to `main` must bump the version in `package.json` *and* `package-lock.json` and add a matching `## []` entry to `CHANGELOG.md`, or tick `- [x] Skip bumping the CLI version` in the PR body. [.github/workflows/cli_version.yml](../.github/workflows/cli_version.yml) enforces this. The `bumping-cli-version` skill has the full procedure. + +`CHANGELOG.md` is also read at runtime by `lib/cli/changelogReader.js`, which extracts entries between two version headings to show users what changed on update — so the heading format is load-bearing beyond CI. From 5f9bffb04b0e727d347717fc0fbfa9cb2bcbbe7b Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 11 Aug 2026 15:37:02 +0100 Subject: [PATCH 16/24] Add guidance to error handling --- .claude/skills/handling-errors/SKILL.md | 115 ++++++++++++++++++ .../skills/handling-errors/legacy-patterns.md | 21 ++++ 2 files changed, 136 insertions(+) create mode 100644 .claude/skills/handling-errors/SKILL.md create mode 100644 .claude/skills/handling-errors/legacy-patterns.md diff --git a/.claude/skills/handling-errors/SKILL.md b/.claude/skills/handling-errors/SKILL.md new file mode 100644 index 00000000..4b93b69d --- /dev/null +++ b/.claude/skills/handling-errors/SKILL.md @@ -0,0 +1,115 @@ +--- +name: handling-errors +description: Use when writing or changing a catch block, adding a process.exit, reporting a new failure mode, or handling ENOENT / EACCES / a 4xx API response in this repo - including "the CLI crashed with a stack trace", "it printed the whole error object", "it exited halfway through the batch", and errors that are silently swallowed. +--- + +# Handling errors in the CLI + +Every failure this CLI reports is one of two things, and the user can tell them apart by what they see: + +| Kind | Cause | What the user sees | Who prints it | +|---|---|---|---| +| **Expected failure** | Something in their repo, command, or firm is wrong: missing file, missing ID, bad date, 404, 403 | One sentence naming the thing, plus the next command to run | a named function in `lib/utils/errorUtils.js` | +| **Bug** | Our code is wrong: `TypeError`, `ReferenceError`, anything unrecognised | Stack trace, versions, and the "open an issue" banner | `errorUtils.uncaughtErrors` only | + +**A stack trace is a bug report, not an error message.** Printing one for a missing file tells the user to open an issue about their own typo. Printing a bare sentence for a `TypeError` throws away the only evidence we would have had. + +## Step 1 — Classify before you write the catch + +Ask: can the user fix this without changing our code? + +- **Yes** → expected failure. It needs a message in `errorUtils.js` and a next step. +- **No** → bug. Hand it to `errorUtils.errorHandler(error)` and write nothing else. `errorHandler` recognises `ENOENT` and routes everything else to `uncaughtErrors`. +- **Don't know yet** → recognise the cases you know (`error.code`, `error.response.status`) and let the rest fall through to `errorHandler`. Never let "don't know" become a generic message. + +## Step 2 — Write the catch block + +A catch block in this repo has four parts, in this order. Anything missing is a defect: + +1. **Recognise** — branch on `error.code` (`ENOENT`, `EACCES`) or `error.response.status` (400, 403, 404, 422). One branch per case you can name. +2. **Report** — call a named function from `lib/utils/errorUtils.js`. New failure mode means a new function there, not an inline `consola.error` string. Messages go through `consola`, never `console.log`; suggested commands get `chalk.bold`. +3. **Keep the cause** — the raw error goes to `consola.debug(error)` so `-v` still shows it. Recognising an error is not a reason to discard it. +4. **Decide the exit** — see Step 3. Falling off the end of a catch block is a decision too, and usually the wrong one. + +```javascript +// lib/utils/errorUtils.js — the message and the next step live here +function missingWorkflowConfig(handle) { + consola.error(`Workflow ${handle}: config.json was not found in the workflows folder`); + consola.log(`Try running: ${chalk.bold(`silverfin import-workflow --handle ${handle}`)}`); + return false; +} + +// the caller recognises, reports, keeps the cause, and decides +try { + return await sfApi.readWorkflow(envId, handle); +} catch (error) { + consola.debug(error); + if (error.code === "ENOENT") { + return errorUtils.missingWorkflowConfig(handle); // false — caller decides what next + } + errorUtils.errorHandler(error); // unrecognised: stack trace + issue URL +} +``` + +## Step 3 — Only the boundary exits + +`process.exit` is a statement about the whole run, so only code that owns the whole run may call it. + +| Layer | On failure | +|---|---| +`bin/cli.js`, `lib/cli/utils.js` | validate input up front and `process.exit(1)` — nothing has happened yet, so stopping is free +`index.js` | report through `errorUtils`, then exit or return depending on whether more work remains +`lib/utils/`, `lib/api/`, `lib/templates/` | `return` a falsy value or `throw`. **Never exit.** These modules do not know whether they are one step of a 200-template loop + +`lib/utils/*` and `lib/api/*` do contain legacy `process.exit(1)` calls. They are the pattern to move away from, not the one to copy: an exit inside a helper cannot be tested without mocking `process.exit`, skips the spinner teardown, and kills a batch that had 199 templates left. + +## Step 4 — In a loop, defer + +A failure on template 3 of 200 must not end the run. Follow the deferred pattern already in `index.js` `publishAll*`: push a tagged object, keep going, summarise at the end. + +```javascript +deferredErrors.push({ kind: "exception", handle, message, stack: error.stack }); +// after the loop +errorUtils.printReconciliationBatchErrorSummary(deferredErrors); +``` + +`kind` is the repo's stand-in for custom error classes: `"missing_id"`, `"update_failed"`, `"exception"`. Reuse those three. A new kind means teaching the matching `printBatchErrorSummary` to print it — a kind nothing prints is a swallowed error. + +## Step 5 — Touching existing code: raise the legacy pattern, don't silently fix it + +Much of the existing error handling predates this skill. When your change lands in or next to code that matches one of the seven patterns in [legacy-patterns.md](legacy-patterns.md), read that file and name the match in your response: the pattern, the location, what it costs the user, the smallest fix, and whether you should do it now. Then let the dev choose. + +Fix it in the same change only when it sits inside the block you were already editing and the fix is a few lines. Wider than that is a separate branch — say so rather than growing the diff. + +Two failure modes here, both wrong: + +- **Silently rewriting** surrounding error handling because it offends this skill. The diff stops being reviewable. +- **Silently copying** it because it is the local convention. New code follows Steps 1–4 even when its neighbours don't. + +## Step 6 — Global handlers are already installed + +`bin/cli.js` calls `cliUtils.handleUncaughtErrors()`, which wires `uncaughtException` and `unhandledRejection` to `uncaughtErrors`. Do not add `process.on` handlers elsewhere, and do not rely on them as your error handling: a promise that reaches them prints "open an issue" for what may be an ordinary missing file. + +## Never + +- **An empty catch.** `catch { continue }` with no message hides a corrupt YAML file as "not found". Log at `consola.debug` at minimum, and say what was skipped. +- **`consola.error(error)` as the whole handler.** It prints an object or a stack where a sentence belongs, and `process.exit(1)` after it means the user gets a crash for a typo. +- **A message that omits the identifier.** Every message names the handle, name, or path it is about — the user has 200 templates. +- **Discarding the cause.** `catch (err) { consola.error("The URL provided is not correct"); }` loses the one fact that would explain why. +- **`try`/`catch` as flow control.** If you are catching to test whether a file exists, call `configExists` instead. +- **Exposing raw axios errors.** They carry request URLs, params, and tokens. `apiUtils.responseErrorHandler` is where status codes get turned into messages; extend it rather than printing `error.response` at a call site. + +## Red flags + +- "I'll just `consola.error(error)` and exit here" — that is the pattern this skill exists to stop +- A `process.exit` in a file under `lib/utils/` or `lib/api/` +- A new error string typed directly into `index.js` or `bin/cli.js` +- A catch block whose `error` parameter is never read +- A batch loop with `process.exit(1)` inside it +- `errorHandler` called for a failure you could name — it will tell the user to open an issue +- "The file already does it this way, so I'll match it" — Step 5, not a licence +- A diff that quietly rewrites error handling the task never asked about + +## Then test it + +Error paths are the paths users hit. Add the case in the mirrored `tests/` path and run `npx jest ` before claiming it works. Use the **writing-tests** skill for how to mock `consola`, `sfApi`, and `process.exit` in this repo. diff --git a/.claude/skills/handling-errors/legacy-patterns.md b/.claude/skills/handling-errors/legacy-patterns.md new file mode 100644 index 00000000..816a40df --- /dev/null +++ b/.claude/skills/handling-errors/legacy-patterns.md @@ -0,0 +1,21 @@ +# Legacy error-handling patterns + +Seven patterns that are in the codebase today and are **not** the pattern to copy. This is a discussion list, not a fix list: when your change touches one, raise it with the dev in one line and let them decide the scope. + +How to raise it, in your response — the pattern, the location, the cost, the smallest fix, and the call: + +> `index.js:296` catches and prints the raw error. That means a missing config prints a stack instead of a sentence. I can route it through a named `errorUtils` function while I'm here (~5 lines), or leave it and note it. Which? + +Fix it in the same change **only if** it is inside the block you were already editing and the fix is a few lines. Anything wider is a separate branch — say so and move on. + +| # | Pattern | Where it lives | What it costs | Smallest fix | +|---|---|---|---|---| +| 1 | `consola.error(error); process.exit(1);` as the whole handler | ~27 catch blocks in `index.js` (e.g. `:30`, `:63`, `:296`, `:321`) | Prints an object or stack where a sentence belongs; bypasses `errorUtils`, against `docs/ARCHITECTURE.md` | Add a named function to `errorUtils.js`, call it, keep the raw error at `consola.debug` | +| 2 | `process.exit` inside a helper layer | `lib/utils/apiUtils.js:17`, `lib/utils/fsUtils.js:146,341`, `lib/utils/liquidTestUtils.js`, `lib/api/silverfinAuthorizer.js`, `lib/api/axiosFactory.js` | A helper cannot know it is not step 3 of a 200-template loop; kills the batch, skips spinner teardown, forces `process.exit` mocks in tests | Return falsy or `throw`; move the exit up to `index.js` or `bin/cli.js` | +| 3 | Empty or comment-only catch | `lib/utils/fsUtils.js:481` (`catch { continue }`) | Corrupt YAML is reported as absent YAML; the user has no way to find the bad file | `consola.debug` naming the file that was skipped and why | +| 4 | Cause discarded after recognising the error | `lib/utils/liquidTestUtils.js:61`, `lib/utils/fsUtils.js:279` | The one fact that explains the failure is gone even under `-v` | `consola.debug(error)` before the message | +| 5 | Interceptor exits on some statuses, returns `undefined` on others | `lib/utils/apiUtils.js:35-58` (404/400 return, 422/403 exit) | Callers get `undefined`, fail later as a `TypeError`, and route to `uncaughtErrors` — so a plain 404 asks the user to open an issue | Make the branch's contract explicit at the call site: check for the falsy return and report it as an expected failure | +| 6 | Exit codes that misreport the outcome | `lib/cli/utils.js:43` (user declines the prompt → exit 1), `lib/cli/spinner.js:34` (SIGINT → exit 0) | A deliberate cancel scripts as a failure; Ctrl-C scripts as a success. CI cannot tell what happened | Needs a decision on the convention before changing — always raise, never fix silently | +| 7 | No error subclasses; structure only exists as `{ kind }` on batch paths | `lib/utils/errorUtils.js` `print*BatchErrorSummary`, `index.js` `publishAll*` | Single-template paths have no way to carry a machine-readable reason, so each one re-invents ad-hoc branching | Out of scope for an incidental fix. A `class SilverfinError extends Error` is cross-cutting — propose it as its own piece of work | + +Items 1 and 2 are the ones worth pushing on. Item 6 needs a product decision. Item 7 is a project, not a fix. From 0321fa164b47a6e6ee5d2cf0fff8922e926f0940 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 11 Aug 2026 16:06:53 +0100 Subject: [PATCH 17/24] Update AI harness to better handle skill writing --- .claude/skills/adding-methods/SKILL.md | 17 ++++++++++++++--- docs/ARCHITECTURE.md | 4 ++-- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/.claude/skills/adding-methods/SKILL.md b/.claude/skills/adding-methods/SKILL.md index cd9de968..0048d0e0 100644 --- a/.claude/skills/adding-methods/SKILL.md +++ b/.claude/skills/adding-methods/SKILL.md @@ -42,7 +42,17 @@ Three tests it must pass: 2. **The layer test.** It does not both decide and perform I/O. Deciding *which* template to fetch and *fetching* it are separate methods. 3. **The reason test.** You can state one change to the product that would require editing it. Two unrelated reasons means split it. -When you split, the caller keeps the sequencing and each new method keeps one step. Put each part in the home its own row of the table gives it — a split that leaves both halves in the same file has usually not split anything. +When you split, the caller keeps the sequencing and each new method keeps one step. + +A split usually produces a **private helper**: a small unexported function whose only caller is the file it came out of. Leave it there, next to that caller. The Step 2 table places methods other files will reach for; it does not evict a helper from the only file that uses it. Splitting for one responsibility and keeping the pieces together is not a failed split. + +Move a helper out only when one of these is true: + +- a second file needs it — then it goes to the home its own row gives it, and gets exported +- it is generic (it knows nothing about the subject of the file it sits in) **and** you can name the other caller that wants it. "Someone might" is not a caller +- the public function's tests cannot reach one of its branches. That means it is a unit in its own right: move it, export it, and test it directly + +"It has no test of its own" is not a reason to move it. A private helper is tested through the function that calls it. Applies equally to edits: if you are asked to make an existing method "also" do something, the answer is a second method plus a caller, not a longer method. Say that in your response rather than silently growing the function. @@ -52,13 +62,14 @@ State, in one line each: - where you put it and which row of the table put it there - the one responsibility it has -- anything you split out, and where that went +- anything you split out, and where that went — for a private helper you kept in the same file, say so and say why it stayed Then add the test in the mirrored `tests/` path, and run `npx jest ` before claiming it works. ## Red flags -- "I'll just add it to the file I'm already in" +- "I'll just add it to the file I'm already in" — said about a method other files will call +- Exporting a private helper only so a test can reach it - "It's only a few lines, no need to check the doc" - A new method with `fs` and `axios` both in scope - A parameter named `options` that switches behaviour between two unrelated jobs diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index ca70dd96..90197fef 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -93,7 +93,7 @@ Pre-flight checks shared by commands: `loadDefaultFirmId`, `checkDefaultFirm`, ` **Contains:** validation that ends in a clear message + `process.exit` on failure. All new option validation goes here so `bin/cli.js` stays declarative. ### `stats.js` -Coverage reporting over the local template repo. Entry points `generateOverview(sinceDate)` and `generateWorkflowOverview(sinceDate, workflowHandle)`; the rest are internal — counting templates and YAML tests (`getTemplatesSummary`, `getWorkflowTemplateSummary`, `yamlFilesActivity`, `countYamlFiles`), formatting (`displayOverview`, `createRow`, `percentageRoundTwo`) and CSV persistence (`saveOverviewToFile`, `saveWorkflowOverviewToFile`). +Coverage reporting over the local template repo. Entry points `generateOverview(sinceDate)` and `generateWorkflowOverview(sinceDate, workflowHandle)` — the latter returns `false` when nothing could be reported on, leaving the exit to `bin/cli.js`. The rest are internal — workflow selection (`reportOnWorkflow`, `reportOnAllWorkflows`), counting templates and YAML tests (`getTemplatesSummary`, `getWorkflowTemplateSummary`, `yamlFilesActivity`, `countYamlFiles`), formatting (`displayOverview`, `createRow`, `percentageRoundTwo`) and CSV persistence (`saveOverviewToFile`, `saveWorkflowOverviewToFile`). **Contains:** metrics and their presentation. Only the two `generate*` functions should be exported. ### `devMode.js` @@ -141,7 +141,7 @@ Template-type vocabulary and name validation: `TEMPLATES_NAME_ATTRIBUTE`, `TEMPL **Contains:** the mapping between API type names and internal type keys. New template types are registered here first. ### `errorUtils.js` -`uncaughtErrors`, `errorHandler`, `missingConfig`, `missingId`, and `printBatchErrorSummary` for each of the four template types. +`uncaughtErrors`, `errorHandler`, `missingConfig`, `missingId`, and `printBatchErrorSummary` for each of the four template types. Workflow failures: `invalidWorkflowHandle`, `missingWorkflow`, `unparsableWorkflow`, `invalidWorkflow`, `noWorkflowsStored`, `workflowStatisticsNotSaved`, `printWorkflowBatchErrorSummary`. **Contains:** every user-facing error message. New failure modes get a named function here rather than an inline `console.error`. ### `apiUtils.js` From ff09d0888678b5c56e24ba69fe44db87a5bbf99b Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 11 Aug 2026 16:10:27 +0100 Subject: [PATCH 18/24] Update version number --- CHANGELOG.md | 3 +++ package-lock.json | 4 ++-- package.json | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fb52a60..1cca92d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ All notable changes to this project will be documented in this file. +## [1.59.0] (11/08/2026) +Add a workflow filter to the stats command: use `--workflow ` for one workflow or `--workflow` on its own to report on every workflow in the workflows folder. + ## [1.58.0] (31/07/2026) Added the `company-data-copier` command, which triggers the platform Data Copier to copy a source company's data (account values incl. adjustments, text properties, people/company drop and configuration) into a brand-new company in a destination development firm. Intended for BSO developers to reproduce a client's situation in a dev firm without touching the production firm. Only *data* is copied, not template *code* — templates must already exist in the destination firm to be populated. diff --git a/package-lock.json b/package-lock.json index 45fac338..3a3ef741 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "silverfin-cli", - "version": "1.58.0", + "version": "1.59.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "silverfin-cli", - "version": "1.58.0", + "version": "1.59.0", "license": "MIT", "dependencies": { "adm-zip": "^0.6.0", diff --git a/package.json b/package.json index 860a8f68..5cea8b03 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "silverfin-cli", - "version": "1.58.0", + "version": "1.59.0", "description": "Command line tool for Silverfin template development", "main": "index.js", "license": "MIT", From 1817f8908ad7df079b138d99046d2765e7aa79c0 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 11 Aug 2026 16:33:29 +0100 Subject: [PATCH 19/24] Update error catching --- bin/cli.js | 32 ++- docs/ARCHITECTURE.md | 8 +- lib/cli/stats.js | 109 +++++++-- lib/cli/utils.js | 27 +++ lib/utils/constants.js | 10 + lib/utils/errorUtils.js | 108 +++++++++ lib/utils/fsUtils.js | 78 ++++++- lib/utils/templateUtils.js | 46 ++++ tests/TESTS.md | 89 +++++++ tests/lib/cli/stats.test.js | 325 ++++++++++++++++++++++++++ tests/lib/cli/utils.test.js | 108 +++++++++ tests/lib/utils/errorUtils.test.js | 81 +++++++ tests/lib/utils/fsUtils.test.js | 168 +++++++++++++ tests/lib/utils/templateUtils.test.js | 53 +++++ 14 files changed, 1204 insertions(+), 38 deletions(-) create mode 100644 lib/utils/constants.js create mode 100644 tests/lib/cli/stats.test.js create mode 100644 tests/lib/utils/errorUtils.test.js diff --git a/bin/cli.js b/bin/cli.js index 3a764677..bcc9a7ec 100755 --- a/bin/cli.js +++ b/bin/cli.js @@ -626,22 +626,36 @@ program .command("stats") .description("Generate an overview with some statistics") .requiredOption("-s, --since ", "Specify the date which is going to be used to filter the data from (format: YYYY-MM-DD) (mandatory)") - .option("-w, --workflow [handle]", "Filter the statistics by workflow. Without a handle, every workflow stored in the workflows folder is used (optional)") + .option( + "-w, --workflow [handle]", + "Filter the statistics by workflow. Pass a handle (--workflow ) to report on a single workflow, or use --workflow on its own to report on every workflow stored in the workflows folder (optional)" + ) .action(async (options) => { + // Check if the since date is in the correct format cliUtils.checkDateFormat(options.since); - // Commander sets workflow to true when the flag is used without a value - if (typeof options.workflow === "undefined") { + // Commander gives three distinct values: undefined when the workflow flag is absent, + // true when it is used without a value, and the text when a handle is given + if (options.workflow === undefined) { await stats.generateOverview(options.since); return; } - // A blank handle (e.g. an unset --workflow "$HANDLE") is falsy, so it would otherwise be read as "no handle given" and include every workflow - if (typeof options.workflow === "string" && options.workflow.trim() === "") { - consola.error(`An empty workflow handle was provided. Please pass a handle (--workflow ) or use --workflow on its own to include every workflow`); + // The workflow flag was used on its own, so no handle is passed on and the statistics + // cover every workflow stored in the workflows folder + let workflowHandle; + if (options.workflow !== true) { + // A handle was typed. Surrounding spaces are a quoting accident, and a blank handle must + // never be read as "no handle given", so it is checked before it reaches the statistics + workflowHandle = options.workflow.trim(); + // The same command without a handle reports on every workflow stored, which is the + // quickest way for the user to see which handles exist + cliUtils.checkHandleFormat(workflowHandle, "workflow handle", `silverfin stats --since ${options.since} --workflow`); + } + const reported = await stats.generateWorkflowOverview(options.since, workflowHandle); + // Not a single workflow could be reported on. This is the end of the run, so it is + // where the command stops + if (!reported) { process.exit(1); } - // When a handle was typed we pass it on without any surrounding spaces, otherwise the flag was used on its own (Commander gives true instead of text) and undefined means "every workflow" - const workflowHandle = typeof options.workflow === "string" ? options.workflow.trim() : undefined; - await stats.generateWorkflowOverview(options.since, workflowHandle); }); // Set/Get FIRM ID diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 90197fef..2ba3c5a3 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -133,15 +133,19 @@ Runs liquid tests against the API and renders the result. Public: `runTests`, `r ## `lib/utils/` ### `fsUtils.js` -All filesystem access. Constants `FOLDERS`, `TEMPLATE_TYPES`, `WORKFLOWS_FOLDER`, `SILVERFIN_URL_PATHS`. Config I/O (`configExists`, `readConfig`, `writeConfig`, `createConfigIfMissing`, `getTemplateId`, `setTemplateId`); creation (`createFolder`, `createTemplateFolders`, `createSharedPartFolders`, `createTemplateFiles`, `createLiquidFile`, `createLiquidTestFiles`); discovery (`getAllTemplatesOfAType`, `findHandleByID`, `identifyTypeAndHandle`, `listExistingFiles`, `listExistingRelatedLiquidFiles`, `listSharedPartsUsedInTemplate`, `findTemplatesWithLiquidTests`, `scanTextParts`, `checkLiquidTestDependencies`); workflows (`getWorkflow`, `getAllWorkflowHandles`). +All filesystem access. Constants `FOLDERS`, `TEMPLATE_TYPES`, `SILVERFIN_URL_PATHS`. Config I/O (`configExists`, `readConfig`, `writeConfig`, `createConfigIfMissing`, `getTemplateId`, `setTemplateId`); creation (`createFolder`, `createTemplateFolders`, `createSharedPartFolders`, `createTemplateFiles`, `createLiquidFile`, `createLiquidTestFiles`); discovery (`getAllTemplatesOfAType`, `findHandleByID`, `identifyTypeAndHandle`, `listExistingFiles`, `listExistingRelatedLiquidFiles`, `listSharedPartsUsedInTemplate`, `findTemplatesWithLiquidTests`, `scanTextParts`, `checkLiquidTestDependencies`); workflows (`getWorkflow`, `getAllWorkflowHandles`). **Contains:** every `fs` call in the codebase. If a new method needs to read or write a file, it goes here and is called from elsewhere. +### `constants.js` +`WORKFLOWS_FOLDER` — values more than one module in `lib/utils/` must agree on. `fsUtils.js` builds the workflow paths from it; `errorUtils.js` names it in its messages. +**Contains:** plain values and nothing else. It requires no other module, so any module can require it without creating a cycle. A constant used by one file only stays in that file. + ### `templateUtils.js` Template-type vocabulary and name validation: `TEMPLATES_NAME_ATTRIBUTE`, `TEMPLATE_TYPE_NAMES`, `TEMPLATE_MAP_TYPES`, `FILE_NAME_PROBLEMS`, `getTemplateName`, `checkValidName`, `fileNameProblem`, `isSafeName`, `filterParts`, `missingLiquidCode`, `missingNameNL`. **Contains:** the mapping between API type names and internal type keys. New template types are registered here first. ### `errorUtils.js` -`uncaughtErrors`, `errorHandler`, `missingConfig`, `missingId`, and `printBatchErrorSummary` for each of the four template types. Workflow failures: `invalidWorkflowHandle`, `missingWorkflow`, `unparsableWorkflow`, `invalidWorkflow`, `noWorkflowsStored`, `workflowStatisticsNotSaved`, `printWorkflowBatchErrorSummary`. +`uncaughtErrors`, `errorHandler`, `missingConfig`, `missingId`, and `printBatchErrorSummary` for each of the four template types. Command line input: `missingHandle`, `invalidHandleFormat`. Workflow failures: `invalidWorkflowHandle`, `missingWorkflow`, `unparsableWorkflow`, `invalidWorkflow`, `noWorkflowsStored`, `workflowStatisticsNotSaved`, `printWorkflowBatchErrorSummary`. **Contains:** every user-facing error message. New failure modes get a named function here rather than an inline `console.error`. ### `apiUtils.js` diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 3e392697..819e900d 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -3,6 +3,8 @@ const chalk = require("chalk"); const fs = require("fs"); const path = require("path"); const fsUtils = require("../utils/fsUtils"); +const templateUtils = require("../utils/templateUtils"); +const errorUtils = require("../utils/errorUtils"); const yaml = require("yaml"); const { consola } = require("consola"); @@ -15,29 +17,94 @@ async function generateOverview(sinceDate) { saveOverviewToFile(row); } +// Report on a single workflow when a handle is given, or on every workflow stored in the +// workflows folder when it is not +// @param {string} sinceDate The date the statistics start from (YYYY-MM-DD) +// @param {string} [workflowHandle] The workflow to report on. Every workflow when omitted +// @returns {boolean} False when nothing could be reported on. This function does not know +// whether it is the whole run, so stopping is left to the caller async function generateWorkflowOverview(sinceDate, workflowHandle) { - const TODAY = new Date().toJSON().toString().slice(0, 10); - let workflowHandles; if (workflowHandle) { - workflowHandles = [workflowHandle]; - } else { - const workflowsFolder = path.join(process.cwd(), "workflows"); - workflowHandles = fs.readdirSync(workflowsFolder).map((file) => path.basename(file, ".json")); + return reportOnWorkflow(sinceDate, workflowHandle); + } + return reportOnAllWorkflows(sinceDate); +} + +// Build, display and save the statistics of one workflow +// @returns {boolean} False when the workflow could not be read. getWorkflow has already +// said why, so nothing is reported here +async function reportOnWorkflow(sinceDate, workflowHandle) { + const workflow = fsUtils.getWorkflow(workflowHandle); + if (!workflow) { + return false; + } + const TODAY = new Date().toJSON().toString().slice(0, 10); + const templateSummary = await getWorkflowTemplateSummary(workflow); + const yamlSummary = await getYamlSummary(sinceDate, workflow); + displayWorkflowOverview(sinceDate, TODAY, templateSummary, yamlSummary); + const row = createWorkflowRow(sinceDate, TODAY, templateSummary, yamlSummary); + saveWorkflowOverviewToFile(row, workflowHandle); + return true; +} + +// Report on every workflow stored in the workflows folder. One faulty workflow must not +// block the others, so failures are collected and summarised once the loop is over +// @returns {boolean} False when not a single workflow could be reported on +async function reportOnAllWorkflows(sinceDate) { + const workflowHandles = fsUtils.getAllWorkflowHandles(); + if (workflowHandles.length === 0) { + errorUtils.noWorkflowsStored(); + return false; } + + const skippedHandles = []; for (const handle of workflowHandles) { - const workflow = await fsUtils.getWorkflow(handle); - const templateSummary = await getWorkflowTemplateSummary(workflow); - const yamlSummary = await getYamlSummary(sinceDate, workflow); - displayWorkflowOverview(sinceDate, TODAY, templateSummary, yamlSummary); - const row = createWorkflowRow(sinceDate, TODAY, templateSummary, yamlSummary); - saveWorkflowOverviewToFile(row, handle); + const reported = await reportOnWorkflow(sinceDate, handle); + if (!reported) { + consola.warn(`Skipping workflow "${handle}"`); + skippedHandles.push(handle); + } } + + // Make sure a partial run is never mistaken for a complete one + errorUtils.printWorkflowBatchErrorSummary(skippedHandles, workflowHandles.length); + return skippedHandles.length < workflowHandles.length; +} + +// Template names are interpolated into a regular expression. Reconciliation handles are +// restricted to word characters, but account template and export file names are not +function escapeForRegExp(value) { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +// Build the alternation used to match the templates of a workflow (e.g. "(handle_1|handle_2)") +// Returns undefined when there are no templates to match, since an empty alternation +// would silently match nothing at all +function buildTemplatePattern(templateNames) { + if (!templateNames) { + return ".*"; + } + if (templateNames.length === 0) { + return undefined; + } + return `(${templateNames.map(escapeForRegExp).join("|")})`; } // Return an object with the count of activities by file and by type // Type could be: A (added), M (modified), D (deleted) async function yamlFilesActivity(sinceDate, workflow) { const countByType = {}; + + // Files to Search (YAML) + const templatesInWorkflow = workflow ? workflow.templates.reconciliations.concat(workflow.templates.accounts) : undefined; + const TEMPLATE_PATTERN = buildTemplatePattern(templatesInWorkflow); + if (!TEMPLATE_PATTERN) { + consola.info("This workflow contains no reconciliations or account templates, so no YAML file changes can be counted"); + return countByType; + } + const YAML_EXPRESSION = `.*/${TEMPLATE_PATTERN}/tests/.*_liquid_test.*.y(a)?ml`; + const fileTypeRegExp = RegExp(YAML_EXPRESSION, "g"); + const filesChanged = exec.execSync(`git whatchanged --since="${sinceDate}" --name-status --pretty="format:"`); if (!filesChanged) { consola.info("No files were changed since the date provided"); @@ -51,12 +118,6 @@ async function yamlFilesActivity(sinceDate, workflow) { return countByType; } - // Files to Search (YAML) - const templatesInWorkflow = workflow ? workflow.templates.reconciliations.concat(workflow.templates.accounts) : []; - const TEMPLATE_PATTERN = workflow ? `(${templatesInWorkflow.join("|")})` : `.*`; - const YAML_EXPRESSION = `.*/${TEMPLATE_PATTERN}/tests/.*_liquid_test.*.y(a)?ml`; - const fileTypeRegExp = RegExp(YAML_EXPRESSION, "g"); - for (const row of nonEmptyRows) { const fileInfo = row.toString().trim().split("\t"); const fileActivity = fileInfo[0]; @@ -94,9 +155,13 @@ async function yamlFilesActivity(sinceDate, workflow) { // Count how many unit tests are stored. We base on the presence of a title for each unit test // Count how many YAML files have at least two unit tests async function countYamlFiles(templateType, templatesInWorkflow) { + const TEMPLATE_PATTERN = buildTemplatePattern(templatesInWorkflow); + // The workflow holds no template of this type, so there is nothing to count + if (!TEMPLATE_PATTERN) { + return { files: 0, tests: 0, filesWithAtLeastTwoTests: 0 }; + } const files = fsUtils.listExistingFiles("yml"); const FOLDER = fsUtils.FOLDERS[templateType]; - const TEMPLATE_PATTERN = templatesInWorkflow ? `(${templatesInWorkflow.join("|")})` : `.*`; // Issue with counting multiple yml files in the same tests folder? const YAML_EXPRESSION = `.*${FOLDER}/${TEMPLATE_PATTERN}/tests/.*_liquid_test.*.y(a)?ml`; const re = new RegExp(YAML_EXPRESSION, "g"); @@ -599,6 +664,12 @@ function saveOverviewToFile(row) { // content row must be a string with each column separated by ";" function saveWorkflowOverviewToFile(row, workflowHandle) { + // The handle becomes part of the file name, so an unsafe one would write outside ./stats + if (!templateUtils.isSafeName(workflowHandle)) { + errorUtils.workflowStatisticsNotSaved(workflowHandle); + return; + } + const COLUMNS = [ "Workflow Name", "Period - Start", diff --git a/lib/cli/utils.js b/lib/cli/utils.js index 7f83c5e6..6c7daaa9 100644 --- a/lib/cli/utils.js +++ b/lib/cli/utils.js @@ -1,4 +1,5 @@ const errorUtils = require("../utils/errorUtils"); +const templateUtils = require("../utils/templateUtils"); const prompt = require("prompt-sync")({ sigint: true }); const { firmCredentials } = require("../api/firmCredentials"); const { consola } = require("consola"); @@ -75,6 +76,31 @@ function checkDateFormat(dateString) { return true; } +// Stop the CLI when a handle provided on the command line cannot be used to build a file path. +// What makes a handle usable is decided by templateUtils.fileNameProblem and how the failure +// reads is decided by errorUtils. This only picks between the two messages and stops, since +// there is nothing left to run without a handle +// @param {string} handle The handle provided by the user +// @param {string} label How to name the handle in the error message (e.g. "workflow handle"). +// Always written here in the CLI, never taken from user input +// @param {string} [suggestedCommand] A command which lists the handles available, so the user +// is told how to find a valid one instead of only that this one is wrong +function checkHandleFormat(handle, label, suggestedCommand) { + const problem = templateUtils.fileNameProblem(handle); + if (!problem) { + return true; + } + + // A blank handle usually means an unset variable was passed on (e.g. --handle "$HANDLE"), + // which is worth saying instead of listing the characters a handle cannot contain + if (problem === templateUtils.FILE_NAME_PROBLEMS.BLANK) { + errorUtils.missingHandle(label, suggestedCommand); + } else { + errorUtils.invalidHandleFormat(handle, label, suggestedCommand); + } + process.exit(1); +} + // Check unique options function checkUniqueOption(uniqueParameters = [], options) { const optionsToCheck = Object.keys(options).filter((element) => { @@ -176,6 +202,7 @@ module.exports = { promptConfirmation, formatOption, checkDateFormat, + checkHandleFormat, checkUniqueOption, checkRequiredFirmOrPartner, getCommandSettings, diff --git a/lib/utils/constants.js b/lib/utils/constants.js new file mode 100644 index 00000000..55a908c9 --- /dev/null +++ b/lib/utils/constants.js @@ -0,0 +1,10 @@ +// Values which more than one module in lib/utils needs to agree on. +// This module requires nothing, so any module can require it without creating a cycle. + +// Workflows are not templates, so they are not part of fsUtils.FOLDERS. +// fsUtils builds paths from this; errorUtils names it in its messages. +const WORKFLOWS_FOLDER = "workflows"; + +module.exports = { + WORKFLOWS_FOLDER, +}; diff --git a/lib/utils/errorUtils.js b/lib/utils/errorUtils.js index b7b71482..ff97f28d 100644 --- a/lib/utils/errorUtils.js +++ b/lib/utils/errorUtils.js @@ -1,6 +1,10 @@ const pkg = require("../../package.json"); const chalk = require("chalk"); const { consola } = require("consola"); +const { WORKFLOWS_FOLDER } = require("./constants"); + +// How the workflows folder is written in a message, relative to the repository root +const WORKFLOWS_PATH = `./${WORKFLOWS_FOLDER}`; // Uncaught Errors. Open Issue in GitHub function uncaughtErrors(error) { @@ -185,6 +189,101 @@ function printAccountTemplateBatchErrorSummary(errors) { } } +// --- Command line input --- + +/** + * Both handle messages end the same way: the command which lists the handles that exist when + * the caller knows one, and otherwise the shortest thing the user can do about it + * @param {string} label How the handle is named in the message (e.g. "workflow handle") + * @param {string} [suggestedCommand] A command which lists the handles available + */ +function suggestValidHandle(label, suggestedCommand) { + if (suggestedCommand) { + consola.log(`To see the ${label}s available, try running: ${chalk.bold(suggestedCommand)}`); + } else { + consola.log(`Please provide a valid ${label}`); + } +} + +/** + * A handle was expected on the command line but none arrived + * @param {string} label How to name the handle in the message (e.g. "workflow handle") + * @param {string} [suggestedCommand] A command which lists the handles available + * @returns {boolean} False + */ +function missingHandle(label, suggestedCommand) { + consola.error(`No ${label} was provided. Please pass a ${label}, or check that the variable you passed is set`); + suggestValidHandle(label, suggestedCommand); + return false; +} + +/** + * A handle provided on the command line cannot be used to build a file path + * @param {string} handle The handle provided by the user + * @param {string} label How to name the handle in the message (e.g. "workflow handle") + * @param {string} [suggestedCommand] A command which lists the handles available + * @returns {boolean} False + */ +function invalidHandleFormat(handle, label, suggestedCommand) { + consola.error(`Invalid ${label} "${handle}". A ${label} names a file or folder in this repository, so it cannot contain slashes, start with a dot, or contain ".."`); + suggestValidHandle(label, suggestedCommand); + return false; +} + +// --- Workflows --- + +function invalidWorkflowHandle(handle) { + consola.error(`Workflow handle "${handle}" is not valid: it must be the name of a file in ${WORKFLOWS_PATH}`); + return false; +} + +function missingWorkflow(handle, existingHandles = []) { + consola.error(`Workflow "${handle}" was not found in the workflows folder`); + if (existingHandles.length === 0) { + consola.log(`There are no workflows stored in ${WORKFLOWS_PATH}`); + } else { + consola.log(`Workflows available: ${existingHandles.join(", ")}`); + } + return false; +} + +function unparsableWorkflow(handle, reason) { + consola.error(`Workflow "${handle}" could not be parsed as JSON: ${reason}`); + consola.log(`Check ${chalk.bold(`${WORKFLOWS_PATH}/${handle}.json`)}`); + return false; +} + +function invalidWorkflow(handle, problems) { + consola.error(`Workflow "${handle}" is not valid: ${problems.join("; ")}`); + consola.log(`Check ${chalk.bold(`${WORKFLOWS_PATH}/${handle}.json`)}`); + return false; +} + +function noWorkflowsStored() { + consola.error(`No workflows were found in ${WORKFLOWS_PATH}. Please add a workflow file before generating workflow statistics`); + return false; +} + +function workflowStatisticsNotSaved(handle) { + consola.error(`Statistics for "${handle}" were not saved: it is not a valid workflow handle`); + return false; +} + +/** + * Print the workflows skipped during generateWorkflowOverview (after the loop), + * so a partial run is never mistaken for a complete one. + * @param {Array} skippedHandles + * @param {number} total The number of workflows the run started with + */ +function printWorkflowBatchErrorSummary(skippedHandles, total) { + if (!skippedHandles || skippedHandles.length === 0) { + return; + } + consola.log(""); + consola.error(`${skippedHandles.length} of ${total} workflows were skipped: ${skippedHandles.join(", ")}`); + consola.log(`Correct the workflow files in ${WORKFLOWS_PATH} and run the command again`); +} + module.exports = { uncaughtErrors, errorHandler, @@ -197,4 +296,13 @@ module.exports = { printExportFileBatchErrorSummary, printSharedPartBatchErrorSummary, printAccountTemplateBatchErrorSummary, + missingHandle, + invalidHandleFormat, + invalidWorkflowHandle, + missingWorkflow, + unparsableWorkflow, + invalidWorkflow, + noWorkflowsStored, + workflowStatisticsNotSaved, + printWorkflowBatchErrorSummary, }; diff --git a/lib/utils/fsUtils.js b/lib/utils/fsUtils.js index b138ff7b..cbc8a964 100644 --- a/lib/utils/fsUtils.js +++ b/lib/utils/fsUtils.js @@ -2,6 +2,9 @@ const fs = require("fs"); const path = require("path"); const yaml = require("yaml"); const { consola } = require("consola"); +const templateUtils = require("./templateUtils"); +const errorUtils = require("./errorUtils"); +const { WORKFLOWS_FOLDER } = require("./constants"); const FOLDERS = { reconciliationText: "reconciliation_texts", @@ -483,18 +486,76 @@ function checkLiquidTestDependencies(targetHandle) { return dependentHandles; } +// @returns {Array} Array with the handles of every workflow stored in the workflows folder +// @returns {Array} An empty array is returned when the folder is missing or holds no workflow files +function getAllWorkflowHandles() { + const workflowsPath = path.join(process.cwd(), WORKFLOWS_FOLDER); + if (!fs.existsSync(workflowsPath)) { + return []; + } + return fs + .readdirSync(workflowsPath) + .filter((file) => path.extname(file) === ".json") + .map((file) => path.basename(file, ".json")); +} + +// Read a workflow from the workflows folder +// Returns undefined when the handle is unsafe, or when the workflow cannot be found, parsed +// or misses required attributes. It is up to the caller to decide how severe that is function getWorkflow(workflowHandle) { + // The handle is the name of a file in the workflows folder. It cannot be trusted to stay + // inside it, and this function must not exit the process, so an unsafe handle is refused here + if (!templateUtils.isSafeName(workflowHandle)) { + errorUtils.invalidWorkflowHandle(workflowHandle); + return undefined; + } + + const workflowPath = path.join(process.cwd(), WORKFLOWS_FOLDER, `${workflowHandle}.json`); + + if (!fs.existsSync(workflowPath)) { + errorUtils.missingWorkflow(workflowHandle, getAllWorkflowHandles()); + return undefined; + } + + let workflow; try { - const workflowPath = path.join(process.cwd(), "workflows", `${workflowHandle}.json`); - if (!fs.existsSync(workflowPath)) { - throw new Error(`Workflow "${workflowHandle}" not found`); - } - return JSON.parse(fs.readFileSync(workflowPath).toString()); + workflow = JSON.parse(fs.readFileSync(workflowPath).toString()); } catch (error) { - consola.error(`An error occurred when trying to read the workflow "${workflowHandle}"`); - consola.error(error); - process.exit(1); + consola.debug(error); + errorUtils.unparsableWorkflow(workflowHandle, error.message); + return undefined; + } + + const problems = workflowAttributeProblems(workflow); + if (problems.length > 0) { + errorUtils.invalidWorkflow(workflowHandle, problems); + return undefined; + } + + return workflow; +} + +// Check that a workflow holds every attribute the CLI relies on +// @param {Object} workflow The workflow to check +// @returns {Array} An array of messages, empty when the workflow is valid +function workflowAttributeProblems(workflow) { + const problems = []; + if (!workflow || typeof workflow !== "object" || Array.isArray(workflow)) { + return [`it should contain a JSON object`]; + } + if (!workflow.name || typeof workflow.name !== "string") { + problems.push(`"name" is missing or is not a text value`); + } + if (!workflow.templates || typeof workflow.templates !== "object" || Array.isArray(workflow.templates)) { + problems.push(`"templates" is missing or is not an object`); + return problems; + } + for (const attribute of ["reconciliations", "accounts", "exports"]) { + if (!Array.isArray(workflow.templates[attribute])) { + problems.push(`"templates.${attribute}" is missing or is not an array`); + } } + return problems; } // Recursive option for fs.watch is not available in every OS (e.g. Linux) @@ -637,5 +698,6 @@ module.exports = { setTemplateId, checkLiquidTestDependencies, getWorkflow, + getAllWorkflowHandles, scanTextParts, }; diff --git a/lib/utils/templateUtils.js b/lib/utils/templateUtils.js index de3f7f57..d7336968 100644 --- a/lib/utils/templateUtils.js +++ b/lib/utils/templateUtils.js @@ -23,6 +23,12 @@ const TEMPLATE_MAP_TYPES = { account_template: "accountTemplate", }; +/** The reasons a name cannot be used, so callers can tell a missing name from a malformed one. Reported by fileNameProblem */ +const FILE_NAME_PROBLEMS = { + BLANK: "blank", + UNSAFE: "unsafe", +}; + /** Get the name of the template from the template or config object (based on it's type) */ function getTemplateName(template, templateType) { return template[TEMPLATES_NAME_ATTRIBUTE[templateType]]; @@ -50,6 +56,43 @@ function checkValidName(name, templateType) { return true; } +/** + * Report why a name provided by the user cannot be used as a file or folder name. + * Handles are interpolated into paths (e.g. ./workflows/.json), so a name which + * resolves to another folder would read or write files outside the repository. + * This only covers path safety: the characters a template name may hold are checked by + * checkValidName, since those rules differ per template type. + * It is up to the caller to decide how severe a problem is. The CLI stops on one, while + * code which works through several names warns and carries on with the rest + * @param {string} name The name to check + * @returns {string|null} A FILE_NAME_PROBLEMS value, or null when the name can be used + */ +function fileNameProblem(name) { + if (typeof name !== "string" || name.trim() === "") { + return FILE_NAME_PROBLEMS.BLANK; + } + // Separators (both kinds, whatever the OS) and null bytes would point at another folder + if (/[/\\\0]/.test(name)) { + return FILE_NAME_PROBLEMS.UNSAFE; + } + // "." is the folder itself and ".." its parent. A leading dot also hides the file + if (name.startsWith(".") || name.includes("..")) { + return FILE_NAME_PROBLEMS.UNSAFE; + } + return null; +} + +/** + * Check that a name provided by the user is safe to use as a file or folder name. + * For callers which only need to know whether the name can be used. Use fileNameProblem + * when the reason matters, for example to word an error message + * @param {string} name The name to check + * @returns {boolean} True when the name stays within the folder it is joined to + */ +function isSafeName(name) { + return fileNameProblem(name) === null; +} + /** Process response provided by the Silverfin API and return an object with the text parts */ function filterParts(template) { const textPartsReducer = (acc, part) => { @@ -83,8 +126,11 @@ module.exports = { TEMPLATES_NAME_ATTRIBUTE, TEMPLATE_TYPE_NAMES, TEMPLATE_MAP_TYPES, + FILE_NAME_PROBLEMS, getTemplateName, checkValidName, + fileNameProblem, + isSafeName, filterParts, missingLiquidCode, missingNameNL, diff --git a/tests/TESTS.md b/tests/TESTS.md index f61f75e3..e186f2f9 100644 --- a/tests/TESTS.md +++ b/tests/TESTS.md @@ -43,6 +43,7 @@ tests/ │ ├── changelogReader.test.js │ ├── cliUpdater.test.js │ ├── cwdValidator.test.js + │ ├── stats.test.js │ └── utils.test.js ├── templates/ │ ├── reconciliationTexts.test.js @@ -510,6 +511,34 @@ Source: `lib/cli/cwdValidator.js` --- +### `tests/lib/cli/stats.test.js` +Source: `lib/cli/stats.js` + +| Function | Test | Description | +|---|---|---| +| `generateWorkflowOverview` (no handle) | should inform the user and stop when the workflows folder is missing | Verifies that an error naming the `workflows` folder is logged and no `stats` folder is written when the folder does not exist. | +| `generateWorkflowOverview` (no handle) | should inform the user and stop when the workflows folder is empty | Verifies that an empty `workflows` folder produces the same message and no exit. | +| `generateWorkflowOverview` (no handle) | should ignore non-JSON files when discovering workflows | Verifies that a `README.md` in the `workflows` folder is not treated as a workflow. | +| `generateWorkflowOverview` (no handle) | should skip a faulty workflow and still process the valid ones | Verifies that an unparsable workflow is skipped with a warning while the valid one still produces its CSV. | +| `generateWorkflowOverview` (no handle) | should report a tally so a partial run is not mistaken for a complete one | Verifies that the number of skipped workflows and their handles are reported after the loop, and that a partial run still returns `true`. | +| `generateWorkflowOverview` (no handle) | should report a failure when every workflow was skipped | Verifies that `false` is returned when not a single workflow could be reported on. | +| `generateWorkflowOverview` (no handle) | should not report a tally when every workflow is valid | Verifies that no skip summary is printed when nothing was skipped. | +| `generateWorkflowOverview` (handle) | should report a failure when the requested workflow does not exist | Verifies that `false` is returned, the handle is named, and the function does not exit the process itself. | +| `generateWorkflowOverview` (handle) | should report a failure when the requested workflow is malformed | Verifies that the missing attribute is named and `false` is returned. | +| `generateWorkflowOverview` (handle) | should report a failure on a handle which points outside the workflows folder | Verifies that a traversal handle (`../escape`) is refused and writes no file outside the repository. | +| `generateWorkflowOverview` (handle) | should not skip the workflow it was asked for | Verifies that the batch "Skipping workflow" wording is not used when a single handle was requested. | +| `generateWorkflowOverview` (handle) | should produce a CSV named after the workflow handle | Verifies that the statistics are written to `stats/_stats.csv`. | +| `generateWorkflowOverview` (handle) | should count only the templates belonging to the workflow | Verifies that templates outside the workflow are excluded from the counts. | +| `generateWorkflowOverview` (no templates) | should report zero without silently matching every template | Verifies that an empty template list does not fall back to the `.*` pattern used by the repository-wide overview. | +| `generateWorkflowOverview` (no templates) | should explain that no YAML changes can be counted | Verifies that the user is told why the YAML activity counts are empty. | +| `generateWorkflowOverview` (no templates) | should not run the git scan when there is nothing to match | Verifies that `git whatchanged` is not invoked when the workflow holds no templates. | +| `generateWorkflowOverview` (regex characters) | should treat a quantifier in a name literally | Verifies that a template name containing `+` or `*` is escaped before being interpolated into the match pattern. | +| `generateWorkflowOverview` (regex characters) | should not build an invalid regular expression from a name with brackets | Verifies that a name containing brackets does not throw when the pattern is compiled. | +| `generateOverview` | should count every template in the repository | Verifies that the repository-wide overview counts all templates and writes `stats/stats.csv`. | +| `generateOverview` | should append a row when run a second time | Verifies that a second run adds a row rather than replacing the file. | + +--- + ### `tests/lib/cli/utils.test.js` Source: `lib/cli/utils.js` @@ -524,6 +553,22 @@ Source: `lib/cli/utils.js` | `formatOption` | should convert camelCase to kebab-case with leading dash on uppercase | Verifies that `listAll` is converted to `list-all`. | | `formatOption` | should handle single word with no uppercase | Verifies that a single lowercase word is returned unchanged. | | `formatOption` | should convert multiple uppercase letters | Verifies that `importReconciliationText` is converted to `import-reconciliation-text`. | +| `checkDateFormat` | should return true for a valid YYYY-MM-DD date | Verifies that a correctly formatted date is accepted. | +| `checkDateFormat` | should accept a leap day in a leap year | Verifies that `2024-02-29` is accepted. | +| `checkDateFormat` | should call process.exit(1) for a date in the wrong format | Verifies that a date which is not `YYYY-MM-DD` stops the command. | +| `checkDateFormat` | should call process.exit(1) for a non-date string | Verifies that arbitrary text stops the command. | +| `checkDateFormat` | should call process.exit(1) for an empty string | Verifies that an empty value stops the command. | +| `checkDateFormat` | should call process.exit(1) when the value is not a string | Verifies that a non-string value stops the command instead of throwing. | +| `checkDateFormat` | should reject a correctly formatted but impossible calendar date | Verifies that a well-formed but non-existent date is rejected. | +| `checkDateFormat` | should reject a leap day outside a leap year | Verifies that `2023-02-29` is rejected. | +| `checkDateFormat` | should reject a month outside the valid range | Verifies that a month above 12 is rejected. | +| `checkDateFormat` | should reject input containing shell metacharacters | Verifies that the date cannot carry shell syntax into the `git` command built from it. | +| `checkHandleFormat` | should return true for a handle which names a file | Verifies that an ordinary handle is accepted without exiting. | +| `checkHandleFormat` | should call process.exit(1) for the handle %p | Verifies that separators, `..` and leading dots are reported through `errorUtils.invalidHandleFormat` and stop the command. | +| `checkHandleFormat` | should report a blank handle %p as missing | Verifies that an empty or whitespace handle goes to `errorUtils.missingHandle` rather than being reported as malformed. | +| `checkHandleFormat` | should report a missing handle as missing | Verifies that `undefined` goes to `errorUtils.missingHandle`. | +| `checkHandleFormat` | should pass the suggested command on to the error message | Verifies that the command the caller offers reaches `errorUtils.invalidHandleFormat`. | +| `checkHandleFormat` | should pass the suggested command on when the handle is missing | Verifies that the command the caller offers reaches `errorUtils.missingHandle`. | | `checkUniqueOption` | should return true when exactly one unique option is used | Verifies that `true` is returned and no error is logged when exactly one of the mutually exclusive options is set. | | `checkUniqueOption` | should call process.exit(1) when none of the unique options are used | Verifies that an error is logged and `process.exit(1)` is called when none of the required options are present. | | `checkUniqueOption` | should call process.exit(1) when more than one unique option is used | Verifies that an error about incompatible options is logged and `process.exit(1)` is called when multiple exclusive options are set. | @@ -657,6 +702,25 @@ Source: `lib/utils/fsUtils.js` (`checkLiquidTestDependencies`) --- +### `tests/lib/utils/errorUtils.test.js` +Source: `lib/utils/errorUtils.js` + +| Function | Test | Description | +|---|---|---| +| `missingHandle` | should name the label and mention an unset variable | Verifies that the message names the kind of handle expected and points at an unset variable as the likely cause. | +| `missingHandle` | should suggest the command which lists the handles available | Verifies that the caller's command is printed as the next step. | +| `missingHandle` | should ask for a valid handle when no command is suggested | Verifies that the user is still told what to do when the caller offers no command. | +| `missingHandle` | should return false | Verifies that the caller decides what happens next. | +| `invalidHandleFormat` | should name the handle and the label | Verifies that the rejected handle appears in the message. | +| `invalidHandleFormat` | should say what a handle cannot contain | Verifies that the message explains why the handle cannot be used as a file name. | +| `invalidHandleFormat` | should suggest the command which lists the handles available | Verifies that the caller's command is printed as the next step. | +| `invalidHandleFormat` | should ask for a valid handle when no command is suggested | Verifies that the user is still told what to do when the caller offers no command. | +| `invalidHandleFormat` | should return false | Verifies that the caller decides what happens next. | +| `noWorkflowsStored` | should point at the workflows folder fsUtils reads from | Verifies that the folder named in the message comes from `lib/utils/constants.js`, so it cannot drift from the folder `fsUtils` reads. | +| `unparsableWorkflow` | should point at the workflow file inside that folder | Verifies that the file path offered to the user is built from the same shared constant. | + +--- + ### `tests/lib/utils/findTemplatesWithLiquidTests.test.js` Source: `lib/utils/fsUtils.js` (`findTemplatesWithLiquidTests`) @@ -717,6 +781,24 @@ Source: `lib/utils/fsUtils.js` | `createConfigIfMissing` | should create a default config for exportFile when missing | Verifies that a default `config.json` with `name_nl` and `encoding` fields is created for an export file. | | `createConfigIfMissing` | should create a default config for accountTemplate when missing | Verifies that a default `config.json` with `name_nl` and `account_range` fields is created for an account template. | | `createConfigIfMissing` | should not overwrite an existing config | Verifies that calling `createConfigIfMissing` when a `config.json` already exists leaves the file unchanged. | +| `getAllWorkflowHandles` | should return an empty array when the workflows folder does not exist | Verifies that a missing `workflows` folder is not an error. | +| `getAllWorkflowHandles` | should return an empty array when the workflows folder is empty | Verifies that an empty folder returns no handles. | +| `getAllWorkflowHandles` | should return the handles of every workflow file | Verifies that each `.json` file name is returned without its extension. | +| `getAllWorkflowHandles` | should ignore files which are not JSON | Verifies that `README.md` and `.DS_Store` are excluded. | +| `getWorkflow` | should return the parsed workflow when it is valid | Verifies that a valid workflow file is parsed and returned without any error. | +| `getWorkflow` | should accept a workflow with empty template lists | Verifies that empty (but present) template arrays are valid. | +| `getWorkflow` | should return undefined and warn when the workflow does not exist | Verifies that a missing workflow returns `undefined` and names the handle. | +| `getWorkflow` | should list the available handles when the workflow does not exist | Verifies that the workflows actually stored are offered as alternatives. | +| `getWorkflow` | should report when there are no workflows stored at all | Verifies that the message distinguishes "typo" from "nothing imported yet". | +| `getWorkflow` | should return undefined and warn when the file is not valid JSON | Verifies that a parse failure is reported rather than thrown. | +| `getWorkflow` | should return undefined and name the missing attribute when templates.accounts is absent | Verifies that the specific missing attribute is named. | +| `getWorkflow` | should return undefined when templates is missing entirely | Verifies that a workflow without a `templates` object is refused. | +| `getWorkflow` | should return undefined when name is missing | Verifies that a workflow without a `name` is refused. | +| `getWorkflow` | should return undefined when a template attribute is not an array | Verifies that a string where an array is expected is refused. | +| `getWorkflow` | should return undefined when the file holds a JSON array instead of an object | Verifies that a top-level array is refused. | +| `getWorkflow` | should refuse the unsafe handle %p without reading any file | Verifies that a traversal handle is rejected before `readFileSync` is reached. | +| `getWorkflow` | should accept a handle containing a dot | Verifies that a dot inside the name (as opposed to a leading dot or `..`) is allowed. | +| `getWorkflow` | should not exit the process on any invalid workflow | Verifies that the helper leaves the exit decision to its caller. | --- @@ -760,6 +842,13 @@ Source: `lib/utils/templateUtils.js` | `checkValidName` | should return true for empty string (reconciliationText) | Verifies that an empty string passes the reconciliation text regex (matches zero characters). | | `checkValidName` | should return false for string with unicode characters (reconciliationText) | Verifies that a handle with non-ASCII characters fails validation and a warning is logged. | | `checkValidName` | should return true for valid alphanumeric sharedPart name | Verifies that a shared part name with alphanumerics and underscores passes validation. | +| `isSafeName` | should accept the name %p | Verifies that ordinary handles, including ones with dots, spaces and brackets, are accepted. | +| `isSafeName` | should refuse the name %p | Verifies that separators, `..`, leading dots and null bytes are refused. | +| `isSafeName` | should refuse the non-string value %p | Verifies that a non-string value is refused rather than throwing. | +| `fileNameProblem` | should report no problem for the name %p | Verifies that `null` is returned for a usable name. | +| `fileNameProblem` | should report %p as blank | Verifies that empty, whitespace, `undefined` and `null` are reported as `BLANK`. | +| `fileNameProblem` | should report %p as unsafe | Verifies that a path-escaping name is reported as `UNSAFE`, so the caller can word a different message. | +| `fileNameProblem` | should report the non-string value %p as blank | Verifies that non-string values fall into the `BLANK` case. | | `filterParts` | should reduce text_parts array to {name: content} object with 2 parts | Verifies that an array of `{ name, content }` objects is transformed into a `{ name: content }` map. | | `filterParts` | should return empty object for empty array | Verifies that an empty `text_parts` array produces an empty object. | | `filterParts` | should include part with empty name as key | Verifies that a text part with an empty string name is included in the output with `""` as the key. | diff --git a/tests/lib/cli/stats.test.js b/tests/lib/cli/stats.test.js new file mode 100644 index 00000000..c2366bbc --- /dev/null +++ b/tests/lib/cli/stats.test.js @@ -0,0 +1,325 @@ +const fs = require("fs"); +const os = require("os"); +const path = require("path"); +const exec = require("child_process"); + +jest.mock("consola"); + +const { consola } = require("consola"); +const stats = require("../../../lib/cli/stats"); + +const VALID_WORKFLOW = { + name: "Workflow 1", + templates: { + reconciliations: ["reconciliation_text_1"], + accounts: ["account_1"], + exports: ["export_1"], + }, +}; + +describe("cli/stats", () => { + let tempDir; + let originalCwd; + let mockExit; + let execSyncSpy; + + const writeWorkflow = (handle, content) => { + const workflowsPath = path.join(tempDir, "workflows"); + fs.mkdirSync(workflowsPath, { recursive: true }); + fs.writeFileSync(path.join(workflowsPath, `${handle}.json`), typeof content === "string" ? content : JSON.stringify(content)); + }; + + // Create a template folder with a non-empty main.liquid, a config and a liquid test + const writeTemplate = (folder, handle, { unitTests = 0, externallyManaged = false } = {}) => { + const templatePath = path.join(tempDir, folder, handle); + fs.mkdirSync(path.join(templatePath, "tests"), { recursive: true }); + fs.writeFileSync(path.join(templatePath, "main.liquid"), "{% comment %}\nsome liquid\n{% endcomment %}"); + fs.writeFileSync(path.join(templatePath, "config.json"), JSON.stringify({ handle, externally_managed: externallyManaged })); + if (unitTests > 0) { + const testContent = Array.from({ length: unitTests }, (_, index) => `unit_test_${index + 1}:\n context:\n period: 2024-12-31\n`).join(""); + fs.writeFileSync(path.join(templatePath, "tests", `${handle}_liquid_test.yml`), testContent); + } + }; + + beforeEach(() => { + jest.clearAllMocks(); + originalCwd = process.cwd(); + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "sf-cli-stats-test-")); + process.chdir(tempDir); + mockExit = jest.spyOn(process, "exit").mockImplementation(() => {}); + // The stats module shells out to git; no repository exists in the temp dir + execSyncSpy = jest.spyOn(exec, "execSync").mockReturnValue(""); + }); + + afterEach(() => { + process.chdir(originalCwd); + mockExit.mockRestore(); + execSyncSpy.mockRestore(); + if (tempDir && fs.existsSync(tempDir)) { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + // ─── generateWorkflowOverview: workflow discovery ────────────────────────── + + describe("generateWorkflowOverview without a handle", () => { + it("should inform the user and stop when the workflows folder is missing", async () => { + const reported = await stats.generateWorkflowOverview("2024-01-01"); + + expect(reported).toBe(false); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("No workflows were found")); + expect(mockExit).not.toHaveBeenCalled(); + expect(fs.existsSync(path.join(tempDir, "stats"))).toBe(false); + }); + + it("should inform the user and stop when the workflows folder is empty", async () => { + fs.mkdirSync(path.join(tempDir, "workflows"), { recursive: true }); + + const reported = await stats.generateWorkflowOverview("2024-01-01"); + + expect(reported).toBe(false); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("No workflows were found")); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it("should ignore non-JSON files when discovering workflows", async () => { + fs.mkdirSync(path.join(tempDir, "workflows"), { recursive: true }); + fs.writeFileSync(path.join(tempDir, "workflows", "README.md"), "# not a workflow"); + + const reported = await stats.generateWorkflowOverview("2024-01-01"); + + expect(reported).toBe(false); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("No workflows were found")); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it("should skip a faulty workflow and still process the valid ones", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 2 }); + writeTemplate("account_templates", "account_1"); + writeTemplate("export_files", "export_1"); + writeWorkflow("workflow_valid", VALID_WORKFLOW); + writeWorkflow("workflow_broken", "{ not json"); + + await stats.generateWorkflowOverview("2024-01-01"); + + // The valid workflow produced its CSV + expect(fs.existsSync(path.join(tempDir, "stats", "workflow_valid_stats.csv"))).toBe(true); + // The broken one did not + expect(fs.existsSync(path.join(tempDir, "stats", "workflow_broken_stats.csv"))).toBe(false); + expect(mockExit).not.toHaveBeenCalled(); + expect(consola.warn).toHaveBeenCalledWith(expect.stringContaining('Skipping workflow "workflow_broken"')); + }); + + it("should report a tally so a partial run is not mistaken for a complete one", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 1 }); + writeTemplate("account_templates", "account_1"); + writeTemplate("export_files", "export_1"); + writeWorkflow("workflow_valid", VALID_WORKFLOW); + writeWorkflow("workflow_broken", "{ not json"); + writeWorkflow("workflow_missing_accounts", { name: "Missing", templates: { reconciliations: [], exports: [] } }); + + const reported = await stats.generateWorkflowOverview("2024-01-01"); + + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("2 of 3 workflows were skipped")); + // One workflow still produced its statistics, so the run was not a failure + expect(reported).toBe(true); + }); + + it("should report a failure when every workflow was skipped", async () => { + writeWorkflow("workflow_broken", "{ not json"); + writeWorkflow("workflow_missing_accounts", { name: "Missing", templates: { reconciliations: [], exports: [] } }); + + const reported = await stats.generateWorkflowOverview("2024-01-01"); + + expect(reported).toBe(false); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("2 of 2 workflows were skipped")); + }); + + it("should not report a tally when every workflow is valid", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 1 }); + writeTemplate("account_templates", "account_1"); + writeTemplate("export_files", "export_1"); + writeWorkflow("workflow_valid", VALID_WORKFLOW); + + await stats.generateWorkflowOverview("2024-01-01"); + + expect(consola.error).not.toHaveBeenCalledWith(expect.stringContaining("were skipped")); + }); + }); + + // ─── generateWorkflowOverview: explicit handle ───────────────────────────── + + describe("generateWorkflowOverview with an explicit handle", () => { + it("should report a failure when the requested workflow does not exist", async () => { + writeWorkflow("workflow_valid", VALID_WORKFLOW); + + const reported = await stats.generateWorkflowOverview("2024-01-01", "no_such_workflow"); + + expect(reported).toBe(false); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("no_such_workflow")); + // Stopping the run is the caller's decision, not this function's + expect(mockExit).not.toHaveBeenCalled(); + }); + + it("should report a failure when the requested workflow is malformed", async () => { + writeWorkflow("workflow_broken", { name: "Broken", templates: { reconciliations: [] } }); + + const reported = await stats.generateWorkflowOverview("2024-01-01", "workflow_broken"); + + expect(reported).toBe(false); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("templates.accounts")); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it("should report a failure on a handle which points outside the workflows folder", async () => { + writeWorkflow("workflow_valid", VALID_WORKFLOW); + const escapedPath = path.join(tempDir, "..", "escape_stats.csv"); + + const reported = await stats.generateWorkflowOverview("2024-01-01", "../escape"); + + expect(reported).toBe(false); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("is not valid")); + expect(fs.existsSync(escapedPath)).toBe(false); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it("should not skip the workflow it was asked for", async () => { + writeWorkflow("workflow_valid", VALID_WORKFLOW); + + await stats.generateWorkflowOverview("2024-01-01", "no_such_workflow"); + + // The batch wording belongs to the every-workflow run, where carrying on makes sense + expect(consola.warn).not.toHaveBeenCalledWith(expect.stringContaining("Skipping workflow")); + expect(consola.error).not.toHaveBeenCalledWith(expect.stringContaining("were skipped")); + }); + + it("should produce a CSV named after the workflow handle", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 2 }); + writeTemplate("account_templates", "account_1"); + writeTemplate("export_files", "export_1"); + writeWorkflow("workflow_valid", VALID_WORKFLOW); + + await stats.generateWorkflowOverview("2024-01-01", "workflow_valid"); + + const csvPath = path.join(tempDir, "stats", "workflow_valid_stats.csv"); + expect(fs.existsSync(csvPath)).toBe(true); + const csv = fs.readFileSync(csvPath, "utf-8"); + expect(csv).toContain("Workflow Name"); + expect(csv).toContain("Workflow 1"); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it("should count only the templates belonging to the workflow", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 2 }); + // This template has tests but is NOT part of the workflow + writeTemplate("reconciliation_texts", "reconciliation_text_outside", { unitTests: 3 }); + writeTemplate("account_templates", "account_1"); + writeTemplate("export_files", "export_1"); + writeWorkflow("workflow_valid", VALID_WORKFLOW); + + await stats.generateWorkflowOverview("2024-01-01", "workflow_valid"); + + const csv = fs.readFileSync(path.join(tempDir, "stats", "workflow_valid_stats.csv"), "utf-8"); + const values = csv.trim().split("\r\n")[1].split(";"); + // "Reconciliations - templates" and "Reconciliations - unit tests" + expect(values[9]).toBe("1"); + expect(values[12]).toBe("2"); + }); + }); + + // ─── empty template lists ────────────────────────────────────────────────── + + describe("workflow without templates", () => { + it("should report zero without silently matching every template", async () => { + // These templates exist in the repo but belong to no workflow + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 5 }); + writeTemplate("account_templates", "account_1", { unitTests: 4 }); + writeWorkflow("workflow_empty", { name: "Empty Workflow", templates: { reconciliations: [], accounts: [], exports: [] } }); + + await stats.generateWorkflowOverview("2024-01-01", "workflow_empty"); + + const csv = fs.readFileSync(path.join(tempDir, "stats", "workflow_empty_stats.csv"), "utf-8"); + const values = csv.trim().split("\r\n")[1].split(";"); + // All - templates, All - yaml files, All - unit tests + expect(values[5]).toBe("0"); + expect(values[7]).toBe("0"); + expect(values[8]).toBe("0"); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it("should explain that no YAML changes can be counted", async () => { + writeWorkflow("workflow_empty", { name: "Empty Workflow", templates: { reconciliations: [], accounts: [], exports: [] } }); + + await stats.generateWorkflowOverview("2024-01-01", "workflow_empty"); + + expect(consola.info).toHaveBeenCalledWith(expect.stringContaining("no reconciliations or account templates")); + }); + + it("should not run the git scan when there is nothing to match", async () => { + writeWorkflow("workflow_empty", { name: "Empty Workflow", templates: { reconciliations: [], accounts: [], exports: [] } }); + + await stats.generateWorkflowOverview("2024-01-01", "workflow_empty"); + + expect(execSyncSpy).not.toHaveBeenCalled(); + }); + }); + + // ─── regex safety ────────────────────────────────────────────────────────── + + describe("template names containing regular expression characters", () => { + it("should treat a quantifier in a name literally", async () => { + // Unescaped, "account_1+2" means "account_" then one or more "1", so it would + // match "account_1112" and miss the template actually named "account_1+2" + writeTemplate("account_templates", "account_1+2", { unitTests: 2 }); + writeTemplate("account_templates", "account_1112", { unitTests: 7 }); + writeWorkflow("workflow_plus", { name: "Plus", templates: { reconciliations: [], accounts: ["account_1+2"], exports: [] } }); + + await stats.generateWorkflowOverview("2024-01-01", "workflow_plus"); + + const csv = fs.readFileSync(path.join(tempDir, "stats", "workflow_plus_stats.csv"), "utf-8"); + const values = csv.trim().split("\r\n")[1].split(";"); + // "Account Templates - unit tests" must be the 2 from account_1+2, never the 7 from account_1112 + expect(values[16]).toBe("2"); + }); + + it("should not build an invalid regular expression from a name with brackets", async () => { + writeTemplate("account_templates", "account_(1)", { unitTests: 1 }); + writeWorkflow("workflow_brackets", { name: "Brackets", templates: { reconciliations: [], accounts: ["account_(1)"], exports: [] } }); + + await expect(stats.generateWorkflowOverview("2024-01-01", "workflow_brackets")).resolves.not.toThrow(); + expect(mockExit).not.toHaveBeenCalled(); + }); + }); + + // ─── generateOverview (whole repository) ─────────────────────────────────── + + describe("generateOverview", () => { + it("should count every template in the repository", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 2 }); + writeTemplate("reconciliation_texts", "reconciliation_text_2", { unitTests: 1 }); + writeTemplate("shared_parts", "shared_part_1"); + writeTemplate("account_templates", "account_1", { unitTests: 3 }); + writeTemplate("export_files", "export_1"); + + await stats.generateOverview("2024-01-01"); + + const csvPath = path.join(tempDir, "stats", "overview.csv"); + expect(fs.existsSync(csvPath)).toBe(true); + const values = fs.readFileSync(csvPath, "utf-8").trim().split("\r\n")[1].split(";"); + // Reconciliations - templates, Reconciliations - unit tests + expect(values[8]).toBe("2"); + expect(values[11]).toBe("3"); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it("should append a row when run a second time", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 1 }); + + await stats.generateOverview("2024-01-01"); + await stats.generateOverview("2024-02-01"); + + const csv = fs.readFileSync(path.join(tempDir, "stats", "overview.csv"), "utf-8"); + expect(csv.trim().split("\r\n")).toHaveLength(3); + }); + }); +}); diff --git a/tests/lib/cli/utils.test.js b/tests/lib/cli/utils.test.js index 8b0e1318..38d05e42 100644 --- a/tests/lib/cli/utils.test.js +++ b/tests/lib/cli/utils.test.js @@ -8,12 +8,15 @@ jest.mock("../../../lib/api/firmCredentials", () => ({ })); jest.mock("../../../lib/utils/errorUtils", () => ({ uncaughtErrors: jest.fn(), + missingHandle: jest.fn(), + invalidHandleFormat: jest.fn(), })); // Mock prompt-sync so no interactive prompts run in tests jest.mock("prompt-sync", () => () => jest.fn()); const { consola } = require("consola"); const { firmCredentials } = require("../../../lib/api/firmCredentials"); +const errorUtils = require("../../../lib/utils/errorUtils"); const cliUtils = require("../../../lib/cli/utils"); describe("cli/utils", () => { @@ -93,6 +96,111 @@ describe("cli/utils", () => { }); }); + // ─── checkDateFormat ─────────────────────────────────────────────────────── + + describe("checkDateFormat", () => { + it("should return true for a valid YYYY-MM-DD date", () => { + const result = cliUtils.checkDateFormat("2024-01-31"); + expect(result).toBe(true); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it("should accept a leap day in a leap year", () => { + const result = cliUtils.checkDateFormat("2024-02-29"); + expect(result).toBe(true); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it("should call process.exit(1) for a date in the wrong format", () => { + cliUtils.checkDateFormat("31-01-2024"); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("YYYY-MM-DD")); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it("should call process.exit(1) for a non-date string", () => { + cliUtils.checkDateFormat("not-a-date"); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("YYYY-MM-DD")); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it("should call process.exit(1) for an empty string", () => { + cliUtils.checkDateFormat(""); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it("should call process.exit(1) when the value is not a string", () => { + cliUtils.checkDateFormat(undefined); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it("should reject a correctly formatted but impossible calendar date", () => { + cliUtils.checkDateFormat("2024-02-31"); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("not an existing calendar date")); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it("should reject a leap day outside a leap year", () => { + cliUtils.checkDateFormat("2023-02-29"); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("not an existing calendar date")); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it("should reject a month outside the valid range", () => { + cliUtils.checkDateFormat("2024-13-01"); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it("should reject input containing shell metacharacters", () => { + cliUtils.checkDateFormat('2024-01-01"; rm -rf /; echo "'); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("YYYY-MM-DD")); + expect(mockExit).toHaveBeenCalledWith(1); + }); + }); + + // ─── checkHandleFormat ───────────────────────────────────────────────────── + + describe("checkHandleFormat", () => { + it("should return true for a handle which names a file", () => { + const result = cliUtils.checkHandleFormat("workflow_1", "workflow handle"); + expect(result).toBe(true); + expect(mockExit).not.toHaveBeenCalled(); + }); + + it.each(["../escape", "../../etc/passwd", "sub/handle", "sub\\handle", "..", ".", ".hidden", "/absolute"])( + "should call process.exit(1) for the handle %p", + (unsafeHandle) => { + cliUtils.checkHandleFormat(unsafeHandle, "workflow handle"); + expect(errorUtils.invalidHandleFormat).toHaveBeenCalledWith(unsafeHandle, "workflow handle", undefined); + expect(mockExit).toHaveBeenCalledWith(1); + } + ); + + // A blank handle is reported as missing rather than as malformed, since it usually + // means an unset variable was passed on rather than a badly chosen name + it.each(["", " "])("should report a blank handle %p as missing", (blankHandle) => { + cliUtils.checkHandleFormat(blankHandle, "workflow handle"); + expect(errorUtils.missingHandle).toHaveBeenCalledWith("workflow handle", undefined); + expect(errorUtils.invalidHandleFormat).not.toHaveBeenCalled(); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it("should report a missing handle as missing", () => { + cliUtils.checkHandleFormat(undefined, "workflow handle"); + expect(errorUtils.missingHandle).toHaveBeenCalledWith("workflow handle", undefined); + expect(mockExit).toHaveBeenCalledWith(1); + }); + + it("should pass the suggested command on to the error message", () => { + cliUtils.checkHandleFormat("../escape", "workflow handle", "silverfin stats --since 2024-01-31 --workflow"); + expect(errorUtils.invalidHandleFormat).toHaveBeenCalledWith("../escape", "workflow handle", "silverfin stats --since 2024-01-31 --workflow"); + }); + + it("should pass the suggested command on when the handle is missing", () => { + cliUtils.checkHandleFormat("", "workflow handle", "silverfin stats --since 2024-01-31 --workflow"); + expect(errorUtils.missingHandle).toHaveBeenCalledWith("workflow handle", "silverfin stats --since 2024-01-31 --workflow"); + }); + }); + // ─── checkUniqueOption ───────────────────────────────────────────────────── describe("checkUniqueOption", () => { diff --git a/tests/lib/utils/errorUtils.test.js b/tests/lib/utils/errorUtils.test.js new file mode 100644 index 00000000..97c08d9d --- /dev/null +++ b/tests/lib/utils/errorUtils.test.js @@ -0,0 +1,81 @@ +jest.mock("consola"); + +const { consola } = require("consola"); +const errorUtils = require("../../../lib/utils/errorUtils"); +const { WORKFLOWS_FOLDER } = require("../../../lib/utils/constants"); + +describe("utils/errorUtils", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + // ─── missingHandle ───────────────────────────────────────────────────────── + + describe("missingHandle", () => { + it("should name the label and mention an unset variable", () => { + errorUtils.missingHandle("workflow handle"); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("No workflow handle was provided")); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("variable you passed is set")); + }); + + it("should suggest the command which lists the handles available", () => { + errorUtils.missingHandle("workflow handle", "silverfin stats --since 2024-01-31 --workflow"); + expect(consola.log).toHaveBeenCalledWith(expect.stringContaining("To see the workflow handles available")); + expect(consola.log).toHaveBeenCalledWith(expect.stringContaining("silverfin stats --since 2024-01-31 --workflow")); + }); + + it("should ask for a valid handle when no command is suggested", () => { + errorUtils.missingHandle("workflow handle"); + expect(consola.log).toHaveBeenCalledWith("Please provide a valid workflow handle"); + }); + + it("should return false", () => { + expect(errorUtils.missingHandle("workflow handle")).toBe(false); + }); + }); + + // ─── invalidHandleFormat ─────────────────────────────────────────────────── + + describe("invalidHandleFormat", () => { + it("should name the handle and the label", () => { + errorUtils.invalidHandleFormat("../escape", "workflow handle"); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining('Invalid workflow handle "../escape"')); + }); + + it("should say what a handle cannot contain", () => { + errorUtils.invalidHandleFormat("sub/handle", "workflow handle"); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("cannot contain slashes")); + }); + + it("should suggest the command which lists the handles available", () => { + errorUtils.invalidHandleFormat("../escape", "reconciliation handle", "silverfin get-reconciliation-id --all"); + expect(consola.log).toHaveBeenCalledWith(expect.stringContaining("To see the reconciliation handles available")); + expect(consola.log).toHaveBeenCalledWith(expect.stringContaining("silverfin get-reconciliation-id --all")); + }); + + it("should ask for a valid handle when no command is suggested", () => { + errorUtils.invalidHandleFormat("../escape", "workflow handle"); + expect(consola.log).toHaveBeenCalledWith("Please provide a valid workflow handle"); + }); + + it("should return false", () => { + expect(errorUtils.invalidHandleFormat("../escape", "workflow handle")).toBe(false); + }); + }); + + // ─── Workflow folder naming ──────────────────────────────────────────────── + + // The folder the messages point the user at must stay the folder fsUtils reads from, + // so both take it from lib/utils/constants.js + describe("workflow messages", () => { + it("should point at the workflows folder fsUtils reads from", () => { + errorUtils.noWorkflowsStored(); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining(`./${WORKFLOWS_FOLDER}`)); + }); + + it("should point at the workflow file inside that folder", () => { + errorUtils.unparsableWorkflow("workflow_a", "Unexpected end of JSON input"); + expect(consola.log).toHaveBeenCalledWith(expect.stringContaining(`./${WORKFLOWS_FOLDER}/workflow_a.json`)); + }); + }); +}); diff --git a/tests/lib/utils/fsUtils.test.js b/tests/lib/utils/fsUtils.test.js index 208c3676..b6db1b2d 100644 --- a/tests/lib/utils/fsUtils.test.js +++ b/tests/lib/utils/fsUtils.test.js @@ -5,6 +5,8 @@ const fsUtils = require("../../../lib/utils/fsUtils"); jest.mock("consola"); +const { consola } = require("consola"); + describe("fsUtils", () => { let tempDir; let originalCwd; @@ -420,4 +422,170 @@ describe("fsUtils", () => { expect(config.id[100]).toBe(12345); }); }); + + // ─── getAllWorkflowHandles ───────────────────────────────────────────────── + + describe("getAllWorkflowHandles", () => { + const writeWorkflowFile = (fileName, content) => { + const workflowsPath = path.join(tempDir, "workflows"); + fs.mkdirSync(workflowsPath, { recursive: true }); + fs.writeFileSync(path.join(workflowsPath, fileName), typeof content === "string" ? content : JSON.stringify(content)); + }; + + it("should return an empty array when the workflows folder does not exist", () => { + expect(fsUtils.getAllWorkflowHandles()).toEqual([]); + }); + + it("should return an empty array when the workflows folder is empty", () => { + fs.mkdirSync(path.join(tempDir, "workflows"), { recursive: true }); + expect(fsUtils.getAllWorkflowHandles()).toEqual([]); + }); + + it("should return the handles of every workflow file", () => { + writeWorkflowFile("workflow_a.json", { name: "A" }); + writeWorkflowFile("workflow_b.json", { name: "B" }); + + const result = fsUtils.getAllWorkflowHandles(); + + expect(result).toHaveLength(2); + expect(result).toEqual(expect.arrayContaining(["workflow_a", "workflow_b"])); + }); + + it("should ignore files which are not JSON", () => { + writeWorkflowFile("workflow_a.json", { name: "A" }); + writeWorkflowFile("README.md", "# not a workflow"); + writeWorkflowFile(".DS_Store", "binary junk"); + + expect(fsUtils.getAllWorkflowHandles()).toEqual(["workflow_a"]); + }); + }); + + // ─── getWorkflow ─────────────────────────────────────────────────────────── + + describe("getWorkflow", () => { + const validWorkflow = { + name: "Workflow 1", + templates: { + reconciliations: ["reconciliation_text_1"], + accounts: ["account_1"], + exports: ["export_1"], + }, + }; + + const writeWorkflow = (handle, content) => { + const workflowsPath = path.join(tempDir, "workflows"); + fs.mkdirSync(workflowsPath, { recursive: true }); + fs.writeFileSync(path.join(workflowsPath, `${handle}.json`), typeof content === "string" ? content : JSON.stringify(content)); + }; + + it("should return the parsed workflow when it is valid", () => { + writeWorkflow("workflow_1", validWorkflow); + + expect(fsUtils.getWorkflow("workflow_1")).toEqual(validWorkflow); + expect(consola.error).not.toHaveBeenCalled(); + }); + + it("should accept a workflow with empty template lists", () => { + writeWorkflow("workflow_empty", { name: "Empty", templates: { reconciliations: [], accounts: [], exports: [] } }); + + expect(fsUtils.getWorkflow("workflow_empty")).toBeDefined(); + expect(consola.error).not.toHaveBeenCalled(); + }); + + it("should return undefined and warn when the workflow does not exist", () => { + writeWorkflow("workflow_1", validWorkflow); + + expect(fsUtils.getWorkflow("no_such_workflow")).toBeUndefined(); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("no_such_workflow")); + }); + + it("should list the available handles when the workflow does not exist", () => { + writeWorkflow("workflow_1", validWorkflow); + writeWorkflow("workflow_2", validWorkflow); + + fsUtils.getWorkflow("typo_handle"); + + expect(consola.log).toHaveBeenCalledWith(expect.stringContaining("workflow_1")); + expect(consola.log).toHaveBeenCalledWith(expect.stringContaining("workflow_2")); + }); + + it("should report when there are no workflows stored at all", () => { + expect(fsUtils.getWorkflow("anything")).toBeUndefined(); + expect(consola.log).toHaveBeenCalledWith(expect.stringContaining("no workflows stored")); + }); + + it("should return undefined and warn when the file is not valid JSON", () => { + writeWorkflow("broken", '{ "name": "Broken", '); + + expect(fsUtils.getWorkflow("broken")).toBeUndefined(); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("could not be parsed as JSON")); + }); + + it("should return undefined and name the missing attribute when templates.accounts is absent", () => { + writeWorkflow("missing_accounts", { name: "Missing", templates: { reconciliations: [], exports: [] } }); + + expect(fsUtils.getWorkflow("missing_accounts")).toBeUndefined(); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("templates.accounts")); + }); + + it("should return undefined when templates is missing entirely", () => { + writeWorkflow("no_templates", { name: "No templates" }); + + expect(fsUtils.getWorkflow("no_templates")).toBeUndefined(); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining(String.raw`"templates" is missing`)); + }); + + it("should return undefined when name is missing", () => { + writeWorkflow("no_name", { templates: { reconciliations: [], accounts: [], exports: [] } }); + + expect(fsUtils.getWorkflow("no_name")).toBeUndefined(); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining(String.raw`"name" is missing`)); + }); + + it("should return undefined when a template attribute is not an array", () => { + writeWorkflow("wrong_type", { name: "Wrong", templates: { reconciliations: "reconciliation_text_1", accounts: [], exports: [] } }); + + expect(fsUtils.getWorkflow("wrong_type")).toBeUndefined(); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("templates.reconciliations")); + }); + + it("should return undefined when the file holds a JSON array instead of an object", () => { + writeWorkflow("array_workflow", ["reconciliation_text_1"]); + + expect(fsUtils.getWorkflow("array_workflow")).toBeUndefined(); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("JSON object")); + }); + + it.each(["../escape", "../../etc/passwd", "sub/handle", "sub\\handle", "..", ".", ".hidden", "/absolute", ""])( + "should refuse the unsafe handle %p without reading any file", + (unsafeHandle) => { + writeWorkflow("workflow_1", validWorkflow); + const readSpy = jest.spyOn(fs, "readFileSync"); + + expect(fsUtils.getWorkflow(unsafeHandle)).toBeUndefined(); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("is not valid")); + expect(readSpy).not.toHaveBeenCalled(); + + readSpy.mockRestore(); + } + ); + + it("should accept a handle containing a dot", () => { + writeWorkflow("my.workflow", validWorkflow); + + expect(fsUtils.getWorkflow("my.workflow")).toEqual(validWorkflow); + expect(consola.error).not.toHaveBeenCalled(); + }); + + it("should not exit the process on any invalid workflow", () => { + const mockExit = jest.spyOn(process, "exit").mockImplementation(() => {}); + writeWorkflow("broken", "{ nope"); + + fsUtils.getWorkflow("broken"); + fsUtils.getWorkflow("does_not_exist"); + + expect(mockExit).not.toHaveBeenCalled(); + mockExit.mockRestore(); + }); + }); }); diff --git a/tests/lib/utils/templateUtils.test.js b/tests/lib/utils/templateUtils.test.js index f90031ed..b4717620 100644 --- a/tests/lib/utils/templateUtils.test.js +++ b/tests/lib/utils/templateUtils.test.js @@ -125,6 +125,59 @@ describe("templateUtils", () => { }); }); + // ─── isSafeName ─────────────────────────────────────────────────────────── + + describe("isSafeName", () => { + it.each(["workflow_1", "Workflow-1", "my.workflow", "reconciliation_text_1", "Fixed assets (2024)"])("should accept the name %p", (name) => { + expect(templateUtils.isSafeName(name)).toBe(true); + }); + + it.each([ + ["a relative parent", "../escape"], + ["a deeper traversal", "../../etc/passwd"], + ["a forward slash", "sub/handle"], + ["a back slash", "sub\\handle"], + ["an absolute path", "/etc/passwd"], + ["the parent folder", ".."], + ["the current folder", "."], + ["a hidden file", ".hidden"], + ["a traversal without a separator", "handle..name"], + ["a null byte", "handle\0.json"], + ["an empty string", ""], + ["only whitespace", " "], + ])("should refuse %s (%p)", (_description, name) => { + expect(templateUtils.isSafeName(name)).toBe(false); + }); + + it.each([undefined, null, 42, {}, ["handle"]])("should refuse the non-string value %p", (value) => { + expect(templateUtils.isSafeName(value)).toBe(false); + }); + }); + + // ─── fileNameProblem ────────────────────────────────────────────────────────── + + describe("fileNameProblem", () => { + it.each(["workflow_1", "Workflow-1", "my.workflow", "reconciliation_text_1", "Fixed assets (2024)"])("should report no problem for the name %p", (name) => { + expect(templateUtils.fileNameProblem(name)).toBeNull(); + }); + + // A missing name is told apart from a malformed one so callers can word their own message + it.each(["", " ", undefined, null])("should report %p as blank", (value) => { + expect(templateUtils.fileNameProblem(value)).toBe(templateUtils.FILE_NAME_PROBLEMS.BLANK); + }); + + it.each(["../escape", "../../etc/passwd", "sub/handle", "sub\\handle", "/etc/passwd", "..", ".", ".hidden", "handle..name", "handle\0.json"])( + "should report %p as unsafe", + (name) => { + expect(templateUtils.fileNameProblem(name)).toBe(templateUtils.FILE_NAME_PROBLEMS.UNSAFE); + } + ); + + it.each([42, {}, ["handle"]])("should report the non-string value %p as blank", (value) => { + expect(templateUtils.fileNameProblem(value)).toBe(templateUtils.FILE_NAME_PROBLEMS.BLANK); + }); + }); + // ─── filterParts ───────────────────────────────────────────────────────── describe("filterParts", () => { From 20917e6180485c83820cbe02d2de53d17238fc3f Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 18 Aug 2026 17:10:06 +0100 Subject: [PATCH 20/24] Handle CSV write failures in stats fs.writeFileSync is synchronous: its third parameter is an options object, so the error callback passed to it was coerced and discarded. A failed write threw an unhandled synchronous error at the user instead. saveOverviewToFile had the same bug. Both save functions now share writeStatisticsRow, which wraps the folder creation, the header write and the append in a try/catch, keeps the cause at debug level and reports through the new errorUtils.statisticsNotWritten. The summary is already on screen by then, so the run reports the lost row and ends normally rather than exiting. --- lib/cli/stats.js | 46 +++++++++++++++++++------------------ lib/utils/errorUtils.js | 14 +++++++++++ tests/lib/cli/stats.test.js | 18 +++++++++++++++ 3 files changed, 56 insertions(+), 22 deletions(-) diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 819e900d..12343281 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -43,6 +43,8 @@ async function reportOnWorkflow(sinceDate, workflowHandle) { const yamlSummary = await getYamlSummary(sinceDate, workflow); displayWorkflowOverview(sinceDate, TODAY, templateSummary, yamlSummary); const row = createWorkflowRow(sinceDate, TODAY, templateSummary, yamlSummary); + // A failed write is reported by saveWorkflowOverviewToFile and does not make this a skipped + // workflow: the statistics were gathered and displayed, only the CSV row was lost saveWorkflowOverviewToFile(row, workflowHandle); return true; } @@ -611,6 +613,26 @@ function createWorkflowRow(sinceDate, today, templateSummary, yamlSummary) { return row; } +// Write one row of statistics, creating the file and its header columns when needed +// The summary has already been displayed, so a write failure is reported and the run ends +// normally instead of throwing at the user +// @returns {boolean} False when the statistics could not be written +function writeStatisticsRow(csvPath, rowHeader, row) { + try { + if (!fs.existsSync("./stats")) { + fs.mkdirSync("stats"); + } + if (!fs.existsSync(csvPath)) { + fs.writeFileSync(csvPath, rowHeader); + } + fs.appendFileSync(csvPath, row); + return true; + } catch (error) { + consola.debug(error); + return errorUtils.statisticsNotWritten(csvPath, error); + } +} + // content row must be a string with each column separated by ";" function saveOverviewToFile(row) { const COLUMNS = [ @@ -649,17 +671,7 @@ function saveOverviewToFile(row) { ]; const ROW_HEADER = `${COLUMNS.join(";")}`; const CSV_PATH = `./stats/overview.csv`; - // Create file and header columns - if (!fs.existsSync("./stats")) { - fs.mkdirSync("stats"); - } - if (!fs.existsSync(CSV_PATH)) { - fs.writeFileSync(CSV_PATH, ROW_HEADER, (err) => { - consola.error(err); - }); - } - // Append content - fs.appendFileSync(CSV_PATH, row); + return writeStatisticsRow(CSV_PATH, ROW_HEADER, row); } // content row must be a string with each column separated by ";" @@ -704,17 +716,7 @@ function saveWorkflowOverviewToFile(row, workflowHandle) { ]; const ROW_HEADER = `${COLUMNS.join(";")}`; const CSV_PATH = `./stats/${workflowHandle}_stats.csv`; - // Create file and header columns - if (!fs.existsSync("./stats")) { - fs.mkdirSync("stats"); - } - if (!fs.existsSync(CSV_PATH)) { - fs.writeFileSync(CSV_PATH, ROW_HEADER, (err) => { - consola.error(err); - }); - } - // Append content - fs.appendFileSync(CSV_PATH, row); + return writeStatisticsRow(CSV_PATH, ROW_HEADER, row); } module.exports = { generateOverview, generateWorkflowOverview }; diff --git a/lib/utils/errorUtils.js b/lib/utils/errorUtils.js index ff97f28d..cdcadf84 100644 --- a/lib/utils/errorUtils.js +++ b/lib/utils/errorUtils.js @@ -269,6 +269,19 @@ function workflowStatisticsNotSaved(handle) { return false; } +/** + * The statistics were gathered, but the CSV file could not be written. The summary is + * already on screen, so this names the file and the likely cause instead of stopping the run + * @param {string} csvPath The file the statistics were meant to be written to + * @param {Error} error The write failure. Kept for the debug output of the caller + */ +function statisticsNotWritten(csvPath, error) { + const reason = error && error.code === "EACCES" ? " (no permission to write to it)" : ""; + consola.error(`The statistics could not be written to ${csvPath}${reason}. They are shown above`); + consola.log(`Check that the file is not open in another program and run the command again`); + return false; +} + /** * Print the workflows skipped during generateWorkflowOverview (after the loop), * so a partial run is never mistaken for a complete one. @@ -304,5 +317,6 @@ module.exports = { invalidWorkflow, noWorkflowsStored, workflowStatisticsNotSaved, + statisticsNotWritten, printWorkflowBatchErrorSummary, }; diff --git a/tests/lib/cli/stats.test.js b/tests/lib/cli/stats.test.js index c2366bbc..c923b768 100644 --- a/tests/lib/cli/stats.test.js +++ b/tests/lib/cli/stats.test.js @@ -312,6 +312,24 @@ describe("cli/stats", () => { expect(mockExit).not.toHaveBeenCalled(); }); + it("should report a write failure instead of throwing", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 1 }); + const appendSpy = jest.spyOn(fs, "appendFileSync").mockImplementation(() => { + const error = new Error("permission denied"); + error.code = "EACCES"; + throw error; + }); + + try { + await expect(stats.generateOverview("2024-01-01")).resolves.not.toThrow(); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("stats/overview.csv")); + expect(consola.error).toHaveBeenCalledWith(expect.stringContaining("no permission to write to it")); + expect(mockExit).not.toHaveBeenCalled(); + } finally { + appendSpy.mockRestore(); + } + }); + it("should append a row when run a second time", async () => { writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 1 }); From c8e980ce01df0a587b5530766480b978ae35aa89 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Tue, 18 Aug 2026 17:10:06 +0100 Subject: [PATCH 21/24] Record where a private helper belongs in a file Extracting writeStatisticsRow put it between the two functions calling it, which reads worse than either alternative: it separates the pair and shows the helper before the reason for it. The skill said which file a helper belongs in but not where in that file. State the convention the repo already follows - directly above the first caller, above the first of two when shared - with the examples that show it, and note createLiquidFile as the older exception. Add the mistake to the red flags. --- .claude/skills/adding-methods/SKILL.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.claude/skills/adding-methods/SKILL.md b/.claude/skills/adding-methods/SKILL.md index 0048d0e0..b3f625f4 100644 --- a/.claude/skills/adding-methods/SKILL.md +++ b/.claude/skills/adding-methods/SKILL.md @@ -54,6 +54,8 @@ Move a helper out only when one of these is true: "It has no test of its own" is not a reason to move it. A private helper is tested through the function that calls it. +**Where it goes in the file:** directly above its first caller — `escapeForRegExp`, `buildTemplatePattern` and `percentageRoundTwo` in `lib/cli/stats.js`, `suggestValidHandle` in `lib/utils/errorUtils.js`. When two functions share the helper, it goes above the first of them so they stay adjacent; never between them, which separates the pair and shows the reader the helper before the reason for it. (`createLiquidFile` in `fsUtils.js` sits below its caller — an older exception, not the pattern to copy.) + Applies equally to edits: if you are asked to make an existing method "also" do something, the answer is a second method plus a caller, not a longer method. Say that in your response rather than silently growing the function. ## Step 4 — Report @@ -71,6 +73,7 @@ Then add the test in the mirrored `tests/` path, and run `npx jest ` befor - "I'll just add it to the file I'm already in" — said about a method other files will call - Exporting a private helper only so a test can reach it - "It's only a few lines, no need to check the doc" +- A shared helper dropped between the two functions that call it - A new method with `fs` and `axios` both in scope - A parameter named `options` that switches behaviour between two unrelated jobs - A boolean parameter that selects which of two things the method does — that is two methods From 86a0e50d5bc62ef55124ce97cfd696affe9bcc84 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Wed, 19 Aug 2026 09:39:34 +0100 Subject: [PATCH 22/24] Count templates instead of yaml files in stats countYamlFiles counted test files while the percentages divide by a number of templates, so a template holding two liquid test files was counted twice and the coverage percentage could exceed 100%. That was the open question left in a comment above the expression. The matched files are now grouped per template through a capture group in the template pattern: a template counts once when it holds any non-empty test file, its unit tests are added up across its files, and "at least two tests" is judged on that combined total. An unparsable YAML file is reported at debug level rather than swallowed. The summary fields, the CSV column labels and the on-screen lines say templates rather than yaml files, so the yaml columns of an existing overview.csv are not comparable with the rows written from now on. --- lib/cli/stats.js | 257 ++++++++++++++++++------------------ tests/lib/cli/stats.test.js | 62 +++++++++ 2 files changed, 193 insertions(+), 126 deletions(-) diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 12343281..99728114 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -80,11 +80,13 @@ function escapeForRegExp(value) { } // Build the alternation used to match the templates of a workflow (e.g. "(handle_1|handle_2)") +// The group is always capturing, so callers can read back which template a path belongs to // Returns undefined when there are no templates to match, since an empty alternation // would silently match nothing at all function buildTemplatePattern(templateNames) { if (!templateNames) { - return ".*"; + // A single folder name, not ".*": a greedy match would swallow the rest of the path + return "([^/]+)"; } if (templateNames.length === 0) { return undefined; @@ -153,48 +155,51 @@ async function yamlFilesActivity(sinceDate, workflow) { return countByType; } -// Count how many YAML files are stored. We base on the presence of a non empty file -// Count how many unit tests are stored. We base on the presence of a title for each unit test -// Count how many YAML files have at least two unit tests +// Count how many templates hold at least one non empty YAML file, how many unit tests are +// stored (we base on the presence of a title for each unit test) and how many templates hold +// at least two unit tests +// A template is counted once no matter how many test files sit in its tests folder, since the +// counts are compared against a number of templates async function countYamlFiles(templateType, templatesInWorkflow) { const TEMPLATE_PATTERN = buildTemplatePattern(templatesInWorkflow); // The workflow holds no template of this type, so there is nothing to count if (!TEMPLATE_PATTERN) { - return { files: 0, tests: 0, filesWithAtLeastTwoTests: 0 }; + return { templatesWithTests: 0, tests: 0, templatesWithAtLeastTwoTests: 0 }; } - const files = fsUtils.listExistingFiles("yml"); + const yamlFiles = fsUtils.listExistingFiles("yml"); const FOLDER = fsUtils.FOLDERS[templateType]; - // Issue with counting multiple yml files in the same tests folder? const YAML_EXPRESSION = `.*${FOLDER}/${TEMPLATE_PATTERN}/tests/.*_liquid_test.*.y(a)?ml`; - const re = new RegExp(YAML_EXPRESSION, "g"); - let countFiles = 0; - let countFilesWithAtLeastTwoTests = 0; - let countTests = 0; - for (const file of files) { - const found = file.match(re); - if (found && fs.existsSync(file)) { - const fileContent = fs.readFileSync(file).toString(); + // No global flag: the first match is the one we need, and its capture group names the template + const re = new RegExp(YAML_EXPRESSION); + // Unit tests found per template, so several test files in one tests folder are added up + const testsPerTemplate = {}; + for (const yamlFile of yamlFiles) { + const found = yamlFile.match(re); + if (found && fs.existsSync(yamlFile)) { + const templateName = found[1]; + const fileContent = fs.readFileSync(yamlFile).toString(); const contentRows = fileContent.split("\n"); if (contentRows.length > 1) { - countFiles += 1; + testsPerTemplate[templateName] = testsPerTemplate[templateName] || 0; try { const yamlContent = await yaml.parse(fileContent, { maxAliasCount: 10000, }); - const unitTestsCount = Object.keys(yamlContent).length || 0; - countTests += unitTestsCount; - - if (unitTestsCount >= 2) { - countFilesWithAtLeastTwoTests += 1; - } - } catch (e) { - // Error while parsing the YAML file - // consola.log(e); + testsPerTemplate[templateName] += Object.keys(yamlContent).length || 0; + } catch (error) { + // The file exists but cannot be read as YAML. It still counts as a test file, and the + // template keeps the unit tests found in its other files + consola.debug(`${yamlFile} could not be parsed as YAML: ${error.message}`); } } } } - return { files: countFiles, tests: countTests, filesWithAtLeastTwoTests: countFilesWithAtLeastTwoTests }; + const testCounts = Object.values(testsPerTemplate); + return { + templatesWithTests: testCounts.length, + tests: testCounts.reduce((total, count) => total + count, 0), + templatesWithAtLeastTwoTests: testCounts.filter((count) => count >= 2).length, + }; } // Return an array with non empty template names of a given type @@ -251,11 +256,11 @@ async function getTemplatesSummary() { total: 0, externallyManaged: 0, externallyManagedPerc: 0, - yamlFiles: 0, - yamlFilesPerc: 0, + templatesWithTests: 0, + templatesWithTestsPerc: 0, unitTests: 0, - yamlFilesWithAtLeastTwoTests: 0, - yamlFilesWithAtLeastTwoTestsPerc: 0, + templatesWithAtLeastTwoTests: 0, + templatesWithAtLeastTwoTestsPerc: 0, }, sharedParts: { total: 0, @@ -271,21 +276,21 @@ async function getTemplatesSummary() { total: 0, externallyManaged: 0, externallyManagedPerc: 0, - yamlFiles: 0, - yamlFilesPerc: 0, + templatesWithTests: 0, + templatesWithTestsPerc: 0, unitTests: 0, - yamlFilesWithAtLeastTwoTests: 0, - yamlFilesWithAtLeastTwoTestsPerc: 0, + templatesWithAtLeastTwoTests: 0, + templatesWithAtLeastTwoTestsPerc: 0, }, all: { total: 0, externallyManaged: 0, externallyManagedPerc: 0, - yamlFiles: 0, - yamlFilesPerc: 0, + templatesWithTests: 0, + templatesWithTestsPerc: 0, unitTests: 0, - yamlFilesWithAtLeastTwoTests: 0, - yamlFilesWithAtLeastTwoTestsPerc: 0, + templatesWithAtLeastTwoTests: 0, + templatesWithAtLeastTwoTestsPerc: 0, }, }; @@ -296,11 +301,11 @@ async function getTemplatesSummary() { summary.reconciliations.total = reconciliationsNonEmpty.length; summary.reconciliations.externallyManaged = reconciliationsExtMan.length; summary.reconciliations.externallyManagedPerc = percentageRoundTwo(summary.reconciliations.externallyManaged, summary.reconciliations.total); - summary.reconciliations.yamlFiles = reconciliationsTests.files; - summary.reconciliations.yamlFilesPerc = percentageRoundTwo(summary.reconciliations.yamlFiles, summary.reconciliations.total); + summary.reconciliations.templatesWithTests = reconciliationsTests.templatesWithTests; + summary.reconciliations.templatesWithTestsPerc = percentageRoundTwo(summary.reconciliations.templatesWithTests, summary.reconciliations.total); summary.reconciliations.unitTests = reconciliationsTests.tests; - summary.reconciliations.yamlFilesWithAtLeastTwoTests = reconciliationsTests.filesWithAtLeastTwoTests; - summary.reconciliations.yamlFilesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.reconciliations.yamlFilesWithAtLeastTwoTests, summary.reconciliations.total); + summary.reconciliations.templatesWithAtLeastTwoTests = reconciliationsTests.templatesWithAtLeastTwoTests; + summary.reconciliations.templatesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.reconciliations.templatesWithAtLeastTwoTests, summary.reconciliations.total); // Shared Parts const sharedPartsNonEmpty = await listNonEmptyTemplates("sharedPart"); @@ -323,22 +328,22 @@ async function getTemplatesSummary() { summary.accountTemplates.total = accountTemplatesNonEmpty.length; summary.accountTemplates.externallyManaged = accountTemplatesExtMan.length; summary.accountTemplates.externallyManagedPerc = percentageRoundTwo(summary.accountTemplates.externallyManaged, summary.accountTemplates.total); - summary.accountTemplates.yamlFiles = accountTemplatesTests.files; - summary.accountTemplates.yamlFilesPerc = percentageRoundTwo(summary.accountTemplates.yamlFiles, summary.accountTemplates.total); + summary.accountTemplates.templatesWithTests = accountTemplatesTests.templatesWithTests; + summary.accountTemplates.templatesWithTestsPerc = percentageRoundTwo(summary.accountTemplates.templatesWithTests, summary.accountTemplates.total); summary.accountTemplates.unitTests = accountTemplatesTests.tests; - summary.accountTemplates.yamlFilesWithAtLeastTwoTests = accountTemplatesTests.filesWithAtLeastTwoTests; - summary.accountTemplates.yamlFilesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.accountTemplates.yamlFilesWithAtLeastTwoTests, summary.accountTemplates.total); + summary.accountTemplates.templatesWithAtLeastTwoTests = accountTemplatesTests.templatesWithAtLeastTwoTests; + summary.accountTemplates.templatesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.accountTemplates.templatesWithAtLeastTwoTests, summary.accountTemplates.total); // All summary.all.total = summary.reconciliations.total + summary.sharedParts.total + summary.exportFiles.total + summary.accountTemplates.total; summary.all.externallyManaged = summary.reconciliations.externallyManaged + summary.sharedParts.externallyManaged + summary.exportFiles.externallyManaged + summary.accountTemplates.externallyManaged; summary.all.externallyManagedPerc = percentageRoundTwo(summary.all.externallyManaged, summary.all.total); - summary.all.yamlFiles = summary.reconciliations.yamlFiles + summary.accountTemplates.yamlFiles; - summary.all.yamlFilesPerc = percentageRoundTwo(summary.all.yamlFiles, summary.reconciliations.total + summary.accountTemplates.total); + summary.all.templatesWithTests = summary.reconciliations.templatesWithTests + summary.accountTemplates.templatesWithTests; + summary.all.templatesWithTestsPerc = percentageRoundTwo(summary.all.templatesWithTests, summary.reconciliations.total + summary.accountTemplates.total); summary.all.unitTests = summary.reconciliations.unitTests + summary.accountTemplates.unitTests; - summary.all.yamlFilesWithAtLeastTwoTests = summary.reconciliations.yamlFilesWithAtLeastTwoTests + summary.accountTemplates.yamlFilesWithAtLeastTwoTests; - summary.all.yamlFilesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.all.yamlFilesWithAtLeastTwoTests, summary.reconciliations.total + summary.accountTemplates.total); + summary.all.templatesWithAtLeastTwoTests = summary.reconciliations.templatesWithAtLeastTwoTests + summary.accountTemplates.templatesWithAtLeastTwoTests; + summary.all.templatesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.all.templatesWithAtLeastTwoTests, summary.reconciliations.total + summary.accountTemplates.total); return summary; } @@ -350,11 +355,11 @@ async function getWorkflowTemplateSummary(workflow) { total: 0, externallyManaged: 0, externallyManagedPerc: 0, - yamlFiles: 0, - yamlFilesPerc: 0, + templatesWithTests: 0, + templatesWithTestsPerc: 0, unitTests: 0, - yamlFilesWithAtLeastTwoTests: 0, - yamlFilesWithAtLeastTwoTestsPerc: 0, + templatesWithAtLeastTwoTests: 0, + templatesWithAtLeastTwoTestsPerc: 0, }, exportFiles: { total: 0, @@ -365,21 +370,21 @@ async function getWorkflowTemplateSummary(workflow) { total: 0, externallyManaged: 0, externallyManagedPerc: 0, - yamlFiles: 0, - yamlFilesPerc: 0, + templatesWithTests: 0, + templatesWithTestsPerc: 0, unitTests: 0, - yamlFilesWithAtLeastTwoTests: 0, - yamlFilesWithAtLeastTwoTestsPerc: 0, + templatesWithAtLeastTwoTests: 0, + templatesWithAtLeastTwoTestsPerc: 0, }, all: { total: 0, externallyManaged: 0, externallyManagedPerc: 0, - yamlFiles: 0, - yamlFilesPerc: 0, + templatesWithTests: 0, + templatesWithTestsPerc: 0, unitTests: 0, - yamlFilesWithAtLeastTwoTests: 0, - yamlFilesWithAtLeastTwoTestsPerc: 0, + templatesWithAtLeastTwoTests: 0, + templatesWithAtLeastTwoTestsPerc: 0, }, }; @@ -394,11 +399,11 @@ async function getWorkflowTemplateSummary(workflow) { summary.reconciliations.total = reconciliationsInWorkflow.length; summary.reconciliations.externallyManaged = reconciliationsExtMan.length; summary.reconciliations.externallyManagedPerc = percentageRoundTwo(summary.reconciliations.externallyManaged, summary.reconciliations.total); - summary.reconciliations.yamlFiles = reconciliationsTests.files; - summary.reconciliations.yamlFilesPerc = percentageRoundTwo(summary.reconciliations.yamlFiles, summary.reconciliations.total); + summary.reconciliations.templatesWithTests = reconciliationsTests.templatesWithTests; + summary.reconciliations.templatesWithTestsPerc = percentageRoundTwo(summary.reconciliations.templatesWithTests, summary.reconciliations.total); summary.reconciliations.unitTests = reconciliationsTests.tests; - summary.reconciliations.yamlFilesWithAtLeastTwoTests = reconciliationsTests.filesWithAtLeastTwoTests; - summary.reconciliations.yamlFilesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.reconciliations.yamlFilesWithAtLeastTwoTests, summary.reconciliations.total); + summary.reconciliations.templatesWithAtLeastTwoTests = reconciliationsTests.templatesWithAtLeastTwoTests; + summary.reconciliations.templatesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.reconciliations.templatesWithAtLeastTwoTests, summary.reconciliations.total); // Export Files const exportFilesInWorkflow = workflow.templates.exports; @@ -414,22 +419,22 @@ async function getWorkflowTemplateSummary(workflow) { summary.accountTemplates.total = accountTemplatesInWorkflow.length; summary.accountTemplates.externallyManaged = accountTemplatesExtMan.length; summary.accountTemplates.externallyManagedPerc = percentageRoundTwo(summary.accountTemplates.externallyManaged, summary.accountTemplates.total); - summary.accountTemplates.yamlFiles = accountTemplatesTests.files; - summary.accountTemplates.yamlFilesPerc = percentageRoundTwo(summary.accountTemplates.yamlFiles, summary.accountTemplates.total); + summary.accountTemplates.templatesWithTests = accountTemplatesTests.templatesWithTests; + summary.accountTemplates.templatesWithTestsPerc = percentageRoundTwo(summary.accountTemplates.templatesWithTests, summary.accountTemplates.total); summary.accountTemplates.unitTests = accountTemplatesTests.tests; - summary.accountTemplates.yamlFilesWithAtLeastTwoTests = accountTemplatesTests.filesWithAtLeastTwoTests; - summary.accountTemplates.yamlFilesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.accountTemplates.yamlFilesWithAtLeastTwoTests, summary.accountTemplates.total); + summary.accountTemplates.templatesWithAtLeastTwoTests = accountTemplatesTests.templatesWithAtLeastTwoTests; + summary.accountTemplates.templatesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.accountTemplates.templatesWithAtLeastTwoTests, summary.accountTemplates.total); // All summary.all.total = summary.reconciliations.total + summary.exportFiles.total + summary.accountTemplates.total; summary.all.externallyManaged = summary.reconciliations.externallyManaged + summary.exportFiles.externallyManaged + summary.accountTemplates.externallyManaged; summary.all.externallyManagedPerc = percentageRoundTwo(summary.all.externallyManaged, summary.all.total); - summary.all.yamlFiles = summary.reconciliations.yamlFiles + summary.accountTemplates.yamlFiles; - summary.all.yamlFilesPerc = percentageRoundTwo(summary.all.yamlFiles, summary.reconciliations.total + summary.accountTemplates.total); + summary.all.templatesWithTests = summary.reconciliations.templatesWithTests + summary.accountTemplates.templatesWithTests; + summary.all.templatesWithTestsPerc = percentageRoundTwo(summary.all.templatesWithTests, summary.reconciliations.total + summary.accountTemplates.total); summary.all.unitTests = summary.reconciliations.unitTests + summary.accountTemplates.unitTests; - summary.all.yamlFilesWithAtLeastTwoTests = summary.reconciliations.yamlFilesWithAtLeastTwoTests + summary.accountTemplates.yamlFilesWithAtLeastTwoTests; - summary.all.yamlFilesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.all.yamlFilesWithAtLeastTwoTests, summary.reconciliations.total + summary.accountTemplates.total); + summary.all.templatesWithAtLeastTwoTests = summary.reconciliations.templatesWithAtLeastTwoTests + summary.accountTemplates.templatesWithAtLeastTwoTests; + summary.all.templatesWithAtLeastTwoTestsPerc = percentageRoundTwo(summary.all.templatesWithAtLeastTwoTests, summary.reconciliations.total + summary.accountTemplates.total); return summary; } @@ -455,19 +460,19 @@ function displayOverview(sinceDate, today, templateSummary, yamlSummary) { consola.log(`${chalk.bold("Reconciliations:")}`); consola.log(`Templates: ${templateSummary.reconciliations.total}`); consola.log(`Externally Managed: ${templateSummary.reconciliations.externallyManaged} (${templateSummary.reconciliations.externallyManagedPerc}%)`); - consola.log(`YAML files: ${templateSummary.reconciliations.yamlFiles} (${templateSummary.reconciliations.yamlFilesPerc}%)`); + consola.log(`Templates with YAML tests: ${templateSummary.reconciliations.templatesWithTests} (${templateSummary.reconciliations.templatesWithTestsPerc}%)`); consola.log(`Unit Tests: ${templateSummary.reconciliations.unitTests}`); consola.log( - `YAML files with at least two unit tests: ${templateSummary.reconciliations.yamlFilesWithAtLeastTwoTests} (${templateSummary.reconciliations.yamlFilesWithAtLeastTwoTestsPerc}%)` + `Templates with at least two unit tests: ${templateSummary.reconciliations.templatesWithAtLeastTwoTests} (${templateSummary.reconciliations.templatesWithAtLeastTwoTestsPerc}%)` ); consola.log(""); consola.log(`${chalk.bold("Account Templates:")}`); consola.log(`Templates: ${templateSummary.accountTemplates.total}`); consola.log(`Externally Managed: ${templateSummary.accountTemplates.externallyManaged} (${templateSummary.accountTemplates.externallyManagedPerc}%)`); - consola.log(`YAML files: ${templateSummary.accountTemplates.yamlFiles} (${templateSummary.accountTemplates.yamlFilesPerc}%)`); + consola.log(`Templates with YAML tests: ${templateSummary.accountTemplates.templatesWithTests} (${templateSummary.accountTemplates.templatesWithTestsPerc}%)`); consola.log(`Unit Tests: ${templateSummary.accountTemplates.unitTests}`); consola.log( - `YAML files with at least two unit tests: ${templateSummary.accountTemplates.yamlFilesWithAtLeastTwoTests} (${templateSummary.accountTemplates.yamlFilesWithAtLeastTwoTestsPerc}%)` + `Templates with at least two unit tests: ${templateSummary.accountTemplates.templatesWithAtLeastTwoTests} (${templateSummary.accountTemplates.templatesWithAtLeastTwoTestsPerc}%)` ); consola.log(""); consola.log(`${chalk.bold("Shared Parts:")}`); @@ -481,9 +486,9 @@ function displayOverview(sinceDate, today, templateSummary, yamlSummary) { consola.log(`${chalk.bold("All:")}`); consola.log(`Templates: ${templateSummary.all.total}`); consola.log(`Externally Managed: ${templateSummary.all.externallyManaged} (${templateSummary.all.externallyManagedPerc}%)`); - consola.log(`YAML files: ${templateSummary.all.yamlFiles} (${templateSummary.all.yamlFilesPerc}%)`); + consola.log(`Templates with YAML tests: ${templateSummary.all.templatesWithTests} (${templateSummary.all.templatesWithTestsPerc}%)`); consola.log(`Unit Tests: ${templateSummary.all.unitTests}`); - consola.log(`YAML files with at least two unit tests: ${templateSummary.all.yamlFilesWithAtLeastTwoTests} (${templateSummary.all.yamlFilesWithAtLeastTwoTestsPerc}%)`); + consola.log(`Templates with at least two unit tests: ${templateSummary.all.templatesWithAtLeastTwoTests} (${templateSummary.all.templatesWithAtLeastTwoTestsPerc}%)`); consola.log(""); consola.log("------------------------------------"); } @@ -504,20 +509,20 @@ function displayWorkflowOverview(sinceDate, today, templateSummary, yamlSummary) consola.log(`${chalk.bold("Reconciliations:")}`); consola.log(`Templates: ${templateSummary.reconciliations.total}`); consola.log(`Externally Managed: ${templateSummary.reconciliations.externallyManaged} (${templateSummary.reconciliations.externallyManagedPerc}%)`); - consola.log(`YAML files: ${templateSummary.reconciliations.yamlFiles} (${templateSummary.reconciliations.yamlFilesPerc}%)`); + consola.log(`Templates with YAML tests: ${templateSummary.reconciliations.templatesWithTests} (${templateSummary.reconciliations.templatesWithTestsPerc}%)`); consola.log(`Unit Tests: ${templateSummary.reconciliations.unitTests}`); consola.log( - `YAML files with at least two unit tests: ${templateSummary.reconciliations.yamlFilesWithAtLeastTwoTests} (${templateSummary.reconciliations.yamlFilesWithAtLeastTwoTestsPerc}%)` + `Templates with at least two unit tests: ${templateSummary.reconciliations.templatesWithAtLeastTwoTests} (${templateSummary.reconciliations.templatesWithAtLeastTwoTestsPerc}%)` ); consola.log(""); // Account Templates consola.log(`${chalk.bold("Account Templates:")}`); consola.log(`Templates: ${templateSummary.accountTemplates.total}`); consola.log(`Externally Managed: ${templateSummary.accountTemplates.externallyManaged} (${templateSummary.accountTemplates.externallyManagedPerc}%)`); - consola.log(`YAML files: ${templateSummary.accountTemplates.yamlFiles} (${templateSummary.accountTemplates.yamlFilesPerc}%)`); + consola.log(`Templates with YAML tests: ${templateSummary.accountTemplates.templatesWithTests} (${templateSummary.accountTemplates.templatesWithTestsPerc}%)`); consola.log(`Unit Tests: ${templateSummary.accountTemplates.unitTests}`); consola.log( - `YAML files with at least two unit tests: ${templateSummary.accountTemplates.yamlFilesWithAtLeastTwoTests} (${templateSummary.accountTemplates.yamlFilesWithAtLeastTwoTestsPerc}%)` + `Templates with at least two unit tests: ${templateSummary.accountTemplates.templatesWithAtLeastTwoTests} (${templateSummary.accountTemplates.templatesWithAtLeastTwoTestsPerc}%)` ); consola.log(""); // Export Files @@ -529,9 +534,9 @@ function displayWorkflowOverview(sinceDate, today, templateSummary, yamlSummary) consola.log(`${chalk.bold("All:")}`); consola.log(`Templates: ${templateSummary.all.total}`); consola.log(`Externally Managed: ${templateSummary.all.externallyManaged} (${templateSummary.all.externallyManagedPerc}%)`); - consola.log(`YAML files: ${templateSummary.all.yamlFiles} (${templateSummary.all.yamlFilesPerc}%)`); + consola.log(`Templates with YAML tests: ${templateSummary.all.templatesWithTests} (${templateSummary.all.templatesWithTestsPerc}%)`); consola.log(`Unit Tests: ${templateSummary.all.unitTests}`); - consola.log(`YAML files with at least two unit tests: ${templateSummary.all.yamlFilesWithAtLeastTwoTests} (${templateSummary.all.yamlFilesWithAtLeastTwoTestsPerc}%)`); + consola.log(`Templates with at least two unit tests: ${templateSummary.all.templatesWithAtLeastTwoTests} (${templateSummary.all.templatesWithAtLeastTwoTestsPerc}%)`); consola.log(""); consola.log("------------------------------------"); } @@ -545,15 +550,15 @@ function createRow(sinceDate, today, templateSummary, yamlSummary) { yamlSummary.updated, templateSummary.all.total, templateSummary.all.externallyManaged, - templateSummary.all.yamlFiles, + templateSummary.all.templatesWithTests, templateSummary.all.unitTests, templateSummary.reconciliations.total, templateSummary.reconciliations.externallyManaged, - templateSummary.reconciliations.yamlFiles, + templateSummary.reconciliations.templatesWithTests, templateSummary.reconciliations.unitTests, templateSummary.accountTemplates.total, templateSummary.accountTemplates.externallyManaged, - templateSummary.accountTemplates.yamlFiles, + templateSummary.accountTemplates.templatesWithTests, templateSummary.accountTemplates.unitTests, templateSummary.sharedParts.total, templateSummary.sharedParts.externallyManaged, @@ -564,13 +569,13 @@ function createRow(sinceDate, today, templateSummary, yamlSummary) { templateSummary.accountTemplates.externallyManagedPerc, templateSummary.sharedParts.externallyManagedPerc, templateSummary.exportFiles.externallyManagedPerc, - templateSummary.all.yamlFilesPerc, - templateSummary.reconciliations.yamlFilesPerc, - templateSummary.accountTemplates.yamlFilesPerc, - templateSummary.reconciliations.yamlFilesWithAtLeastTwoTests, - templateSummary.reconciliations.yamlFilesWithAtLeastTwoTestsPerc, - templateSummary.accountTemplates.yamlFilesWithAtLeastTwoTests, - templateSummary.accountTemplates.yamlFilesWithAtLeastTwoTestsPerc, + templateSummary.all.templatesWithTestsPerc, + templateSummary.reconciliations.templatesWithTestsPerc, + templateSummary.accountTemplates.templatesWithTestsPerc, + templateSummary.reconciliations.templatesWithAtLeastTwoTests, + templateSummary.reconciliations.templatesWithAtLeastTwoTestsPerc, + templateSummary.accountTemplates.templatesWithAtLeastTwoTests, + templateSummary.accountTemplates.templatesWithAtLeastTwoTestsPerc, ]; const row = `\r\n${rowContent.join(";")}`; return row; @@ -585,15 +590,15 @@ function createWorkflowRow(sinceDate, today, templateSummary, yamlSummary) { yamlSummary.updated, templateSummary.all.total, templateSummary.all.externallyManaged, - templateSummary.all.yamlFiles, + templateSummary.all.templatesWithTests, templateSummary.all.unitTests, templateSummary.reconciliations.total, templateSummary.reconciliations.externallyManaged, - templateSummary.reconciliations.yamlFiles, + templateSummary.reconciliations.templatesWithTests, templateSummary.reconciliations.unitTests, templateSummary.accountTemplates.total, templateSummary.accountTemplates.externallyManaged, - templateSummary.accountTemplates.yamlFiles, + templateSummary.accountTemplates.templatesWithTests, templateSummary.accountTemplates.unitTests, templateSummary.exportFiles.total, templateSummary.exportFiles.externallyManaged, @@ -601,13 +606,13 @@ function createWorkflowRow(sinceDate, today, templateSummary, yamlSummary) { templateSummary.reconciliations.externallyManagedPerc, templateSummary.accountTemplates.externallyManagedPerc, templateSummary.exportFiles.externallyManagedPerc, - templateSummary.all.yamlFilesPerc, - templateSummary.reconciliations.yamlFilesPerc, - templateSummary.accountTemplates.yamlFilesPerc, - templateSummary.reconciliations.yamlFilesWithAtLeastTwoTests, - templateSummary.reconciliations.yamlFilesWithAtLeastTwoTestsPerc, - templateSummary.accountTemplates.yamlFilesWithAtLeastTwoTests, - templateSummary.accountTemplates.yamlFilesWithAtLeastTwoTestsPerc, + templateSummary.all.templatesWithTestsPerc, + templateSummary.reconciliations.templatesWithTestsPerc, + templateSummary.accountTemplates.templatesWithTestsPerc, + templateSummary.reconciliations.templatesWithAtLeastTwoTests, + templateSummary.reconciliations.templatesWithAtLeastTwoTestsPerc, + templateSummary.accountTemplates.templatesWithAtLeastTwoTests, + templateSummary.accountTemplates.templatesWithAtLeastTwoTestsPerc, ]; const row = `\r\n${rowContent.join(";")}`; return row; @@ -642,15 +647,15 @@ function saveOverviewToFile(row) { "yaml files modified in period", "All - templates", "All - externally managed", - "All - yaml files", + "All - templates with yaml tests", "All - unit tests", "Reconciliations - templates", "Reconciliations - externally managed", - "Reconciliations - yaml files", + "Reconciliations - templates with yaml tests", "Reconciliations - unit tests", "Account Templates - templates", "Account Templates - externally managed", - "Account Templates - yaml files", + "Account Templates - templates with yaml tests", "Account Templates - unit tests", "Shared Parts - templates", "Shared Parts - externally managed", @@ -661,13 +666,13 @@ function saveOverviewToFile(row) { "Account Templates - externally managed (%)", "Shared Parts - externally managed (%)", "Export Files - externally managed (%)", - "All - yaml files (%)", - "Reconciliations - yaml files (%)", - "Account Templates - yaml files (%)", - "Reconciliations - yaml files with at least two tests", - "Reconciliations - yaml files with at least two tests (%)", - "Account Templates - yaml files with at least two tests", - "Account Templates - yaml files with at least two tests (%)", + "All - templates with yaml tests (%)", + "Reconciliations - templates with yaml tests (%)", + "Account Templates - templates with yaml tests (%)", + "Reconciliations - templates with at least two tests", + "Reconciliations - templates with at least two tests (%)", + "Account Templates - templates with at least two tests", + "Account Templates - templates with at least two tests (%)", ]; const ROW_HEADER = `${COLUMNS.join(";")}`; const CSV_PATH = `./stats/overview.csv`; @@ -690,15 +695,15 @@ function saveWorkflowOverviewToFile(row, workflowHandle) { "yaml files modified in period", "All - templates", "All - externally managed", - "All - yaml files", + "All - templates with yaml tests", "All - unit tests", "Reconciliations - templates", "Reconciliations - externally managed", - "Reconciliations - yaml files", + "Reconciliations - templates with yaml tests", "Reconciliations - unit tests", "Account Templates - templates", "Account Templates - externally managed", - "Account Templates - yaml files", + "Account Templates - templates with yaml tests", "Account Templates - unit tests", "Export Files - templates", "Export Files - externally managed", @@ -706,13 +711,13 @@ function saveWorkflowOverviewToFile(row, workflowHandle) { "Reconciliations - externally managed (%)", "Account Templates - externally managed (%)", "Export Files - externally managed (%)", - "All - yaml files (%)", - "Reconciliations - yaml files (%)", - "Account Templates - yaml files (%)", - "Reconciliations - yaml files with at least two tests", - "Reconciliations - yaml files with at least two tests (%)", - "Account Templates - yaml files with at least two tests", - "Account Templates - yaml files with at least two tests (%)", + "All - templates with yaml tests (%)", + "Reconciliations - templates with yaml tests (%)", + "Account Templates - templates with yaml tests (%)", + "Reconciliations - templates with at least two tests", + "Reconciliations - templates with at least two tests (%)", + "Account Templates - templates with at least two tests", + "Account Templates - templates with at least two tests (%)", ]; const ROW_HEADER = `${COLUMNS.join(";")}`; const CSV_PATH = `./stats/${workflowHandle}_stats.csv`; diff --git a/tests/lib/cli/stats.test.js b/tests/lib/cli/stats.test.js index c923b768..a37112d7 100644 --- a/tests/lib/cli/stats.test.js +++ b/tests/lib/cli/stats.test.js @@ -291,6 +291,68 @@ describe("cli/stats", () => { }); }); + // ─── several test files in one tests folder ──────────────────────────────── + + describe("a template with more than one liquid test file", () => { + // The counts are compared against a number of templates, so a template with two test + // files must not be counted twice + const writeExtraTestFile = (folder, handle, fileName, unitTests) => { + const testContent = Array.from({ length: unitTests }, (_, index) => `extra_unit_test_${index + 1}:\n context:\n period: 2024-12-31\n`).join(""); + fs.writeFileSync(path.join(tempDir, folder, handle, "tests", fileName), testContent); + }; + + it("should count the template once and add up its unit tests in the repository overview", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 2 }); + writeExtraTestFile("reconciliation_texts", "reconciliation_text_1", "extra_liquid_test.yml", 3); + + await stats.generateOverview("2024-01-01"); + + const values = fs.readFileSync(path.join(tempDir, "stats", "overview.csv"), "utf-8").trim().split("\r\n")[1].split(";"); + // Reconciliations - templates, - templates with yaml tests, - unit tests + expect(values[8]).toBe("1"); + expect(values[10]).toBe("1"); + expect(values[11]).toBe("5"); + }); + + it("should count the template once and add up its unit tests in a workflow overview", async () => { + writeTemplate("account_templates", "account_1", { unitTests: 1 }); + writeExtraTestFile("account_templates", "account_1", "account_1_liquid_test_extra.yml", 1); + writeWorkflow("workflow_1", { name: "Workflow 1", templates: { reconciliations: [], accounts: ["account_1"], exports: [] } }); + + await stats.generateWorkflowOverview("2024-01-01", "workflow_1"); + + const values = fs.readFileSync(path.join(tempDir, "stats", "workflow_1_stats.csv"), "utf-8").trim().split("\r\n")[1].split(";"); + // Account Templates - templates, - templates with yaml tests, - unit tests + expect(values[13]).toBe("1"); + expect(values[15]).toBe("1"); + expect(values[16]).toBe("2"); + }); + + it("should count the template towards at least two tests on its combined total", async () => { + // One unit test per file, two files: the template has two unit tests + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 1 }); + writeExtraTestFile("reconciliation_texts", "reconciliation_text_1", "extra_liquid_test.yml", 1); + + await stats.generateOverview("2024-01-01"); + + const values = fs.readFileSync(path.join(tempDir, "stats", "overview.csv"), "utf-8").trim().split("\r\n")[1].split(";"); + // Reconciliations - templates with at least two tests + expect(values[28]).toBe("1"); + }); + + it("should never report more than 100% of the templates as covered", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 1 }); + writeExtraTestFile("reconciliation_texts", "reconciliation_text_1", "extra_liquid_test.yml", 1); + writeExtraTestFile("reconciliation_texts", "reconciliation_text_1", "another_liquid_test.yml", 1); + + await stats.generateOverview("2024-01-01"); + + const values = fs.readFileSync(path.join(tempDir, "stats", "overview.csv"), "utf-8").trim().split("\r\n")[1].split(";"); + // Reconciliations - templates with yaml tests (%) + expect(Number(values[26])).toBe(100); + }); + }); + // ─── generateOverview (whole repository) ─────────────────────────────────── describe("generateOverview", () => { From 9382d21754cabe503d6d382efe20d2b5424eda55 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Wed, 19 Aug 2026 09:46:52 +0100 Subject: [PATCH 23/24] Skip workflow templates missing from the repository A handle can be listed in a workflow file without ever having been imported. Counting it overstated the workflow, and worse, listExternallyManagedTemplates read its config, which made createConfigIfMissing create the template folder and a config.json marked externally_managed. A typo in a workflow file scaffolded a template through a read-only reporting command and inflated the externally managed count with it. reportOnWorkflow now narrows the workflow to the templates the repository holds, reusing fsUtils.getAllTemplatesOfAType, before the counts and the git scan run, so both cover the same population. The templates left out are named in one warning per workflow through errorUtils.workflowTemplatesMissing, and no config is read for a template which is not stored. --- lib/cli/stats.js | 35 ++++++++++++++++++++-- lib/utils/errorUtils.js | 13 +++++++++ tests/lib/cli/stats.test.js | 58 +++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 2 deletions(-) diff --git a/lib/cli/stats.js b/lib/cli/stats.js index 99728114..ec4503c0 100644 --- a/lib/cli/stats.js +++ b/lib/cli/stats.js @@ -8,6 +8,13 @@ const errorUtils = require("../utils/errorUtils"); const yaml = require("yaml"); const { consola } = require("consola"); +// The template lists a workflow file holds, and the template type each of them is about +const WORKFLOW_TEMPLATE_TYPES = { + reconciliations: "reconciliationText", + accounts: "accountTemplate", + exports: "exportFile", +}; + async function generateOverview(sinceDate) { const TODAY = new Date().toJSON().toString().slice(0, 10); const templateSummary = await getTemplatesSummary(); @@ -39,8 +46,11 @@ async function reportOnWorkflow(sinceDate, workflowHandle) { return false; } const TODAY = new Date().toJSON().toString().slice(0, 10); - const templateSummary = await getWorkflowTemplateSummary(workflow); - const yamlSummary = await getYamlSummary(sinceDate, workflow); + // Both the counts and the git scan must cover the same templates, so the workflow is + // narrowed down to what the repository actually holds before either of them runs + const workflowInRepository = narrowWorkflowToRepository(workflow); + const templateSummary = await getWorkflowTemplateSummary(workflowInRepository); + const yamlSummary = await getYamlSummary(sinceDate, workflowInRepository); displayWorkflowOverview(sinceDate, TODAY, templateSummary, yamlSummary); const row = createWorkflowRow(sinceDate, TODAY, templateSummary, yamlSummary); // A failed write is reported by saveWorkflowOverviewToFile and does not make this a skipped @@ -73,6 +83,27 @@ async function reportOnAllWorkflows(sinceDate) { return skippedHandles.length < workflowHandles.length; } +// The templates of a workflow which the repository holds, per template type +// A handle can be listed in a workflow file without ever having been imported. Counting it +// would overstate the workflow and reading its config would create the folder it is missing, +// so it is left out and named to the user +// @param {Object} workflow The workflow as it is stored +// @returns {Object} A copy of the workflow, holding only the templates present in the repository +function narrowWorkflowToRepository(workflow) { + const templatesPresent = {}; + const missingTemplates = []; + for (const [attribute, templateType] of Object.entries(WORKFLOW_TEMPLATE_TYPES)) { + const listed = workflow.templates[attribute]; + const stored = fsUtils.getAllTemplatesOfAType(templateType); + templatesPresent[attribute] = listed.filter((name) => stored.includes(name)); + missingTemplates.push(...listed.filter((name) => !stored.includes(name))); + } + if (missingTemplates.length > 0) { + errorUtils.workflowTemplatesMissing(workflow.name, missingTemplates); + } + return { ...workflow, templates: templatesPresent }; +} + // Template names are interpolated into a regular expression. Reconciliation handles are // restricted to word characters, but account template and export file names are not function escapeForRegExp(value) { diff --git a/lib/utils/errorUtils.js b/lib/utils/errorUtils.js index cdcadf84..6a14e316 100644 --- a/lib/utils/errorUtils.js +++ b/lib/utils/errorUtils.js @@ -269,6 +269,18 @@ function workflowStatisticsNotSaved(handle) { return false; } +/** + * Templates listed in a workflow file which the repository does not hold. They are left out of + * the statistics, so the user is told which ones and why the totals may be lower than expected + * @param {string} workflowName The name of the workflow the templates were listed in + * @param {Array} missingTemplates The handles or names which have no template folder + */ +function workflowTemplatesMissing(workflowName, missingTemplates) { + consola.warn(`${workflowName}: ${missingTemplates.length} template(s) are listed but not stored in this repository: ${missingTemplates.join(", ")}`); + consola.log(`They are left out of the statistics. Import them, or correct the workflow file in ${WORKFLOWS_PATH}`); + return false; +} + /** * The statistics were gathered, but the CSV file could not be written. The summary is * already on screen, so this names the file and the likely cause instead of stopping the run @@ -317,6 +329,7 @@ module.exports = { invalidWorkflow, noWorkflowsStored, workflowStatisticsNotSaved, + workflowTemplatesMissing, statisticsNotWritten, printWorkflowBatchErrorSummary, }; diff --git a/tests/lib/cli/stats.test.js b/tests/lib/cli/stats.test.js index a37112d7..8a53e5b6 100644 --- a/tests/lib/cli/stats.test.js +++ b/tests/lib/cli/stats.test.js @@ -291,6 +291,64 @@ describe("cli/stats", () => { }); }); + // ─── templates listed in a workflow but not stored ───────────────────────── + + describe("a workflow listing a template which is not in the repository", () => { + it("should leave the missing template out of the counts", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 1, externallyManaged: true }); + writeWorkflow("workflow_1", { + name: "Workflow 1", + templates: { reconciliations: ["reconciliation_text_1", "reconciliation_text_typo"], accounts: [], exports: [] }, + }); + + await stats.generateWorkflowOverview("2024-01-01", "workflow_1"); + + const values = fs.readFileSync(path.join(tempDir, "stats", "workflow_1_stats.csv"), "utf-8").trim().split("\r\n")[1].split(";"); + // Reconciliations - templates, - externally managed + expect(values[9]).toBe("1"); + expect(values[10]).toBe("1"); + }); + + it("should name the missing templates", async () => { + writeTemplate("account_templates", "account_1", { unitTests: 1 }); + writeWorkflow("workflow_1", { + name: "Workflow 1", + templates: { reconciliations: [], accounts: ["account_1", "account_typo"], exports: ["export_typo"] }, + }); + + await stats.generateWorkflowOverview("2024-01-01", "workflow_1"); + + expect(consola.warn).toHaveBeenCalledWith(expect.stringContaining("account_typo, export_typo")); + expect(consola.warn).toHaveBeenCalledWith(expect.stringContaining("Workflow 1")); + }); + + it("should not create a folder or a config for the missing template", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 1 }); + writeWorkflow("workflow_1", { + name: "Workflow 1", + templates: { reconciliations: ["reconciliation_text_typo"], accounts: ["account_typo"], exports: [] }, + }); + + await stats.generateWorkflowOverview("2024-01-01", "workflow_1"); + + expect(fs.existsSync(path.join(tempDir, "reconciliation_texts", "reconciliation_text_typo"))).toBe(false); + expect(fs.existsSync(path.join(tempDir, "account_templates", "account_typo"))).toBe(false); + }); + + it("should not report the workflow as skipped", async () => { + writeTemplate("reconciliation_texts", "reconciliation_text_1", { unitTests: 1 }); + writeWorkflow("workflow_1", { + name: "Workflow 1", + templates: { reconciliations: ["reconciliation_text_1", "reconciliation_text_typo"], accounts: [], exports: [] }, + }); + + const reported = await stats.generateWorkflowOverview("2024-01-01", "workflow_1"); + + expect(reported).toBe(true); + expect(mockExit).not.toHaveBeenCalled(); + }); + }); + // ─── several test files in one tests folder ──────────────────────────────── describe("a template with more than one liquid test file", () => { From b2713ddda0094491a13d2fde61cc56ac415445b7 Mon Sep 17 00:00:00 2001 From: Toby Masters Date: Wed, 19 Aug 2026 09:50:33 +0100 Subject: [PATCH 24/24] Document stats workflow counting Say in the --workflow help and the README that workflow totals cover the templates the workflow file lists which are stored in the repository, excluding shared parts, so they are not comparable with a repository-wide run, and that a template counts once however many liquid test files it holds. Rewrite the unreleased 1.59.0 changelog entry as one bullet per user-visible change, add the missing bullet for the CSV write failure, and move the date to the current one. Update the stats.js and errorUtils.js inventories in ARCHITECTURE.md and catalogue the new tests. --- CHANGELOG.md | 5 ++++- README.md | 7 +++++++ docs/ARCHITECTURE.md | 4 ++-- tests/TESTS.md | 9 +++++++++ 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1cca92d9..6ec7fc6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,11 @@ All notable changes to this project will be documented in this file. -## [1.59.0] (11/08/2026) +## [1.59.0] (19/08/2026) Add a workflow filter to the stats command: use `--workflow ` for one workflow or `--workflow` on its own to report on every workflow in the workflows folder. +- Workflow statistics cover the templates the workflow file lists which are stored in this repository, excluding shared parts. A listed template you have not imported is left out of the totals and named in a warning, instead of being counted and having a template folder created for it. +- The YAML columns now count templates rather than test files: a template holding several liquid test files is counted once, its unit tests are added up, and the coverage percentages can no longer exceed 100%. The affected column headings were renamed, so the YAML columns of rows written before this release are not comparable with the ones written from now on. +- A CSV file which cannot be written (open in another program, no permission) is now reported with its path, and the statistics are still shown in the terminal. Previously the command ended in a stack trace. ## [1.58.0] (31/07/2026) Added the `company-data-copier` command, which triggers the platform Data Copier to copy a source company's data (account values incl. adjustments, text properties, people/company drop and configuration) into a brand-new company in a destination development firm. Intended for BSO developers to reproduce a client's situation in a dev firm without touching the production firm. Only *data* is copied, not template *code* — templates must already exist in the destination firm to be populated. diff --git a/README.md b/README.md index 6afeabf0..f8756737 100644 --- a/README.md +++ b/README.md @@ -329,6 +329,8 @@ silverfin stats --since 2024-01-01 This writes to `./stats/overview.csv` and covers every template in the repository, Shared Parts included. +A template counts once towards the YAML columns no matter how many Liquid Test files sit in its `tests` folder: `templates with yaml tests` counts the templates holding at least one non-empty test file, `unit tests` adds up every unit test found, and `templates with at least two tests` uses each template's combined total across its test files. + To report per workflow instead, use `--workflow`. See [Workflows](#workflows) for the expected file format. ```bash @@ -341,6 +343,11 @@ silverfin stats --since 2024-01-01 --workflow Each workflow gets its own file, `./stats/_stats.csv`. When a handle is passed explicitly and it cannot be found or read, the command reports the problem and stops. When every workflow is included, a faulty workflow file is skipped with a warning and the remaining ones are still processed, followed by a summary of what was skipped. +Workflow totals and repository totals count different things, so do not compare them directly: + +- The repository overview counts every non-empty template it finds, Shared Parts included. +- A workflow overview counts the templates its workflow file lists, excluding Shared Parts. A listed template which is not stored in the repository is left out of the totals and named in a warning, so the numbers only ever describe templates you actually have. + ## Contributing If you find any bug or you have any suggestion, please feel free to open an issue in this repository. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 2ba3c5a3..d06292b0 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -93,7 +93,7 @@ Pre-flight checks shared by commands: `loadDefaultFirmId`, `checkDefaultFirm`, ` **Contains:** validation that ends in a clear message + `process.exit` on failure. All new option validation goes here so `bin/cli.js` stays declarative. ### `stats.js` -Coverage reporting over the local template repo. Entry points `generateOverview(sinceDate)` and `generateWorkflowOverview(sinceDate, workflowHandle)` — the latter returns `false` when nothing could be reported on, leaving the exit to `bin/cli.js`. The rest are internal — workflow selection (`reportOnWorkflow`, `reportOnAllWorkflows`), counting templates and YAML tests (`getTemplatesSummary`, `getWorkflowTemplateSummary`, `yamlFilesActivity`, `countYamlFiles`), formatting (`displayOverview`, `createRow`, `percentageRoundTwo`) and CSV persistence (`saveOverviewToFile`, `saveWorkflowOverviewToFile`). +Coverage reporting over the local template repo. Entry points `generateOverview(sinceDate)` and `generateWorkflowOverview(sinceDate, workflowHandle)` — the latter returns `false` when nothing could be reported on, leaving the exit to `bin/cli.js`. The rest are internal — workflow selection (`reportOnWorkflow`, `reportOnAllWorkflows`, `narrowWorkflowToRepository`), counting templates and YAML tests (`getTemplatesSummary`, `getWorkflowTemplateSummary`, `yamlFilesActivity`, `countYamlFiles`, `buildTemplatePattern`), formatting (`displayOverview`, `createRow`, `percentageRoundTwo`) and CSV persistence (`saveOverviewToFile`, `saveWorkflowOverviewToFile`, `writeStatisticsRow`). **Contains:** metrics and their presentation. Only the two `generate*` functions should be exported. ### `devMode.js` @@ -145,7 +145,7 @@ Template-type vocabulary and name validation: `TEMPLATES_NAME_ATTRIBUTE`, `TEMPL **Contains:** the mapping between API type names and internal type keys. New template types are registered here first. ### `errorUtils.js` -`uncaughtErrors`, `errorHandler`, `missingConfig`, `missingId`, and `printBatchErrorSummary` for each of the four template types. Command line input: `missingHandle`, `invalidHandleFormat`. Workflow failures: `invalidWorkflowHandle`, `missingWorkflow`, `unparsableWorkflow`, `invalidWorkflow`, `noWorkflowsStored`, `workflowStatisticsNotSaved`, `printWorkflowBatchErrorSummary`. +`uncaughtErrors`, `errorHandler`, `missingConfig`, `missingId`, and `printBatchErrorSummary` for each of the four template types. Command line input: `missingHandle`, `invalidHandleFormat`. Workflow failures: `invalidWorkflowHandle`, `missingWorkflow`, `unparsableWorkflow`, `invalidWorkflow`, `noWorkflowsStored`, `workflowStatisticsNotSaved`, `workflowTemplatesMissing`, `printWorkflowBatchErrorSummary`. Statistics: `statisticsNotWritten`. **Contains:** every user-facing error message. New failure modes get a named function here rather than an inline `console.error`. ### `apiUtils.js` diff --git a/tests/TESTS.md b/tests/TESTS.md index e186f2f9..63a345db 100644 --- a/tests/TESTS.md +++ b/tests/TESTS.md @@ -534,7 +534,16 @@ Source: `lib/cli/stats.js` | `generateWorkflowOverview` (no templates) | should not run the git scan when there is nothing to match | Verifies that `git whatchanged` is not invoked when the workflow holds no templates. | | `generateWorkflowOverview` (regex characters) | should treat a quantifier in a name literally | Verifies that a template name containing `+` or `*` is escaped before being interpolated into the match pattern. | | `generateWorkflowOverview` (regex characters) | should not build an invalid regular expression from a name with brackets | Verifies that a name containing brackets does not throw when the pattern is compiled. | +| `generateWorkflowOverview` (missing templates) | should leave the missing template out of the counts | Verifies that a handle listed in the workflow but not stored in the repository is excluded from the totals. | +| `generateWorkflowOverview` (missing templates) | should name the missing templates | Verifies that the warning names the workflow and every handle which has no template folder. | +| `generateWorkflowOverview` (missing templates) | should not create a folder or a config for the missing template | Verifies that reading the statistics never scaffolds a template folder or a `config.json` for an unknown handle. | +| `generateWorkflowOverview` (missing templates) | should not report the workflow as skipped | Verifies that missing templates lower the totals but still count as a successful report. | +| `countYamlFiles` (several test files) | should count the template once and add up its unit tests in the repository overview | Verifies that two liquid test files in one `tests` folder produce one template and the sum of their unit tests. | +| `countYamlFiles` (several test files) | should count the template once and add up its unit tests in a workflow overview | Verifies the same grouping when the counts are filtered by a workflow. | +| `countYamlFiles` (several test files) | should count the template towards at least two tests on its combined total | Verifies that two files holding one unit test each make the template count as having at least two tests. | +| `countYamlFiles` (several test files) | should never report more than 100% of the templates as covered | Verifies that three test files for one template give a coverage percentage of 100, not 300. | | `generateOverview` | should count every template in the repository | Verifies that the repository-wide overview counts all templates and writes `stats/stats.csv`. | +| `generateOverview` | should report a write failure instead of throwing | Verifies that a failing `appendFileSync` is reported with the CSV path and the reason, without exiting. | | `generateOverview` | should append a row when run a second time | Verifies that a second run adds a row rather than replacing the file. | ---