test: more rules #7
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughThis change introduces comprehensive test suites and supporting package files for the Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Test Suite
participant Rsbuild as Rsbuild Build System
participant Plugin as pluginAreTheTypesWrong
participant Logger as Logger
participant FS as File System
TestRunner->>Rsbuild: Initialize build with plugin and options
Rsbuild->>Plugin: Invoke plugin during build lifecycle
Plugin->>Logger: Log error or success messages based on scenario and config
Plugin->>FS: Attempt to create package file (conditional)
FS-->>Plugin: Confirm presence or absence of package file
Plugin-->>Rsbuild: Return build result or throw error
Rsbuild-->>TestRunner: Build result or error
TestRunner->>Logger: Spy/assert on logged messages
TestRunner->>FS: Assert package file not created
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (10)
test/unexpected-module-syntax/src/index.js (1)
1-1: Intentional ESM-in-CJS: annotate and ensure linters don’t block this testWith
"type": "commonjs"in the package,exportis invalid at runtime—presumably intentional for the “UnexpectedModuleSyntax” test. Biome flagged a parse error; CI may fail unless ignored.
- Add an explanatory comment at the top to prevent “fixes” later.
- Ignore this file in Biome (parse errors can’t be suppressed inline).
Proposed header comment:
+// Intentionally uses ESM syntax in a CJS package to trigger the "UnexpectedModuleSyntax" rule in tests. export const hello = 'world';Biome ignore (biome.json):
{ "$schema": "https://biomejs.dev/schemas/1.8.0/schema.json", "files": { "ignore": ["test/unexpected-module-syntax/src/index.js"] } }test/missing-export-equals/src/index.js (1)
1-2:module.exports.defaultis overwritten on the next line
module.exports = fooreplaces the entire exports object, discardingmodule.exports.defaultset just before it. If the goal is to mimic a CJS module that only exposesmodule.exports(to trigger “MissingExportEquals”), either:
- Remove the first line for clarity, or
- Add a comment explaining that the overwrite is intentional.
Option A (clarify by removal):
-module.exports.default = foo; module.exports = foo;Option B (document the intention):
-module.exports.default = foo; +// Intentionally set then overwrite to simulate a default type without a runtime default export. module.exports = foo;test/internal-resolution-error/src/index.js (1)
1-5: Inline the default export for clarityEquivalent behavior, avoids pre-declaration reference, and improves readability.
-export default foo; - -function foo() { - return 42; -} +export default function foo() { + return 42; +}test/missing-export-equals/package.json (1)
8-13: Be explicit about CJS resolution inexportsTo make CJS intent unambiguous, add the
requirecondition alongsidedefault. This is a nice-to-have for clarity and parity with Node’s condition resolution."exports": { ".": { - "types": "./src/index.d.ts", - "default": "./src/index.js" + "types": "./src/index.d.ts", + "require": "./src/index.js", + "default": "./src/index.js" } }test/missing-export-equals/index.test.ts (2)
33-66: Document why this test is skipped (or convert to runtime conditional)Add a reason/TODO to the skip so future maintainers know when to enable it, or gate it conditionally (e.g., only run when local env supports node16 resolutions).
-test.skip("should be able to ignore resolution node16-*", async () => { +// TODO(colinaaa): Enable when ATTW supports ignoring node16-* resolutions in this scenario. +test.skip("should be able to ignore resolution node16-*", async () => {
112-120: Optionally assert no error logs whenenable: falseNot mandatory, but it makes the intent explicit.
- const success = vi.spyOn(logger, "success"); + const success = vi.spyOn(logger, "success"); + const error = vi.spyOn(logger, "error"); @@ expect(success).not.toBeCalled(); + expect(error).not.toBeCalled();test/unexpected-module-syntax/index.test.ts (2)
10-16: Nit: tighten test descriptionMinor grammar fix for readability.
-test("should throw when have unexpected module syntax", async () => { +test("should throw on unexpected module syntax", async () => {
112-120: Optionally assert no error logs when disabledComplements the existing assertion.
- const success = vi.spyOn(logger, "success"); + const success = vi.spyOn(logger, "success"); + const error = vi.spyOn(logger, "error"); @@ expect(success).not.toBeCalled(); + expect(error).not.toBeCalled();test/internal-resolution-error/index.test.ts (2)
10-16: Nit: tighten test descriptionImprove readability.
-test("should throw when have internal resolution error", async () => { +test("should throw on internal resolution error", async () => {
112-120: Optionally also assert no error logs when disabledMakes the disabled-path expectations explicit.
- const success = vi.spyOn(logger, "success"); + const success = vi.spyOn(logger, "success"); + const error = vi.spyOn(logger, "error"); @@ expect(success).not.toBeCalled(); + expect(error).not.toBeCalled();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
test/internal-resolution-error/__snapshots__/index.test.ts.snapis excluded by!**/*.snaptest/missing-export-equals/__snapshots__/index.test.ts.snapis excluded by!**/*.snaptest/unexpected-module-syntax/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
test/internal-resolution-error/index.test.ts(1 hunks)test/internal-resolution-error/package.json(1 hunks)test/internal-resolution-error/src/index.d.cts(1 hunks)test/internal-resolution-error/src/index.d.ts(1 hunks)test/internal-resolution-error/src/index.js(1 hunks)test/internal-resolution-error/src/types.d.ts(1 hunks)test/missing-export-equals/index.test.ts(1 hunks)test/missing-export-equals/package.json(1 hunks)test/missing-export-equals/src/index.d.ts(1 hunks)test/missing-export-equals/src/index.js(1 hunks)test/unexpected-module-syntax/index.test.ts(1 hunks)test/unexpected-module-syntax/package.json(1 hunks)test/unexpected-module-syntax/src/index.d.ts(1 hunks)test/unexpected-module-syntax/src/index.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-10T13:56:09.286Z
Learnt from: colinaaa
PR: colinaaa/rsbuild-plugin-arethetypeswrong#6
File: test/cjs-resolves-to-esm/index.test.ts:10-31
Timestamp: 2025-08-10T13:56:09.286Z
Learning: The `rsbuild` instance created by `createRsbuild()` from `rsbuild/core` does not have a `close()` method. In test files, it's acceptable to not perform cleanup when the build fails with an error, as cleanup is not required in error cases.
Applied to files:
test/missing-export-equals/index.test.tstest/unexpected-module-syntax/index.test.tstest/internal-resolution-error/index.test.ts
📚 Learning: 2025-08-10T09:28:46.764Z
Learnt from: colinaaa
PR: colinaaa/rsbuild-plugin-arethetypeswrong#3
File: src/index.ts:49-51
Timestamp: 2025-08-10T09:28:46.764Z
Learning: In the rsbuild-plugin-arethetypeswrong project, Rslib is used to bundle the plugin, which means all imported dependencies (including `arethetypeswrong/core`) are bundled into the final output. Therefore, these dependencies can be placed in `devDependencies` rather than `dependencies` since they are only needed at build time, not at runtime for end users.
Applied to files:
test/missing-export-equals/index.test.tstest/unexpected-module-syntax/index.test.tstest/internal-resolution-error/index.test.ts
🧬 Code Graph Analysis (2)
test/unexpected-module-syntax/src/index.d.ts (1)
test/unexpected-module-syntax/src/index.js (2)
hello(1-1)hello(1-1)
test/unexpected-module-syntax/index.test.ts (1)
src/index.ts (4)
pluginAreTheTypesWrong(18-78)options(21-60)setup(26-59)PluginAreTheTypesWrongOptions(11-19)
🪛 Biome (2.1.2)
test/unexpected-module-syntax/src/index.js
[error] 1-1: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
🔇 Additional comments (6)
test/unexpected-module-syntax/package.json (1)
5-12: Config aligns with test intent (CJS + explicit export map)This shape (CJS +
exportswithdefaultandtypes) is good for exercising the rule. Note: whenexportsis present, Node ignoresmain; fine for the test package.test/internal-resolution-error/package.json (1)
6-16: Norequireruntime export whenexportsis present (Node will ignoremain)Because
exportsexists, Node ignoresmain. There’s no"require"condition for runtime, sorequire('...')will fail—likely by design to trigger “InternalResolutionError”.Please confirm this is intentional and tests don’t rely on CommonJS consumption. If not intentional, add:
".": { "types": { "import": "./src/index.d.ts", "require": "./src/index.d.cts" }, - "import": "./src/index.js" + "import": "./src/index.js", + "require": "./src/index.js" }test/unexpected-module-syntax/src/index.d.ts (1)
1-1: LGTMDeclaration matches the JS export and is syntactically correct.
test/missing-export-equals/index.test.ts (1)
6-6: Restore spies between testsRestore all spies after each test to avoid cross-test leakage.
-import { expect, test, vi } from "vitest"; +import { afterEach, expect, test, vi } from "vitest";Add after the imports:
+afterEach(() => { + vi.restoreAllMocks(); +});⛔ Skipped due to learnings
Learnt from: colinaaa PR: colinaaa/rsbuild-plugin-arethetypeswrong#6 File: test/cjs-resolves-to-esm/index.test.ts:6-7 Timestamp: 2025-08-10T13:57:06.293Z Learning: In Vitest test files, when spies are created using `vi.spyOn()` within individual test function scopes (not at module level), they don't need to be restored between tests since each test creates its own isolated spy instance that goes out of scope when the test completes.test/unexpected-module-syntax/index.test.ts (1)
6-6: Restore spies between testsSame rationale as other suites: prevent cross-test leakage.
-import { expect, test, vi } from "vitest"; +import { afterEach, expect, test, vi } from "vitest"; + +afterEach(() => { + vi.restoreAllMocks(); +});⛔ Skipped due to learnings
Learnt from: colinaaa PR: colinaaa/rsbuild-plugin-arethetypeswrong#6 File: test/cjs-resolves-to-esm/index.test.ts:6-7 Timestamp: 2025-08-10T13:57:06.293Z Learning: In Vitest test files, when spies are created using `vi.spyOn()` within individual test function scopes (not at module level), they don't need to be restored between tests since each test creates its own isolated spy instance that goes out of scope when the test completes.test/internal-resolution-error/index.test.ts (1)
6-6: Restore spies between testsAvoid cross-test interference.
-import { expect, test, vi } from "vitest"; +import { afterEach, expect, test, vi } from "vitest"; + +afterEach(() => { + vi.restoreAllMocks(); +});⛔ Skipped due to learnings
Learnt from: colinaaa PR: colinaaa/rsbuild-plugin-arethetypeswrong#6 File: test/cjs-resolves-to-esm/index.test.ts:6-7 Timestamp: 2025-08-10T13:57:06.293Z Learning: In Vitest test files, when spies are created using `vi.spyOn()` within individual test function scopes (not at module level), they don't need to be restored between tests since each test creates its own isolated spy instance that goes out of scope when the test completes.
colinaaa
force-pushed
the
colin/0810/test-1
branch
from
8bd0908 to
1b7b872
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
test/build-tools/index.test.ts (1)
10-33: Reduce duplication by extracting a small helperThree tests repeat the same build/spy/assert flow. Factor into a helper to keep tests concise and easier to maintain.
Example helper (place near the top of the file):
type Options = Parameters<typeof pluginAreTheTypesWrong>[0] | undefined; async function runATTWTest(options?: Options) { const rsbuild = await createRsbuild({ cwd: import.meta.dirname, rsbuildConfig: { plugins: [pluginAreTheTypesWrong(options)] }, }); const success = vi.spyOn(logger, "success"); const { close } = await rsbuild.build(); const messages = success.mock.calls .flatMap(call => call .filter(message => typeof message === "string" && message.includes("[arethetypeswrong]")) .map(stripVTControlCharacters)); expect(messages).toMatchSnapshot(); expect(existsSync(expectedTarball)).toBeFalsy(); await close(); }Then the tests become:
test("should run arethetypeswrong as expected", async () => { await runATTWTest(); }); test("should run arethetypeswrong without emoji", async () => { await runATTWTest({ areTheTypesWrongOptions: { emoji: false } }); }); test("should run arethetypeswrong without summary", async () => { await runATTWTest({ areTheTypesWrongOptions: { emoji: false, summary: false } }); });Also applies to: 35-63, 64-92
test/exports/index.test.ts (2)
22-28: Make the spy output extraction simplerThe current nested
flatMap+filter+mapworks but is verbose. Flatten arguments first, then filter.- expect( - success.mock.calls.flatMap(call => - call - .filter(message => typeof message === "string" && message.includes("[arethetypeswrong]")) - .map(stripVTControlCharacters) - ), - ).toMatchSnapshot(); + expect( + success.mock.calls + .flat() + .filter((message): message is string => typeof message === "string" && message.includes("[arethetypeswrong]")) + .map(stripVTControlCharacters), + ).toMatchSnapshot();Also applies to: 51-57, 81-87
10-16: Optional: deduplicate repeated test setup via a helperAll three tests share the same structure; consider extracting a small
runhelper to reduce duplication and ease future changes.Example helper (add near top of file):
async function run(options?: Parameters<typeof pluginAreTheTypesWrong>[0]) { const rsbuild = await createRsbuild({ cwd: import.meta.dirname, rsbuildConfig: { plugins: [pluginAreTheTypesWrong(options)] }, }); const success = vi.spyOn(logger, "success"); const error = vi.spyOn(logger, "error"); const result = await rsbuild.build(); try { const messages = success.mock.calls .flat() .filter((m): m is string => typeof m === "string" && m.includes("[arethetypeswrong]")) .map(stripVTControlCharacters); expect(messages).toMatchSnapshot(); expect(error).not.toHaveBeenCalled(); expect(existsSync(path.join(import.meta.dirname, "test-exports-0.0.0.tgz"))).toBe(false); } finally { await result.close(); vi.restoreAllMocks(); } }Then:
test("should run arethetypeswrong as expected", async () => run()); test("should run arethetypeswrong without emoji", async () => run({ areTheTypesWrongOptions: { emoji: false } })); test("should run arethetypeswrong without summary", async () => run({ areTheTypesWrongOptions: { emoji: false, summary: false } }));Also applies to: 35-45, 64-75
test/wildcard/index.test.ts (1)
10-33: Test structure is solid but consider reducing duplication.The test correctly validates plugin behavior with default configuration. The use of snapshot testing for logger output and file existence checks is appropriate.
Consider extracting the common test logic into a helper function to reduce duplication:
async function runPluginTest(options?: Parameters<typeof pluginAreTheTypesWrong>[0]) { const rsbuild = await createRsbuild({ cwd: import.meta.dirname, rsbuildConfig: { plugins: [pluginAreTheTypesWrong(options)], }, }); const success = vi.spyOn(logger, "success"); const { close } = await rsbuild.build(); const logMessages = success.mock.calls.flatMap(call => call .filter(message => typeof message === "string" && message.includes("[arethetypeswrong]")) .map(stripVTControlCharacters) ); return { logMessages, close }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
test/build-tools/__snapshots__/index.test.ts.snapis excluded by!**/*.snaptest/cjs-resolves-to-esm/__snapshots__/index.test.ts.snapis excluded by!**/*.snaptest/exports/__snapshots__/index.test.ts.snapis excluded by!**/*.snaptest/internal-resolution-error/__snapshots__/index.test.ts.snapis excluded by!**/*.snaptest/missing-export-equals/__snapshots__/index.test.ts.snapis excluded by!**/*.snaptest/unexpected-module-syntax/__snapshots__/index.test.ts.snapis excluded by!**/*.snaptest/wildcard/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (39)
test/build-tools/index.test.ts(1 hunks)test/build-tools/package.json(1 hunks)test/build-tools/src/index.d.ts(1 hunks)test/build-tools/src/index.js(1 hunks)test/cjs-resolves-to-esm/index.test.ts(1 hunks)test/exports/index.test.ts(1 hunks)test/exports/package.json(1 hunks)test/exports/src/bar.cjs(1 hunks)test/exports/src/bar.d.cts(1 hunks)test/exports/src/bar.d.ts(1 hunks)test/exports/src/bar.js(1 hunks)test/exports/src/foo.cjs(1 hunks)test/exports/src/foo.d.cts(1 hunks)test/exports/src/foo.d.ts(1 hunks)test/exports/src/foo.js(1 hunks)test/internal-resolution-error/index.test.ts(1 hunks)test/internal-resolution-error/package.json(1 hunks)test/internal-resolution-error/src/index.d.cts(1 hunks)test/internal-resolution-error/src/index.d.ts(1 hunks)test/internal-resolution-error/src/index.js(1 hunks)test/internal-resolution-error/src/types.d.ts(1 hunks)test/missing-export-equals/index.test.ts(1 hunks)test/missing-export-equals/package.json(1 hunks)test/missing-export-equals/src/index.d.ts(1 hunks)test/missing-export-equals/src/index.js(1 hunks)test/unexpected-module-syntax/index.test.ts(1 hunks)test/unexpected-module-syntax/package.json(1 hunks)test/unexpected-module-syntax/src/index.d.ts(1 hunks)test/unexpected-module-syntax/src/index.js(1 hunks)test/wildcard/index.test.ts(1 hunks)test/wildcard/package.json(1 hunks)test/wildcard/src/bar.cjs(1 hunks)test/wildcard/src/bar.d.cts(1 hunks)test/wildcard/src/bar.d.ts(1 hunks)test/wildcard/src/bar.js(1 hunks)test/wildcard/src/foo.cjs(1 hunks)test/wildcard/src/foo.d.cts(1 hunks)test/wildcard/src/foo.d.ts(1 hunks)test/wildcard/src/foo.js(1 hunks)
✅ Files skipped from review due to trivial changes (11)
- test/exports/src/bar.cjs
- test/exports/src/bar.js
- test/exports/src/foo.d.ts
- test/wildcard/src/bar.d.ts
- test/wildcard/src/foo.cjs
- test/cjs-resolves-to-esm/index.test.ts
- test/build-tools/package.json
- test/exports/src/foo.cjs
- test/wildcard/src/bar.cjs
- test/exports/package.json
- test/wildcard/package.json
🚧 Files skipped from review as they are similar to previous changes (13)
- test/unexpected-module-syntax/src/index.d.ts
- test/internal-resolution-error/src/types.d.ts
- test/unexpected-module-syntax/package.json
- test/missing-export-equals/src/index.d.ts
- test/internal-resolution-error/src/index.d.cts
- test/internal-resolution-error/src/index.d.ts
- test/internal-resolution-error/package.json
- test/internal-resolution-error/src/index.js
- test/missing-export-equals/src/index.js
- test/missing-export-equals/package.json
- test/missing-export-equals/index.test.ts
- test/unexpected-module-syntax/index.test.ts
- test/internal-resolution-error/index.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-10T09:28:46.764Z
Learnt from: colinaaa
PR: colinaaa/rsbuild-plugin-arethetypeswrong#3
File: src/index.ts:49-51
Timestamp: 2025-08-10T09:28:46.764Z
Learning: In the rsbuild-plugin-arethetypeswrong project, Rslib is used to bundle the plugin, which means all imported dependencies (including `arethetypeswrong/core`) are bundled into the final output. Therefore, these dependencies can be placed in `devDependencies` rather than `dependencies` since they are only needed at build time, not at runtime for end users.
Applied to files:
test/build-tools/index.test.tstest/exports/index.test.tstest/wildcard/index.test.ts
🧬 Code Graph Analysis (9)
test/wildcard/src/foo.d.cts (1)
test/wildcard/src/foo.js (1)
foo(1-3)
test/build-tools/src/index.js (3)
test/exports/src/foo.js (1)
foo(1-3)test/wildcard/src/foo.js (1)
foo(1-3)test/named-exports/index.test.ts (2)
rsbuild(33-64)message(89-89)
test/exports/src/bar.d.cts (1)
test/exports/src/bar.js (1)
bar(1-3)
test/build-tools/src/index.d.ts (1)
test/build-tools/src/index.js (1)
foo(1-3)
test/wildcard/src/bar.d.cts (2)
test/exports/src/bar.js (1)
bar(1-3)test/wildcard/src/bar.js (1)
bar(1-3)
test/wildcard/src/foo.d.ts (1)
test/wildcard/src/foo.js (1)
foo(1-3)
test/exports/src/foo.d.cts (1)
test/exports/src/foo.js (1)
foo(1-3)
test/wildcard/src/foo.js (12)
test/build-tools/src/index.js (1)
foo(1-3)test/exports/src/foo.js (1)
foo(1-3)test/untyped-resolution/src/index.cjs (1)
foo(1-3)test/untyped-resolution/src/index.mjs (1)
foo(1-3)test/basic/src/index.js (1)
foo(1-3)test/false-esm/src/index.mjs (1)
foo(1-3)test/no-resolution/src/index.js (1)
foo(1-3)test/untyped/src/index.js (1)
foo(1-3)test/false-esm/src/index.cjs (1)
foo(1-3)test/with-scope/src/index.js (1)
foo(1-3)test/named-exports/index.test.ts (1)
message(89-89)test/false-cjs/src/index.mjs (1)
main(1-3)
test/exports/index.test.ts (8)
src/index.ts (1)
pluginAreTheTypesWrong(18-78)test/basic/index.test.ts (2)
rsbuild(64-92)rsbuild(35-62)test/named-exports/index.test.ts (1)
rsbuild(66-97)test/with-scope/index.test.ts (2)
rsbuild(64-92)rsbuild(35-62)test/cjs-only-exports-default/index.test.ts (1)
rsbuild(33-65)test/untyped-resolution/index.test.ts (1)
rsbuild(98-131)test/false-cjs/index.test.ts (1)
rsbuild(33-64)test/false-export-default/index.test.ts (1)
rsbuild(33-67)
🪛 Biome (2.1.2)
test/unexpected-module-syntax/src/index.js
[error] 1-1: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (windows-latest)
🔇 Additional comments (15)
test/build-tools/src/index.js (1)
1-3: LGTM: simple named export matches usageImplementation is minimal and correct.
test/build-tools/src/index.d.ts (1)
1-1: LGTM: declaration matches implementation
fooreturns a number and aligns withsrc/index.js.test/build-tools/index.test.ts (1)
6-9: Restore mocks between tests to avoid spy-on conflicts and cross-test leakageWithout restoration, multiple
vi.spyOn(logger, "success")calls across tests can collide or leak calls. Add a global afterEach to restore all mocks.Apply this diff:
-import { expect, test, vi } from "vitest"; +import { expect, test, vi, afterEach } from "vitest"; @@ import { pluginAreTheTypesWrong } from "../../src"; +afterEach(() => { + vi.restoreAllMocks(); +});⛔ Skipped due to learnings
Learnt from: colinaaa PR: colinaaa/rsbuild-plugin-arethetypeswrong#6 File: test/cjs-resolves-to-esm/index.test.ts:6-7 Timestamp: 2025-08-10T13:57:06.293Z Learning: In Vitest test files, when spies are created using `vi.spyOn()` within individual test function scopes (not at module level), they don't need to be restored between tests since each test creates its own isolated spy instance that goes out of scope when the test completes.test/exports/src/foo.d.cts (1)
1-1: Validate TypeScript version for.d.ctsnamed exportsThis pattern (
export function foo(): number;in a.d.ctsfile) has been supported since TypeScript 4.7. Please ensure your project is using TS 4.7 or later so that this named‐export declaration correctly maps toexports.fooat runtime.• File to check:
test/exports/src/foo.d.cts– no changes needed if TS ≥ 4.7.• If you must support older TS versions, switch to a CommonJS‐style default export:
declare function foo(): number; export = foo;test/exports/src/bar.d.ts (1)
1-1: LGTM — declaration matches implementation intentThe
barfunction declaration is straightforward and consistent with the fixture’s JS implementation.test/exports/src/bar.d.cts (1)
1-1: .d.cts declaration forbarverifiedThe
test/exports/package.jsoncorrectly maps the CJS type entry forbar:
"types.require": "./src/bar.d.cts"As long as you’re on TypeScript ≥ 4.7 with a Node-16 or bundler-style module resolution, named exports in
.d.ctsfiles work as expected. No further changes needed.test/exports/src/foo.js (1)
1-3: LGTM — simple ESM export with deterministic returnFunction is minimal, deterministic, and fits the test fixture purpose.
test/exports/index.test.ts (3)
18-33: Prevent spy leakage and ensure cleanup with try/finallySpies are not restored between tests; this can cause cumulative
mock.callsand flaky snapshots. Also prefer deterministic boolean assertions.Apply:
- const success = vi.spyOn(logger, "success"); - - const { close } = await rsbuild.build(); - - expect( + const success = vi.spyOn(logger, "success"); + const error = vi.spyOn(logger, "error"); + const result = await rsbuild.build(); + try { + expect( success.mock.calls.flatMap(call => call .filter(message => typeof message === "string" && message.includes("[arethetypeswrong]")) .map(stripVTControlCharacters) ), - ).toMatchSnapshot(); - - expect(existsSync(path.join(import.meta.dirname, "test-exports-0.0.0.tgz"))).toBeFalsy(); - - await close(); + ).toMatchSnapshot(); + expect(error).not.toHaveBeenCalled(); + expect(existsSync(path.join(import.meta.dirname, "test-exports-0.0.0.tgz"))).toBe(false); + } finally { + await result.close(); + vi.restoreAllMocks(); + }⛔ Skipped due to learnings
Learnt from: colinaaa PR: colinaaa/rsbuild-plugin-arethetypeswrong#6 File: test/cjs-resolves-to-esm/index.test.ts:6-7 Timestamp: 2025-08-10T13:57:06.293Z Learning: In Vitest test files, when spies are created using `vi.spyOn()` within individual test function scopes (not at module level), they don't need to be restored between tests since each test creates its own isolated spy instance that goes out of scope when the test completes.Learnt from: colinaaa PR: colinaaa/rsbuild-plugin-arethetypeswrong#6 File: test/cjs-resolves-to-esm/index.test.ts:10-31 Timestamp: 2025-08-10T13:56:09.286Z Learning: The `rsbuild` instance created by `createRsbuild()` from `rsbuild/core` does not have a `close()` method. In test files, it's acceptable to not perform cleanup when the build fails with an error, as cleanup is not required in error cases.Learnt from: colinaaa PR: colinaaa/rsbuild-plugin-arethetypeswrong#10 File: test/pnpm/index.test.ts:20-33 Timestamp: 2025-08-10T15:53:16.194Z Learning: In the rsbuild-plugin-arethetypeswrong project's test files, it's not necessary to ensure that Rsbuild instances are closed using try/finally blocks or other cleanup patterns. The `close()` function returned from `rsbuild.build()` doesn't need to be guaranteed to run even when test assertions fail.
77-92: Repeat cleanup fix in test 3 and assert no errors loggedSame isolation concerns apply; add error assertion and deterministic boolean check.
- const success = vi.spyOn(logger, "success"); - - const { close } = await rsbuild.build(); - - expect( + const success = vi.spyOn(logger, "success"); + const error = vi.spyOn(logger, "error"); + const result = await rsbuild.build(); + try { + expect( success.mock.calls.flatMap(call => call .filter(message => typeof message === "string" && message.includes("[arethetypeswrong]")) .map(stripVTControlCharacters) ), - ).toMatchSnapshot(); - - expect(existsSync(path.join(import.meta.dirname, "test-exports-0.0.0.tgz"))).toBeFalsy(); - - await close(); + ).toMatchSnapshot(); + expect(error).not.toHaveBeenCalled(); + expect(existsSync(path.join(import.meta.dirname, "test-exports-0.0.0.tgz"))).toBe(false); + } finally { + await result.close(); + vi.restoreAllMocks(); + }⛔ Skipped due to learnings
Learnt from: colinaaa PR: colinaaa/rsbuild-plugin-arethetypeswrong#6 File: test/cjs-resolves-to-esm/index.test.ts:10-31 Timestamp: 2025-08-10T13:56:09.286Z Learning: The `rsbuild` instance created by `createRsbuild()` from `rsbuild/core` does not have a `close()` method. In test files, it's acceptable to not perform cleanup when the build fails with an error, as cleanup is not required in error cases.Learnt from: colinaaa PR: colinaaa/rsbuild-plugin-arethetypeswrong#10 File: test/pnpm/index.test.ts:20-33 Timestamp: 2025-08-10T15:53:16.194Z Learning: In the rsbuild-plugin-arethetypeswrong project's test files, it's not necessary to ensure that Rsbuild instances are closed using try/finally blocks or other cleanup patterns. The `close()` function returned from `rsbuild.build()` doesn't need to be guaranteed to run even when test assertions fail.
47-62: Repeat cleanup fix in test 2 to isolate spies and resultsMirror the try/finally cleanup and add an
errorspy to assert no failures are logged.- const success = vi.spyOn(logger, "success"); - - const { close } = await rsbuild.build(); - - expect( + const success = vi.spyOn(logger, "success"); + const error = vi.spyOn(logger, "error"); + const result = await rsbuild.build(); + try { + expect( success.mock.calls.flatMap(call => call .filter(message => typeof message === "string" && message.includes("[arethetypeswrong]")) .map(stripVTControlCharacters) ), - ).toMatchSnapshot(); - - expect(existsSync(path.join(import.meta.dirname, "test-exports-0.0.0.tgz"))).toBeFalsy(); - - await close(); + ).toMatchSnapshot(); + expect(error).not.toHaveBeenCalled(); + expect(existsSync(path.join(import.meta.dirname, "test-exports-0.0.0.tgz"))).toBe(false); + } finally { + await result.close(); + vi.restoreAllMocks(); + }⛔ Skipped due to learnings
Learnt from: colinaaa PR: colinaaa/rsbuild-plugin-arethetypeswrong#6 File: test/cjs-resolves-to-esm/index.test.ts:10-31 Timestamp: 2025-08-10T13:56:09.286Z Learning: The `rsbuild` instance created by `createRsbuild()` from `rsbuild/core` does not have a `close()` method. In test files, it's acceptable to not perform cleanup when the build fails with an error, as cleanup is not required in error cases.Learnt from: colinaaa PR: colinaaa/rsbuild-plugin-arethetypeswrong#6 File: test/cjs-resolves-to-esm/index.test.ts:6-7 Timestamp: 2025-08-10T13:57:06.293Z Learning: In Vitest test files, when spies are created using `vi.spyOn()` within individual test function scopes (not at module level), they don't need to be restored between tests since each test creates its own isolated spy instance that goes out of scope when the test completes.Learnt from: colinaaa PR: colinaaa/rsbuild-plugin-arethetypeswrong#10 File: test/pnpm/index.test.ts:20-33 Timestamp: 2025-08-10T15:53:16.194Z Learning: In the rsbuild-plugin-arethetypeswrong project's test files, it's not necessary to ensure that Rsbuild instances are closed using try/finally blocks or other cleanup patterns. The `close()` function returned from `rsbuild.build()` doesn't need to be guaranteed to run even when test assertions fail.test/wildcard/src/foo.js (1)
1-3: LGTM: Simple ESM named export matches expected usageNamed export foo returning 42 is consistent with similar fixtures and the associated type decls.
test/wildcard/src/bar.js (1)
1-3: LGTM: Consistent ESM named exportbar returns 42 and mirrors the pattern used across tests and declarations.
test/wildcard/src/foo.d.ts (1)
1-1: LGTM: Declaration aligns with implementationexport function foo(): number; matches the JS ESM export and is appropriate for ESM type consumers.
test/wildcard/src/foo.d.cts (1)
1-1: LGTM: CJS declaration mirrors ESM signatureexport function foo(): number; is appropriate for a CJS-typed module when using named exports and complements the .d.ts.
test/wildcard/index.test.ts (1)
1-8: LGTM!The import statements are well-organized and include all necessary dependencies for the test scenarios.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/unexpected-module-syntax/src/index.js (1)
1-1: Clarify fixture intent; exclude from Biome or mark as ESM to avoid parse errorBiome parses this as CJS/script and errors on
export. If the invalid ESM-in-CJS is intentional to trigger “unexpected-module-syntax”, keep it—but exclude this fixture from Biome to avoid CI failures. If not intentional, make it ESM by adding"type": "module"to the fixture package.json or renaming to.mjs.
- Exclude in Biome (preferred for this negative test):
# biome.json / biome.jsonc { + "files": { + "ignore": [ + "test/unexpected-module-syntax/**" + ] + } }
- Or make it a valid ESM module (only if you don’t want this as a negative fixture):
# test/unexpected-module-syntax/package.json { "name": "test-unexpected-module-syntax", - "type": "commonjs" + "type": "module" }Run to verify current setup:
#!/bin/bash set -e echo "Fixture package.json type (expect commonjs if negative test):" if [ -f test/unexpected-module-syntax/package.json ]; then jq -r '.type // "(absent)"' test/unexpected-module-syntax/package.json || cat test/unexpected-module-syntax/package.json else echo "No test/unexpected-module-syntax/package.json" fi echo echo "Biome configs and ignore entries (ensure test/unexpected-module-syntax/** is ignored):" fd -HI -a "biome.json" "biome.jsonc" || true for f in $(fd -HI "biome.json" "biome.jsonc"); do echo "---- $f ----" sed -n '1,160p' "$f" echo echo "Ignore contains test/unexpected-module-syntax?:" jq -e '.files.ignore[]? | test("test/unexpected-module-syntax/")' "$f" >/dev/null 2>&1 && echo "Yes" || echo "No" echo done echo echo "Fixture files present:" fd -a "test/unexpected-module-syntax" --extension js --extension d.ts || true
🧹 Nitpick comments (1)
vitest.config.ts (1)
21-24: Reduce flakiness risk from tight thresholds (esp. 100% functions)Coverage can vary slightly across Node/V8 versions and remapping. Consider a small buffer to avoid CI flakes while keeping ambition high.
Apply this diff:
- lines: 96.74, - functions: 100, - branches: 94.62, - statements: 96.92, + lines: 96.5, + functions: 99.5, + branches: 94.5, + statements: 96.5,Optionally, if you want thresholds to apply to all source files (not only those touched by tests), add this outside the selected lines:
// inside coverage: all: true,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
test/build-tools/package.json(1 hunks)test/internal-resolution-error/index.test.ts(1 hunks)test/internal-resolution-error/package.json(1 hunks)test/internal-resolution-error/src/index.d.cts(1 hunks)test/internal-resolution-error/src/index.d.ts(1 hunks)test/internal-resolution-error/src/types.d.ts(1 hunks)test/missing-export-equals/index.test.ts(1 hunks)test/missing-export-equals/package.json(1 hunks)test/missing-export-equals/src/index.d.ts(1 hunks)test/unexpected-module-syntax/index.test.ts(1 hunks)test/unexpected-module-syntax/package.json(1 hunks)test/unexpected-module-syntax/src/index.d.ts(1 hunks)test/unexpected-module-syntax/src/index.js(1 hunks)vitest.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/unexpected-module-syntax/package.json
🚧 Files skipped from review as they are similar to previous changes (11)
- test/internal-resolution-error/src/index.d.cts
- test/missing-export-equals/src/index.d.ts
- test/internal-resolution-error/src/index.d.ts
- test/internal-resolution-error/src/types.d.ts
- test/unexpected-module-syntax/src/index.d.ts
- test/missing-export-equals/package.json
- test/build-tools/package.json
- test/internal-resolution-error/package.json
- test/missing-export-equals/index.test.ts
- test/unexpected-module-syntax/index.test.ts
- test/internal-resolution-error/index.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/unexpected-module-syntax/src/index.js (3)
test/unexpected-module-syntax/src/index.d.ts (1)
hello(1-1)test/named-exports/index.test.ts (1)
message(89-89)test/cjs-only-exports-default/index.test.ts (4)
message(90-90)rsbuild(33-65)rsbuild(67-98)rsbuild(10-31)
🪛 Biome (2.1.2)
test/unexpected-module-syntax/src/index.js
[error] 1-1: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (windows-latest)
🔇 Additional comments (1)
vitest.config.ts (1)
21-24: LGTM: higher coverage thresholds match the expanded test suitesRaising thresholds here is consistent with the new tests and signals strong confidence in coverage.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Chores