test: split e2e and unit cases #421
Conversation
|
WalkthroughThis pull request updates various configurations and test setups. It removes a Changes
Sequence Diagram(s)sequenceDiagram
participant TR as Test Runner
participant DS as Directory Scanner
participant YE as Yarn Executor
participant ESL as ESLint Executor
participant SM as Snapshot Matcher
TR->>DS: Scan e2e directories (skip "__snapshots__")
DS-->>TR: Return valid test directories
TR->>YE: Check for yarn.lock & execute "yarn --immutable"
TR->>ESL: Run ESLint (with custom config if present)
ESL-->>TR: Return ESLint results
TR->>SM: Compare results with stored snapshots
Assessment against linked issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
.lintstagedrc.jsOops! Something went wrong! :( ESLint: 9.23.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js auto-imports.d.tsOops! Something went wrong! :( ESLint: 9.23.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js eslint.config.jsOops! Something went wrong! :( ESLint: 9.23.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected], npm/[email protected] |
commit: |
size-limit report 📦
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/unit/unit.spec.ts (1)
9-33: Consider improving test robustness.While the test correctly validates PnP functionality, there are a few improvements to consider:
- The test depends on external
yarn installwhich might cause flakiness in CI environments- Consider adding cleanup after the test to remove any temporary files
- The snapshots contain absolute paths which might cause issues across different environments
it('should work with pnp', async () => { + // Setup temporary directory or cleanup previous runs await exec('yarn', ['install'], { nodeOptions: { cwd: pnpDir, }, }) const resolver = createTypeScriptImportResolver() const testfile = path.resolve(pnpDir, '__test__.js') expect(resolver.resolve('pnpapi', testfile)).toMatchInlineSnapshot(` { "found": true, - "path": "<ROOT>/tests/unit/pnp/.pnp.cjs", + "path": ${expect.stringMatching(/.*\/\.pnp\.cjs$/)}", } `) expect(resolver.resolve('lodash.zip', testfile)).toMatchInlineSnapshot(` { "found": true, - "path": "<ROOT>/tests/unit/pnp/.yarn/cache/lodash.zip-npm-4.2.0-5299417ec8-cb06530d81.zip/node_modules/lodash.zip/index.js", + "path": ${expect.stringMatching(/.*\/lodash\.zip\/index\.js$/)}", } `) + + // Add cleanup if necessary }, 10_000)tests/e2e/importXResolverV3/eslint.config.js (1)
18-35: Improve Node.js version check readability.The current version check using unary plus and string splitting works, but could be made more readable for better maintainability.
-export default +process.versions.node.split('.')[0] <= 16 +// Extract the major Node.js version number +const nodeMajorVersion = parseInt(process.versions.node.split('.')[0], 10) + +export default nodeMajorVersion <= 16 ? {} : { files: ['**/*.ts', '**/*.tsx'], plugins: { 'import-x': importX, }, settings: { ...flatConfigs.typescript.settings, 'import-x/resolver-next': [ createTypeScriptImportResolver({ project: absoluteGlobPath, noWarnOnMultipleProjects: true, }), ], }, rules: base_().rules, }tests/e2e/e2e.spec.ts (1)
19-23: Consider adding error handling for yarn command executionWhile the code correctly checks for the existence of a yarn.lock file before running yarn, there's no error handling for the yarn command execution itself.
if (tryFile('yarn.lock', false, absoluteDir)) { - await exec('yarn', ['--immutable']) + try { + await exec('yarn', ['--immutable']) + } catch (error) { + console.error(`Failed to run yarn in ${absoluteDir}:`, error) + } }tests/e2e/withJsExtension/test.cjs (1)
2-2: Updated ESLint disable directiveThe ESLint directive has been expanded to explicitly disable both
no-require-importsandno-var-requiresrules. The trailing comma in the directive should be removed.-/* eslint-disable @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires, */ +/* eslint-disable @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
tests/e2e/__snapshots__/e2e.spec.ts.snapis excluded by!**/*.snaptests/e2e/nestedPackageJson/yarn.lockis excluded by!**/yarn.lock,!**/*.locktests/e2e/withQuerystring/image.svgis excluded by!**/*.svgtests/e2e/withQuerystring/subfolder/image.svgis excluded by!**/*.svgtests/unit/pnp/.yarn/cache/lodash.zip-npm-4.2.0-5299417ec8-cb06530d81.zipis excluded by!**/.yarn/**,!**/*.ziptests/unit/pnp/.yarn/releases/yarn-4.8.0.cjsis excluded by!**/.yarn/**tests/unit/pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
.env.yarn(0 hunks).lintstagedrc.js(1 hunks).prettierignore(1 hunks)auto-imports.d.ts(1 hunks)eslint.config.js(2 hunks)package.json(3 hunks)tests/e2e/absoluteAlias/tsconfig.json(1 hunks)tests/e2e/e2e.spec.ts(1 hunks)tests/e2e/importXResolverV3/eslint.config.js(1 hunks)tests/e2e/package.json(1 hunks)tests/e2e/withJsExtension/.eslintrc.cjs(0 hunks)tests/e2e/withJsExtension/index.ts(1 hunks)tests/e2e/withJsExtension/test.cjs(2 hunks)tests/e2e/withQuerystring/.eslintrc.cjs(1 hunks)tests/e2e/withoutPaths/.eslintrc.cjs(1 hunks)tests/importXResolverV3/eslint.config.cjs(0 hunks)tests/nestedPackageJson/.eslintrc.cjs(0 hunks)tests/pnp/pnp.spec.ts(0 hunks)tests/tsconfig.json(1 hunks)tests/unit/unit.spec.ts(1 hunks)tests/withJsconfig/.eslintrc.cjs(0 hunks)tsconfig.base.json(1 hunks)tsconfig.json(1 hunks)tsconfig.staged.json(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (6)
- .env.yarn
- tests/e2e/withJsExtension/.eslintrc.cjs
- tests/withJsconfig/.eslintrc.cjs
- tests/nestedPackageJson/.eslintrc.cjs
- tests/pnp/pnp.spec.ts
- tests/importXResolverV3/eslint.config.cjs
🔇 Additional comments (24)
vitest.config.ts (1)
1-1: Good addition of auto-import plugin for VitestAdding the auto-import plugin for Vitest functions is a good enhancement that will reduce boilerplate in test files. This change streamlines the testing experience by making common testing functions globally available without explicit imports.
Also applies to: 5-9
.prettierignore (2)
2-2: Appropriate ignore for auto-generated declaration fileAdding
/auto-imports.d.tsto the ignore list is appropriate since this file is auto-generated by the unplugin-auto-import plugin and shouldn't be manually formatted.
4-5:✅ Verification successful
Verify the updated ignore patterns
The pattern changes from more specific to more general (
.pnp.*) and from more general to more specific (*.svgvs previous**/*.svg). Please verify that:
- The broader
.pnp.*pattern won't accidentally ignore files that should be formatted- The more restrictive
*.svgpattern won't cause SVG files in subdirectories to be unexpectedly formatted if they should be ignored
🏁 Script executed:
#!/bin/bash # Find all .pnp.* files to verify the new ignore pattern covers all intended files echo "All .pnp.* files in the repository:" fd ".pnp\." # Find all SVG files to check if any exist in subdirectories that might be affected echo -e "\nAll SVG files in the repository:" fd ".svg$"Length of output: 270
Ignore pattern verification successful
- The
.pnp.*pattern did not match any files, indicating it isn’t inadvertently ignoring files that should be formatted.- The
*.svgpattern, although seemingly more restrictive than**/*.svg, correctly ignores SVG files in all directories (as confirmed by the presence of SVG files in subdirectories whose basenames match the pattern).eslint.config.js (1)
3-3:❓ Verification inconclusive
Verify the impact of switching from 'recommended' to 'base' ESLint config
This change switches from using the 'recommended' configuration to the 'base' configuration from '@1stg/eslint-config'. This could significantly alter the linting behavior of the project.
Let's check the difference between these configurations:
Also applies to: 13-13
🏁 Script executed:
#!/bin/bash # Check what these configurations actually contain by examining the package echo "Checking for '@1stg/eslint-config' in package.json" grep -A 5 "@1stg/eslint-config" package.json echo -e "\nTrying to find the configuration definitions:" # Look for exported configurations in the package npx eslint-config-info@latest '@1stg/eslint-config' # Alternative approach if the above doesn't work echo -e "\nAlternative check:" find node_modules/@1stg/eslint-config -type f -name "*.js" | xargs cat | grep -A 10 -B 10 "recommended\|base"Length of output: 718
Manual Verification Required: Confirm ESLint Configuration Impact
The changes in
eslint.config.js(Line 3 importing'base'instead of'recommended'and the corresponding spread change on Line 13) switch the ESLint config from a likely more opinionated setup to a minimal one. Our automated check on the contents of@1stg/eslint-configwas inconclusive (no output and errors were encountered), so we cannot determine the exact differences between the'base'and'recommended'configurations.Please verify manually that:
- The
'base'configuration provides the intended set of essential linting rules.- There are no unintended relaxations or tightenings in linting behaviors that might adversely impact the project.
Review the package documentation or source (if available) to ensure this switch aligns with your linting strategy.
.lintstagedrc.js (1)
1-7: Good enhancement for e2e test type checkingThis change improves the lint-staged configuration by adding specific TypeScript checking for e2e test files. Running the TypeScript compiler with
--noEmiton staged e2e test files will help catch type errors earlier in the development process, which aligns well with the PR objective of improving test infrastructure.tests/e2e/package.json (1)
1-5: Module System Update: Changing to ECMAScript Modules
The update of the"type"field from"commonjs"to"module"properly aligns the e2e tests with ECMAScript module semantics. Ensure that any related tooling, imports, or scripts are also updated for ESM compatibility.tests/e2e/withoutPaths/.eslintrc.cjs (1)
1-2: ESLint Configuration Inheritance
This new ESLint configuration file correctly leverages the base configuration by passing__dirnameto it. This ensures consistency and centralized management of ESLint rules across different test directories.tsconfig.json (1)
1-7: TypeScript OutDir Specification
The addition of the"outDir": "./lib"property withincompilerOptionsis a clear and useful enhancement. It directs the compiled TypeScript files to the designated output directory. Please verify that all build or deployment scripts reference the new output folder as needed.tsconfig.staged.json (1)
1-5: Staged TSConfig for Environment-specific Compilation
Introducingtsconfig.staged.jsonto extend the base configuration while excludingtests/e2e/absoluteAliasis a good move towards refining and isolating TypeScript behavior for staged processes. Just ensure that this configuration is integrated into your CI/build pipeline as intended.tests/e2e/withQuerystring/.eslintrc.cjs (1)
1-2: Consistent ESLint Configuration Setup
This file reuses the base ESLint configuration by requiring../base.eslintrc.cjswith the current directory. This consistent approach helps maintain uniform linting behavior across different testing environments.tests/e2e/absoluteAlias/tsconfig.json (1)
2-2: Good addition of base configuration.Extending from a base TypeScript configuration ensures consistency across the project and reduces duplication. This change follows best practices for TypeScript project organization.
tests/tsconfig.json (1)
1-7: Well-structured test configuration.This new configuration appropriately extends the base config while setting up the test-specific structure. Including both e2e and unit test directories along with auto-imports declaration aligns perfectly with the PR objective of splitting test cases.
tests/e2e/importXResolverV3/eslint.config.js (1)
6-6:✅ Verification successful
Verify the base configuration import.
Ensure that the imported
base_.jsreturns a function as it's being called in line 34 withbase_().rules.
🏁 Script executed:
#!/bin/bash # Verify the structure of the base ESLint configuration file cat tests/e2e/base.eslintrc.cjs || cat tests/base.eslintrc.cjsLength of output: 1078
Action: Confirm the base config import.
The ESLint base config located at
tests/e2e/base.eslintrc.cjsis verified to export a function. When this function is invoked (as in line 34 withbase_().rules), it returns an object containing therulesproperty as expected. No changes are needed in the import or its usage.tests/e2e/e2e.spec.ts (3)
1-6: Clean import organizationThe imports are well-structured with Node.js native modules separated from external dependencies. The code properly uses the newer 'node:' prefix for Node.js built-in modules, which is a good practice.
7-17: Well-structured test suite with directory filteringThe test suite is well-organized, using async/await pattern appropriately with the describe block. The directory filtering logic correctly excludes the snapshot directory and non-directory entries.
25-50: Robust ESLint configuration detection and executionThe test case properly detects whether to use a custom ESLint config or default settings based on the presence of an eslint.config.js file. It also correctly sets up the environment variables needed for different ESLint modes.
The snapshot matching approach is a good way to detect changes in ESLint output across test runs.
tsconfig.base.json (1)
4-5: Appropriate TypeScript configuration updatesThe addition of
rootDirandverbatimModuleSyntaxsettings is appropriate:
rootDir: "."establishes the base directory for TypeScript compilation, which is useful for properly organizing output files.verbatimModuleSyntax: falseallows more flexibility in imports without requiring explicit 'import type' syntax.These changes align well with a project that's reorganizing its test structure.
tests/e2e/withJsExtension/test.cjs (2)
7-7: Updated import path reflects new directory structureThe import path has been changed to accommodate the restructured test directory organization, which aligns with the PR's goal of splitting e2e and unit tests.
131-131: Path correction for dummy.js symlinkThe path to the dummy.js symlink has been updated to reflect the new relative positioning after the test reorganization.
package.json (3)
61-61: Simplified prepare scriptThe prepare script has been streamlined to focus on essential setup tasks without running yarn in nested directories. This change simplifies the development workflow.
63-63: Updated test script to include both e2e and unit testsThe test script now explicitly runs both a specific e2e test and the Vitest test suite, which aligns perfectly with the PR's goal of separating but maintaining both test types.
93-117: New dependencies to support testing infrastructureThe addition of
@pkgr/coreandunplugin-auto-importsupports the enhanced testing structure:
@pkgr/coreis used in the e2e tests for file operations (as seen in thetryFilefunction)unplugin-auto-importlikely helps with automatically importing test utilitiesThese dependencies are appropriate for the changes being made to the test infrastructure.
auto-imports.d.ts (1)
1-22: Well-structured global declarations for testing utilities.This auto-generated declaration file properly sets up global TypeScript types for Vitest testing functions, which will allow test files to use these functions without explicit imports. This is a good pattern that reduces boilerplate and supports the PR's objective of improving the testing infrastructure.
tests/e2e/withJsExtension/index.ts (1)
3-3: Good expansion of test cases.The updated extension in the import and the additional import paths will help test more scenarios for the resolver, improving test coverage for different file extension patterns and nested directory structures.
Also applies to: 6-9
JounQin
force-pushed
the
chore/split_e2e_unit
branch
2 times, most recently
from
7372c12 to
f1c8b6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tsconfig.base.json (1)
4-5: Module Syntax Configuration Verification.
The addition of"verbatimModuleSyntax": falsehere changes how module imports are emitted by the compiler. Since several options were removed in this base config, please verify that this setting appropriately aligns with your project’s module-handling requirements and that any derived configurations override it as intended if necessary.tests/e2e/e2e.spec.ts (3)
21-23: Consider checking the exit code of yarn command.The yarn installation is being executed but its result isn't being checked. If yarn fails, the test might proceed with an incomplete environment.
- if (tryFile('yarn.lock', false, absoluteDir)) { - await exec('yarn', ['--immutable']) - } + if (tryFile('yarn.lock', false, absoluteDir)) { + const { exitCode } = await exec('yarn', ['--immutable']) + if (exitCode !== 0) { + throw new Error(`Failed to install dependencies in ${absoluteDir}`) + } + }
44-44: Consider making NODE_OPTIONS configurable.The
--no-warnings=ESLintRCWarningoption is hardcoded, which might be too specific and could mask other useful warnings.- NODE_OPTIONS: '--no-warnings=ESLintRCWarning', + NODE_OPTIONS: process.env.E2E_NODE_OPTIONS || '--no-warnings=ESLintRCWarning',
7-24: Consider using describe.each for cleaner test organization.The current implementation uses a for-loop to dynamically create tests, but Vitest supports a more declarative approach with describe.each.
Here's how you could refactor this part:
describe('e2e cases', () => { const { dirname } = import.meta beforeAll(async () => { const dirents = await fs.readdir(dirname, { withFileTypes: true }) const testDirs = dirents .filter(dirent => dirent.isDirectory() && dirent.name !== '__snapshots__') .map(dirent => dirent.name) // Install dependencies for all test directories with yarn.lock files for (const dirName of testDirs) { const absoluteDir = path.resolve(dirname, dirName) if (tryFile('yarn.lock', false, absoluteDir)) { await exec('yarn', ['--immutable'], { nodeOptions: { cwd: absoluteDir } }) } } }) it.each(/* array of test directories */)( 'should exec eslint successfully for %s', async (dirName) => { // Test implementation } ) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
tests/e2e/__snapshots__/e2e.spec.ts.snapis excluded by!**/*.snaptests/e2e/nestedPackageJson/yarn.lockis excluded by!**/yarn.lock,!**/*.locktests/e2e/withQuerystring/image.svgis excluded by!**/*.svgtests/e2e/withQuerystring/subfolder/image.svgis excluded by!**/*.svgtests/unit/pnp/.yarn/cache/lodash.zip-npm-4.2.0-5299417ec8-cb06530d81.zipis excluded by!**/.yarn/**,!**/*.ziptests/unit/pnp/.yarn/releases/yarn-4.8.0.cjsis excluded by!**/.yarn/**tests/unit/pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
.env.yarn(0 hunks).lintstagedrc.js(1 hunks).prettierignore(1 hunks)auto-imports.d.ts(1 hunks)eslint.config.js(2 hunks)package.json(3 hunks)tests/e2e/absoluteAlias/tsconfig.json(1 hunks)tests/e2e/e2e.spec.ts(1 hunks)tests/e2e/importXResolverV3/eslint.config.js(1 hunks)tests/e2e/package.json(1 hunks)tests/e2e/withJsExtension/.eslintrc.cjs(0 hunks)tests/e2e/withJsExtension/index.ts(1 hunks)tests/e2e/withJsExtension/test.cjs(2 hunks)tests/e2e/withQuerystring/.eslintrc.cjs(1 hunks)tests/e2e/withoutPaths/.eslintrc.cjs(1 hunks)tests/importXResolverV3/eslint.config.cjs(0 hunks)tests/nestedPackageJson/.eslintrc.cjs(0 hunks)tests/pnp/pnp.spec.ts(0 hunks)tests/tsconfig.json(1 hunks)tests/unit/unit.spec.ts(1 hunks)tests/withJsconfig/.eslintrc.cjs(0 hunks)tsconfig.base.json(1 hunks)tsconfig.json(1 hunks)tsconfig.staged.json(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (6)
- .env.yarn
- tests/e2e/withJsExtension/.eslintrc.cjs
- tests/nestedPackageJson/.eslintrc.cjs
- tests/withJsconfig/.eslintrc.cjs
- tests/pnp/pnp.spec.ts
- tests/importXResolverV3/eslint.config.cjs
🚧 Files skipped from review as they are similar to previous changes (14)
- tests/e2e/package.json
- tests/e2e/absoluteAlias/tsconfig.json
- tsconfig.staged.json
- tests/e2e/withQuerystring/.eslintrc.cjs
- tests/tsconfig.json
- vitest.config.ts
- tests/e2e/withoutPaths/.eslintrc.cjs
- tests/e2e/importXResolverV3/eslint.config.js
- eslint.config.js
- .lintstagedrc.js
- .prettierignore
- auto-imports.d.ts
- tests/e2e/withJsExtension/test.cjs
- tests/e2e/withJsExtension/index.ts
🧰 Additional context used
🪛 GitHub Check: Lint and Test with Node.js 20 on ubuntu-latest
tests/e2e/e2e.spec.ts
[failure] 49-49: tests/e2e/e2e.spec.ts > e2e cases > should exec eslint successfully
Error: Snapshot e2e cases > should exec eslint successfully > nestedPackageJson 1 mismatched
- Expected
-
Received
{
- "exitCode": 0,
- "exitCode": 1,
"stderr": "",
- "stdout": "",
- "stdout": "
- /tests/e2e/nestedPackageJson/test.ts
- 1:25 error Unable to resolve path to module '@atlaskit/pragmatic-drag-and-drop/combine' import-x/no-unresolved
- ✖ 1 problem (1 error, 0 warnings)
- ",
}
❯ tests/e2e/e2e.spec.ts:49:9
🪛 GitHub Check: Lint and Test with Node.js 22 on macos-latest
tests/e2e/e2e.spec.ts
[failure] 49-49: tests/e2e/e2e.spec.ts > e2e cases > should exec eslint successfully
Error: Snapshot e2e cases > should exec eslint successfully > nestedPackageJson 1 mismatched
- Expected
-
Received
{
- "exitCode": 0,
- "exitCode": 1,
"stderr": "",
- "stdout": "",
- "stdout": "
- /tests/e2e/nestedPackageJson/test.ts
- 1:25 error Unable to resolve path to module '@atlaskit/pragmatic-drag-and-drop/combine' import-x/no-unresolved
- ✖ 1 problem (1 error, 0 warnings)
- ",
}
❯ tests/e2e/e2e.spec.ts:49:9
🪛 GitHub Check: Lint and Test with Node.js 20 on macos-latest
tests/e2e/e2e.spec.ts
[failure] 49-49: tests/e2e/e2e.spec.ts > e2e cases > should exec eslint successfully
Error: Snapshot e2e cases > should exec eslint successfully > nestedPackageJson 1 mismatched
- Expected
-
Received
{
- "exitCode": 0,
- "exitCode": 1,
"stderr": "",
- "stdout": "",
- "stdout": "
- /tests/e2e/nestedPackageJson/test.ts
- 1:25 error Unable to resolve path to module '@atlaskit/pragmatic-drag-and-drop/combine' import-x/no-unresolved
- ✖ 1 problem (1 error, 0 warnings)
- ",
}
❯ tests/e2e/e2e.spec.ts:49:9
🪛 GitHub Check: Lint and Test with Node.js 22 on windows-latest
tests/e2e/e2e.spec.ts
[failure] 49-49: tests/e2e/e2e.spec.ts > e2e cases > should exec eslint successfully
Error: Snapshot e2e cases > should exec eslint successfully > nestedPackageJson 1 mismatched
- Expected
-
Received
{
- "exitCode": 0,
- "exitCode": 1,
"stderr": "",
- "stdout": "",
- "stdout": "
- /tests/e2e/nestedPackageJson/test.ts
- 1:25 error Unable to resolve path to module '@atlaskit/pragmatic-drag-and-drop/combine' import-x/no-unresolved
- ✖ 1 problem (1 error, 0 warnings)
- ",
}
❯ tests/e2e/e2e.spec.ts:49:9
🪛 GitHub Check: Lint and Test with Node.js 18 on windows-latest
tests/e2e/e2e.spec.ts
[failure] 49-49: tests/e2e/e2e.spec.ts > e2e cases > should exec eslint successfully
Error: Snapshot e2e cases > should exec eslint successfully > nestedPackageJson 1 mismatched
- Expected
-
Received
{
- "exitCode": 0,
- "exitCode": 1,
"stderr": "",
- "stdout": "",
- "stdout": "
- /tests/e2e/nestedPackageJson/test.ts
- 1:25 error Unable to resolve path to module '@atlaskit/pragmatic-drag-and-drop/combine' import-x/no-unresolved
- ✖ 1 problem (1 error, 0 warnings)
- ",
}
❯ tests/e2e/e2e.spec.ts:49:9
🪛 GitHub Check: Lint and Test with Node.js 18 on macos-latest
tests/e2e/e2e.spec.ts
[failure] 49-49: tests/e2e/e2e.spec.ts > e2e cases > should exec eslint successfully
Error: Snapshot e2e cases > should exec eslint successfully > nestedPackageJson 1 mismatched
- Expected
-
Received
{
- "exitCode": 0,
- "exitCode": 1,
"stderr": "",
- "stdout": "",
- "stdout": "
- /tests/e2e/nestedPackageJson/test.ts
- 1:25 error Unable to resolve path to module '@atlaskit/pragmatic-drag-and-drop/combine' import-x/no-unresolved
- ✖ 1 problem (1 error, 0 warnings)
- ",
}
❯ tests/e2e/e2e.spec.ts:49:9
🪛 GitHub Check: Lint and Test with Node.js 22 on ubuntu-latest
tests/e2e/e2e.spec.ts
[failure] 49-49: tests/e2e/e2e.spec.ts > e2e cases > should exec eslint successfully
Error: Snapshot e2e cases > should exec eslint successfully > nestedPackageJson 1 mismatched
- Expected
-
Received
{
- "exitCode": 0,
- "exitCode": 1,
"stderr": "",
- "stdout": "",
- "stdout": "
- /tests/e2e/nestedPackageJson/test.ts
- 1:25 error Unable to resolve path to module '@atlaskit/pragmatic-drag-and-drop/combine' import-x/no-unresolved
- ✖ 1 problem (1 error, 0 warnings)
- ",
}
❯ tests/e2e/e2e.spec.ts:49:9
🪛 GitHub Check: Lint and Test with Node.js 20 on windows-latest
tests/e2e/e2e.spec.ts
[failure] 49-49: tests/e2e/e2e.spec.ts > e2e cases > should exec eslint successfully
Error: Snapshot e2e cases > should exec eslint successfully > nestedPackageJson 1 mismatched
- Expected
-
Received
{
- "exitCode": 0,
- "exitCode": 1,
"stderr": "",
- "stdout": "",
- "stdout": "
- /tests/e2e/nestedPackageJson/test.ts
- 1:25 error Unable to resolve path to module '@atlaskit/pragmatic-drag-and-drop/combine' import-x/no-unresolved
- ✖ 1 problem (1 error, 0 warnings)
- ",
}
❯ tests/e2e/e2e.spec.ts:49:9
🪛 GitHub Check: Lint and Test with Node.js 18 on ubuntu-latest
tests/e2e/e2e.spec.ts
[failure] 49-49: tests/e2e/e2e.spec.ts > e2e cases > should exec eslint successfully
Error: Snapshot e2e cases > should exec eslint successfully > nestedPackageJson 1 mismatched
- Expected
-
Received
{
- "exitCode": 0,
- "exitCode": 1,
"stderr": "",
- "stdout": "",
- "stdout": "
- /tests/e2e/nestedPackageJson/test.ts
- 1:25 error Unable to resolve path to module '@atlaskit/pragmatic-drag-and-drop/combine' import-x/no-unresolved
- ✖ 1 problem (1 error, 0 warnings)
- ",
}
❯ tests/e2e/e2e.spec.ts:49:9
🪛 GitHub Actions: CI
tests/e2e/e2e.spec.ts
[error] 49-49: Snapshot e2e cases > should exec eslint successfully > nestedPackageJson 1 mismatched. Expected exitCode: 0, Received exitCode: 1. Error: Unable to resolve path to module '@atlaskit/pragmatic-drag-and-drop/combine' (import-x/no-unresolved)
🔇 Additional comments (7)
package.json (4)
61-61: Revise Prepare Script ChangesThe
preparescript now runs:"prepare": "simple-git-hooks && yarn-berry-deduplicate || exit 0"This change removes the previous directory change and
yarncall. Please verify that this omission is intentional and that none of the setup steps in thetests/nestedPackageJsondirectory are required anymore. Additionally, ensure that using|| exit 0as a fallback does not inadvertently mask potential issues with these commands.
63-63: Clarify Test Script FlowThe updated
testscript now executes:"test": "node tests/e2e/withJsExtension/test.cjs && vitest --run"This clearly separates end-to-end tests from unit tests by running the specified e2e test file first followed by running Vitest. Confirm that the sequential execution is intended, as any failure in the e2e tests will prevent the unit tests from running. If independent execution is desired, consider whether a parallel or grouped execution might be more appropriate.
93-93: New Dev Dependency: @pkgr/coreThe addition of
"@pkgr/core": "^0.2.0"indevDependenciessuggests that new build or packaging tasks might be leveraging this dependency. Please ensure that its usage is properly documented and integrated within the relevant scripts or configuration files.
117-117: New Dev Dependency: unplugin-auto-importThe dependency
"unplugin-auto-import": "^19.1.2"is added to support automatic imports (likely for testing utilities or global declarations). Verify that the corresponding configuration (e.g., in yourvitest.config.tsor similar configuration files) has been updated accordingly, and confirm that this addition aligns with your intended testing setup improvements.tsconfig.json (2)
3-6: Compiler Options Override Check.
In this configuration, you’ve reintroduced"outDir": "./lib"and explicitly set"verbatimModuleSyntax": trueunder"compilerOptions". Note that this diverges from the base configuration (which sets"verbatimModuleSyntax": false). Please confirm that this override is deliberate and that the intended module syntax behavior is achieved during the build process.
7-8: Include Paths Verification.
The"include": ["src", "eslint.config.js"]setting appears to restrict compilation to specific sources. Ensure that excluding test files or other directories here is intentional and that it aligns with the overall testing and build strategy outlined by the PR objectives.tests/unit/unit.spec.ts (1)
1-34: Well-structured unit test for PnP functionality.This is a good implementation of a unit test that verifies the TypeScript import resolver's compatibility with Yarn's Plug'n'Play. The test properly sets up the environment, executes the resolver with relevant modules, and validates the results with snapshots.
JounQin
force-pushed
the
chore/split_e2e_unit
branch
from
f1c8b6d to
4507350
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
package.json (1)
61-61: Review theprepareScript for Error Suppression.
The updatedpreparescript now only runs:"prepare": "simple-git-hooks && yarn-berry-deduplicate || exit 0"Please verify if intentionally using
|| exit 0to suppress errors fromsimple-git-hooksoryarn-berry-deduplicate. This pattern may mask critical failures, so if errors should be caught instead, consider adjusting the logic or adding logging.tests/unit/unit.spec.ts (6)
6-8: Consider documenting the PnP testing purposeThe PnP (Plug'n'Play) testing setup could benefit from a brief comment explaining what aspects of the resolver are being tested with PnP and why this is important.
9-14: Add error handling for Yarn installationThe
execcall for Yarn installation lacks error handling. Consider adding specific error handling to provide clearer feedback if the installation fails.- await exec('yarn', ['install'], { + try { + await exec('yarn', ['install'], { + nodeOptions: { + cwd: pnpDir, + }, + }) + } catch (error) { + console.error('Failed to install PnP dependencies:', error) + throw error + } - nodeOptions: { - cwd: pnpDir, - }, - })
16-19: Consider caching the resolver instanceSince the resolver is created without arguments, it could potentially be shared across multiple tests if more tests are added in the future.
33-34: Consider documentation for the long timeoutThe 10-second timeout is appropriate for a test involving file operations, but a brief comment explaining why this longer timeout is necessary would improve maintainability.
- }, 10_000) + }, 10_000) // Longer timeout for Yarn install operations
1-34: Add cleanup step after test completionThe test creates or modifies files in the PnP directory but doesn't clean up afterward. Consider adding a cleanup step to restore the environment to its original state, especially important for CI environments.
+ afterAll(async () => { + // Clean up PnP installation artifacts if needed + try { + await exec('rm', ['-rf', path.resolve(pnpDir, '.yarn')], { + nodeOptions: { + cwd: pnpDir, + }, + }) + } catch (error) { + console.warn('Failed to clean up PnP artifacts:', error) + } + })
1-34: Consider optimization for repeated test runsThe test currently runs
yarn installon every execution. Consider adding a check to skip installation if the PnP setup already exists, which would speed up test execution during development.import { exec } from 'tinyexec' + import fs from 'node:fs' import { createTypeScriptImportResolver } from 'eslint-import-resolver-typescript' import path from 'node:path' describe('createTypeScriptImportResolver', () => { const pnpDir = path.resolve(import.meta.dirname, 'pnp') it('should work with pnp', async () => { + const pnpSetupExists = fs.existsSync(path.resolve(pnpDir, '.pnp.cjs')) + + if (!pnpSetupExists) { await exec('yarn', ['install'], { nodeOptions: { cwd: pnpDir, }, }) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
tests/e2e/__snapshots__/e2e.spec.ts.snapis excluded by!**/*.snaptests/e2e/nestedPackageJson/yarn.lockis excluded by!**/yarn.lock,!**/*.locktests/e2e/withQuerystring/image.svgis excluded by!**/*.svgtests/e2e/withQuerystring/subfolder/image.svgis excluded by!**/*.svgtests/unit/pnp/.yarn/cache/lodash.zip-npm-4.2.0-5299417ec8-cb06530d81.zipis excluded by!**/.yarn/**,!**/*.ziptests/unit/pnp/.yarn/releases/yarn-4.8.0.cjsis excluded by!**/.yarn/**tests/unit/pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
.env.yarn(0 hunks).lintstagedrc.js(1 hunks).prettierignore(1 hunks)auto-imports.d.ts(1 hunks)eslint.config.js(2 hunks)package.json(3 hunks)tests/e2e/absoluteAlias/tsconfig.json(1 hunks)tests/e2e/e2e.spec.ts(1 hunks)tests/e2e/importXResolverV3/eslint.config.js(1 hunks)tests/e2e/package.json(1 hunks)tests/e2e/withJsExtension/.eslintrc.cjs(0 hunks)tests/e2e/withJsExtension/index.ts(1 hunks)tests/e2e/withJsExtension/test.cjs(2 hunks)tests/e2e/withQuerystring/.eslintrc.cjs(1 hunks)tests/e2e/withoutPaths/.eslintrc.cjs(1 hunks)tests/importXResolverV3/eslint.config.cjs(0 hunks)tests/nestedPackageJson/.eslintrc.cjs(0 hunks)tests/pnp/pnp.spec.ts(0 hunks)tests/tsconfig.json(1 hunks)tests/unit/unit.spec.ts(1 hunks)tests/withJsconfig/.eslintrc.cjs(0 hunks)tsconfig.base.json(1 hunks)tsconfig.json(1 hunks)tsconfig.staged.json(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (6)
- .env.yarn
- tests/withJsconfig/.eslintrc.cjs
- tests/e2e/withJsExtension/.eslintrc.cjs
- tests/nestedPackageJson/.eslintrc.cjs
- tests/pnp/pnp.spec.ts
- tests/importXResolverV3/eslint.config.cjs
🚧 Files skipped from review as they are similar to previous changes (16)
- tests/e2e/withoutPaths/.eslintrc.cjs
- tests/e2e/absoluteAlias/tsconfig.json
- tests/e2e/withQuerystring/.eslintrc.cjs
- tsconfig.json
- tests/e2e/importXResolverV3/eslint.config.js
- tests/tsconfig.json
- eslint.config.js
- tests/e2e/e2e.spec.ts
- tsconfig.base.json
- .prettierignore
- tests/e2e/package.json
- tests/e2e/withJsExtension/test.cjs
- .lintstagedrc.js
- vitest.config.ts
- tests/e2e/withJsExtension/index.ts
- auto-imports.d.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
🔇 Additional comments (6)
tsconfig.staged.json (1)
1-7: Well-structured new TypeScript configuration.
The file correctly extends from the base configuration (./tsconfig.base), specifies therootDiras".", and excludes"tests/e2e/absoluteAlias"from compilation. Please verify that the exclusion of theabsoluteAliasfolder is in line with the intended separation of end-to-end and unit tests.package.json (3)
63-63: Validate the Updated Test Command Workflow.
Thetestscript has been updated to:"test": "node tests/e2e/withJsExtension/test.cjs && vitest run"This change indicates that an end-to-end test is executed before running unit tests with Vitest, aligning with the PR objective of splitting e2e and unit cases. Please confirm that this sequential approach is intended and that it covers all necessary test scenarios.
93-93: Confirm the Addition of@pkgr/core.
A new devDependency has been added:"@pkgr/core": "^0.2.0"Ensure that this dependency is required for our build/test processes and that its versioning aligns with project expectations.
117-117: Confirm the Addition ofunplugin-auto-import.
A new devDependency has been introduced:"unplugin-auto-import": "^19.1.2"This dependency supports auto-import functionality (likely configured in
vitest.config.ts). Please verify that this version meets our requirements and is properly integrated into our testing setup.tests/unit/unit.spec.ts (2)
1-5: Imports look appropriate for the testThe imports provide all necessary functionality:
execfor running Yarn commands, the resolver function being tested, and path utilities.
20-32: Use of inline snapshots is appropriateThe inline snapshots effectively verify the resolver's behavior with PnP modules. Note that the paths in the snapshots use
<ROOT>which appears to be a path normalization helper, which is good practice.
JounQin
force-pushed
the
chore/split_e2e_unit
branch
from
4507350 to
628e792
Compare
There was a problem hiding this comment.
Pull Request Overview
A PR to split end-to-end and unit tests by updating and refining related ESLint and lint-staged configurations.
- Updated the e2e ESLint configuration by replacing node import rules with an import order rule and modifying file override patterns.
- Renamed and updated imported configuration variables in both eslint.config.js and .lintstagedrc.js for consistency.
- Added a custom tsc command for linting e2e tests with absolute aliases.
Reviewed Changes
Copilot reviewed 172 out of 178 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/e2e/base.eslintrc.cjs | Replaced node import rules with a new rule and updated file override pattern. |
| eslint.config.js | Renamed the imported ESLint configuration from "recommended" to "base". |
| .lintstagedrc.js | Modified the lint-staged config to include a custom command for e2e tests. |
Files not reviewed (6)
- .env.yarn: Language not supported
- .prettierignore: Language not supported
- .vscode/settings.json: Language not supported
- package.json: Language not supported
- tests/e2e/snapshots/e2e.spec.ts.snap: Language not supported
- tests/e2e/absoluteAlias/tsconfig.json: Language not supported
Comments suppressed due to low confidence (3)
tests/e2e/base.eslintrc.cjs:32
- Ensure that the removal of 'node/no-extraneous-import' and 'node/no-missing-import' is intentional, as this change could affect import validation. Consider adding an inline comment for clarity if this change is deliberate.
'node/no-extraneous-import': 0,
eslint.config.js:3
- [nitpick] The variable name was changed from 'recommended' to 'base'. Verify that this naming change is consistently reflected in all related documentation and configuration references.
import base from '@1stg/eslint-config'
.lintstagedrc.js:5
- [nitpick] Verify that the glob pattern for 'tests/e2e/absoluteAlias/**/*.ts?(x)' covers all intended files. Adjust the pattern if necessary to ensure that no relevant test files are omitted.
'tests/e2e/absoluteAlias/**/*.ts?(x)': () => 'tsc -p tests/e2e/absoluteAlias --noEmit',
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
package.json (2)
61-61: Refine the "prepare" script.
The updated "prepare" script now only runssimple-git-hooks && yarn-berry-deduplicate || exit 0. This simplification is acceptable if the removal of the directory change and extra commands was intentional. However, consider if silently swallowing errors (via|| exit 0) might hide issues that you want to catch during preparation.
63-63: Review the "test" script execution order.
The "test" script now executesnode tests/e2e/withJsExtension/test.cjs && vitest run, which sequentially runs the end-to-end tests before the unit tests. Ensure that this ordering is intentional, as any failure in the e2e tests will prevent the unit tests from running. If sequential execution is desired, then this is fine; otherwise, you might consider running them concurrently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
tests/e2e/__snapshots__/e2e.spec.ts.snapis excluded by!**/*.snaptests/e2e/nestedPackageJson/yarn.lockis excluded by!**/yarn.lock,!**/*.locktests/e2e/withQuerystring/image.svgis excluded by!**/*.svgtests/e2e/withQuerystring/subfolder/image.svgis excluded by!**/*.svgtests/unit/pnp/.yarn/cache/lodash.zip-npm-4.2.0-5299417ec8-cb06530d81.zipis excluded by!**/.yarn/**,!**/*.ziptests/unit/pnp/.yarn/releases/yarn-4.8.0.cjsis excluded by!**/.yarn/**tests/unit/pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (27)
.env.yarn(0 hunks).lintstagedrc.js(1 hunks).prettierignore(1 hunks).vscode/settings.json(0 hunks)auto-imports.d.ts(1 hunks)eslint.config.js(2 hunks)package.json(3 hunks)tests/e2e/absoluteAlias/tsconfig.json(1 hunks)tests/e2e/base.eslintrc.cjs(1 hunks)tests/e2e/e2e.spec.ts(1 hunks)tests/e2e/importXResolverV3/eslint.config.js(1 hunks)tests/e2e/package.json(1 hunks)tests/e2e/withJsExtension/.eslintrc.cjs(0 hunks)tests/e2e/withJsExtension/index.ts(1 hunks)tests/e2e/withJsExtension/test.cjs(2 hunks)tests/e2e/withQuerystring/.eslintrc.cjs(1 hunks)tests/e2e/withoutPaths/.eslintrc.cjs(1 hunks)tests/importXResolverV3/eslint.config.cjs(0 hunks)tests/nestedPackageJson/.eslintrc.cjs(0 hunks)tests/pnp/pnp.spec.ts(0 hunks)tests/tsconfig.json(1 hunks)tests/unit/unit.spec.ts(1 hunks)tests/withJsconfig/.eslintrc.cjs(0 hunks)tsconfig.base.json(1 hunks)tsconfig.json(1 hunks)tsconfig.staged.json(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (7)
- .vscode/settings.json
- tests/e2e/withJsExtension/.eslintrc.cjs
- .env.yarn
- tests/withJsconfig/.eslintrc.cjs
- tests/pnp/pnp.spec.ts
- tests/importXResolverV3/eslint.config.cjs
- tests/nestedPackageJson/.eslintrc.cjs
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/e2e/package.json
- tests/e2e/withoutPaths/.eslintrc.cjs
- tests/e2e/absoluteAlias/tsconfig.json
- tests/e2e/withQuerystring/.eslintrc.cjs
- tsconfig.staged.json
- tests/tsconfig.json
- tests/e2e/importXResolverV3/eslint.config.js
- tsconfig.json
- eslint.config.js
- tsconfig.base.json
- vitest.config.ts
- .lintstagedrc.js
- tests/e2e/e2e.spec.ts
- .prettierignore
- tests/e2e/withJsExtension/test.cjs
- auto-imports.d.ts
- tests/unit/unit.spec.ts
- tests/e2e/withJsExtension/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
🔇 Additional comments (4)
tests/e2e/base.eslintrc.cjs (2)
35-43: Good improvement: Adding the import-x/order rule enhances code quality.Adding the
import-x/orderrule with alphabetization and newlines between import groups will ensure consistent import ordering across the codebase. This improves readability and maintainability.
47-47: Good pattern update to include TypeScript CommonJS files.Changing the file pattern from a potential dot-prefixed pattern to explicitly include both
.cjsand.ctsfiles ensures proper application of the ESLint rules to TypeScript CommonJS files.package.json (2)
93-93: New devDependency: "@pkgr/core".
The addition of"@pkgr/core": "^0.2.0"in the devDependencies appears valid. Please confirm that the new dependency integrates well with your build or test workflows and that the version constraint aligns with your project's needs.
117-117: New devDependency: "unplugin-auto-import".
Adding"unplugin-auto-import": "^19.1.2"will help facilitate auto-imports for the Vitest testing utilities, as indicated by your TypeScript declarations. This is a useful enhancement that aligns with your testing infrastructure improvements.
JounQin
force-pushed
the
chore/split_e2e_unit
branch
from
628e792 to
5470f74
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/unit.spec.ts (2)
9-41: Consider optimizing the PnP test for reliability and speedThe test runs an actual
yarn installcommand which makes it:
- Dependent on network connectivity
- Slower than a typical unit test (hence the 10s timeout)
- Potentially brittle if package versions change
Consider one of these approaches:
- Mock the filesystem interactions to avoid actual yarn install
- Use a pre-built fixture that can be copied rather than generated
- Add cleanup logic to ensure the test environment is reset between runs
- Consider caching the yarn install step to avoid redundant installations
describe('createTypeScriptImportResolver', () => { const pnpDir = path.resolve(import.meta.dirname, 'pnp') + // Consider adding beforeAll/afterAll hooks for setup/cleanup + beforeAll(async () => { + // Check if already installed to avoid redundant operations + if (!existsSync(path.resolve(pnpDir, '.pnp.cjs'))) { + await exec('yarn', ['install'], { + nodeOptions: { + cwd: pnpDir, + }, + }) + } + }) it( 'should work with pnp', async () => { - await exec('yarn', ['install'], { - nodeOptions: { - cwd: pnpDir, - }, - }) const resolver = createTypeScriptImportResolver()
25-37: Consider using path-independent assertionsThe inline snapshots contain absolute paths that might vary across different environments, potentially causing test failures.
Instead of using inline snapshots with specific paths, consider validating just the essential properties:
- expect(resolver.resolve('pnpapi', testfile)).toMatchInlineSnapshot(` - { - "found": true, - "path": "<ROOT>/tests/unit/pnp/.pnp.cjs", - } - `) + const pnpapiResult = resolver.resolve('pnpapi', testfile); + expect(pnpapiResult.found).toBe(true); + expect(pnpapiResult.path).toMatch(/\.pnp\.cjs$/); - expect(resolver.resolve('lodash.zip', testfile)).toMatchInlineSnapshot(` - { - "found": true, - "path": "<ROOT>/tests/unit/pnp/.yarn/cache/lodash.zip-npm-4.2.0-5299417ec8-cb06530d81.zip/node_modules/lodash.zip/index.js", - } - `) + const lodashResult = resolver.resolve('lodash.zip', testfile); + expect(lodashResult.found).toBe(true); + expect(lodashResult.path).toMatch(/lodash\.zip.*index\.js$/);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
tests/e2e/__snapshots__/e2e.spec.ts.snapis excluded by!**/*.snaptests/e2e/nestedPackageJson/yarn.lockis excluded by!**/yarn.lock,!**/*.locktests/e2e/withQuerystring/image.svgis excluded by!**/*.svgtests/e2e/withQuerystring/subfolder/image.svgis excluded by!**/*.svgtests/unit/pnp/.yarn/cache/lodash.zip-npm-4.2.0-5299417ec8-cb06530d81.zipis excluded by!**/.yarn/**,!**/*.ziptests/unit/pnp/.yarn/releases/yarn-4.8.0.cjsis excluded by!**/.yarn/**tests/unit/pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (27)
.env.yarn(0 hunks).lintstagedrc.js(1 hunks).prettierignore(1 hunks).vscode/settings.json(0 hunks)auto-imports.d.ts(1 hunks)eslint.config.js(2 hunks)package.json(3 hunks)tests/e2e/absoluteAlias/tsconfig.json(1 hunks)tests/e2e/base.eslintrc.cjs(1 hunks)tests/e2e/e2e.spec.ts(1 hunks)tests/e2e/importXResolverV3/eslint.config.js(1 hunks)tests/e2e/package.json(1 hunks)tests/e2e/withJsExtension/.eslintrc.cjs(0 hunks)tests/e2e/withJsExtension/index.ts(1 hunks)tests/e2e/withJsExtension/test.cjs(2 hunks)tests/e2e/withQuerystring/.eslintrc.cjs(1 hunks)tests/e2e/withoutPaths/.eslintrc.cjs(1 hunks)tests/importXResolverV3/eslint.config.cjs(0 hunks)tests/nestedPackageJson/.eslintrc.cjs(0 hunks)tests/pnp/pnp.spec.ts(0 hunks)tests/tsconfig.json(1 hunks)tests/unit/unit.spec.ts(1 hunks)tests/withJsconfig/.eslintrc.cjs(0 hunks)tsconfig.base.json(1 hunks)tsconfig.json(1 hunks)tsconfig.staged.json(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (7)
- tests/nestedPackageJson/.eslintrc.cjs
- .env.yarn
- tests/e2e/withJsExtension/.eslintrc.cjs
- .vscode/settings.json
- tests/withJsconfig/.eslintrc.cjs
- tests/importXResolverV3/eslint.config.cjs
- tests/pnp/pnp.spec.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- tests/e2e/package.json
- tests/e2e/absoluteAlias/tsconfig.json
- tests/e2e/withoutPaths/.eslintrc.cjs
- tsconfig.staged.json
- tests/e2e/withQuerystring/.eslintrc.cjs
- .lintstagedrc.js
- vitest.config.ts
- tsconfig.json
- tests/tsconfig.json
- tsconfig.base.json
- tests/e2e/importXResolverV3/eslint.config.js
- eslint.config.js
- tests/e2e/e2e.spec.ts
- .prettierignore
- tests/e2e/base.eslintrc.cjs
- auto-imports.d.ts
- tests/e2e/withJsExtension/test.cjs
🔇 Additional comments (4)
tests/e2e/withJsExtension/index.ts (1)
3-3: Expanded import test cases for TSX files.The addition of
import './tsxImportee.js'complements the existing import of the JSX version, following the established pattern used for other import tests in this file. This ensures testing coverage for both extension resolution scenarios.package.json (3)
63-63: Test script now correctly separates e2e and unit testsThe updated test script now explicitly runs the e2e test separately before running vitest, which aligns with the PR objective of splitting e2e and unit tests.
This separation makes the testing process more organized and potentially allows for more targeted test runs in the future.
93-93:✅ Verification successful
New dependencies support the updated testing structure
The addition of these development dependencies supports the reorganized test structure:
@pkgr/core: Provides package management utilitiesunplugin-auto-import: Enables automatic imports of testing utilitiesThese dependencies help streamline the testing infrastructure, particularly with auto-imports reducing the need for repetitive import statements in test files.
Also applies to: 117-117
🏁 Script executed:
#!/bin/bash # Check how unplugin-auto-import is configured in the project fd -t f -e ts -e js "vitest.config" .Length of output: 56
Approved: Updated testing dependencies integrated successfully
The changes in package.json (lines 93 and 117) introducing the
@pkgr/coreandunplugin-auto-importdependencies are correctly supporting the updated testing structure. We confirmed the presence of a Vitest configuration file (vitest.config.ts), which indicates that the auto-import functionality is properly configured for the tests.
61-61:❓ Verification inconclusive
Simplified prepare script removes nested package test
The prepare script has been simplified to only run git hooks and yarn deduplication.
Are the nested package JSON tests that were previously run during prepare now integrated into the regular test suite? Let's check:
🏁 Script executed:
#!/bin/bash # Check if nestedPackageJson tests are now handled elsewhere fd -t d "nestedPackageJson" tests grep -r "nestedPackageJson" tests --include="*.ts" --include="*.js"Length of output: 130
Action: Verify Integration of Nested Package JSON Tests
The simplified prepare script now only runs git hooks and deduplication. However, we still see an existing directory at
tests/e2e/nestedPackageJson/, and our grep search didn’t reveal any explicit references to “nestedPackageJson” inside test files. This makes it unclear if those tests are now being executed as part of the regular test suite.
- Verify that the tests in
tests/e2e/nestedPackageJson/are being picked up and run by your main test runner (e.g., check your test configuration likejest.config.jsor similar).- If these tests are meant to be integrated into the regular suite, ensure that their execution is confirmed via the test runner’s output.
- Otherwise, confirm that it is intentional to remove them from the prepare script and that their removal won’t reduce test coverage.
Please manually verify the integration of these tests to ensure the intended behavior.
JounQin
force-pushed
the
chore/split_e2e_unit
branch
from
5470f74 to
40fd25d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
61-61: Prepare Script Simplification
Thepreparescript has been simplified to runsimple-git-hooks && yarn-berry-deduplicate || exit 0. Note that the|| exit 0ensures that even if one of these commands fails, the script exits with a success status. Please confirm that ignoring potential failures here is intentional; if not, consider removing the fallback so that errors propagate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
tests/e2e/__snapshots__/e2e.spec.ts.snapis excluded by!**/*.snaptests/e2e/nestedPackageJson/yarn.lockis excluded by!**/yarn.lock,!**/*.locktests/e2e/withQuerystring/image.svgis excluded by!**/*.svgtests/e2e/withQuerystring/subfolder/image.svgis excluded by!**/*.svgtests/unit/pnp/.yarn/cache/lodash.zip-npm-4.2.0-5299417ec8-cb06530d81.zipis excluded by!**/.yarn/**,!**/*.ziptests/unit/pnp/.yarn/releases/yarn-4.8.0.cjsis excluded by!**/.yarn/**tests/unit/pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (27)
.env.yarn(0 hunks).lintstagedrc.js(1 hunks).prettierignore(1 hunks).vscode/settings.json(0 hunks)auto-imports.d.ts(1 hunks)eslint.config.js(2 hunks)package.json(3 hunks)tests/e2e/absoluteAlias/tsconfig.json(1 hunks)tests/e2e/base.eslintrc.cjs(1 hunks)tests/e2e/e2e.spec.ts(1 hunks)tests/e2e/importXResolverV3/eslint.config.js(1 hunks)tests/e2e/package.json(1 hunks)tests/e2e/withJsExtension/.eslintrc.cjs(0 hunks)tests/e2e/withJsExtension/index.ts(1 hunks)tests/e2e/withJsExtension/test.cjs(2 hunks)tests/e2e/withQuerystring/.eslintrc.cjs(1 hunks)tests/e2e/withoutPaths/.eslintrc.cjs(1 hunks)tests/importXResolverV3/eslint.config.cjs(0 hunks)tests/nestedPackageJson/.eslintrc.cjs(0 hunks)tests/pnp/pnp.spec.ts(0 hunks)tests/tsconfig.json(1 hunks)tests/unit/unit.spec.ts(1 hunks)tests/withJsconfig/.eslintrc.cjs(0 hunks)tsconfig.base.json(1 hunks)tsconfig.json(1 hunks)tsconfig.staged.json(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (7)
- .vscode/settings.json
- tests/e2e/withJsExtension/.eslintrc.cjs
- .env.yarn
- tests/nestedPackageJson/.eslintrc.cjs
- tests/withJsconfig/.eslintrc.cjs
- tests/pnp/pnp.spec.ts
- tests/importXResolverV3/eslint.config.cjs
🚧 Files skipped from review as they are similar to previous changes (19)
- tests/e2e/package.json
- tests/e2e/withoutPaths/.eslintrc.cjs
- tests/e2e/absoluteAlias/tsconfig.json
- tsconfig.staged.json
- tests/tsconfig.json
- tests/e2e/withQuerystring/.eslintrc.cjs
- eslint.config.js
- vitest.config.ts
- tests/e2e/withJsExtension/index.ts
- tsconfig.json
- .prettierignore
- tests/unit/unit.spec.ts
- .lintstagedrc.js
- tests/e2e/base.eslintrc.cjs
- tests/e2e/withJsExtension/test.cjs
- tsconfig.base.json
- auto-imports.d.ts
- tests/e2e/importXResolverV3/eslint.config.js
- tests/e2e/e2e.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
🔇 Additional comments (3)
package.json (3)
63-63: Test Script Update
Thetestscript now sequentially runs an end-to-end test (node tests/e2e/withJsExtension/test.cjs) before executingvitest runfor unit tests. Keep in mind that if the e2e test fails, the subsequent unit tests won’t run. Please verify that this behavior aligns with your testing strategy; if independent execution is desired, you might consider splitting these into separate scripts or using parallel execution.
93-93: New Dependency: @pkgr/core
A new devDependency,"@pkgr/core": "^0.2.0", has been added. Ensure that this package is required by your tooling or build process and that its usage is adequately documented.
117-117: New Dependency: unplugin-auto-import
The addition of"unplugin-auto-import": "^19.1.2"is aligned with enabling automatic imports (likely used in yourvitest.config.ts). Please confirm that the version is compatible with your other dev tools and that its behavior is as expected.
JounQin
force-pushed
the
chore/split_e2e_unit
branch
from
40fd25d to
53ddbf7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
package.json (2)
61-61: Review the simplifiedpreparescript and fallback behavior.
The updated script"prepare": "simple-git-hooks && yarn-berry-deduplicate || exit 0"is more concise; however, note that using|| exit 0will suppress any failure from the preceding commands. Please confirm that suppressing errors fromyarn-berry-deduplicateis intended and that critical failures (if any) will not go unnoticed.
63-63: Verify the new test script's separation of e2e and unit tests.
Thetestscript now executes"node tests/e2e/withJsExtension/test.cjs && vitest run", clearly separating e2e tests from unit tests. Ensure that if the e2e tests fail, the entire test process fails as expected and that the ordering meets your overall testing strategy.tests/unit/unit.spec.ts (3)
7-15: Consider handling async setup more consistentlyThe test is using top-level await within the
describecallback. While this works in environments that support top-level await in test files, it might be better to:
- Move the async setup into a
beforeAllhook- Add explicit error handling for the Yarn command
- Consider setting a specific timeout for this setup
-describe('createTypeScriptImportResolver', async () => { +describe('createTypeScriptImportResolver', () => { const pnpDir = path.resolve(import.meta.dirname, 'pnp') - await exec('yarn', [], { - nodeOptions: { - cwd: pnpDir, - }, - }) + beforeAll(async () => { + try { + await exec('yarn', [], { + nodeOptions: { + cwd: pnpDir, + }, + }) + } catch (error) { + console.error('Failed to set up PnP environment:', error) + throw error + } + }, 30000) // Explicit timeout for Yarn command
18-34: Add timeout to PnP test caseThe test relies on external module resolution which could potentially be slow. Add an explicit timeout to prevent test failures due to performance issues.
- it('should work with pnp', async () => { + it('should work with pnp', async () => { const testfile = path.resolve(pnpDir, '__test__.js') expect(resolver.resolve('pnpapi', testfile)).toMatchInlineSnapshot(` { "found": true, "path": "<ROOT>/tests/unit/pnp/.pnp.cjs", } `) expect(resolver.resolve('lodash.zip', testfile)).toMatchInlineSnapshot(` { "found": true, "path": "<ROOT>/tests/unit/pnp/.yarn/cache/lodash.zip-npm-4.2.0-5299417ec8-cb06530d81.zip/node_modules/lodash.zip/index.js", } `) - }) + }, 10000) // Add explicit timeout for potentially slow resolution
1-36: Consider adding cleanup and more robust testingThe test creates files but doesn't clean them up, which could affect subsequent test runs. Also, it only tests the "happy path" without testing error scenarios.
Consider adding:
- An
afterAllhook to clean up after tests- Additional test cases for error scenarios (e.g., resolving non-existent modules)
- Verification that
testfileexists before using it// Example afterAll hook to add afterAll(async () => { // Clean up any files or directories created during testing if needed }) // Example additional test case it('should handle non-existent modules correctly', async () => { const testfile = path.resolve(pnpDir, '__test__.js') const result = resolver.resolve('non-existent-module', testfile) expect(result.found).toBe(false) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
tests/e2e/__snapshots__/e2e.spec.ts.snapis excluded by!**/*.snaptests/e2e/nestedPackageJson/yarn.lockis excluded by!**/yarn.lock,!**/*.locktests/e2e/withQuerystring/image.svgis excluded by!**/*.svgtests/e2e/withQuerystring/subfolder/image.svgis excluded by!**/*.svgtests/unit/pnp/.yarn/cache/lodash.zip-npm-4.2.0-5299417ec8-cb06530d81.zipis excluded by!**/.yarn/**,!**/*.ziptests/unit/pnp/.yarn/releases/yarn-4.8.0.cjsis excluded by!**/.yarn/**tests/unit/pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (27)
.env.yarn(0 hunks).lintstagedrc.js(1 hunks).prettierignore(1 hunks).vscode/settings.json(0 hunks)auto-imports.d.ts(1 hunks)eslint.config.js(2 hunks)package.json(3 hunks)tests/e2e/absoluteAlias/tsconfig.json(1 hunks)tests/e2e/base.eslintrc.cjs(1 hunks)tests/e2e/e2e.spec.ts(1 hunks)tests/e2e/importXResolverV3/eslint.config.js(1 hunks)tests/e2e/package.json(1 hunks)tests/e2e/withJsExtension/.eslintrc.cjs(0 hunks)tests/e2e/withJsExtension/index.ts(1 hunks)tests/e2e/withJsExtension/test.cjs(2 hunks)tests/e2e/withQuerystring/.eslintrc.cjs(1 hunks)tests/e2e/withoutPaths/.eslintrc.cjs(1 hunks)tests/importXResolverV3/eslint.config.cjs(0 hunks)tests/nestedPackageJson/.eslintrc.cjs(0 hunks)tests/pnp/pnp.spec.ts(0 hunks)tests/tsconfig.json(1 hunks)tests/unit/unit.spec.ts(1 hunks)tests/withJsconfig/.eslintrc.cjs(0 hunks)tsconfig.base.json(1 hunks)tsconfig.json(1 hunks)tsconfig.staged.json(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (7)
- .env.yarn
- tests/e2e/withJsExtension/.eslintrc.cjs
- .vscode/settings.json
- tests/withJsconfig/.eslintrc.cjs
- tests/nestedPackageJson/.eslintrc.cjs
- tests/pnp/pnp.spec.ts
- tests/importXResolverV3/eslint.config.cjs
🚧 Files skipped from review as they are similar to previous changes (18)
- tests/e2e/package.json
- tests/e2e/withoutPaths/.eslintrc.cjs
- tsconfig.staged.json
- tests/e2e/withQuerystring/.eslintrc.cjs
- tests/tsconfig.json
- tsconfig.json
- tests/e2e/withJsExtension/index.ts
- eslint.config.js
- tests/e2e/e2e.spec.ts
- tests/e2e/importXResolverV3/eslint.config.js
- tests/e2e/base.eslintrc.cjs
- tests/e2e/absoluteAlias/tsconfig.json
- .lintstagedrc.js
- tsconfig.base.json
- .prettierignore
- auto-imports.d.ts
- vitest.config.ts
- tests/e2e/withJsExtension/test.cjs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
🔇 Additional comments (4)
package.json (2)
93-93: Confirm the necessity of the new@pkgr/coredependency.
The new dev dependency"@pkgr/core": "^0.2.0"appears to have been added for build or packaging enhancements. Please verify that this package is required and that its version meets the project’s requirements.
117-117: Ensure proper integration ofunplugin-auto-import.
The addition of"unplugin-auto-import": "^19.1.2"in devDependencies aligns with its usage in the testing configuration (e.g., invitest.config.ts). Confirm that the auto-import configuration is updated accordingly and that it benefits the intended test automation.tests/unit/unit.spec.ts (2)
1-5: LGTM: Import statements are clear and appropriateThe import statements are properly organized with Node.js built-ins first, followed by external dependencies, and finally the module under test.
9-9:✅ Verification successful
Verify the PnP directory existence
Make sure the PnP directory exists before attempting to execute commands in it.
🏁 Script executed:
#!/bin/bash # Check if the PnP directory exists in the expected location if [ ! -d "tests/unit/pnp" ]; then echo "Error: PnP directory not found at tests/unit/pnp" exit 1 else echo "PnP directory exists at tests/unit/pnp" ls -la tests/unit/pnp fiLength of output: 727
PNP Directory Check Verified
The verification script confirmed that the "tests/unit/pnp" directory exists and contains the expected files. No further actions are required regarding the directory existence check in the tests.
|
Codecov has always been enabled for every repo in this org. |
@ljharb Oh, great, then can you help to add
|
|
You can add yours in https://github.com/import-js/eslint-import-resolver-typescript/settings/secrets/actions - i use the tokenless one. |
@ljharb I don't have permission on Codecov's side. |
|
k, done |
close #412
Summary by CodeRabbit
New Features
Tests
Chores