[WIP] Add tests for .ignore file behavior with customPatterns (#959) #960
Conversation
Add comprehensive test coverage to verify that .ignore files work correctly when customPatterns is defined in the configuration. These tests serve as regression tests for issue #959. Tests added: - Config merge behavior when customPatterns is defined - Integration test with actual .ignore files and customPatterns - Zod schema parsing behavior for ignore configuration All tests currently pass, indicating the reported issue cannot be reproduced in the current codebase. Related to #959
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Deploying repomix with
|
| Latest commit: |
4eeb116
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6195c699.repomix.pages.dev |
| Branch Preview URL: | https://fix-959-dotignore-with-custo.repomix.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #960 +/- ##
=======================================
Coverage 89.68% 89.68%
=======================================
Files 111 111
Lines 7843 7843
Branches 1504 1504
=======================================
Hits 7034 7034
Misses 809 809 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PR Review - Test Coverage for Issue #959I've reviewed the test additions for investigating the .ignore file behavior with customPatterns. Here's my comprehensive feedback: ✅ StrengthsWell-Structured Test CoverageThe PR provides three complementary test layers that effectively cover the issue:
This layered approach is excellent for isolating potential issues at different levels. Good Test Design
🔍 Issues & Recommendations1. Schema Test Reveals a Potential Root CauseIn const result = repomixConfigFileSchema.parse(input);
// useDotIgnore should not be set when not provided in input
expect(result.ignore?.useDotIgnore).toBeUndefined();This is actually revealing the likely bug! When users define only ignore: {
...baseConfig.ignore, // useDotIgnore: true
...fileConfig.ignore, // useDotIgnore: undefined (overwrites!)
...cliConfig.ignore,
// ...
}When spreading an object with 2. Integration Test May Have False NegativesIn ignore: {
useGitignore: false,
useDotIgnore: true, // ← Explicitly set
useDefaultPatterns: false,
customPatterns: ['bin/'],
}This doesn't reproduce the user's scenario! The bug report says users define ONLY ignore: {
customPatterns: ['bin/'], // Only this, like the user's config
}Then rely on 3. createMockConfig May Hide the BugIn ignore: {
...defaultConfig.ignore, // Always applies defaults
...config.ignore,
// ...
}This works differently than the actual 🎯 Recommendations for Next StepsTo Reproduce the Bug:
To Fix the Bug:The merge logic in ignore: {
...baseConfig.ignore,
...(fileConfig.ignore ? Object.fromEntries(
Object.entries(fileConfig.ignore).filter(([_, v]) => v !== undefined)
) : {}),
...(cliConfig.ignore ? Object.fromEntries(
Object.entries(cliConfig.ignore).filter(([_, v]) => v !== undefined)
) : {}),
customPatterns: [
...(baseConfig.ignore.customPatterns || []),
...(fileConfig.ignore?.customPatterns || []),
...(cliConfig.ignore?.customPatterns || []),
],
}Or better yet, merge each field individually with null-coalescing: ignore: {
useGitignore: cliConfig.ignore?.useGitignore ?? fileConfig.ignore?.useGitignore ?? baseConfig.ignore.useGitignore,
useDotIgnore: cliConfig.ignore?.useDotIgnore ?? fileConfig.ignore?.useDotIgnore ?? baseConfig.ignore.useDotIgnore,
// ...
}⚡ Premortem Analysis - Potential Failure ScenariosEdge Cases to Test:
Integration Issues:
Deployment Risks:
📋 Minor IssuesCode Quality:
SummaryThe tests are well-structured but may not expose the bug due to test utilities that differ from production behavior. The schema test actually hints at the root cause: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| const result = repomixConfigFileSchema.parse(input); | ||
|
|
||
| // useDotIgnore should not be set when not provided in input |
There was a problem hiding this comment.
This test is revealing the likely root cause of issue #959!
When repomixConfigFileSchema.parse() receives a config with only customPatterns, it returns an object where useDotIgnore is undefined (not present). Then when mergeConfigs() does:
ignore: {
...baseConfig.ignore, // useDotIgnore: true
...fileConfig.ignore, // useDotIgnore: undefined
...cliConfig.ignore,
}The spread of fileConfig.ignore will include useDotIgnore: undefined, which overwrites the default true value.
Recommendation: This is actually the expected Zod behavior - it only includes fields that are explicitly set. The fix should be in mergeConfigs() to handle undefined values properly (see my main review comment for solutions).
| const config = createMockConfig({ | ||
| ignore: { |
There was a problem hiding this comment.
Critical issue: This test doesn't match the bug scenario reported in #959!
The bug report shows users defining ONLY customPatterns:
{
"ignore": {
"customPatterns": ["bin/"]
}
}But this test explicitly sets all flags including useDotIgnore: true. This could mask the bug if the issue is that useDotIgnore defaults aren't being applied properly when only customPatterns is defined.
| const config = createMockConfig({ | |
| ignore: { | |
| // Create config with ONLY customPatterns (matching user's scenario in #959) | |
| const fileConfig: RepomixConfigFile = { | |
| ignore: { | |
| customPatterns: ['bin/'], | |
| }, | |
| }; | |
| const cliConfig: RepomixConfigCli = {}; | |
| const config = mergeConfigs(testDir, fileConfig, cliConfig); | |
| // Verify the merge preserved the default useDotIgnore: true | |
| expect(config.ignore.useDotIgnore).toBe(true); |
This would test the actual merge behavior and expose whether the bug exists.
|
|
||
| const merged = mergeConfigs(cwd, fileConfig, cliConfig); | ||
|
|
||
| // useDotIgnore should be true by default and should not be affected by customPatterns |
There was a problem hiding this comment.
The comment states this "should not be affected" but that's exactly what we're testing for in #959!
If the bug exists, this assertion might actually fail (which would be correct - exposing the bug). Consider adding an explicit check:
// useDotIgnore should be true by default and should not be affected by customPatterns
expect(merged.ignore.useDotIgnore).toBe(true);Also, it would be valuable to verify what the fileConfig.ignore object looks like after Zod parsing to confirm whether useDotIgnore is undefined or not present.
| try { | ||
| await fs.rm(testDir, { recursive: true, force: true }); | ||
| } catch { | ||
| // Ignore errors |
There was a problem hiding this comment.
Potential test isolation issue: Using a fixed directory name ignore-test-temp could cause conflicts if tests run in parallel or if cleanup fails.
Consider using a unique directory per test run:
const testDir = path.join(process.cwd(), 'tests/fixtures', `ignore-test-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`);Or use vitest's test context to ensure proper isolation.
Summary
This PR adds comprehensive test coverage for the issue reported in #959 where
.ignorefiles are allegedly ignored whencustomPatternsis defined in the configuration.What's Included
.ignorefilesCurrent Status
All tests pass, indicating that the reported issue cannot be reproduced in the current codebase (v1.9.1). Further investigation is needed to understand the exact reproduction steps.
Checklist
npm run testnpm run lint