Skip to content

Comprehensive testing overhaul: fixtures, unit tests, E2E CLI tests#254

Open
michieldegezelle wants to merge 16 commits into
mainfrom
testing-overhaul
Open

Comprehensive testing overhaul: fixtures, unit tests, E2E CLI tests#254
michieldegezelle wants to merge 16 commits into
mainfrom
testing-overhaul

Conversation

@michieldegezelle
Copy link
Copy Markdown
Contributor

Summary

This PR adds a comprehensive test suite overhaul across 11 focused commits, growing the test count from 222 → 485 tests across 17 → 39 test suites. No source files were modified — all changes are in fixtures/, tests/, and a new tests/TESTS.md doc.

What changed

Commit Change
Enrich fixtures Full config.json + .liquid files for all 12 market-repo templates (3 per type); new fixtures/api-responses/ with Silverfin API response JSON for all 4 template types
Refactor template tests Replace inline mock objects in the 4 template class tests with require() calls into the shared fixture files
templateUtils.test.js New — covers getTemplateName, checkValidName, filterParts, missingLiquidCode, missingNameNL, and the three exported constants
apiUtils.test.js New — covers env var validation, all HTTP error handlers (400/401/403/404/422), and checkAuthorizePartners
fsUtils.test.js New — covers all functions in the 570-line core FS utilities module (folder creation, config read/write, template ID lookup, template discovery)
sfApi.test.js New — covers all CRUD endpoints for all 4 template types via axios-mock-adapter; API response bodies loaded from fixtures
Complete toolkit.test.js Adds the ~35 previously untested fetchXxx / newXxx / publishXxxByHandle/All / addSharedPart / removeSharedPart / getTemplateId functions
liquidTestRunner.test.js New — covers YAML parsing, API submission, result polling, and pass/fail evaluation
exportFileInstanceGenerator.test.js + cli/utils.test.js New — covers instance generation/polling and CLI option validation helpers
15 E2E test files in tests/bin/cli/ Import / update / create / get-id commands for all 4 template types; each has happy path, not-found, and config-merge scenarios
tests/TESTS.md Full test catalogue, fixture guide, and 10 flagged code inconsistencies for follow-up PRs

Flagged inconsistencies (not fixed here — tracked in TESTS.md)

  1. Mixed .test.js / .spec.js file naming — standardise on .test.js
  2. Inconsistent temp dir paths across test files
  3. No coverage thresholds in jest.config.js
  4. No --forceExit flag (async tests can hang CI)
  5. sfApi.js calls apiUtils.checkRequiredEnvVariables() at module load time, complicating isolation
  6. Recursive pagination in fetchAllXxx has no explicit depth guard at the toolkit layer
  7. errorUtils.js imported inconsistently across test files
  8. Mixed fs / fs.promises usage in test setup code
  9. shared_part_3/config.json had a copy-paste bug (name referenced shared_part_2) — fixed in the fixtures commit
  10. toolkit.test.js previously only covered publishXxxById functions

Test plan

  • npm test passes with 485 tests, 0 failures
  • No source files (lib/, bin/, index.js) were modified
  • Each commit was verified green before the next was started

🤖 Generated with Claude Code

michieldegezelle and others added 11 commits May 26, 2026 10:56
Enriches the previously near-empty fixtures/ directory with full, realistic
data for all 12 market-repo templates (3 per type) and introduces a new
fixtures/api-responses/ directory holding the JSON shapes the Silverfin API
returns. Tests can now require() these shared fixtures instead of embedding
duplicate inline objects.

