Add comprehensive unit tests for rspack exports #884
Conversation
Addresses issue #844 by adding Jest unit tests for the rspack public API. These tests verify all exports from package/rspack/index.ts work correctly. Tests cover: - Webpack-merge function exports (merge, mergeWithRules, etc.) - Configuration object exports (config, devServer, env, rules, baseConfig) - Utility function exports (moduleExists, canProcess, inliningCss) - generateRspackConfig() behavior: - Returns immutable config objects - Properly merges extra configuration - Validates input arguments - Includes required plugins and optimization - Respects NODE_ENV for environment-specific configs - Falls back to base config for unknown environments - Merges environment and extra configs correctly The baseConfig export is verified to be a valid rspack configuration object, which helps catch issues like those reported in #843. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ 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 |
Greptile OverviewGreptile SummaryThis PR adds a new Jest test suite under The tests follow the same overall structure as the existing Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant T as Jest test
participant I as package/rspack/index
participant V as validateDependencies
participant E as env
participant FS as fs
participant P as plugins/optimization
participant WM as webpack-merge
T->>I: require("package/rspack/index")
T->>T: set SHAKAPACKER_ASSETS_BUNDLER=rspack
T->>T: jest.mock(validateDependencies, helpers)
T->>I: generateRspackConfig(extraConfig?)
I->>V: validateRspackDependencies()
I->>E: read env.nodeEnv (from NODE_ENV)
I->>FS: existsSync(environments/{nodeEnv}.js)
alt env config exists
I->>I: require(environment config)
else missing
I->>I: use baseConfig
end
I->>P: getPlugins(), getOptimization()
I->>WM: merge({}, baseRspackConfig, extraConfig)
WM-->>T: merged config object
|
|
@greptile, review this PR. |
|
Claude finished @justin808's task in 58s —— View job Code Review Complete ✅Todo List:
SummaryThis PR adds excellent comprehensive test coverage for the rspack public API. The tests are well-structured, follow existing patterns, and properly verify all exports and behaviors. Approved - ready to merge! Strengths1. Comprehensive Coverage ✅
2. Proper Test Setup ✅
3. Mirrors Existing Patterns ✅
4. Important Validations ✅
5. Addresses Issue #844 ✅
Minor Observations1. Test Consistency with webpack tests 2. Missing export {
merge,
mergeWithCustomize,
mergeWithRules,
unique
} from "webpack-merge"Actually, 3. Environment Variable Testing
This is thorough and covers all code paths in the source. Code QualityFormatting ✅
Best Practices ✅
RecommendationsNone - this is excellent test coverage that follows all project conventions. The PR is ready to merge as-is. Test Coverage AnalysisAll 13 exports verified:
All
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| beforeEach(() => { | ||
| jest.resetModules() | ||
| resetEnv() | ||
| process.env.SHAKAPACKER_ASSETS_BUNDLER = "rspack" | ||
| }) |
There was a problem hiding this comment.
NODE_ENV leaks across tests
Several tests set process.env.NODE_ENV and call jest.resetModules(), but beforeEach() does not restore NODE_ENV back to its prior value. This can make later tests in this file (or other test files, depending on Jest execution order) run under the wrong environment. Consider capturing the original NODE_ENV at file scope and restoring it in beforeEach()/afterEach() (similar to how resetEnv() is used).
| test("generateRspackConfig errors if multiple configs are provided", () => { | ||
| const { generateRspackConfig } = require("../../../package/rspack/index") | ||
|
|
||
| expect(() => generateRspackConfig({}, {})).toThrow( | ||
| "use webpack-merge to merge configs before passing them to Shakapacker" | ||
| ) |
There was a problem hiding this comment.
Brittle error message assertion
This assertion matches a specific substring that currently differs from the actual thrown message in package/rspack/index.ts (it includes the prefix "Only one extra config may be passed here - ..."). As written, this test will fail even when the code behaves correctly. Align the expectation with the real message (or assert on a more stable substring / error type).
1 participant
Summary
Addresses #844 by adding comprehensive Jest unit tests for the rspack public API exports in
package/rspack/index.ts.What was tested
All 17 tests pass and verify:
✅ Webpack-merge function exports - merge, mergeWithRules, mergeWithCustomize, unique
✅ Configuration object exports - config, devServer, env, rules, baseConfig
✅ Utility function exports - moduleExists, canProcess, inliningCss
✅ generateRspackConfig() behavior:
Key improvements
validateRspackDependencies()to avoid requiring actual rspack dependencies during test runsSHAKAPACKER_ASSETS_BUNDLER=rspackto ensure rspack-specific code paths are testedTest file location
test/package/rspack/index.test.js- mirrors the structure of existingtest/package/index.test.jsfor webpackTest plan
🤖 Generated with Claude Code