test: enforce test catalog consistency and Jest discovery#85
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideAdds an automated validator to keep tests/catalog.md consistent with actual Jest-discovered test files, wires it into the lint pipeline, and expands the catalog to cover all currently executed script and unit test suites. Sequence diagram for validate test catalog execution and Jest discoverysequenceDiagram
actor Developer
participant Npm as npm_cli
participant Lint as npm_script_lint
participant CatalogScript as validate_test_catalog_js
participant FS as file_system
participant Jest as jest_runner
Developer->>Npm: run npm run lint
Npm->>Lint: execute lint script pipeline
Lint->>CatalogScript: run npm run test:catalog
CatalogScript->>FS: read tests/catalog.md
FS-->>CatalogScript: catalog_entries
CatalogScript->>Jest: run jest with config jest.config.js
Jest-->>CatalogScript: discovered_test_files
CatalogScript->>CatalogScript: build_catalog_path_set
CatalogScript->>CatalogScript: build_jest_path_set
CatalogScript->>CatalogScript: diff sets for missing_or_extra_tests
alt catalog consistent with Jest discovery
CatalogScript-->>Lint: exit code 0
else catalog inconsistent
CatalogScript-->>Lint: exit non_zero
Lint-->>Npm: propagate failure
Npm-->>Developer: lint command fails
end
Flow diagram for updated lint pipeline with test catalog validationflowchart LR
subgraph NpmScripts
A[npm_run_lint]
B[npm_run_format_check]
C[eslint_src_and_tests]
D[npm_run_lint_md]
E[npm_run_test_catalog]
F[npm_run_changelog_validate]
end
A --> B --> C --> D --> E --> F
subgraph TestCatalogValidation
E --> G[node_scripts_validate_test_catalog_js]
G --> H[read_tests_catalog_md]
G --> I[run_jest_discovery_config_jest_config_js]
H --> J[build_catalog_test_path_set]
I --> K[build_jest_discovered_test_path_set]
J --> L[compare_catalog_and_jest_sets]
K --> L
L --> M{catalog_matches_jest?}
M -->|yes| N[exit_code_0]
M -->|no| O[exit_with_failure_non_zero]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughA new test catalog validation script is introduced alongside an updated test catalog and comprehensive test suite. The script verifies consistency between test catalog references and Jest-discovered tests, integrated into the lint workflow to run validation checks during the build process. Changes
Sequence DiagramsequenceDiagram
participant Lint as Lint Workflow
participant Val as validate-test-catalog.js
participant FS as File System
participant Jest as Jest Config
participant Cat as Catalog.md
Lint->>Val: Execute validation script
Val->>Cat: Read catalog.md
Cat-->>Val: Return catalog content
Val->>Val: Extract catalog references
Val->>Jest: Load Jest configuration
Jest-->>Val: Return testMatch & ignorePatterns
Val->>FS: Recursively list test files
FS-->>Val: Return executable test files
Val->>Val: Compile ignore patterns
Val->>Val: Match files against Jest patterns<br/>while respecting ignore rules
Val->>Val: Compute discrepancies<br/>(missing refs, unlisted, not discovered)
Val-->>Lint: Exit with status & report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoEnforce test catalog consistency and Jest discovery validation
WalkthroughsDescription• Add test catalog validator script to enforce consistency between tests/catalog.md and actual test files • Integrate catalog validation into npm run lint workflow • Expand tests/catalog.md with newly added test suite entries • Implement Jest discovery checks to prevent unlisted or out-of-scope test files Diagramflowchart LR
A["validate-test-catalog.js"] -- "validates" --> B["tests/catalog.md"]
B -- "references" --> C["test files"]
A -- "checks against" --> D["Jest config"]
D -- "discovers" --> C
A -- "integrated into" --> E["npm run lint"]
File Changes1. scripts/validate-test-catalog.js
|
Summary of ChangesHello @Mehdi-Bl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a test catalog validation script to ensure consistency between the test catalog and Jest's test discovery. It enhances the linting process by verifying that all test files are correctly referenced and discovered, improving the reliability and maintainability of the test suite. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
normalizeTestMatchPatterns, therootDirparameter is unused—either remove it from the signature or incorporate it into the normalization logic to avoid confusion. compileIgnorePatternssilently discards invalid regex patterns; consider surfacing which pattern failed (e.g., via a warning) so Jest ignore misconfigurations don’t go unnoticed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `normalizeTestMatchPatterns`, the `rootDir` parameter is unused—either remove it from the signature or incorporate it into the normalization logic to avoid confusion.
- `compileIgnorePatterns` silently discards invalid regex patterns; consider surfacing which pattern failed (e.g., via a warning) so Jest ignore misconfigurations don’t go unnoticed.
## Individual Comments
### Comment 1
<location> `tests/unit/scripts/validate-test-catalog.test.js:20` </location>
<code_context>
+ return fs.mkdtempSync(path.join(os.tmpdir(), 'validate-test-catalog-'));
+}
+
+describe('validate-test-catalog script', () => {
+ test('extracts unique test catalog references from markdown content', () => {
+ const content = `
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for catalog read failures (missing or unreadable catalog.md)
Since the function returns a specific failure shape when `fs.readFileSync` throws (e.g., missing file, permission error), please add a test that calls `validateTestCatalog` with a non-existent `catalogPath` (and/or one made unreadable via `fs.chmodSync`) and asserts that `isValid === false`, `errors` includes `Unable to read test catalog`, and the other arrays are empty. This will lock in the error-handling behavior for these failure cases.
Suggested implementation:
```javascript
describe('validate-test-catalog script', () => {
test('extracts unique test catalog references from markdown content', () => {
const content = `
# Test Catalog
- \`tests/unit/a.test.ts\`
- \`tests/unit/a.test.ts\`
- \`tests/integration/b.test.ts\`
`;
expect(extractCatalogPathReferences(content)).toEqual([
'tests/integration/b.test.ts',
'tests/unit/a.test.ts',
]);
});
test('returns failure result when catalog cannot be read', () => {
const workspaceRoot = createWorkspace();
const nonExistentCatalogPath = path.join(
workspaceRoot,
'does-not-exist',
'catalog.md',
);
const result = validateTestCatalog({
catalogPath: nonExistentCatalogPath,
workspaceRoot,
});
expect(result.isValid).toBe(false);
expect(result.errors).toEqual(
expect.arrayContaining([
expect.stringContaining('Unable to read test catalog'),
]),
);
expect(result.missingInCatalog).toEqual([]);
expect(result.missingInRepo).toEqual([]);
expect(result.extraInCatalog).toEqual([]);
```
Because only part of the file is visible, you may need to:
1. **Adjust the `validateTestCatalog` call signature**
If `validateTestCatalog` is not invoked with an options object elsewhere, update the new test to match the existing pattern, e.g.:
- `const result = validateTestCatalog(nonExistentCatalogPath, workspaceRoot);` or
- `const result = validateTestCatalog(nonExistentCatalogPath);`
2. **Align result shape property names**
If the returned object uses different property names for the arrays, update the expectations accordingly. For example, if they are `missing`, `extra`, etc., replace:
- `missingInCatalog`, `missingInRepo`, `extraInCatalog`
with the actual property names from the implementation.
3. **Ensure imports are present**
Confirm that `validateTestCatalog`, `path`, `fs`, and `os` are already imported at the top of the test file. If `validateTestCatalog` is not yet imported, add:
```js
const { validateTestCatalog } = require('../../../scripts/validate-test-catalog');
```
or whatever path/name matches the rest of the file.
4. **Placement in the file**
Make sure the inserted `test('returns failure result when catalog cannot be read', ...)` sits within the same top-level `describe('validate-test-catalog script', ...)` block and does not accidentally nest inside another test if the surrounding structure differs slightly from the snippet above.
</issue_to_address>
### Comment 2
<location> `tests/unit/scripts/validate-test-catalog.test.js:21` </location>
<code_context>
+}
+
+describe('validate-test-catalog script', () => {
+ test('extracts unique test catalog references from markdown content', () => {
+ const content = `
+# Test Catalog
</code_context>
<issue_to_address>
**suggestion (testing):** Extend `extractCatalogPathReferences` tests to cover wildcards and non-test file references
Since wildcard catalog entries are skipped by the validator’s existence checks and paths are extracted purely from markdown patterns, please add coverage for entries like `tests/unit/*.test.ts` and `tests/README.md`. The assertions should verify that both the wildcard and non-test paths are returned as-is, confirming that `extractCatalogPathReferences` doesn’t filter by extension or glob semantics.
</issue_to_address>
### Comment 3
<location> `tests/unit/scripts/validate-test-catalog.test.js:36-45` </location>
<code_context>
+ });
+
+ test('fails when catalog references missing paths', () => {
+ const rootDir = createWorkspace();
+
+ writeFile(rootDir, 'tests/catalog.md', '`tests/unit/missing.test.ts`');
+
+ const report = validateTestCatalog({
+ rootDir,
+ catalogPath: path.join(rootDir, 'tests/catalog.md'),
+ jestConfig: {
+ testMatch: ['<rootDir>/tests/**/*.{js,jsx,ts,tsx}'],
+ testPathIgnorePatterns: [],
+ },
+ });
+
+ expect(report.isValid).toBe(false);
+ expect(report.missingCatalogPaths).toEqual(['tests/unit/missing.test.ts']);
+ expect(report.errors[0]).toContain('Catalog references missing paths');
+
+ fs.rmSync(rootDir, { recursive: true, force: true });
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Refactor workspace cleanup into `afterEach` or `try/finally` to avoid leaks on test failure
If an assertion fails before `fs.rmSync(rootDir, ...)`, the temporary workspace is left behind, which clutters local runs. Centralize cleanup in an `afterEach` hook or a helper that wraps the test body in `try/finally` so `rmSync` always executes, even on failure.
Suggested implementation:
```javascript
test('fails when catalog references missing paths', () => {
return withWorkspace((rootDir) => {
writeFile(rootDir, 'tests/catalog.md', '`tests/unit/missing.test.ts`');
const report = validateTestCatalog({
rootDir,
catalogPath: path.join(rootDir, 'tests/catalog.md'),
jestConfig: {
testMatch: ['<rootDir>/tests/**/*.{js,jsx,ts,tsx}'],
testPathIgnorePatterns: [],
},
});
expect(report.isValid).toBe(false);
expect(report.missingCatalogPaths).toEqual(['tests/unit/missing.test.ts']);
expect(report.errors[0]).toContain('Catalog references missing paths');
});
});
```
```javascript
function createWorkspace() {
return fs.mkdtempSync(path.join(os.tmpdir(), 'validate-test-catalog-'));
}
function withWorkspace(testFn) {
const rootDir = createWorkspace();
const cleanup = () =>
fs.rmSync(rootDir, {
recursive: true,
force: true,
});
try {
const result = testFn(rootDir);
// Support both sync and async test bodies
if (result && typeof result.then === 'function') {
return result.finally(cleanup);
}
cleanup();
return result;
} catch (error) {
cleanup();
throw error;
}
}
```
Scan the rest of `tests/unit/scripts/validate-test-catalog.test.js` for other tests that:
1. Call `createWorkspace()` directly, and
2. Manually invoke `fs.rmSync(rootDir, ...)` at the end.
Refactor those tests to use `return withWorkspace((rootDir) => { ... })` in the same way, and remove their manual `rmSync` calls, so all temporary workspaces are consistently cleaned up via `try/finally`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable script for validating the test catalog against Jest's discovery, a positive step towards better test documentation and maintenance. However, a security audit revealed critical vulnerabilities in the new validate-test-catalog.js script: a high-severity Path Traversal vulnerability and a critical-severity Code Injection vulnerability, both stemming from improper handling of command-line arguments. Immediate remediation is crucial to prevent potential compromise. Additionally, the code review suggests enhancing the script's validation logic for better alignment with Jest's behavior and improving the test setup for reliability and proper cleanup.
| jestConfigPath: jestConfigArg | ||
| ? path.resolve(process.cwd(), jestConfigArg) | ||
| : DEFAULT_JEST_CONFIG_PATH, |
There was a problem hiding this comment.
The script is vulnerable to arbitrary code execution. The jestConfigArg variable, which is derived from user-supplied command-line input (process.argv[3]), is used to construct a file path for a Jest configuration file. This path is then passed to require() on line 51. An attacker can provide a path to a malicious JavaScript file, leading to its execution with the privileges of the running process. This is a critical vulnerability, especially if the script is run in a CI/CD environment or any automated context. The user-provided path for the Jest config should be validated to ensure it is a valid and trusted file within the project's directory. Ideally, the script should only allow loading files from a specific, hardcoded directory.
| const catalogArg = process.argv[2]; | ||
| const jestConfigArg = process.argv[3]; | ||
| const result = validateTestCatalog({ | ||
| catalogPath: catalogArg ? path.resolve(process.cwd(), catalogArg) : DEFAULT_CATALOG_PATH, |
There was a problem hiding this comment.
The script is vulnerable to Path Traversal. The catalogArg variable, which is derived from user-supplied command-line input (process.argv[2]), is used to construct a file path using path.resolve() without proper sanitization. An attacker could provide a malicious path (e.g., ../../../../etc/passwd) to read arbitrary files from the filesystem where the script is executed. Before using the user-provided path, it should be validated to ensure it resolves to a location within the intended project directory.
| if (testMatchPatterns.length === 0) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
When testMatchPatterns is empty, the function currently returns true, implying all files are matched. This doesn't align with Jest's default behavior. If testMatch is not specified in the Jest configuration, Jest uses a default set of patterns (**/__tests__/**/*.[jt]s?(x) and **/?(*.)+(spec|test).[jt]s?(x)). To make this script more robust and align with Jest's behavior, it should apply these default patterns when testMatchPatterns is empty.
if (testMatchPatterns.length === 0) {
const JEST_DEFAULT_PATTERNS = [
'**/__tests__/**/*.[jt]s?(x)',
'**/?(*.)+(spec|test).[jt]s?(x)',
];
return JEST_DEFAULT_PATTERNS.some((pattern) => minimatch(relativeFilePath, pattern, { dot: true }));
}| .filter((relativePath) => { | ||
| const absolutePath = path.join(rootDir, relativePath); | ||
| if (isIgnoredByJest(absolutePath, ignorePatterns)) { | ||
| return false; | ||
| } | ||
|
|
||
| return !discoveredTestFileSet.has(relativePath); | ||
| }) |
There was a problem hiding this comment.
The current logic for identifying listedButNotDiscoveredTestFiles explicitly ignores files that are listed in the catalog but are ignored by Jest's testPathIgnorePatterns. This seems to contradict the goal of ensuring the catalog is consistent with Jest's discovery, as ignored files are not discovered or run by Jest. If a test is listed in the catalog, it should probably be discovered by Jest. If it's intentionally ignored, it might be better to remove it from the catalog to avoid confusion. Consider changing this logic to flag cataloged tests that are ignored by Jest.
.filter((relativePath) => !discoveredTestFileSet.has(relativePath))| return fs.mkdtempSync(path.join(os.tmpdir(), 'validate-test-catalog-')); | ||
| } | ||
|
|
||
| describe('validate-test-catalog script', () => { |
There was a problem hiding this comment.
The tests create and clean up a temporary directory inside each test block. If an assertion fails, the cleanup code (fs.rmSync) will not be reached, potentially leaving temporary files on the file system. It's a better practice to use beforeEach to create the workspace and afterEach to clean it up. This ensures that cleanup always runs, regardless of whether the test passes or fails.
Code Review by Qodo
✅ 1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| function compileIgnorePatterns(patterns = []) { | ||
| return patterns | ||
| .map((pattern) => { | ||
| try { | ||
| return new RegExp(pattern); | ||
| } catch (_error) { | ||
| return null; | ||
| } | ||
| }) | ||
| .filter(Boolean); |
There was a problem hiding this comment.
1. compileignorepatterns() swallows regex errors 📘 Rule violation ⛯ Reliability
Invalid testPathIgnorePatterns entries are silently dropped when new RegExp(pattern) fails, which can cause incorrect catalog validation results without indicating why. This reduces diagnosability and violates the requirement to handle failure points with meaningful context.
Agent Prompt
## Issue description
`compileIgnorePatterns()` silently ignores invalid regex patterns in `testPathIgnorePatterns` by catching errors and returning `null` without surfacing the failure. This can lead to incorrect validation output and makes debugging configuration issues difficult.
## Issue Context
This script is intended to enforce consistency between `tests/catalog.md` and Jest discovery. Dropping invalid ignore patterns silently changes what is considered “discovered,” undermining the validator.
## Fix Focus Areas
- scripts/validate-test-catalog.js[61-70]
- scripts/validate-test-catalog.js[96-187]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| function listExecutableTestFiles(rootDir = ROOT_DIR) { | ||
| const testsRoot = path.join(rootDir, 'tests'); | ||
| const files = collectFilesRecursively(testsRoot); | ||
|
|
||
| return files | ||
| .map((absolutePath) => toPosixPath(path.relative(rootDir, absolutePath))) | ||
| .filter((relativePath) => EXECUTABLE_TEST_FILE_PATTERN.test(relativePath)) | ||
| .sort(); | ||
| } |
There was a problem hiding this comment.
2. Validator mis-models jest discovery 🐞 Bug ✓ Correctness
The validator only treats *.test.*/*.spec.* files as “discovered”, but this repo’s Jest
testMatch includes all tests/**/*.{js,jsx,ts,tsx}; Jest will execute files like tests/setup.ts
that the validator ignores. This can allow real Jest-executed suites/files to be missing from
tests/catalog.md without npm run lint failing, contradicting the intended enforcement.
Agent Prompt
### Issue description
The catalog validator only considers files matching `\.(test|spec)\.(js|jsx|ts|tsx)$` as “Jest discovered”, but the repo’s `jest.config.js` discovers **all** `tests/**/*.{js,jsx,ts,tsx}`. This mismatch means `npm run test:catalog` can pass even while Jest executes non-`.test`/`.spec` files (e.g. `tests/setup.ts`) that are not enforced in `tests/catalog.md`.
### Issue Context
- Current Jest config `testMatch` is broader than the validator’s `EXECUTABLE_TEST_FILE_PATTERN`.
- `tests/setup.ts` contains a real Jest suite and is also included in `setupFilesAfterEnv`, so it will be executed under the current `testMatch`.
### Fix Focus Areas
Choose one approach and make the validator + Jest consistent.
**Approach A (recommended): narrow Jest discovery to match validator intent**
- Update Jest `testMatch` to only include actual test files (e.g. default Jest pattern within `tests/`).
- Remove the “dummy test” from `tests/setup.ts` (it will still run as a setup file via `setupFilesAfterEnv`, but should not itself be a test suite).
**Approach B: broaden validator discovery to match Jest config**
- Treat any file under `tests/` that matches Jest `testMatch` (and isn’t ignored) as discovered; then decide whether helper files should be required in the catalog.
Add/adjust unit tests to cover whichever approach you choose.
Fix locations:
- scripts/validate-test-catalog.js[10-12]
- scripts/validate-test-catalog.js[86-138]
- jest.config.js[8-18]
- tests/setup.ts[1-10]
- tests/unit/scripts/validate-test-catalog.test.js[35-139]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
1 participant
Summary
tests/catalog.mdreferences existing pathsnpm run lintand expandtests/catalog.mdto include currently executed suitesValidation
Summary by Sourcery
Enforce consistency between the Jest test suite and tests/catalog.md via a new validation script wired into linting and test workflows.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit