test(core): add coverage tooling, extend tests, and prune low-value tests#50
Merged
test(core): add coverage tooling, extend tests, and prune low-value tests#50
Conversation
Configure v8 coverage provider in both core and cli packages with text, lcov, and json-summary reporters. Add test:coverage scripts at package and root level. Add coverage/ to .gitignore.
…s, providers, and utils Add tests for LikeC4 relationship queries and model-not-loaded guards, Bitbucket requestText/requestVoid methods, GitLab upsert/delete/close operations, config file error handling, pipeline component resolution and connection analysis, provider factory creation, and retry utility.
Remove 25 tests that provide no meaningful safety: tautological assertions, TypeScript type-system duplicates, pure mock-call verification, and exact duplicates. Remove ~46 comments that restate what the next line of code does and redundant JSDoc that merely echoes property names. Also fix pre-existing lint errors in api-client.test.ts and retry.test.ts.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Vitest coverage configuration and test scripts across root and workspaces, introduces many new unit tests, removes several older tests and inline comments, and relaxes a quick-validation check in the model patcher and adds guarded file stat handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/package.json (1)
43-49:⚠️ Potential issue | 🟠 MajorAlign
@vitest/coverage-v8and vitest versions.The
@vitest/coverage-v8@^4.0.18package has a peer dependency requirement forvitest@4.0.18, but the package.json specifiesvitest@^4.0.14. This version mismatch will cause a peer dependency warning during installation and may lead to compatibility issues at runtime.🔧 Suggested fixes
Update vitest to match the coverage plugin:
- "vitest": "^4.0.14" + "vitest": "^4.0.18"Or downgrade the coverage plugin:
- "@vitest/coverage-v8": "^4.0.18", + "@vitest/coverage-v8": "^4.0.14",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/package.json` around lines 43 - 49, Change the mismatched Vitest versions so the `@vitest/coverage-v8` peer dependency is satisfied: update the "vitest" entry in package.json from "^4.0.14" to "^4.0.18" (to match "@vitest/coverage-v8": "^4.0.18"), then reinstall dependencies and regenerate the lockfile; ensure the updated "vitest" version is reflected in CI and any related scripts referencing vitest.
🧹 Nitpick comments (8)
packages/core/src/platforms/bitbucket/__tests__/api-client.test.ts (1)
198-218: Consider adding an explicit assertion for the void success case.The test at lines 199-207 implicitly passes if no error is thrown, which is technically correct for a void method. However, making the intent explicit would improve test clarity and catch unexpected return values.
✨ Optional: Add explicit assertion
it('should resolve void on success', async () => { vi.spyOn(globalThis, 'fetch').mockResolvedValueOnce(mockFetchResponse('', true, 204)); - await client.requestVoid('/repos/org/repo/approve', { + await expect(client.requestVoid('/repos/org/repo/approve', { method: 'POST', - }); - - // requestVoid returns void on success (204) + })).resolves.toBeUndefined(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/platforms/bitbucket/__tests__/api-client.test.ts` around lines 198 - 218, The success spec for requestVoid should assert the explicit void return; update the 'should resolve void on success' test to assert that calling client.requestVoid('/repos/org/repo/approve', { method: 'POST' }) resolves to undefined (e.g. using await expect(...).resolves.toBeUndefined() or awaiting the call and asserting the result is undefined) while keeping the existing fetch mock (mockFetchResponse) and spy.packages/core/src/analysis/analysis-types.ts (1)
89-89: Consider improving grammar: "Non-critical violations".The comment "Not critical violations" is grammatically awkward. Consider changing it to "Non-critical violations" or "Warnings/non-critical issues" for better clarity.
📝 Suggested comment improvement
- /** Not critical violations */ + /** Non-critical violations */ warnings?: string[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/analysis/analysis-types.ts` at line 89, Update the inline comment in analysis-types.ts that currently reads "Not critical violations" to a clearer phrase such as "Non-critical violations" or "Warnings / non-critical issues" (the comment immediately above the relevant type definitions in analysis-types.ts). Replace only the comment text to improve grammar and clarity so future readers see the improved wording.packages/core/src/pipelines/__tests__/components.test.ts (1)
36-46: Consider adding edge case tests.The current test covers the happy path. Consider adding tests for:
- Error handling when
loadAndListComponentsrejects- Empty component list scenario
- Validation failure cases
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/pipelines/__tests__/components.test.ts` around lines 36 - 46, Add additional unit tests for runComponents to cover edge cases: (1) simulate adapter.loadAndListComponents (mockLoadAndListComponents) rejecting and assert runComponents propagates or handles the error as expected, (2) simulate it resolving to an empty array and assert runComponents returns an empty list or a specific fallback, and (3) simulate invalid component shapes (e.g., missing required fields) and assert runComponents triggers validation behavior (throws or filters) — reference the existing test harness and mocks (runComponents, mockLoadAndListComponents/adapter.loadAndListComponents) to wire the different mockResolvedValue/mockRejectedValue scenarios and corresponding expect assertions.packages/core/package.json (1)
71-78: Same version alignment consideration as in CLI package.The
@vitest/coverage-v8(^4.0.18) andvitest(^4.0.14) versions are misaligned here as well. For consistency across the monorepo and to avoid potential compatibility issues, consider aligning both packages' versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/package.json` around lines 71 - 78, The vitest-related devDependency versions are misaligned: update either "vitest" or "@vitest/coverage-v8" in packages/core package.json so both use the same compatible version (e.g., change "vitest" to "^4.0.18" to match "@vitest/coverage-v8" or change both to the target version used across the monorepo), then reinstall dependencies (or update the lockfile) to ensure the lockfile and node_modules reflect the change; look for the "vitest" and "@vitest/coverage-v8" entries in package.json to make this edit.packages/core/src/adapters/likec4/__tests__/adapter.test.ts (2)
342-382: Consider using a helper to reduce repetition in error verification tests.Each test duplicates the pattern:
expect().toThrow()followed bytry/catchto verify error properties. This is correct but verbose. A helper function could reduce duplication.♻️ Optional helper to reduce boilerplate
function expectAdapterNotLoadedError(fn: () => void) { expect(fn).toThrow(AdapterError); try { fn(); } catch (error) { expect(error).toBeInstanceOf(AdapterError); const adapterError = error as AdapterError; expect(adapterError.code).toBe(ErrorCode.MODEL_NOT_INITIALIZED); expect(adapterError.adapterType).toBe('likec4'); } } // Usage: it('should throw AdapterError when getComponentRelationships is called before load', () => { const adapter = new LikeC4Adapter(); expectAdapterNotLoadedError(() => adapter.getComponentRelationships('any')); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/adapters/likec4/__tests__/adapter.test.ts` around lines 342 - 382, Tests repeat the same error-verification pattern for LikeC4Adapter methods (getComponentRelationships, getComponentDependents, getAllRelationships); extract that logic into a reusable helper (e.g., expectAdapterNotLoadedError) that accepts a thunk, asserts it throws AdapterError, invokes it inside a try/catch to verify error is instance of AdapterError with code ErrorCode.MODEL_NOT_INITIALIZED and adapterType 'likec4', then replace the duplicated blocks with calls to this helper passing () => adapter.method(...).
1-520: File slightly exceeds 500-line guideline (520 lines).The file is 20 lines over the maximum file length guideline. Consider extracting the "Component Exclusion" tests (lines 385-520) into a separate file like
adapter-exclusion.test.tsif this grows further. For now, this is minor given the comprehensive test coverage.As per coding guidelines: "Maintain a maximum file length of 500 lines"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/adapters/likec4/__tests__/adapter.test.ts` around lines 1 - 520, The file exceeds the 500-line guideline because the large "Component Exclusion" test suite is included; extract the entire describe('Component Exclusion') block along with its helpers (setupExclusion, findId) and any test-specific imports/fixtures (TestLikeC4Adapter, createMockModel) into a separate test file, leaving the rest of the LikeC4Adapter tests intact and updating imports/exports as needed so tests run the same.packages/core/src/utils/__tests__/config-file.test.ts (1)
180-208: Redundant mock setup and assertions in each test.Each test calls
mockReadFileSyncandloadConfigFromFiletwice - once for checking the error type and again for checking the message. This is inefficient; you can combine both assertions on a single throw.♻️ Proposed simplification
it('throws ConfigurationError when file contains invalid JSON', () => { mockReadFileSync.mockReturnValue('not valid json {'); - expect(() => loadConfigFromFile('/fake/path')).toThrow(ConfigurationError); - expect(() => { - mockReadFileSync.mockReturnValue('not valid json {'); - return loadConfigFromFile('/fake/path'); - }).toThrow(/Failed to load config file/); + expect(() => loadConfigFromFile('/fake/path')).toThrow(ConfigurationError); + expect(() => loadConfigFromFile('/fake/path')).toThrow(/Failed to load config file/); }); it('throws ConfigurationError when file contains an array', () => { mockReadFileSync.mockReturnValue('[1, 2, 3]'); - expect(() => loadConfigFromFile('/fake/path')).toThrow(ConfigurationError); - expect(() => { - mockReadFileSync.mockReturnValue('[1, 2, 3]'); - return loadConfigFromFile('/fake/path'); - }).toThrow(/must contain a JSON object/); + expect(() => loadConfigFromFile('/fake/path')).toThrow(ConfigurationError); + expect(() => loadConfigFromFile('/fake/path')).toThrow(/must contain a JSON object/); }); it('throws ConfigurationError when file contains null', () => { mockReadFileSync.mockReturnValue('null'); - expect(() => loadConfigFromFile('/fake/path')).toThrow(ConfigurationError); - expect(() => { - mockReadFileSync.mockReturnValue('null'); - return loadConfigFromFile('/fake/path'); - }).toThrow(/must contain a JSON object/); + expect(() => loadConfigFromFile('/fake/path')).toThrow(ConfigurationError); + expect(() => loadConfigFromFile('/fake/path')).toThrow(/must contain a JSON object/); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/utils/__tests__/config-file.test.ts` around lines 180 - 208, Replace the duplicated mock and double expect calls in the three tests that use mockReadFileSync and loadConfigFromFile by setting mockReadFileSync once, invoking loadConfigFromFile once inside a try/catch, and asserting both the error type and message on the caught error (e.g., expect(err).toBeInstanceOf(ConfigurationError) and expect(err.message).toMatch(/.../)); update the tests that currently reference mockReadFileSync and loadConfigFromFile in the 'invalid JSON', 'array', and 'null' cases to use this single-invocation pattern.packages/core/src/providers/__tests__/provider-factory.test.ts (1)
74-180: Add one fallback-provider regression test.The suite exercises explicit provider values only. A single case for “provider unset ⇒ Gemini” would pin the factory’s default behavior and catch config-loading regressions.
Based on learnings,
provider-factory.tsshould default to gemini whenAI_PROVIDERis unset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/providers/__tests__/provider-factory.test.ts` around lines 74 - 180, Add a regression test that verifies createAIProvider falls back to Gemini when the AI provider is unset: in the existing describe('createAIProvider') block add a test where mockConfig.ai.provider is undefined (or deleted), call createAIProvider(), assert the returned value is mockGeminiInstance and that GeminiProvider was called with the expected gemini config (apiKey, fastModel, advancedModel). This pins the default behavior in provider-factory (createAIProvider) to gemini and will catch config-loading regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/platforms/gitlab/__tests__/writer.test.ts`:
- Around line 350-366: The tests for closeChangeRequest are not asserting that
the MR's source branch matches the target branch, so a bug that closes any open
MR would pass; update the positive case by having mockMrAll return an MR object
whose source_branch (or equivalent field used by closeChangeRequest) equals
'feature-branch' and keep expecting mockMrEdit('org/repo', 42, { stateEvent:
'close' }), and add a new negative case where mockMrAll returns an MR whose
source_branch is a different branch (e.g., 'other-branch') and assert mockMrEdit
is not called; modify the existing tests that call writer.closeChangeRequest and
use mockMrAll/mockMrEdit to encode these branch values so the branch-matching
behavior of closeChangeRequest is verified.
- Around line 289-325: The tests must assert that the writer preserves the
upsertMarker in the saved note body so future upserts can find it; update the
expectations in the tests around writer.commentOnChangeRequest to verify that
mockMrNotesEdit and mockMrNotesCreate are called with a body that
appends/includes the upsertMarker (e.g., 'new content <!-- erode-marker -->')
instead of just 'new content' — specifically update the assertions for
mockMrNotesEdit in the "should update existing note" case and for
mockMrNotesCreate in both "should create new note when upsertMarker not found"
and "should create new note when no notes exist" to expect the marker to be
present in the body argument.
In `@packages/core/src/providers/__tests__/provider-factory.test.ts`:
- Around line 20-28: The mocks (MockGeminiProvider, MockOpenAIProvider,
MockAnthropicProvider) currently ignore constructor input and always return
instances, so update each mock constructor to validate its incoming config like
the real provider constructors and throw/reject an AUTH_KEY_MISSING error when
the required API key is absent; this ensures createAIProvider tests exercise the
same key-validation behavior as real providers—check the constructor arg name
used by the real classes (e.g., options.apiKey or apiKey) and mirror that check
in the mock factories so tests for AUTH_KEY_MISSING fail when no key is
provided.
In `@packages/core/src/utils/__tests__/retry.test.ts`:
- Around line 66-71: The test currently uses expect(withRetry(operation,
FAST_POLICY)).rejects.toThrow(error) which checks deep equality; change the
assertion to expect(withRetry(operation, FAST_POLICY)).rejects.toBe(error) to
ensure the exact ErodeError instance (created as error) is propagated unchanged
by withRetry; update the test in retry.test.ts that references ErodeError,
FAST_POLICY, and operation to use rejects.toBe(error) and keep the rest of the
setup (operation mock and call count) the same.
---
Outside diff comments:
In `@packages/cli/package.json`:
- Around line 43-49: Change the mismatched Vitest versions so the
`@vitest/coverage-v8` peer dependency is satisfied: update the "vitest" entry in
package.json from "^4.0.14" to "^4.0.18" (to match "@vitest/coverage-v8":
"^4.0.18"), then reinstall dependencies and regenerate the lockfile; ensure the
updated "vitest" version is reflected in CI and any related scripts referencing
vitest.
---
Nitpick comments:
In `@packages/core/package.json`:
- Around line 71-78: The vitest-related devDependency versions are misaligned:
update either "vitest" or "@vitest/coverage-v8" in packages/core package.json so
both use the same compatible version (e.g., change "vitest" to "^4.0.18" to
match "@vitest/coverage-v8" or change both to the target version used across the
monorepo), then reinstall dependencies (or update the lockfile) to ensure the
lockfile and node_modules reflect the change; look for the "vitest" and
"@vitest/coverage-v8" entries in package.json to make this edit.
In `@packages/core/src/adapters/likec4/__tests__/adapter.test.ts`:
- Around line 342-382: Tests repeat the same error-verification pattern for
LikeC4Adapter methods (getComponentRelationships, getComponentDependents,
getAllRelationships); extract that logic into a reusable helper (e.g.,
expectAdapterNotLoadedError) that accepts a thunk, asserts it throws
AdapterError, invokes it inside a try/catch to verify error is instance of
AdapterError with code ErrorCode.MODEL_NOT_INITIALIZED and adapterType 'likec4',
then replace the duplicated blocks with calls to this helper passing () =>
adapter.method(...).
- Around line 1-520: The file exceeds the 500-line guideline because the large
"Component Exclusion" test suite is included; extract the entire
describe('Component Exclusion') block along with its helpers (setupExclusion,
findId) and any test-specific imports/fixtures (TestLikeC4Adapter,
createMockModel) into a separate test file, leaving the rest of the
LikeC4Adapter tests intact and updating imports/exports as needed so tests run
the same.
In `@packages/core/src/analysis/analysis-types.ts`:
- Line 89: Update the inline comment in analysis-types.ts that currently reads
"Not critical violations" to a clearer phrase such as "Non-critical violations"
or "Warnings / non-critical issues" (the comment immediately above the relevant
type definitions in analysis-types.ts). Replace only the comment text to improve
grammar and clarity so future readers see the improved wording.
In `@packages/core/src/pipelines/__tests__/components.test.ts`:
- Around line 36-46: Add additional unit tests for runComponents to cover edge
cases: (1) simulate adapter.loadAndListComponents (mockLoadAndListComponents)
rejecting and assert runComponents propagates or handles the error as expected,
(2) simulate it resolving to an empty array and assert runComponents returns an
empty list or a specific fallback, and (3) simulate invalid component shapes
(e.g., missing required fields) and assert runComponents triggers validation
behavior (throws or filters) — reference the existing test harness and mocks
(runComponents, mockLoadAndListComponents/adapter.loadAndListComponents) to wire
the different mockResolvedValue/mockRejectedValue scenarios and corresponding
expect assertions.
In `@packages/core/src/platforms/bitbucket/__tests__/api-client.test.ts`:
- Around line 198-218: The success spec for requestVoid should assert the
explicit void return; update the 'should resolve void on success' test to assert
that calling client.requestVoid('/repos/org/repo/approve', { method: 'POST' })
resolves to undefined (e.g. using await expect(...).resolves.toBeUndefined() or
awaiting the call and asserting the result is undefined) while keeping the
existing fetch mock (mockFetchResponse) and spy.
In `@packages/core/src/providers/__tests__/provider-factory.test.ts`:
- Around line 74-180: Add a regression test that verifies createAIProvider falls
back to Gemini when the AI provider is unset: in the existing
describe('createAIProvider') block add a test where mockConfig.ai.provider is
undefined (or deleted), call createAIProvider(), assert the returned value is
mockGeminiInstance and that GeminiProvider was called with the expected gemini
config (apiKey, fastModel, advancedModel). This pins the default behavior in
provider-factory (createAIProvider) to gemini and will catch config-loading
regressions.
In `@packages/core/src/utils/__tests__/config-file.test.ts`:
- Around line 180-208: Replace the duplicated mock and double expect calls in
the three tests that use mockReadFileSync and loadConfigFromFile by setting
mockReadFileSync once, invoking loadConfigFromFile once inside a try/catch, and
asserting both the error type and message on the caught error (e.g.,
expect(err).toBeInstanceOf(ConfigurationError) and
expect(err.message).toMatch(/.../)); update the tests that currently reference
mockReadFileSync and loadConfigFromFile in the 'invalid JSON', 'array', and
'null' cases to use this single-invocation pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f138954b-739e-4970-9120-6d774746f8ca
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
.gitignorepackage.jsonpackages/cli/package.jsonpackages/cli/vitest.config.tspackages/core/package.jsonpackages/core/src/adapters/base-patcher.tspackages/core/src/adapters/likec4/__tests__/adapter-url-validation.test.tspackages/core/src/adapters/likec4/__tests__/adapter.test.tspackages/core/src/adapters/likec4/__tests__/integration.test.tspackages/core/src/adapters/model-patcher.tspackages/core/src/adapters/structurizr/__tests__/integration.test.tspackages/core/src/adapters/structurizr/adapter.tspackages/core/src/adapters/structurizr/patcher.tspackages/core/src/analysis/analysis-types.tspackages/core/src/pipelines/__tests__/analyze.test.tspackages/core/src/pipelines/__tests__/components.test.tspackages/core/src/pipelines/__tests__/connections.test.tspackages/core/src/pipelines/__tests__/pr-creation.test.tspackages/core/src/platforms/__tests__/platform-factory.test.tspackages/core/src/platforms/__tests__/reader-contract.tspackages/core/src/platforms/bitbucket/__tests__/api-client.test.tspackages/core/src/platforms/bitbucket/__tests__/reader.test.tspackages/core/src/platforms/bitbucket/writer.tspackages/core/src/platforms/gitlab/__tests__/reader.test.tspackages/core/src/platforms/gitlab/__tests__/writer.test.tspackages/core/src/providers/__tests__/provider-factory.test.tspackages/core/src/providers/anthropic/__tests__/provider.test.tspackages/core/src/providers/anthropic/provider.tspackages/core/src/providers/gemini/__tests__/provider.test.tspackages/core/src/providers/openai/__tests__/provider.test.tspackages/core/src/utils/__tests__/config-file.test.tspackages/core/src/utils/__tests__/retry.test.tspackages/core/src/utils/validation.tspackages/core/vitest.config.ts
💤 Files with no reviewable changes (18)
- packages/core/src/utils/validation.ts
- packages/core/src/providers/anthropic/provider.ts
- packages/core/src/platforms/bitbucket/tests/reader.test.ts
- packages/core/src/providers/gemini/tests/provider.test.ts
- packages/core/src/platforms/tests/platform-factory.test.ts
- packages/core/src/platforms/tests/reader-contract.ts
- packages/core/src/providers/openai/tests/provider.test.ts
- packages/core/src/platforms/gitlab/tests/reader.test.ts
- packages/core/src/pipelines/tests/analyze.test.ts
- packages/core/src/platforms/bitbucket/writer.ts
- packages/core/src/adapters/structurizr/tests/integration.test.ts
- packages/core/src/pipelines/tests/pr-creation.test.ts
- packages/core/src/providers/anthropic/tests/provider.test.ts
- packages/core/src/adapters/likec4/tests/adapter-url-validation.test.ts
- packages/core/src/adapters/base-patcher.ts
- packages/core/src/adapters/structurizr/patcher.ts
- packages/core/src/adapters/structurizr/adapter.ts
- packages/core/src/adapters/model-patcher.ts
Comment on lines
+289
to
+325
| it('should update existing note via .edit when upsertMarker matches', async () => { | ||
| mockMrNotesAll.mockResolvedValue([{ id: 10, body: 'old content <!-- erode-marker -->' }]); | ||
| mockMrNotesEdit.mockResolvedValue({}); | ||
| const ref = makeRef(); | ||
|
|
||
| await writer.commentOnChangeRequest(ref, 'new content', { | ||
| upsertMarker: '<!-- erode-marker -->', | ||
| }); | ||
|
|
||
| expect(mockMrNotesEdit).toHaveBeenCalledWith('org/repo', 42, 10, { body: 'new content' }); | ||
| expect(mockMrNotesCreate).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should create new note when upsertMarker not found', async () => { | ||
| mockMrNotesAll.mockResolvedValue([{ id: 10, body: 'unrelated note' }]); | ||
| mockMrNotesCreate.mockResolvedValue({}); | ||
| const ref = makeRef(); | ||
|
|
||
| await writer.commentOnChangeRequest(ref, 'new content', { | ||
| upsertMarker: '<!-- erode-marker -->', | ||
| }); | ||
|
|
||
| expect(mockMrNotesCreate).toHaveBeenCalledWith('org/repo', 42, 'new content'); | ||
| expect(mockMrNotesEdit).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should create new note when no notes exist', async () => { | ||
| mockMrNotesAll.mockResolvedValue([]); | ||
| mockMrNotesCreate.mockResolvedValue({}); | ||
| const ref = makeRef(); | ||
|
|
||
| await writer.commentOnChangeRequest(ref, 'new content', { | ||
| upsertMarker: '<!-- erode-marker -->', | ||
| }); | ||
|
|
||
| expect(mockMrNotesCreate).toHaveBeenCalledWith('org/repo', 42, 'new content'); | ||
| }); |
There was a problem hiding this comment.
Assert that the persisted note still contains the upsert marker.
These expectations currently pass even if the writer strips upsertMarker before saving the note. That breaks the next upsert/delete cycle because the note can no longer be found by marker.
💡 Suggested assertion update
- expect(mockMrNotesEdit).toHaveBeenCalledWith('org/repo', 42, 10, { body: 'new content' });
+ expect(mockMrNotesEdit).toHaveBeenCalledWith(
+ 'org/repo',
+ 42,
+ 10,
+ expect.objectContaining({
+ body: expect.stringContaining('<!-- erode-marker -->'),
+ })
+ );
- expect(mockMrNotesCreate).toHaveBeenCalledWith('org/repo', 42, 'new content');
+ expect(mockMrNotesCreate).toHaveBeenCalledWith(
+ 'org/repo',
+ 42,
+ expect.stringContaining('<!-- erode-marker -->')
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/platforms/gitlab/__tests__/writer.test.ts` around lines 289
- 325, The tests must assert that the writer preserves the upsertMarker in the
saved note body so future upserts can find it; update the expectations in the
tests around writer.commentOnChangeRequest to verify that mockMrNotesEdit and
mockMrNotesCreate are called with a body that appends/includes the upsertMarker
(e.g., 'new content <!-- erode-marker -->') instead of just 'new content' —
specifically update the assertions for mockMrNotesEdit in the "should update
existing note" case and for mockMrNotesCreate in both "should create new note
when upsertMarker not found" and "should create new note when no notes exist" to
expect the marker to be present in the body argument.
Comment on lines
+350
to
+366
| it('should close open MR via .edit with stateEvent close', async () => { | ||
| mockMrAll.mockResolvedValue([ | ||
| { iid: 42, web_url: 'https://gitlab.com/org/repo/-/merge_requests/42' }, | ||
| ]); | ||
| mockMrEdit.mockResolvedValue({}); | ||
|
|
||
| await writer.closeChangeRequest('feature-branch'); | ||
|
|
||
| expect(mockMrEdit).toHaveBeenCalledWith('org/repo', 42, { stateEvent: 'close' }); | ||
| }); | ||
|
|
||
| it('should no-op when no open MR exists', async () => { | ||
| mockMrAll.mockResolvedValue([]); | ||
|
|
||
| await writer.closeChangeRequest('feature-branch'); | ||
|
|
||
| expect(mockMrEdit).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Make the close-MR cases prove branch matching.
Right now these fixtures don't encode the branch that closeChangeRequest() is supposed to target, so a buggy implementation that closes any open MR would still pass. Please include the matching branch in the positive case and add a negative case where only a different branch is open.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/platforms/gitlab/__tests__/writer.test.ts` around lines 350
- 366, The tests for closeChangeRequest are not asserting that the MR's source
branch matches the target branch, so a bug that closes any open MR would pass;
update the positive case by having mockMrAll return an MR object whose
source_branch (or equivalent field used by closeChangeRequest) equals
'feature-branch' and keep expecting mockMrEdit('org/repo', 42, { stateEvent:
'close' }), and add a new negative case where mockMrAll returns an MR whose
source_branch is a different branch (e.g., 'other-branch') and assert mockMrEdit
is not called; modify the existing tests that call writer.closeChangeRequest and
use mockMrAll/mockMrEdit to encode these branch values so the branch-matching
behavior of closeChangeRequest is verified.
Comment on lines
+20
to
+28
| MockGeminiProvider: vi.fn(function () { | ||
| return geminiInstance; | ||
| }), | ||
| MockOpenAIProvider: vi.fn(function () { | ||
| return openaiInstance; | ||
| }), | ||
| MockAnthropicProvider: vi.fn(function () { | ||
| return anthropicInstance; | ||
| }), |
There was a problem hiding this comment.
Make the mocked constructors reject missing API keys.
These doubles ignore their input and always return an instance, so the AUTH_KEY_MISSING cases below only pass if createAIProvider() duplicates validation that already exists in the real provider constructors. That makes the factory tests brittle to an otherwise safe refactor.
🧪 Suggested fix
- MockGeminiProvider: vi.fn(function () {
+ MockGeminiProvider: vi.fn(function (config: { apiKey?: string }) {
+ if (!config.apiKey) {
+ throw new ErodeError(
+ 'A Gemini API key is needed',
+ ErrorCode.AUTH_KEY_MISSING,
+ 'Missing Gemini API key'
+ );
+ }
return geminiInstance;
}),
- MockOpenAIProvider: vi.fn(function () {
+ MockOpenAIProvider: vi.fn(function (config: { apiKey?: string }) {
+ if (!config.apiKey) {
+ throw new ErodeError(
+ 'An OpenAI API key is needed',
+ ErrorCode.AUTH_KEY_MISSING,
+ 'Missing OpenAI API key'
+ );
+ }
return openaiInstance;
}),
- MockAnthropicProvider: vi.fn(function () {
+ MockAnthropicProvider: vi.fn(function (config: { apiKey?: string }) {
+ if (!config.apiKey) {
+ throw new ErodeError(
+ 'An Anthropic API key is needed',
+ ErrorCode.AUTH_KEY_MISSING,
+ 'Missing Anthropic API key'
+ );
+ }
return anthropicInstance;
}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/providers/__tests__/provider-factory.test.ts` around lines
20 - 28, The mocks (MockGeminiProvider, MockOpenAIProvider,
MockAnthropicProvider) currently ignore constructor input and always return
instances, so update each mock constructor to validate its incoming config like
the real provider constructors and throw/reject an AUTH_KEY_MISSING error when
the required API key is absent; this ensures createAIProvider tests exercise the
same key-validation behavior as real providers—check the constructor arg name
used by the real classes (e.g., options.apiKey or apiKey) and mirror that check
in the mock factories so tests for AUTH_KEY_MISSING fail when no key is
provided.
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
1 participant
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
@vitest/coverage-v8to bothcoreandclipackages with coverage configuration invitest.config.tsWhat changed
Coverage tooling (
ecd304f)@vitest/coverage-v8as a dev dependencytext,lcov), and exclusion patterns in bothpackages/core/vitest.config.tsandpackages/cli/vitest.config.tscoverage/to.gitignoreNew and extended tests (
e61c491)providers/__tests__/provider-factory.test.ts(new) — covers factory creation for all three providers, default provider, unknown provider error, and missing API key errorspipelines/__tests__/components.test.ts(new) — covers thelistComponentspipelinepipelines/__tests__/connections.test.ts(new) — covers thelistConnectionspipelineutils/__tests__/retry.test.ts(new) — coverswithRetrysuccess, retry on transient errors, non-retryable errors, retry exhaustion, and verbose loggingadapters/likec4/__tests__/adapter.test.ts,platforms/bitbucket/__tests__/api-client.test.ts,platforms/gitlab/__tests__/writer.test.ts,utils/__tests__/config-file.test.tsPruned low-value tests (
d16253d)Removed 25 tests across 16 files that provided no meaningful safety:
expect(new Provider()).toBeDefined(),expect(elapsed > 0).toBe(true)typeof reader.method === 'function',typeof ref.number === 'number'adapter-url-validation.test.ts(duplicate ofadapter.test.ts), consistency tests duplicated across contract and implementation filesexpect(ENV_VAR_NAMES[key]).toBe(value)where key/value come from the same objectPruned redundant comments (
d16253d)Removed ~46 comments across 8 source files:
// Check if branch existsbeforelet branchExists = false// --- Private helpers ---(theprivatekeyword communicates this)/** The severity */on a property namedseverityDocs update (
ccaf5b2)Test plan
npm testpasses (812 tests across 47 files)npm run lintcleannpm run format:checkcleanSummary by CodeRabbit
New Features
Tests
Bug Fixes
Chores