@coderabbitai Generated Unit Tests: Add unit tests#21
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
andrmaz
changed the title
andrmaz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/api/src/app.module.spec.ts (1)
14-20: 💤 Low valueConsider removing redundant compilation test.
This test is redundant with lines 6-12, which already verify that module compilation succeeds without throwing. The pattern
.resolves.not.toThrow()adds no additional coverage beyond confirming the module compiles and is defined.This is acceptable for auto-generated test suites but could be consolidated during manual maintenance.
🤖 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 `@apps/api/src/app.module.spec.ts` around lines 14 - 20, Remove the redundant test that asserts module compilation with `.resolves.not.toThrow()` by deleting the `it("should not throw when creating the application module", async () => { ... })` block (the Test.createTestingModule({...}).compile() call referencing AppModule); keep the existing test that already verifies the module compiles and is defined (lines with Test.createTestingModule(...).compile() and expect(app).toBeDefined()) so coverage remains unchanged.apps/api/src/mcp/mcp.module.spec.ts (1)
24-30: 💤 Low valueConsider removing redundant compilation test.
This test verifies that module compilation doesn't throw, but lines 6-12 already perform this check (if
compile()threw, the first test would fail). The duplicate check adds minimal value.For auto-generated test suites, this level of redundancy is acceptable, but consider consolidating if maintaining these tests manually.
🤖 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 `@apps/api/src/mcp/mcp.module.spec.ts` around lines 24 - 30, Remove the redundant test that asserts Test.createTestingModule({ imports: [MCPModule] }).compile() resolves without throwing (the "should not throw when creating the module" it block); consolidate by keeping the existing compilation/assertion in the earlier test and delete this duplicate it block referencing MCPModule and Test.createTestingModule to avoid redundant checks.apps/api/src/mcp/mcp.controller.spec.ts (2)
153-177: 💤 Low valueConsider removing redundant consistency tests.
These tests verify that
contextandpolicyremain constant across different queries:
- Lines 153-164: Context consistency check
- Lines 166-177: Policy consistency check
However, earlier tests (lines 27-34 and 42-48) already verify the exact values returned, which implicitly guarantees consistency. These additional checks add minimal value unless the implementation might introduce non-determinism.
Acceptable for auto-generated tests, but consider consolidation during manual maintenance.
🤖 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 `@apps/api/src/mcp/mcp.controller.spec.ts` around lines 153 - 177, Remove or consolidate the two redundant consistency tests that call controller.handleMCP ("should return consistent context regardless of query" and "should return consistent policy regardless of query") since the earlier assertions already check exact returned values; update the spec by deleting these two it blocks (or merge their intent into a single test if you prefer a compact check) and ensure remaining tests still exercise controller.handleMCP with validRequest and assert the expected context and policy.
57-76: 💤 Low valueConsider consolidating overlapping answer format tests.
These three tests verify the same behavior with slight variations:
- Lines 57-63: Exact match against expected format
- Lines 65-69: Regex check for prefix (redundant if exact match passes)
- Lines 71-76: Template literal check (also redundant if exact match passes)
The exact match test (lines 57-63) already validates both the prefix and query inclusion, making the additional tests redundant.
For auto-generated test suites, this level of thoroughness is acceptable. However, if maintaining manually, consider keeping only the exact match test and the edge case tests (empty string, special characters, etc.).
🤖 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 `@apps/api/src/mcp/mcp.controller.spec.ts` around lines 57 - 76, The three tests around answer formatting are redundant; consolidate by keeping a single authoritative test that asserts the exact expected string returned from controller.handleMCP (e.g., the existing "should include the query in the answer using the expected format") and remove the overlapping tests ("should prefix the answer..." and "should reflect the query verbatim..."); update or add focused edge-case tests for controller.handleMCP such as an empty query and queries with special characters to ensure coverage without duplication.
🤖 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 `@apps/api/jest.config.js`:
- Line 26: The Jest moduleNameMapper entry for "^`@cortex/shared`$" is pointing to
"<rootDir>/../../packages/shared/src/index.ts" but with rootDir set to "src"
that resolves one level too shallow; update the mapping in jest.config.js to
point to the package from the src root (e.g. change
"<rootDir>/../../packages/shared/src/index.ts" to
"<rootDir>/../../../packages/shared/src/index.ts") so the "^`@cortex/shared`$"
alias correctly resolves to the shared package entrypoint.
In `@packages/shared/package.json`:
- Around line 5-10: The package.json for `@cortex/shared` points
main/types/exports["."] at ./src/index.ts but there is no build pipeline or
tsconfig and no compiled artifacts; either add a proper build by creating a
tsconfig.json and build scripts (e.g., add "build" to package.json that compiles
src -> dist and emit types) and change "main", "types", and exports["."] to
point to dist/index.js and dist/index.d.ts, or, if the package is internal-only
and not published, ensure ./src/index.ts actually exists and is the intended
public surface (and remove/adjust missing build fields); update package.json
accordingly so main/types/exports refer to existing files and add the
build/prepack/prepare scripts if you choose the compiled dist route.
---
Nitpick comments:
In `@apps/api/src/app.module.spec.ts`:
- Around line 14-20: Remove the redundant test that asserts module compilation
with `.resolves.not.toThrow()` by deleting the `it("should not throw when
creating the application module", async () => { ... })` block (the
Test.createTestingModule({...}).compile() call referencing AppModule); keep the
existing test that already verifies the module compiles and is defined (lines
with Test.createTestingModule(...).compile() and expect(app).toBeDefined()) so
coverage remains unchanged.
In `@apps/api/src/mcp/mcp.controller.spec.ts`:
- Around line 153-177: Remove or consolidate the two redundant consistency tests
that call controller.handleMCP ("should return consistent context regardless of
query" and "should return consistent policy regardless of query") since the
earlier assertions already check exact returned values; update the spec by
deleting these two it blocks (or merge their intent into a single test if you
prefer a compact check) and ensure remaining tests still exercise
controller.handleMCP with validRequest and assert the expected context and
policy.
- Around line 57-76: The three tests around answer formatting are redundant;
consolidate by keeping a single authoritative test that asserts the exact
expected string returned from controller.handleMCP (e.g., the existing "should
include the query in the answer using the expected format") and remove the
overlapping tests ("should prefix the answer..." and "should reflect the query
verbatim..."); update or add focused edge-case tests for controller.handleMCP
such as an empty query and queries with special characters to ensure coverage
without duplication.
In `@apps/api/src/mcp/mcp.module.spec.ts`:
- Around line 24-30: Remove the redundant test that asserts
Test.createTestingModule({ imports: [MCPModule] }).compile() resolves without
throwing (the "should not throw when creating the module" it block); consolidate
by keeping the existing compilation/assertion in the earlier test and delete
this duplicate it block referencing MCPModule and Test.createTestingModule to
avoid redundant checks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3f01d40-1e67-435b-b1de-637bbee91838
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/api/jest.config.jsapps/api/package.jsonapps/api/src/app.module.spec.tsapps/api/src/mcp/mcp.controller.spec.tsapps/api/src/mcp/mcp.module.spec.tspackages/shared/jest.config.jspackages/shared/package.jsonpackages/shared/src/index.spec.tspackages/shared/src/types.spec.ts
| coverageDirectory: "../coverage", | ||
| testEnvironment: "node", | ||
| moduleNameMapper: { | ||
| "^@cortex/shared$": "<rootDir>/../../packages/shared/src/index.ts", |
There was a problem hiding this comment.
Fix @cortex/shared alias path; current relative target is off by one directory.
With rootDir: "src", ../../packages/... resolves under apps/, so Jest won’t resolve the shared package entrypoint.
Proposed fix
moduleNameMapper: {
- "^`@cortex/shared`$": "<rootDir>/../../packages/shared/src/index.ts",
+ "^`@cortex/shared`$": "<rootDir>/../../../packages/shared/src/index.ts",
},🤖 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 `@apps/api/jest.config.js` at line 26, The Jest moduleNameMapper entry for
"^`@cortex/shared`$" is pointing to "<rootDir>/../../packages/shared/src/index.ts"
but with rootDir set to "src" that resolves one level too shallow; update the
mapping in jest.config.js to point to the package from the src root (e.g. change
"<rootDir>/../../packages/shared/src/index.ts" to
"<rootDir>/../../../packages/shared/src/index.ts") so the "^`@cortex/shared`$"
alias correctly resolves to the shared package entrypoint.
| "main": "./src/index.ts", | ||
| "types": "./src/index.ts", | ||
| "exports": { | ||
| ".": { | ||
| "types": "./src/index.ts", | ||
| "default": "./src/index.ts" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for TypeScript build config and package build setup
echo "=== Checking for tsconfig.json ==="
fd -t f '^tsconfig\.json$' packages/shared
echo -e "\n=== Checking tsconfig references to shared package ==="
fd -t f 'tsconfig.*\.json$' | xargs rg -l '"`@cortex/shared`"' 2>/dev/null || echo "No references found"
echo -e "\n=== Checking for build scripts or dist output ==="
rg -n '"build":|"prepare":|"prepack":' packages/shared/package.json
fd -t d '^dist$' packages/shared
echo -e "\n=== Checking how other packages import `@cortex/shared` ==="
rg -n "from ['\"]`@cortex/shared`" --type ts -g '!**/node_modules/**' -C2 | head -20Repository: andrmaz/cortex
Length of output: 927
Fix @cortex/shared package entrypoints pointing to missing/unfinished build artifacts
packages/shared/package.json sets main, types, and exports["."] to ./src/index.ts, but packages/shared has no tsconfig.json and no build/prepare/prepack scripts (and no dist output). Update the package to either (a) build to dist/ and point main/types/exports at the compiled JS + d.ts, or (b) if this package is never published, ensure ./src/index.ts exists and is the intended export surface for all consumers.
🤖 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 `@packages/shared/package.json` around lines 5 - 10, The package.json for
`@cortex/shared` points main/types/exports["."] at ./src/index.ts but there is no
build pipeline or tsconfig and no compiled artifacts; either add a proper build
by creating a tsconfig.json and build scripts (e.g., add "build" to package.json
that compiles src -> dist and emit types) and change "main", "types", and
exports["."] to point to dist/index.js and dist/index.d.ts, or, if the package
is internal-only and not published, ensure ./src/index.ts actually exists and is
the intended public surface (and remove/adjust missing build fields); update
package.json accordingly so main/types/exports refer to existing files and add
the build/prepack/prepare scripts if you choose the compiled dist route.
|
@cursoragent Fix all CI issues and conversations |
|
All CI checks are now passing. Here's a summary of the fixes I made: Fixes Applied1. Fixed Jest moduleNameMapper path in
2. Added
3. Added Node.js globals to
All CI checks (Lint & Type Check, Validate Prisma Schema, Build, CodeRabbit) are now passing.   |
- Fix @cortex/shared moduleNameMapper path in apps/api/jest.config.js With rootDir: 'src', the relative path was off by one directory level. Changed from '../../packages' to '../../../packages' - Add @types/node to packages/shared devDependencies The test files use 'require()' which needs Node.js type definitions for TypeScript to recognize the global Co-authored-by: Andrea Mazzucchelli <andrmaz@users.noreply.github.com>
ESLint was reporting 'module' is not defined for the CommonJS jest.config.js file. Added configuration to recognize Node.js globals (module, require, __dirname, __filename) for this file. Co-authored-by: Andrea Mazzucchelli <andrmaz@users.noreply.github.com>
2 participants
Unit test generation was requested by @andrmaz.
The following files were modified:
apps/api/jest.config.jsapps/api/package.jsonapps/api/src/app.module.spec.tsapps/api/src/mcp/mcp.controller.spec.tsapps/api/src/mcp/mcp.module.spec.tspackages/shared/jest.config.jspackages/shared/package.jsonpackages/shared/src/index.spec.tspackages/shared/src/types.spec.tspnpm-lock.yamlSummary by CodeRabbit
Tests
Chores