Also fixes a copy-paste bug in shared_part_3/config.json where the name and
text fields incorrectly referenced shared_part_2.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces inline JS mock objects in all 4 template test files with
require() calls into fixtures/api-responses/ and fixtures/market-repo/.
Test logic and coverage are unchanged — only the data source is shared.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers all exported functions: getTemplateName, checkValidName,
filterParts, missingLiquidCode, missingNameNL, and the three constants.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers checkRequiredEnvVariables (env var validation), all HTTP error
status handlers (400, 401, 403, 404, 422, unhandled), and
checkAuthorizePartners.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers all exported functions: folder/file creation, config read/write,
template ID get/set, template discovery, and handle lookup.
Uses real temp directories matching the established test pattern.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers all CRUD operations for all 4 template types using
axios-mock-adapter for HTTP responses. API response bodies are loaded
from the shared fixtures/api-responses/ directory.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds tests for all previously untested functions: fetchXxx, newXxx,
publishXxxByHandle/All, addSharedPart, removeSharedPart, getTemplateId,
getAllTemplatesId, and updateFirmName across all 4 template types.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers checkAllTestsErrorsPresent (pure logic), getHTML (URL handler
delegation), runTests (YAML loading, API submission, polling), runTestsWithOutput
(pass/fail/error output formatting), and runTestsStatusOnly (multi-handle
status aggregation) with real temp-dir filesystem setup.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ExportFileInstanceGenerator: covers constructor validation, polling
loop (pending → created via fake timers), validation-error warnings,
and failure-state error handling.

cli/utils: covers loadDefaultFirmId (config vs env fallback),
checkDefaultFirm, formatOption, checkUniqueOption, checkRequiredFirmOrPartner,
getCommandSettings, checkPartnerSupport, and logCurrentHost.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Creates 15 integration-style test files covering import, update, create,
and get-id commands for reconciliation texts, export files, account templates,
and shared parts. Tests exercise real toolkit functions with mocked SF API
calls and real temporary filesystem state.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Documents the directory layout of all 39 test files, the two fixture
trees (api-responses and market-repo), and the three test patterns used:
unit tests, API tests (axios-mock-adapter), and E2E command tests.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds API response and market-repo fixtures plus a large Jest test suite covering CLI create/get/import/update flows, sfApi integration, template save/read behavior, filesystem utilities, export-file instance polling, and liquid test runner tooling.

Changes

Test Fixtures and Comprehensive Coverage

