test: add comprehensive tests for NormalModuleReplacementPlugin#20709
Conversation
|
There was a problem hiding this comment.
Pull request overview
Adds a new config case to exercise NormalModuleReplacementPlugin behaviors via bundled runtime assertions, aiming to cover both beforeResolve and afterResolve replacement paths.
Changes:
- Add a new
configCasesfixture with awebpack.config.jsthat configures multipleNormalModuleReplacementPluginscenarios (string + function replacements). - Add runtime assertions (
it/expect) that require modules expected to be replaced. - Add fixture modules for original and replacement targets used by the plugin.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/configCases/plugins/normal-module-replacement-plugin/webpack.config.js | Adds plugin setup to test string/function replacements and afterResolve resource rewriting. |
| test/configCases/plugins/normal-module-replacement-plugin/index.js | Adds runtime assertions validating the replacement outcomes. |
| test/configCases/plugins/normal-module-replacement-plugin/b.js | Replacement module for string-based replacement. |
| test/configCases/plugins/normal-module-replacement-plugin/c.js | Replacement module for function-based replacement. |
| test/configCases/plugins/normal-module-replacement-plugin/after.js | Replacement module for after-resolve scenarios. |
| test/configCases/plugins/normal-module-replacement-plugin/before-string.js | “Should be replaced” sentinel module for beforeResolve string case. |
| test/configCases/plugins/normal-module-replacement-plugin/before-function.js | “Should be replaced” sentinel module for beforeResolve function case. |
| test/configCases/plugins/normal-module-replacement-plugin/after-function.js | “Should be replaced” sentinel module for afterResolve function case. |
| test/configCases/plugins/normal-module-replacement-plugin/after-relative.js | “Should be replaced” sentinel module for afterResolve relative-string case. |
| test/configCases/plugins/normal-module-replacement-plugin/a.js | Adds an extra fixture module (currently not referenced in the shown diffs). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| new NormalModuleReplacementPlugin(/\/after-function\.js$/, (result) => { | ||
| if (result.createData && result.createData.resource) { | ||
| const dir = path.dirname(result.createData.resource); | ||
| result.createData.resource = path.join(dir, "after.js"); | ||
| } | ||
| }), | ||
| new NormalModuleReplacementPlugin(/\/after-relative\.js$/, "./after.js") |
There was a problem hiding this comment.
The /\/after-.../ regexes are POSIX-specific and won’t match Windows paths (which typically contain \ in resource / createData.resource during afterResolve). Use a cross-platform separator pattern (e.g., [/\\\\]after-function\\.js$ and [/\\\\]after-relative\\.js$) so the test behaves consistently across OSes.
| new NormalModuleReplacementPlugin(/\/after-function\.js$/, (result) => { | |
| if (result.createData && result.createData.resource) { | |
| const dir = path.dirname(result.createData.resource); | |
| result.createData.resource = path.join(dir, "after.js"); | |
| } | |
| }), | |
| new NormalModuleReplacementPlugin(/\/after-relative\.js$/, "./after.js") | |
| new NormalModuleReplacementPlugin(/[\/\\]after-function\.js$/, (result) => { | |
| if (result.createData && result.createData.resource) { | |
| const dir = path.dirname(result.createData.resource); | |
| result.createData.resource = path.join(dir, "after.js"); | |
| } | |
| }), | |
| new NormalModuleReplacementPlugin(/[\/\\]after-relative\.js$/, "./after.js") |
There was a problem hiding this comment.
Let's fix it, test case looks good
| new NormalModuleReplacementPlugin(/\/after-function\.js$/, (result) => { | ||
| if (result.createData && result.createData.resource) { | ||
| const dir = path.dirname(result.createData.resource); | ||
| result.createData.resource = path.join(dir, "after.js"); | ||
| } | ||
| }), | ||
| new NormalModuleReplacementPlugin(/\/after-relative\.js$/, "./after.js") |
There was a problem hiding this comment.
The PR description mentions covering “relative and absolute path-joining logic in the afterResolve phase,” but the added fixtures only explicitly exercise a relative string replacement ("./after.js"). If you intended to also cover the plugin’s handling of an absolute replacement string, add a dedicated case that sets the replacement to an absolute path (e.g., path.resolve(__dirname, "after.js")) and assert it resolves correctly.
|
@alexander-akait done .please have a look thanks! |
Merging this PR will degrade performance by 25.64%
Performance Changes
Comparing 
|
Great! We need more such tests, thanks |
|
This PR is packaged and the instant preview is available (118e0c5). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@118e0c5
yarn add -D webpack@https://pkg.pr.new/webpack@118e0c5
pnpm add -D webpack@https://pkg.pr.new/webpack@118e0c5 |
Alright on it Alexander, Thanks! |
Summary
Adds comprehensive test coverage for NormalModuleReplacementPlugin, ensuring that both
beforeResolveandafterResolvehooks are fully tested. This PR covers:afterResolvephase.What kind of change does this PR introduce?
testDid you add tests for your changes?
Yes.Does this PR introduce a breaking change?
No.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
N/A.Use of AI
NO