✅ Add tests for vitest setupFiles duplication fix#84
Conversation
Code Review ✅Overall Assessment: This PR adds excellent regression tests for the setupFiles duplication fix from #82. The tests are well-written, clearly documented, and validate the correct behavior. ✅ Strengths
💡 Suggestions1. Consider Testing the Merged ResultThe current tests verify what the plugin returns, but don't verify what Vitest actually sees after merging. Consider adding a test that validates the final merged config to prove setupFiles aren't duplicated: it('should result in correct merged config after vitest merging', async () => {
const { vizzlyPlugin } = await import('../src/index.js');
const userConfig = {
test: {
setupFiles: ['./user-setup.js'],
},
};
const plugin = vizzlyPlugin();
const pluginConfig = plugin.config(userConfig, { mode: 'test' });
// Simulate Vitest's merging behavior
const mergedSetupFiles = [
...(userConfig.test.setupFiles || []),
...(pluginConfig.test.setupFiles || []),
];
// Should have exactly 2 files (user's + plugin's), not 3
expect(mergedSetupFiles).toHaveLength(2);
expect(mergedSetupFiles).toContain('./user-setup.js');
expect(mergedSetupFiles.some(f => f.includes('setup.js'))).toBe(true);
});This would make the regression test even more explicit about the bug that was fixed. 2. Minor: Variable Naming ConsistencyIn the first test you use // First test
let userConfig = { test: { setupFiles: ['./user-setup.js'] } };
let pluginConfig = plugin.config(userConfig, { mode: 'test' });
// Second test - could also use pluginConfig for consistency
let userConfig = { test: { setupFiles: './user-setup.js' } };
let pluginConfig = plugin.config(userConfig, { mode: 'test' });3. Documentation EnhancementConsider adding a brief comment at the top of these tests explaining the Vitest plugin config merging behavior for future maintainers: // Vitest's plugin system automatically merges plugin config with user config.
// When a plugin returns { test: { setupFiles: [...] } }, Vitest merges it with
// the user's test.setupFiles array. If the plugin spreads the user's files into
// its return value, those files would run twice.
// See: https://vitest.dev/guide/using-plugins.html🔒 Security & Performance
🧪 Test Coverage
📝 Final RecommendationApprove with optional improvements. The tests are solid as-is and ready to merge. The suggestions above would make them even more robust but aren't blockers. Great work validating the fix! 🎉 |
938a3b7 to
138d7ed
Compare
Adds regression tests to verify the plugin doesn't duplicate user-provided setupFiles. Vitest automatically merges plugin config with user config, so the plugin should only return its own setup file. These tests will pass after PR #82 is merged.
138d7ed to
44f610a
Compare
- Adds regression tests validating that the Vitest plugin doesn't duplicate user-provided `setupFiles` - Tests both array and string `setupFiles` configurations These tests validate the fix from #82. Vitest automatically merges plugin config with user config, so the plugin should only return its own setup file—not spread the user's files into its return value. - [x] Merge #82 first - [x] Rebase this branch onto main - [ ] Tests should pass
Summary
setupFilessetupFilesconfigurationsContext
These tests validate the fix from #82. Vitest automatically merges plugin config with user config, so the plugin should only return its own setup file—not spread the user's files into its return value.
Test plan