Comprehensive testing overhaul: fixtures, unit tests, E2E CLI tests#254
Merged
Conversation
|
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)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/lib/templates/sharedParts.test.js (1)
120-128:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMinor:
SharedPart.read(name)isasyncand should be awaited to avoid timing flakiness in this test.
SharedPart.readis declared asstatic async read(name); in this test it’s called withoutawait, while assertions immediately check for file existence and contents. Awaiting the call ensures the file write completes before assertions.
[tests/lib/templates/sharedParts.test.js (around lines 120-128)]🔧 Proposed fix
- SharedPart.read(name); + await SharedPart.read(name);🤖 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/templates/sharedParts.test.js` around lines 120 - 128, The test calls the async static method SharedPart.read(name) without awaiting it, causing race conditions; update the test to await SharedPart.read(name) so the file creation/writes complete before asserting on fs.existsSync(mainLiquidPath) and reading mainLiquidPath; locate the call to SharedPart.read in the test and add an await before it.
🧹 Nitpick comments (2)
.gitignore (1)
4-5: 💤 Low valueConsider using consistent notation for tmp patterns.
Line 4 uses
./tmp(explicit relative path) while line 5 usestmp-*(matches anywhere in tree). If both should only match at the repository root, use/tmpand/tmp-*instead. Iftmp-*should match anywhere, consider changing./tmpto justtmpfor consistency.📝 Suggested fix for root-only matching
-./tmp -tmp-* +/tmp +/tmp-*🤖 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 @.gitignore around lines 4 - 5, The .gitignore entries are inconsistent: './tmp' (explicit relative) vs 'tmp-*' (matches anywhere). Update the patterns for consistency — preferably make both root-only by replacing './tmp' and 'tmp-*' with '/tmp' and '/tmp-*' respectively; alternatively, if you want them to match anywhere, change './tmp' to 'tmp'. Modify the entries that reference './tmp' and 'tmp-*' accordingly so both patterns use the same root-scoping behavior.tests/lib/exportFileInstanceGenerator.test.js (1)
61-140: ⚡ Quick winGuarantee fake-timer cleanup via
afterEach.The polling test enables fake timers inline and only restores them with
jest.useRealTimers()at the end of theit. If an assertion throws earlier, fake timers leak into the later async tests (failed/no-content_urlcases), which awaitsetTimeout-driven polling and can hang or contaminate the suite. Resetting inafterEachmakes cleanup unconditional.♻️ Proposed cleanup
describe("generateAndOpenFile", () => { + afterEach(() => { + jest.useRealTimers(); + }); + it("should log error and return false when createExportFileInstance returns no id", async () => {🤖 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 61 - 140, Add an unconditional fake-timer cleanup in this test file by adding an afterEach that calls jest.useRealTimers() and jest.clearAllTimers() so any tests that enable fake timers (e.g., the "should retry while state is pending..." case that calls jest.useFakeTimers()) are always restored even if assertions throw; update tests/lib/exportFileInstanceGenerator.test.js to include this afterEach alongside existing setup/teardown so SF.createExportFileInstance, SF.getExportFileInstance, and timer-driven polling in ExportFileInstanceGenerator.generateAndOpenFile are not affected by leaked fake timers.
🤖 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.
Outside diff comments:
In `@tests/lib/templates/sharedParts.test.js`:
- Around line 120-128: The test calls the async static method
SharedPart.read(name) without awaiting it, causing race conditions; update the
test to await SharedPart.read(name) so the file creation/writes complete before
asserting on fs.existsSync(mainLiquidPath) and reading mainLiquidPath; locate
the call to SharedPart.read in the test and add an await before it.
---
Nitpick comments:
In @.gitignore:
- Around line 4-5: The .gitignore entries are inconsistent: './tmp' (explicit
relative) vs 'tmp-*' (matches anywhere). Update the patterns for consistency —
preferably make both root-only by replacing './tmp' and 'tmp-*' with '/tmp' and
'/tmp-*' respectively; alternatively, if you want them to match anywhere, change
'./tmp' to 'tmp'. Modify the entries that reference './tmp' and 'tmp-*'
accordingly so both patterns use the same root-scoping behavior.
In `@tests/lib/exportFileInstanceGenerator.test.js`:
- Around line 61-140: Add an unconditional fake-timer cleanup in this test file
by adding an afterEach that calls jest.useRealTimers() and jest.clearAllTimers()
so any tests that enable fake timers (e.g., the "should retry while state is
pending..." case that calls jest.useFakeTimers()) are always restored even if
assertions throw; update tests/lib/exportFileInstanceGenerator.test.js to
include this afterEach alongside existing setup/teardown so
SF.createExportFileInstance, SF.getExportFileInstance, and timer-driven polling
in ExportFileInstanceGenerator.generateAndOpenFile are not affected by leaked
fake timers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2b9a62d-e039-40e7-b903-0401886bafd1
📒 Files selected for processing (77)
.gitignoreREADME.mdfixtures/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.jsonjest.config.jstests/TESTS.mdtests/bin/cli.test.jstests/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-reconciliation.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/changelogReader.test.jstests/lib/cli/cliUpdater.test.jstests/lib/cli/cwdValidator.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
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/bin/cli/create-account-template.test.js (1)
18-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore the exact pre-test working directory.
This issue was flagged in a previous review and remains unaddressed. Line 36 resets
cwdto a repo-relative path, not the pre-testcwd, which can leak process state when tests are invoked from a different start directory.💡 Suggested fix
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 - 39, The afterEach resets cwd to a repo-relative path instead of the original working directory; capture and save the initial cwd in beforeEach (e.g., const originalCwd = process.cwd() or reuse a variable like originalExit alongside originalCwd) and then restore it in afterEach via process.chdir(originalCwd); ensure the saved originalCwd is declared in the test scope so beforeEach assigns it and afterEach uses it, and keep restoring process.exit and cleaning tempDir as-is.
🧹 Nitpick comments (3)
tests/bin/cli/create-shared-part.test.js (1)
1-2: 💤 Low valueConsolidate fs imports.
Both
fsandfs.promisesare imported separately. You can simplify this by importing justfsand accessingfs.promiseswhen needed, or import onlyfs.promisesif synchronous methods aren't required.♻️ Proposed consolidation
-const fs = require("fs"); -const fsPromises = require("fs").promises; +const fs = require("fs"); +const fsPromises = fs.promises;Or if only async methods are used after line 58:
-const fs = require("fs"); -const fsPromises = require("fs").promises; +const { promises: fsPromises } = require("fs"); +const fs = require("fs"); // Only if needed for readFileSync on line 58🤖 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-shared-part.test.js` around lines 1 - 2, Consolidate the duplicate fs imports by keeping a single require and deriving promises from it: remove the separate const fsPromises = require("fs").promises line and instead use the existing const fs = require("fs") and access fs.promises where needed (or assign const fsPromises = fs.promises immediately after the single require); update any uses of fsPromises in create-shared-part.test.js to reference the consolidated symbol.tests/bin/cli/create-export-file.test.js (1)
18-39: ⚖️ Poor tradeoffOptional: extract the shared CLI test harness.
This
beforeEach/afterEachblock (tempDir viamkdtemp,process.chdir,process.exitstub, consola mocks,rmcleanup) is duplicated verbatim across thetests/bin/cli/*suites. A shared helper (e.g.setupCliTestEnv()) would reduce drift and keep cleanup consistent. Deferable, not blocking.🤖 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-export-file.test.js` around lines 18 - 39, Extract the duplicated beforeEach/afterEach test harness into a shared helper (e.g. setupCliTestEnv) and replace the verbatim blocks in tests/bin/cli/* with calls to that helper; the helper should create tempDir via fsPromises.mkdtemp, process.chdir to the tempDir, stub process.exit (restoring originalExit on cleanup), mock consola methods (success, error, info, log, warn), and perform cleanup with fsPromises.rm and restoring process.cwd and process.exit — update each test file to call setupCliTestEnv() (or teardown counterpart if needed) instead of repeating the beforeEach/afterEach logic so tempDir, mkdtemp, process.chdir, process.exit stub, consola mocks, and rm cleanup are centrally maintained.tests/bin/cli/get-account-template-id.test.js (1)
54-56: 💤 Low valueConsider using
fsPromises.readFilefor consistency.The test body uses
async/awaitwithfsPromisesthroughout, but line 55 uses synchronousfs.readFileSync. For consistency and to avoid mixing sync/async file operations, useawait fsPromises.readFile(configPath, "utf8")instead.♻️ Proposed consistency fix
- const configPath = path.join(tempDir, "account_templates", "account_1", "config.json"); - const config = JSON.parse(fs.readFileSync(configPath, "utf8")); - expect(config.id["2000"]).toBe(99007); + const configPath = path.join(tempDir, "account_templates", "account_1", "config.json"); + const config = JSON.parse(await fsPromises.readFile(configPath, "utf8")); + expect(config.id["2000"]).toBe(99007);🤖 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/get-account-template-id.test.js` around lines 54 - 56, Replace the synchronous read of the config file with the async fsPromises API to avoid mixing sync/async IO: change the call that assigns config using fs.readFileSync(configPath, "utf8") to await fsPromises.readFile(configPath, "utf8") and then JSON.parse that result so the test remains async; update the reference to configPath and ensure the test function is async/await-compatible and imports or references fsPromises where other tests do.
🤖 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-shared-part.test.js`:
- Around line 18-39: The tests mutate global state via process.chdir in
beforeEach/afterEach which can cause race conditions; instead, stop changing the
process working directory and either (A) update the tests to pass the created
tempDir into the code under test (replace reliance on process.cwd() by injecting
the directory where appropriate in the tested functions), or (B) mock
process.cwd() within beforeEach (e.g., jest.spyOn(process,
'cwd').mockReturnValue(tempDir)) and restore it in afterEach, keeping tempDir
from fsPromises.mkdtemp and preserving originalExit handling; update references
to process.chdir and any code that reads process.cwd() accordingly (look for
beforeEach, afterEach, process.chdir, process.cwd, tempDir).
---
Duplicate comments:
In `@tests/bin/cli/create-account-template.test.js`:
- Around line 18-39: The afterEach resets cwd to a repo-relative path instead of
the original working directory; capture and save the initial cwd in beforeEach
(e.g., const originalCwd = process.cwd() or reuse a variable like originalExit
alongside originalCwd) and then restore it in afterEach via
process.chdir(originalCwd); ensure the saved originalCwd is declared in the test
scope so beforeEach assigns it and afterEach uses it, and keep restoring
process.exit and cleaning tempDir as-is.
---
Nitpick comments:
In `@tests/bin/cli/create-export-file.test.js`:
- Around line 18-39: Extract the duplicated beforeEach/afterEach test harness
into a shared helper (e.g. setupCliTestEnv) and replace the verbatim blocks in
tests/bin/cli/* with calls to that helper; the helper should create tempDir via
fsPromises.mkdtemp, process.chdir to the tempDir, stub process.exit (restoring
originalExit on cleanup), mock consola methods (success, error, info, log,
warn), and perform cleanup with fsPromises.rm and restoring process.cwd and
process.exit — update each test file to call setupCliTestEnv() (or teardown
counterpart if needed) instead of repeating the beforeEach/afterEach logic so
tempDir, mkdtemp, process.chdir, process.exit stub, consola mocks, and rm
cleanup are centrally maintained.
In `@tests/bin/cli/create-shared-part.test.js`:
- Around line 1-2: Consolidate the duplicate fs imports by keeping a single
require and deriving promises from it: remove the separate const fsPromises =
require("fs").promises line and instead use the existing const fs =
require("fs") and access fs.promises where needed (or assign const fsPromises =
fs.promises immediately after the single require); update any uses of fsPromises
in create-shared-part.test.js to reference the consolidated symbol.
In `@tests/bin/cli/get-account-template-id.test.js`:
- Around line 54-56: Replace the synchronous read of the config file with the
async fsPromises API to avoid mixing sync/async IO: change the call that assigns
config using fs.readFileSync(configPath, "utf8") to await
fsPromises.readFile(configPath, "utf8") and then JSON.parse that result so the
test remains async; update the reference to configPath and ensure the test
function is async/await-compatible and imports or references fsPromises where
other tests do.
🪄 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: 31305875-393a-46d3-95c3-c3b85a2fe1e1
📒 Files selected for processing (77)
.gitignoreREADME.mdfixtures/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.jsonjest.config.jstests/TESTS.mdtests/bin/cli.test.jstests/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-reconciliation.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/changelogReader.test.jstests/lib/cli/cliUpdater.test.jstests/lib/cli/cwdValidator.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
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/lib/utils/checkLiquidTestDependencies.test.js (1)
409-448:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate test case.
This
it("should not include the target handle itself (self-reference)")block is an exact copy of the one at Lines 368-407 (same title and same body). It adds no coverage and one will silently shadow intent. Either remove it or repurpose it for the distinct scenario it was likely meant to cover (e.g., target referencing itself across multiple test cases, or via array/key forms).Want me to remove the duplicate, or draft a distinct self-reference scenario to replace it?
🤖 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/utils/checkLiquidTestDependencies.test.js` around lines 409 - 448, The test block titled "should not include the target handle itself (self-reference)" in tests/lib/utils/checkLiquidTestDependencies.test.js is a duplicate of an earlier block and should be removed or replaced; either delete this redundant it() block, or repurpose it to cover a different self-reference scenario for the function checkLiquidTestDependencies (for example: a template referencing itself across multiple test_case entries, or self-reference expressed as an array/key form), and adjust the test setup and expectations to assert that the returned array contains only external dependents (e.g., expect(result).toContain(dependentHandle) and expect(result).not.toContain(targetHandle)) accordingly.
🤖 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.
Outside diff comments:
In `@tests/lib/utils/checkLiquidTestDependencies.test.js`:
- Around line 409-448: The test block titled "should not include the target
handle itself (self-reference)" in
tests/lib/utils/checkLiquidTestDependencies.test.js is a duplicate of an earlier
block and should be removed or replaced; either delete this redundant it()
block, or repurpose it to cover a different self-reference scenario for the
function checkLiquidTestDependencies (for example: a template referencing itself
across multiple test_case entries, or self-reference expressed as an array/key
form), and adjust the test setup and expectations to assert that the returned
array contains only external dependents (e.g.,
expect(result).toContain(dependentHandle) and
expect(result).not.toContain(targetHandle)) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 404ae661-9662-4cc8-b94f-db7a47989474
📒 Files selected for processing (7)
tests/lib/templates/accountTemplates.test.jstests/lib/templates/exportFiles.test.jstests/lib/templates/reconciliationTexts.test.jstests/lib/templates/sharedParts.test.jstests/lib/utils/checkLiquidTestDependencies.test.jstests/lib/utils/findTemplatesWithLiquidTests.test.jstests/lib/utils/fsUtils.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/lib/templates/reconciliationTexts.test.js
- tests/lib/templates/accountTemplates.test.js
- tests/lib/templates/exportFiles.test.js
- tests/lib/utils/fsUtils.test.js
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>
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>
- 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>
- Move jest.mock('axiosFactory') from beforeAll to module scope in
sfApi.test.js so Jest's hoisting works correctly; require SF and
AxiosFactory at module level alongside the other mocks
- Fix import-reconciliation.test.js: use os.tmpdir() for temp dirs
(consistent with all other E2E tests) and remove the process.exit(1)
call in the afterEach cleanup catch block (which could mask real
test failures)
- Add tmp-* to .gitignore so crash-leftover temp dirs under the repo
root (used by template class and fsUtils tests) are never committed
- Fix account-templates/single.json description: remove the incorrect
"mapping list ranges" mention (mapping_list_ranges is [] in this
fixture; that field belongs in single-with-mapping-list-ranges.json)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Coverage: - Add tests for addSharedPart, removeSharedPart, addAllSharedParts (index.js ~848–1080) including ID resolution fallback, per-templateType dispatch, used_in config updates, and error paths - Add bulk-helper tests for all three non-reconciliation types: fetchExisting/publishAll/newAll for exportFiles, accountTemplates, and sharedParts — matching the existing reconciliation coverage - Add sfApi tests for 13 previously untested endpoints: shared-part link/unlink helpers (all 3 template types × add/remove), createTestRun, readTestRun, verifyLiquid, getFirmDetails, createExportFileInstance, getExportFileInstance, getPeriods Test quality: - Fix liquidTestRunner.test.js: apply jest.useFakeTimers() for polling tests — suite time drops from ~9s to <5s - Fix 4 misleading test names in toolkit.test.js: "should return false when template reading fails" → "should return undefined when template reading returns null" (matches actual return value) - Replace inline mockApiResponse objects in import-shared-part, import-export-file, import-account-template E2E tests with require() calls into fixtures/api-responses/ Consistency: - Rename changelogReader/cliUpdater/cwdValidator .spec.js → .test.js - Add testTimeout: 10000 to jest.config.js New test file: - tests/bin/cli.test.js: Commander wiring tests — verifies --help output contains expected command names and option flags Docs: - README.md: add Contributing section linking to tests/TESTS.md - TESTS.md: update structure tree, add naming note for tests/bin/cli/, add catalogue entries for new and renamed files 540 tests pass across 40 suites, 0 lint errors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- shared-parts/list.json: add second used_in entry (reconciliation_text_2) to match single.json, which lists both reconciliation_text_1 and reconciliation_text_2 as templates that include shared_part_1 - liquidTestRunner.test.js: rename "should log info and return false when YAML file is empty" to "return undefined" — the assertion is toBeUndefined() which was already correct; the title is now aligned Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- sharedParts.test.js: await SharedPart.read(name) so the async file creation completes before asserting on fs.existsSync / readFile - .gitignore: normalize tmp patterns to root-only (/tmp, /tmp-*) so both entries use the same scoping behaviour as each other - exportFileInstanceGenerator.test.js: add afterEach that calls jest.useRealTimers() + jest.clearAllTimers() in the generateAndOpenFile describe block so fake timers are always cleaned up even when an assertion throws mid-test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add forceExit: true to jest.config.js to prevent async tests hanging CI
- Fix import-reconciliation.test.js: move error handling describe out of
"from partners" (it tests firm env), move "import by handle" describe
out of "import by id" (it is a sibling, not a child), convert
conditional fixture rename guards to unconditional expect+rename,
strengthen weak consola.error assertion to toHaveBeenCalledWith, and
remove if-guard around handle-import file assertions
- Fix toolkit.test.js: replace require("consola").error re-requires
inside assertions with the already-imported consola reference
- Add Known inconsistencies section to tests/TESTS.md with 7 tracked
items including the new consola import style inconsistency
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use os.tmpdir() instead of the project root so abandoned tmp directories (e.g. from a killed test run) don't accumulate inside the repo. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
michieldegezelle
force-pushed
the
testing-overhaul
branch
from
397b268 to
483cc5f
Compare
All afterEach blocks were restoring process.cwd() to a hard-coded path.resolve(__dirname, "../../..") instead of the actual pre-test working directory. Capture process.cwd() at the start of each beforeEach and restore that value in afterEach, so the tests are safe to run from any invoking directory. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Benjvandam
approved these changes
Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Flagged inconsistencies (not fixed here — tracked in TESTS.md)