Layer / File(s) Summary
API Response Fixtures
fixtures/api-responses/account-templates/*, fixtures/api-responses/export-files/*, fixtures/api-responses/reconciliation-texts/*, fixtures/api-responses/shared-parts/*
JSON fixtures define API response structures for list and single-item endpoints, establishing contracts for account templates, export files, reconciliation texts, and shared parts with multilingual names, ranges, flags, and templated content.
Market Repo Account Template Fixtures
fixtures/market-repo/account_templates/account_1/*, fixtures/market-repo/account_templates/account_2/*, fixtures/market-repo/account_templates/account_3/*
Three account template development fixtures with config.json, Liquid main templates, text_parts (detail), and YAML test cases using period.accounts data and currency formatting.
Market Repo Export File Fixtures
fixtures/market-repo/export_files/export_1/*, fixtures/market-repo/export_files/export_2/*, fixtures/market-repo/export_files/export_3/*
Three export file development fixtures with config.json, Liquid templates for main and header text parts, and file metadata including encoding and filename settings.
Market Repo Reconciliation Text Fixtures
fixtures/market-repo/reconciliation_texts/reconciliation_text_1/*, fixtures/market-repo/reconciliation_texts/reconciliation_text_2/*, fixtures/market-repo/reconciliation_texts/reconciliation_text_3/*
Three reconciliation text fixtures with complete configs, Liquid templates, text_parts, and YAML test definitions; demonstrates basic, no-reconciliation-needed, and multi-account debit/credit patterns.
Market Repo Shared Parts Fixtures
fixtures/market-repo/shared_parts/shared_part_1/*, fixtures/market-repo/shared_parts/shared_part_2/*, fixtures/market-repo/shared_parts/shared_part_3/*
Three shared-part fixtures with updated config structure (externally_managed, hide_code flags) and Liquid content for template reuse.
Test Documentation and Config
tests/TESTS.md, fixtures/silverfin/config.json
Test suite documentation explaining test structure, commands, fixtures, and CLI test conventions; credentials fixture updated with access/refresh tokens for firms and API tokens for partners.
CLI Create Command Tests
tests/bin/cli/create-account-template.test.js, tests/bin/cli/create-export-file.test.js, tests/bin/cli/create-reconciliation.test.js, tests/bin/cli/create-shared-part.test.js
Jest test suites for CLI create operations verifying successful remote creation with config persistence and skipping when resources already exist remotely.
CLI Get ID and Import Tests
tests/bin/cli/get-account-template-id.test.js, tests/bin/cli/get-export-file-id.test.js, tests/bin/cli/get-reconciliation-id.test.js, tests/bin/cli/get-shared-part-id.test.js, tests/bin/cli/import-account-template.test.js, tests/bin/cli/import-export-file.test.js, tests/bin/cli/import-shared-part.test.js
Jest tests for template ID lookup, persistence into local configs, and import operations that fetch by ID/name and create local directory structures with main.liquid and config.json.
CLI Update/Publish Tests
tests/bin/cli/update-account-template.test.js, tests/bin/cli/update-export-file.test.js, tests/bin/cli/update-reconciliation.test.js, tests/bin/cli/update-shared-part.test.js
Jest tests for CLI update/publish operations verifying API call arguments and success/error logging outcomes.
SfApi Integration Tests
tests/lib/api/sfApi.test.js
Jest test suite wiring real Axios with mock adapter, validating API endpoints, success/error responses across HTTP statuses, and "not found" behavior for authorization, reconciliation texts, shared parts, export files, and account templates.
CLI and Utility Helpers Tests
tests/lib/cli/utils.test.js, tests/lib/utils/apiUtils.test.js, tests/lib/utils/templateUtils.test.js, tests/lib/utils/fsUtils.test.js
Jest tests for CLI option parsing (camelCase to kebab-case), firm/partner defaults, environment validation, response error handling, template name validation (slashes, Unicode), and filesystem operations (folders, configs, template IDs).
Instance Generator and Liquid Test Runner Tests
tests/lib/exportFileInstanceGenerator.test.js, tests/lib/liquidTestRunner.test.js
Jest tests for export file instance polling (pending to created), content URL handling, and Liquid test runner (checkAllTestsErrorsPresent, runTests variants) with temp directory setup and mocked process.exit.
Template Save/Read Tests
tests/lib/templates/accountTemplates.test.js, tests/lib/templates/exportFiles.test.js, tests/lib/templates/reconciliationTexts.test.js, tests/lib/templates/sharedParts.test.js
Jest tests for template classes verifying save (config writing, file creation), read (config parsing), merging existing configs, preserving liquid/test files, and updated text_parts naming (detail, header, part).
Toolkit Integration Tests
tests/lib/toolkit.test.js
Jest tests for Toolkit orchestration covering fetch (by ID/handle/name), bulk fetch, publish, create, and helper utilities (getTemplateId, getAllTemplatesId, updateFirmName) with config persistence and exit behavior validation.

🎯 3 (Moderate) | ⏱️ ~25 minutes

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch testing-overhaul

The `read` describe blocks in reconciliationTexts and accountTemplates
tests called process.chdir(tempDir) in beforeEach but did not restore CWD
in afterEach. Jest reuses workers across test files, so a subsequent file
(import-reconciliation.test.js) would capture the already-deleted tempDir
as its originalCwd and fail on chdir back.

Fixes:
- Add process.chdir(repoRoot) before cleanup in both read afterEach blocks
- Hoist originalCwd to module-level in import-reconciliation.test.js so
  it always resolves to the repo root rather than process.cwd() at test time

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 10

🧹 Nitpick comments (4)
tests/TESTS.md (2)

68-68: 💤 Low value

Add language specifier to fenced code block.

The fenced code block should specify a language identifier for better rendering and consistency with markdown best practices.

📝 Proposed fix
-```
+```text
 fixtures/
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/TESTS.md` at line 68, The fenced code block in tests/TESTS.md that
contains the line "fixtures/" is missing a language specifier; update the
opening fence from ``` to include a language identifier (for example change ```
to ```text or ```bash) so the block becomes ```text (or another appropriate
language) to ensure proper markdown rendering and consistent formatting.

13-13: 💤 Low value

Add language specifier to fenced code block.

The fenced code block should specify a language identifier for better rendering and consistency with markdown best practices.

📝 Proposed fix
-```
+```text
 tests/
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/TESTS.md` at line 13, Update the fenced code block that contains the
"tests/" listing by adding a language specifier to the opening fence (change the
opening "```" that precedes the tests/ block to "```text" or another appropriate
language like "```bash"); ensure you only modify the opening fence so the
closing "```" remains unchanged and the block renders with the specified
language.
tests/lib/exportFileInstanceGenerator.test.js (2)

62-70: ⚡ Quick win

Assert the returned false value explicitly.

The test name claims generateAndOpenFile() returns false, but no assertion checks the return value. Capture and assert it to lock the contract.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/lib/exportFileInstanceGenerator.test.js` around lines 62 - 70, The test
should assert that generateAndOpenFile() returns false when
SF.createExportFileInstance resolves to null: call const result = await
gen.generateAndOpenFile(); then add an expectation that result is false; locate
the test using ExportFileInstanceGenerator and the mocked
SF.createExportFileInstance to add the assertion (expect(result).toBe(false) or
equivalent).

124-136: ⚡ Quick win

Harden fake-timer cleanup in afterEach.

jest.useRealTimers() is only called in-test. If this test fails early, timer mode can leak into later tests. Move/reset this in an afterEach for reliability.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/lib/exportFileInstanceGenerator.test.js` around lines 124 - 136, The
test uses jest.useFakeTimers() but only calls jest.useRealTimers() inside the
test, which can leak fake timers if the test exits early; add a cleanup in the
test suite's afterEach to always restore timers (call jest.useRealTimers()) so
timer mode is reset for subsequent tests—apply this near the tests that
instantiate ExportFileInstanceGenerator and call generateAndOpenFile() to
guarantee real timers are restored even on failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/bin/cli/create-account-template.test.js`:
- Around line 18-37: beforeEach captures tempDir and overrides process.cwd but
the original working directory isn't saved; update beforeEach to store the
current working directory (e.g., const originalCwd = process.cwd() or assign to
an existing variable originalCwd) before changing directories, and then in
afterEach restore that saved originalCwd with process.chdir(originalCwd); also
ensure the variable name used matches the one referenced in both hooks
(originalCwd), and keep restoring process.exit and any mocked globals as already
done.

In `@tests/bin/cli/import-shared-part.test.js`:
- Around line 94-105: The test swallows any runtime error by using a broad
try/catch; instead make the exit path explicit by mocking process.exit to throw
a sentinel (or use a spy) and assert it was called with 1, then verify
SF.readSharedPartById was not called; update the test around
toolkit.fetchSharedPartByName to set
SF.findSharedPartByName.mockResolvedValue(null), mock/spy process.exit (or have
it throw a known Error), await the call expecting that sentinel, assert
process.exit was invoked with 1, and assert
SF.readSharedPartById.mock.calls.length === 0 to ensure no follow-up read
occurred.

In `@tests/bin/cli/update-account-template.test.js`:
- Around line 1-2: Remove the unused "fs" require and only import the promises
API—delete the const fs = require("fs"); line and keep the existing const
fsPromises = require("fs").promises; so the file no longer defines an unused
symbol and satisfies the linter.

In `@tests/bin/cli/update-export-file.test.js`:
- Around line 1-2: Remove the unused top-level require for "fs" to satisfy the
no-unused-vars rule: delete the line declaring the variable fs (const fs =
require("fs");) and keep the existing fsPromises import (const fsPromises =
require("fs").promises;) so only fsPromises is required and used in
update-export-file.test.js.

In `@tests/bin/cli/update-reconciliation.test.js`:
- Around line 1-2: Remove the unused top-level CommonJS import "fs" and only
keep the existing "fs.promises" import (currently assigned to fsPromises);
update the require statements so that fs is not declared (remove const fs =
require("fs");) and ensure any references use fsPromises where appropriate
(search for the symbols fs and fsPromises in this test file to confirm only
fsPromises is used).

In `@tests/bin/cli/update-shared-part.test.js`:
- Around line 1-2: Remove the unused top-level binding "fs" from the test file
and only keep the promises import used elsewhere: delete the line declaring
const fs = require("fs"); and retain or consolidate to const fsPromises =
require("fs").promises (or switch to require("fs").promises usage throughout) so
only the used identifier "fsPromises" remains in
tests/bin/cli/update-shared-part.test.js.

In `@tests/lib/liquidTestRunner.test.js`:
- Around line 64-72: The function setupFsUtilsMocks declares an unused parameter
templateType which triggers lint errors; remove the templateType parameter from
the function signature (leave handle with its default), and update any callsites
if they pass a second argument to only pass the handle or omit it—ensure the
function body (fsUtils.configExists, fsUtils.readConfig,
fsUtils.listSharedPartsUsedInTemplate) remains unchanged and referenced by the
same name setupFsUtilsMocks.

In `@tests/lib/templates/sharedParts.test.js`:
- Around line 14-16: The imported fixture variable existingConfigFixture in
tests/lib/templates/sharedParts.test.js is unused and triggers no-unused-vars;
remove the import statement requiring
"../../../fixtures/market-repo/shared_parts/shared_part_2/config.json" (the
existingConfigFixture variable) from the top of the file or use it where
intended so the linter no longer reports an unused variable.

In `@tests/lib/toolkit.test.js`:
- Around line 1098-1100: The unused binding "firmCredentials" should be removed
to satisfy ESLint no-unused-vars: delete or comment out the require line that
destructures firmCredentials (the const { firmCredentials } = require(...)) in
the test so the module isn't imported into the scope unused; if the module is
needed for side-effects instead, require it without destructuring (e.g.,
require(".../firmCredentials")) or add a proper mock where it's actually
referenced by tests.

In `@tests/lib/utils/templateUtils.test.js`:
- Around line 110-114: The test case title and assertion for
templateUtils.checkValidName are inconsistent: change either the description or
the assertion so they match; specifically, update the it(...) string from
"should return false for empty string (reconciliationText)" to "should return
true for empty string (reconciliationText)" OR invert the expectation to
expect(result).toBe(false) depending on the intended behavior of checkValidName;
locate the test using templateUtils.checkValidName in
tests/lib/utils/templateUtils.test.js and make the title and asserted value
consistent.

---

Nitpick comments:
In `@tests/lib/exportFileInstanceGenerator.test.js`:
- Around line 62-70: The test should assert that generateAndOpenFile() returns
false when SF.createExportFileInstance resolves to null: call const result =
await gen.generateAndOpenFile(); then add an expectation that result is false;
locate the test using ExportFileInstanceGenerator and the mocked
SF.createExportFileInstance to add the assertion (expect(result).toBe(false) or
equivalent).
- Around line 124-136: The test uses jest.useFakeTimers() but only calls
jest.useRealTimers() inside the test, which can leak fake timers if the test
exits early; add a cleanup in the test suite's afterEach to always restore
timers (call jest.useRealTimers()) so timer mode is reset for subsequent
tests—apply this near the tests that instantiate ExportFileInstanceGenerator and
call generateAndOpenFile() to guarantee real timers are restored even on
failures.

In `@tests/TESTS.md`:
- Line 68: The fenced code block in tests/TESTS.md that contains the line
"fixtures/" is missing a language specifier; update the opening fence from ```
to include a language identifier (for example change ``` to ```text or ```bash)
so the block becomes ```text (or another appropriate language) to ensure proper
markdown rendering and consistent formatting.
- Line 13: Update the fenced code block that contains the "tests/" listing by
adding a language specifier to the opening fence (change the opening "```" that
precedes the tests/ block to "```text" or another appropriate language like
"```bash"); ensure you only modify the opening fence so the closing "```"
remains unchanged and the block renders with the specified language.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bc6f7d03-b7b3-4241-94c9-4cfd3da8371b

📥 Commits

Reviewing files that changed from the base of the PR and between 26dec65 and b83e824.

📒 Files selected for processing (69)
  • fixtures/api-responses/account-templates/list.json
  • fixtures/api-responses/account-templates/single-with-mapping-list-ranges.json
  • fixtures/api-responses/account-templates/single.json
  • fixtures/api-responses/export-files/list.json
  • fixtures/api-responses/export-files/single.json
  • fixtures/api-responses/reconciliation-texts/list.json
  • fixtures/api-responses/reconciliation-texts/single-externally-managed.json
  • fixtures/api-responses/reconciliation-texts/single.json
  • fixtures/api-responses/shared-parts/list.json
  • fixtures/api-responses/shared-parts/single.json
  • fixtures/market-repo/account_templates/account_1/config.json
  • fixtures/market-repo/account_templates/account_1/main.liquid
  • fixtures/market-repo/account_templates/account_1/tests/account_1_liquid_test.yml
  • fixtures/market-repo/account_templates/account_1/text_parts/detail.liquid
  • fixtures/market-repo/account_templates/account_2/config.json
  • fixtures/market-repo/account_templates/account_2/main.liquid
  • fixtures/market-repo/account_templates/account_3/config.json
  • fixtures/market-repo/account_templates/account_3/main.liquid
  • fixtures/market-repo/export_files/export_1/config.json
  • fixtures/market-repo/export_files/export_1/main.liquid
  • fixtures/market-repo/export_files/export_1/text_parts/header.liquid
  • fixtures/market-repo/export_files/export_2/config.json
  • fixtures/market-repo/export_files/export_2/main.liquid
  • fixtures/market-repo/export_files/export_3/config.json
  • fixtures/market-repo/export_files/export_3/main.liquid
  • fixtures/market-repo/reconciliation_texts/reconciliation_text_1/config.json
  • fixtures/market-repo/reconciliation_texts/reconciliation_text_1/main.liquid
  • fixtures/market-repo/reconciliation_texts/reconciliation_text_1/tests/reconciliation_text_1_liquid_test.yml
  • fixtures/market-repo/reconciliation_texts/reconciliation_text_1/text_parts/part_1.liquid
  • fixtures/market-repo/reconciliation_texts/reconciliation_text_1/text_parts/part_2.liquid
  • fixtures/market-repo/reconciliation_texts/reconciliation_text_2/config.json
  • fixtures/market-repo/reconciliation_texts/reconciliation_text_2/main.liquid
  • fixtures/market-repo/reconciliation_texts/reconciliation_text_3/config.json
  • fixtures/market-repo/reconciliation_texts/reconciliation_text_3/main.liquid
  • fixtures/market-repo/shared_parts/shared_part_1/config.json
  • fixtures/market-repo/shared_parts/shared_part_1/shared_part_1.liquid
  • fixtures/market-repo/shared_parts/shared_part_2/config.json
  • fixtures/market-repo/shared_parts/shared_part_2/shared_part_2.liquid
  • fixtures/market-repo/shared_parts/shared_part_3/config.json
  • fixtures/market-repo/shared_parts/shared_part_3/shared_part_3.liquid
  • fixtures/silverfin/config.json
  • tests/TESTS.md
  • tests/bin/cli/create-account-template.test.js
  • tests/bin/cli/create-export-file.test.js
  • tests/bin/cli/create-reconciliation.test.js
  • tests/bin/cli/create-shared-part.test.js
  • tests/bin/cli/get-account-template-id.test.js
  • tests/bin/cli/get-export-file-id.test.js
  • tests/bin/cli/get-reconciliation-id.test.js
  • tests/bin/cli/get-shared-part-id.test.js
  • tests/bin/cli/import-account-template.test.js
  • tests/bin/cli/import-export-file.test.js
  • tests/bin/cli/import-shared-part.test.js
  • tests/bin/cli/update-account-template.test.js
  • tests/bin/cli/update-export-file.test.js
  • tests/bin/cli/update-reconciliation.test.js
  • tests/bin/cli/update-shared-part.test.js
  • tests/lib/api/sfApi.test.js
  • tests/lib/cli/utils.test.js
  • tests/lib/exportFileInstanceGenerator.test.js
  • tests/lib/liquidTestRunner.test.js
  • tests/lib/templates/accountTemplates.test.js
  • tests/lib/templates/exportFiles.test.js
  • tests/lib/templates/reconciliationTexts.test.js
  • tests/lib/templates/sharedParts.test.js
  • tests/lib/toolkit.test.js
  • tests/lib/utils/apiUtils.test.js
  • tests/lib/utils/fsUtils.test.js
  • tests/lib/utils/templateUtils.test.js

Comment on lines +18 to +37
beforeEach(async () => {
jest.clearAllMocks();

tempDir = await fsPromises.mkdtemp(path.join(os.tmpdir(), "sf-cli-test-"));

process.chdir(tempDir);

originalExit = process.exit;
process.exit = jest.fn();

consola.success = jest.fn();
consola.error = jest.fn();
consola.info = jest.fn();
consola.log = jest.fn();
consola.warn = jest.fn();
});

afterEach(async () => {
process.chdir(path.resolve(__dirname, "../../.."));
process.exit = originalExit;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Restore the exact pre-test working directory.

Line 36 resets cwd to a repo-relative path, not the pre-test cwd. This can leak process state when tests are invoked from a different start directory. Capture the original cwd before Line 23 and restore it in afterEach.

💡 Suggested patch
 describe("create-account-template", () => {
   let tempDir;
+  let originalCwd;
 
   let originalExit;
 
   beforeEach(async () => {
     jest.clearAllMocks();
 
     tempDir = await fsPromises.mkdtemp(path.join(os.tmpdir(), "sf-cli-test-"));
 
+    originalCwd = process.cwd();
     process.chdir(tempDir);
 
     originalExit = process.exit;
     process.exit = jest.fn();
@@
   afterEach(async () => {
-    process.chdir(path.resolve(__dirname, "../../.."));
+    process.chdir(originalCwd);
     process.exit = originalExit;
     await fsPromises.rm(tempDir, { recursive: true, force: true });
   });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/bin/cli/create-account-template.test.js` around lines 18 - 37,
beforeEach captures tempDir and overrides process.cwd but the original working
directory isn't saved; update beforeEach to store the current working directory
(e.g., const originalCwd = process.cwd() or assign to an existing variable
originalCwd) before changing directories, and then in afterEach restore that
saved originalCwd with process.chdir(originalCwd); also ensure the variable name
used matches the one referenced in both hooks (originalCwd), and keep restoring
process.exit and any mocked globals as already done.

Comment thread tests/bin/cli/import-shared-part.test.js Outdated
Comment thread tests/bin/cli/update-account-template.test.js Outdated
Comment thread tests/bin/cli/update-export-file.test.js Outdated
Comment thread tests/bin/cli/update-reconciliation.test.js Outdated
Comment thread tests/bin/cli/update-shared-part.test.js Outdated
Comment thread tests/lib/liquidTestRunner.test.js Outdated
Comment thread tests/lib/templates/sharedParts.test.js
Comment thread tests/lib/toolkit.test.js Outdated
Comment thread tests/lib/utils/templateUtils.test.js Outdated
michieldegezelle and others added 4 commits May 26, 2026 12:01
- Remove unused `fs` import from 4 update-* CLI test files
- Rename unused `templateType` parameter to `_templateType` in liquidTestRunner helper
- Change `let configSaved` to `const` where there is no reassignment
- Remove unused `existingConfigFixture` import from sharedParts.test.js
- Remove unused `firmCredentials` require inside toolkit updateFirmName test

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Use sentinel throw pattern instead of try/catch for process.exit testing
- Remove unused _templateType parameter from setupFsUtilsMocks helper
- Fix mislabelled test description for empty string case in checkValidName
- Add text language specifier to bare fenced code blocks in TESTS.md

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Every one of the 485 test cases is now listed in a table with the
function it exercises and a one-sentence description of what it asserts.
Covers all 39 test files across bin/cli, lib/api, lib/cli, lib/templates,
lib/utils, and the top-level lib/ modules.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace `| number_to_currency` with `| currency` across all fixture
  liquid files and api-response JSON fixtures (the filter does not exist
  in Silverfin Liquid; the correct filter is `| currency`)
- Add `{% include 'part_name' %}` calls in main.liquid for every template
  that declares text parts in its config:
  - reconciliation_text_1: includes part_1 and part_2
  - account_1: includes detail
  - export_1: includes header (at top, before main content)
- Standardise shared part include in reconciliation_text_1 to single quotes

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant