Add comprehensive unit tests for rspack exports

[justin808]

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:

• Returns immutable config objects
• Properly merges extra configuration
• Validates input arguments (errors on multiple configs)
• Includes required plugins array
• Includes optimization object
• Includes module rules array
• Respects NODE_ENV for environment-specific configs
• Falls back to base config for unknown environments
• Correctly merges environment and extra configs

Key improvements

• baseConfig verification: Tests confirm baseConfig is a valid rspack configuration object with required properties (output, resolve), helping catch issues like those reported in baseConfig export from rspack is broken, baseConfig for webpack might be broken as well #843
• Dependency validation mocking: Tests mock validateRspackDependencies() to avoid requiring actual rspack dependencies during test runs
• Environment variable setup: Tests properly set SHAKAPACKER_ASSETS_BUNDLER=rspack to ensure rspack-specific code paths are tested

Test file location

test/package/rspack/index.test.js - mirrors the structure of existing test/package/index.test.js for webpack

Test plan

• All 17 new tests pass
• All existing package tests still pass
• Linting passes

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 34 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 227a018 and 8dc7231.

📒 Files selected for processing (1)

• test/package/rspack/index.test.js (1 hunks)

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch jg-/beirut-v2

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Pull Request Review

Summary

This PR adds comprehensive Jest unit tests for the rspack public API exports, addressing issue #844. Overall, this is a high-quality contribution that significantly improves test coverage for the rspack functionality.

✅ Strengths

1. Excellent test coverage: 17 tests cover all major exports and edge cases
2. Good structure: Mirrors the existing test/package/index.test.js pattern for consistency
3. Proper mocking: Correctly mocks validateRspackDependencies() and moduleExists() to avoid dependency requirements
4. Environment setup: Properly sets SHAKAPACKER_ASSETS_BUNDLER=rspack for rspack-specific code paths
5. Thorough edge cases: Tests immutability, config merging, error conditions, and fallback behavior

🔍 Code Quality Observations

Minor Issues

1. Missing trailing newline ⚠️

   • Line 190 of test/package/rspack/index.test.js is missing a trailing newline
   • This will fail the project's linting rules per CLAUDE.md
   • Action required: Add a newline at the end of the file

2. Path inconsistency in mocks

   • The webpack test (test/package/index.test.js) uses require("../../package/utils/helpers")
   • The rspack test uses require("../../../package/utils/helpers")
   • Both are correct for their respective locations, but worth noting the directory structure difference

3. Test description consistency

   • Line 19: "webpackConfig returns an immutable object" (in webpack tests)
   • Line 88: "generateRspackConfig returns an immutable object" (in rspack tests)
   • The rspack version is more precise. Consider updating the webpack test for consistency (separate PR)

🧪 Test Coverage Analysis

All critical functionality is tested:

• ✅ Webpack-merge function exports (merge, mergeWithRules, etc.)
• ✅ Configuration objects (config, devServer, env, rules, baseConfig)
• ✅ Utility functions (moduleExists, canProcess, inliningCss)
• ✅ generateRspackConfig behavior (immutability, merging, validation, plugins, optimization, module rules)
• ✅ Environment-specific configs (NODE_ENV handling)
• ✅ Error conditions (multiple configs error)
• ✅ Fallback behavior (unknown environments)

Test Coverage vs Source Code

Comparing with package/rspack/index.ts:

• ✅ All exports are tested
• ✅ The validateRspackDependencies() call is properly mocked (line 30 in source)
• ✅ Environment config loading logic is tested (lines 37-40 in source)
• ✅ Config merging is tested (line 52 in source)

🔒 Security Considerations

No security concerns identified. The tests:

• Don't execute arbitrary code
• Properly mock external dependencies
• Use safe file system operations via the test helper

⚡ Performance Considerations

• Tests use jest.resetModules() before each test, which is necessary but can be slow
• This is consistent with existing test patterns in the codebase
• No performance issues expected

📝 Best Practices

1. ✅ Follows existing test patterns
2. ✅ Proper setup/teardown with beforeEach and afterAll
3. ✅ Clear, descriptive test names
4. ✅ Tests one concern per test case
5. ✅ Good use of Jest matchers

🐛 Potential Issues

Low Priority

1. resetEnv() behavior (line 54 in helpers.js)
   • process.env = {} completely wipes the environment, which could break Node.js internals
   • However, this is existing code, not introduced by this PR
   • Consider filing a separate issue if this causes problems

📋 Action Items

Required before merge:

• Add trailing newline at end of file (line 190)
• Run yarn lint to verify no linting errors
• Verify all tests pass with yarn test test/package/rspack/index.test.js

Optional improvements (could be follow-up PRs):

