Comprehensive testing overhaul: fixtures, unit tests, E2E CLI tests#254
Comprehensive testing overhaul: fixtures, unit tests, E2E CLI tests#254michieldegezelle wants to merge 16 commits into
Conversation
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>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds 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. ChangesTest Fixtures and Comprehensive Coverage
🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
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>
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
tests/TESTS.md (2)
68-68: 💤 Low valueAdd 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 valueAdd 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 winAssert the returned
falsevalue explicitly.The test name claims
generateAndOpenFile()returnsfalse, 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 winHarden 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 anafterEachfor 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
📒 Files selected for processing (69)
fixtures/api-responses/account-templates/list.jsonfixtures/api-responses/account-templates/single-with-mapping-list-ranges.jsonfixtures/api-responses/account-templates/single.jsonfixtures/api-responses/export-files/list.jsonfixtures/api-responses/export-files/single.jsonfixtures/api-responses/reconciliation-texts/list.jsonfixtures/api-responses/reconciliation-texts/single-externally-managed.jsonfixtures/api-responses/reconciliation-texts/single.jsonfixtures/api-responses/shared-parts/list.jsonfixtures/api-responses/shared-parts/single.jsonfixtures/market-repo/account_templates/account_1/config.jsonfixtures/market-repo/account_templates/account_1/main.liquidfixtures/market-repo/account_templates/account_1/tests/account_1_liquid_test.ymlfixtures/market-repo/account_templates/account_1/text_parts/detail.liquidfixtures/market-repo/account_templates/account_2/config.jsonfixtures/market-repo/account_templates/account_2/main.liquidfixtures/market-repo/account_templates/account_3/config.jsonfixtures/market-repo/account_templates/account_3/main.liquidfixtures/market-repo/export_files/export_1/config.jsonfixtures/market-repo/export_files/export_1/main.liquidfixtures/market-repo/export_files/export_1/text_parts/header.liquidfixtures/market-repo/export_files/export_2/config.jsonfixtures/market-repo/export_files/export_2/main.liquidfixtures/market-repo/export_files/export_3/config.jsonfixtures/market-repo/export_files/export_3/main.liquidfixtures/market-repo/reconciliation_texts/reconciliation_text_1/config.jsonfixtures/market-repo/reconciliation_texts/reconciliation_text_1/main.liquidfixtures/market-repo/reconciliation_texts/reconciliation_text_1/tests/reconciliation_text_1_liquid_test.ymlfixtures/market-repo/reconciliation_texts/reconciliation_text_1/text_parts/part_1.liquidfixtures/market-repo/reconciliation_texts/reconciliation_text_1/text_parts/part_2.liquidfixtures/market-repo/reconciliation_texts/reconciliation_text_2/config.jsonfixtures/market-repo/reconciliation_texts/reconciliation_text_2/main.liquidfixtures/market-repo/reconciliation_texts/reconciliation_text_3/config.jsonfixtures/market-repo/reconciliation_texts/reconciliation_text_3/main.liquidfixtures/market-repo/shared_parts/shared_part_1/config.jsonfixtures/market-repo/shared_parts/shared_part_1/shared_part_1.liquidfixtures/market-repo/shared_parts/shared_part_2/config.jsonfixtures/market-repo/shared_parts/shared_part_2/shared_part_2.liquidfixtures/market-repo/shared_parts/shared_part_3/config.jsonfixtures/market-repo/shared_parts/shared_part_3/shared_part_3.liquidfixtures/silverfin/config.jsontests/TESTS.mdtests/bin/cli/create-account-template.test.jstests/bin/cli/create-export-file.test.jstests/bin/cli/create-reconciliation.test.jstests/bin/cli/create-shared-part.test.jstests/bin/cli/get-account-template-id.test.jstests/bin/cli/get-export-file-id.test.jstests/bin/cli/get-reconciliation-id.test.jstests/bin/cli/get-shared-part-id.test.jstests/bin/cli/import-account-template.test.jstests/bin/cli/import-export-file.test.jstests/bin/cli/import-shared-part.test.jstests/bin/cli/update-account-template.test.jstests/bin/cli/update-export-file.test.jstests/bin/cli/update-reconciliation.test.jstests/bin/cli/update-shared-part.test.jstests/lib/api/sfApi.test.jstests/lib/cli/utils.test.jstests/lib/exportFileInstanceGenerator.test.jstests/lib/liquidTestRunner.test.jstests/lib/templates/accountTemplates.test.jstests/lib/templates/exportFiles.test.jstests/lib/templates/reconciliationTexts.test.jstests/lib/templates/sharedParts.test.jstests/lib/toolkit.test.jstests/lib/utils/apiUtils.test.jstests/lib/utils/fsUtils.test.jstests/lib/utils/templateUtils.test.js
| 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; |
There was a problem hiding this comment.
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.
- 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>
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 newtests/TESTS.mddoc.What changed
config.json+.liquidfiles for all 12 market-repo templates (3 per type); newfixtures/api-responses/with Silverfin API response JSON for all 4 template typesrequire()calls into the shared fixture filestemplateUtils.test.jsgetTemplateName,checkValidName,filterParts,missingLiquidCode,missingNameNL, and the three exported constantsapiUtils.test.jscheckAuthorizePartnersfsUtils.test.jssfApi.test.jsaxios-mock-adapter; API response bodies loaded from fixturestoolkit.test.jsfetchXxx/newXxx/publishXxxByHandle/All/addSharedPart/removeSharedPart/getTemplateIdfunctionsliquidTestRunner.test.jsexportFileInstanceGenerator.test.js+cli/utils.test.jstests/bin/cli/tests/TESTS.mdFlagged inconsistencies (not fixed here — tracked in TESTS.md)
.test.js/.spec.jsfile naming — standardise on.test.jsjest.config.js--forceExitflag (async tests can hang CI)sfApi.jscallsapiUtils.checkRequiredEnvVariables()at module load time, complicating isolationfetchAllXxxhas no explicit depth guard at the toolkit layererrorUtils.jsimported inconsistently across test filesfs/fs.promisesusage in test setup codeshared_part_3/config.jsonhad a copy-paste bug (name referencedshared_part_2) — fixed in the fixtures committoolkit.test.jspreviously only coveredpublishXxxByIdfunctionsTest plan
npm testpasses with 485 tests, 0 failures🤖 Generated with Claude Code