Repository navigation
Conversation
github-actions
Bot
added
the
no-changelog
There was a problem hiding this comment.
🟡 Changes recommended
Standard Vitest aliases, static template literals, and .spec.ts tests can bypass the new rule.
1 open finding
What changed in this PR
Adds lint enforcement against automatic filesystem mocks and migrates affected tests to real temporary files.
Changes:
- Adds and enables
no-filesystem-auto-mocks. - Reworks seven test suites around real filesystem behavior.
- Documents the testing requirement.
| File | Description |
|---|---|
packages/eslint-plugin-cli/rules/no-filesystem-auto-mocks.js |
Implements the lint rule. |
packages/eslint-plugin-cli/rules/no-filesystem-auto-mocks.test.js |
Tests rule behavior. |
packages/eslint-plugin-cli/index.js |
Exports the rule. |
packages/eslint-plugin-cli/config.js |
Enables the rule as an error. |
packages/theme/src/cli/utilities/theme-command.test.ts |
Uses a temporary theme directory. |
packages/cli/src/cli/commands/docs/generate.test.ts |
Writes generated interfaces under temporary directories. |
packages/cli-kit/src/public/node/doctor/framework.test.ts |
Removes an unused filesystem mock. |
packages/cli-kit/src/public/node/context/local.test.ts |
Tests executable detection with a real file. |
packages/cli-kit/src/public/node/api/bulk-operations/query.test.ts |
Tests queries with real files. |
packages/app/src/cli/services/dev/processes/dev-session/dev-session-process.test.ts |
Uses real directories and deterministic async completion. |
packages/app/src/cli/services/dev/processes/app-logs-polling.test.ts |
Uses a real temporary logs directory. |
docs/cli/testing-strategy.md |
Documents lint enforcement. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
isaacroldan
left a comment
isaacroldan
left a comment
There was a problem hiding this comment.
One gap in the automatic filesystem mock detection.
🤖 Generated by the review-pr skill
| if (node.callee.type !== 'MemberExpression' || node.callee.computed) return | ||
| if (!['mock', 'doMock'].includes(node.callee.property.name)) return | ||
| if (!isVitestVi(context.sourceCode.getScope(node), node.callee.object)) return | ||
| if (node.arguments.length !== 1) return |
There was a problem hiding this comment.
This skips automatic filesystem mocks with a second argument, like vi.mock('node:fs', {spy: false}), vi.mock('node:fs', {}), or vi.doMock('node:fs', undefined). I tried the {spy: false} case: lint passes, and readFileSync returns undefined for a real file with content. Can we check the second argument and add these cases to the rule tests, while keeping factories and {spy: true} allowed?
🤖 Generated by the review-pr skill
WHY are these changes introduced?
Automatic filesystem mocks hide file and directory behavior. Filesystem tests must use real temporary files so they can detect incorrect I/O.
WHAT is this pull request doing?
Enable
@shopify/cli/no-filesystem-auto-mocksas an error and replace the seven existing automatic filesystem mocks with temporary files or call-observation spies on real implementations. Dev-session tests also wait for event processing to finish before they check results.The rule covers automatic mocks only. Factory and partial mocks remain outside its scope. It also adds a rule pointer to the filesystem section of the testing guide.
No changeset is needed: this changes internal lint tooling and tests, not CLI behavior.
How to manually test your changes?
In an editor with ESLint diagnostics enabled, add
vi.mock('@shopify/cli-kit/node/fs')to a test file. Confirm that the editor reports the automatic mock. Replace it with a factory mock, such asvi.mock('@shopify/cli-kit/node/fs', async () => ({...})), and confirm that the report is removed.Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add