• Consider adding tests for the re-exported webpack-merge functions (they're tested indirectly but not explicitly)
• Add tests for edge cases like passing undefined or null to generateRspackConfig

✅ Recommendation

APPROVE with minor fix required: This is an excellent addition to the test suite. Once the trailing newline is added, this should be merged.

The tests are well-structured, comprehensive, and directly address the issues mentioned in #844 and help prevent regressions of #843.

---

🤖 Generated with Claude Code

[claude]

Code Review - PR #847: Add comprehensive unit tests for rspack exports

Thank you for this excellent contribution! The test coverage for the rspack public API is much needed and well-implemented. Here's my detailed feedback:

✅ Strengths

1. Comprehensive Coverage: All 17 exports from package/rspack/index.ts are properly tested, including:

   • webpack-merge functions (merge, mergeWithRules, mergeWithCustomize, unique)
   • Configuration objects (config, devServer, env, rules, baseConfig)
   • Utility functions (moduleExists, canProcess, inliningCss)
   • The main generateRspackConfig() function

2. Good Test Structure: The tests mirror the existing webpack tests in test/package/index.test.js, maintaining consistency across the codebase.

3. Proper Mocking: The mocks for moduleExists and validateRspackDependencies are well-designed to isolate the tests from external dependencies.

4. Environment Testing: Tests verify environment-specific behavior (development vs production) and fallback to base config for unknown environments.

5. Immutability Verification: Good testing of the immutability guarantee that generateRspackConfig() provides.

🔍 Issues & Concerns

1. Missing Trailing Newline ⚠️

Per the project's CLAUDE.md guidelines, all files must end with a trailing newline. The test file appears to be missing this at line 190.

Fix: Add a newline after the final closing brace.

2. Inconsistent Error Message Testing

Line 118 tests for error message:

"use webpack-merge to merge configs before passing them to Shakapacker"

However, the actual error message in package/rspack/index.ts:33-34 is:

"Only one extra config may be passed here - use webpack-merge to merge configs before passing them to Shakapacker"

The test only checks for a substring match, which works but could be more precise. The webpack version at package/index.ts:30-40 has a much more helpful multi-line error message with examples.

Suggestion: Either:

• Update the rspack error message to match the webpack version's verbosity
• Update the test to check for the full error message, or at least "Only one extra config"

3. Test Name Inconsistency

Line 89: Test is named "generateRspackConfig returns an immutable object" but the webpack equivalent (line 19) is "webpackConfig returns an immutable object".

This is actually better in the rspack version as it's more descriptive. Consider updating the webpack test for consistency.

4. Potential Race Condition with jest.resetModules()

Lines 152 and 176 call jest.resetModules() inside individual tests to test different NODE_ENV values. This is fine for isolated tests but could cause issues if tests run in parallel or if module state leaks between tests.

Suggestion: Consider using beforeEach to reset modules consistently, or document why resetModules is needed only in specific tests.

5. Missing Type Validation

The rspack implementation uses TypeScript (index.ts) but the tests are in JavaScript (.test.js). While this is fine for runtime behavior, it doesn't verify type safety.

Nice-to-have: Consider adding type-level tests or at least JSDoc comments in the test file to verify TypeScript types are exported correctly.

🎯 Code Quality

• ✅ Readability: Excellent - tests are clear and well-organized
• ✅ Maintainability: Good - follows existing patterns
• ✅ Test isolation: Good - proper use of mocks and beforeEach/afterAll
• ✅ Documentation: The PR description is thorough and explains the test coverage well

🔒 Security Concerns

No security issues identified. The tests are read-only and properly mock external dependencies.

⚡ Performance Considerations

• The test setup calls chdirTestApp() which changes the working directory. The afterAll hook properly restores it.
• Multiple jest.resetModules() calls could slow down test execution slightly, but this is acceptable for thorough environment testing.

📋 Recommendations

Before merging:

1. ✅ Add trailing newline to the test file
2. ⚠️ Consider improving the error message test to be more specific
3. ✅ Run yarn lint to verify (mentioned in PR but good to confirm)

Future improvements:

1. Consider harmonizing error messages between webpack and rspack implementations
2. Add a comment explaining why SHAKAPACKER_ASSETS_BUNDLER=rspack is set in beforeEach
3. Consider adding a test that verifies the module exports exactly match what's in the TypeScript source

📊 Test Coverage Assessment

This PR successfully addresses issue #844 by providing comprehensive unit tests for the rspack public API. The tests verify both the happy path and error cases, which should help catch issues like the baseConfig problem mentioned in issue #843.

✅ Overall Assessment

This is a high-quality PR that significantly improves the project's test coverage and reliability. With the minor fixes above, this is ready to merge.

Recommendation: Approve with minor changes

---

🤖 Generated with Claude Code