Reject automatic filesystem mocks in tests

docs/cli/testing-strategy.md

@@ -57,6 +57,8 @@ See [JSON output tests](json-output.md#test-a-new-command) and [UI tests](../cli
 
 ### Filesystem I/O and temporary directories
 
+The `@shopify/cli/no-filesystem-auto-mocks` ESLint rule rejects automatic filesystem mocks in test files.
+
 For filesystem tests, use real files in a temporary directory. The test must delete the directory after use, including when it fails:
 
 ```ts

packages/app/src/cli/services/dev/processes/app-logs-polling.test.ts

@@ -9,11 +9,11 @@ import {DeveloperPlatformClient} from '../../../utilities/developer-platform-cli
 import * as appLogsUtils from '../../app-logs/utils.js'
 import {AppEventWatcher} from '../app-events/app-event-watcher.js'
 import {AbortSignal, AbortController} from '@shopify/cli-kit/node/abort'
-import {mkdir} from '@shopify/cli-kit/node/fs'
+import {inTemporaryDirectory, mkdir} from '@shopify/cli-kit/node/fs'
+import * as fs from '@shopify/cli-kit/node/fs'
 import {outputDebug} from '@shopify/cli-kit/node/output'
 import {describe, expect, vi, Mock, beforeEach, test} from 'vitest'
 
-vi.mock('@shopify/cli-kit/node/fs')
 vi.mock('@shopify/cli-kit/node/output')
 vi.mock('../../app-logs/dev/poll-app-logs.js')
 
@@ -102,7 +102,7 @@ describe('app-logs-polling', () => {
 
       developerPlatformClient = testDeveloperPlatformClient({subscribeToAppLogs})
 
-      vi.mocked(mkdir).mockResolvedValue()
+      vi.spyOn(fs, 'mkdir')
       vi.mocked(pollAppLogs).mockResolvedValue()
       vi.spyOn(appLogsUtils, 'subscribeToAppLogs').mockResolvedValue(JWT_TOKEN)
     })
@@ -134,7 +134,10 @@ describe('app-logs-polling', () => {
         startTime: [0, 0],
         path: '',
       }
-      await startCallback(appEvent)
+      await inTemporaryDirectory(async (tempDir) => {
+        vi.spyOn(localApp, 'getLogsDir').mockReturnValue(tempDir)
+        await startCallback(appEvent)
+      })
 
       // Then
       expect(outputDebug).toHaveBeenCalledWith('Function extensions detected, starting logs polling')

packages/cli-kit/src/public/node/api/bulk-operations/query.test.ts

@@ -1,15 +1,12 @@
 import {resolveBulkOperationQuery} from './query.js'
-import {fileExists, readFile} from '../../fs.js'
+import {writeFile} from '../../fs.js'
+import * as fs from '../../fs.js'
+import {joinPath} from '../../path.js'
+import {testWithTempDir as test} from '../../testing/test-with-temp-dir.js'
 import {AbortError, BugError} from '../../error.js'
-import {describe, test, expect, vi, beforeEach} from 'vitest'
-
-vi.mock('../../fs.js')
+import {describe, expect, vi} from 'vitest'
 
 describe('resolveBulkOperationQuery', () => {
-  beforeEach(() => {
-    vi.mocked(fileExists).mockResolvedValue(true)
-  })
-
   test('returns a valid inline query', async () => {
     await expect(resolveBulkOperationQuery({query: 'query { shop { name } }'})).resolves.toBe('query { shop { name } }')
   })
@@ -18,20 +15,24 @@ describe('resolveBulkOperationQuery', () => {
     await expect(resolveBulkOperationQuery({query: '   '})).rejects.toThrow(AbortError)
   })
 
-  test('reads and returns a valid query file', async () => {
-    vi.mocked(readFile).mockResolvedValue('query { shop { name } }' as never)
-    await expect(resolveBulkOperationQuery({queryFile: './op.graphql'})).resolves.toBe('query { shop { name } }')
-    expect(readFile).toHaveBeenCalledWith('./op.graphql', {encoding: 'utf8'})
+  test('reads and returns a valid query file', async ({tempDir}) => {
+    const queryFile = joinPath(tempDir, 'op.graphql')
+    await writeFile(queryFile, 'query { shop { name } }')
+    const readFile = vi.spyOn(fs, 'readFile')
+    await expect(resolveBulkOperationQuery({queryFile})).resolves.toBe('query { shop { name } }')
+    expect(readFile).toHaveBeenCalledWith(queryFile, {encoding: 'utf8'})
   })
 
-  test('throws when the query file does not exist', async () => {
-    vi.mocked(fileExists).mockResolvedValue(false)
-    await expect(resolveBulkOperationQuery({queryFile: './missing.graphql'})).rejects.toThrow(AbortError)
+  test('throws when the query file does not exist', async ({tempDir}) => {
+    await expect(resolveBulkOperationQuery({queryFile: joinPath(tempDir, 'missing.graphql')})).rejects.toThrow(
+      AbortError,
+    )
   })
 
-  test('throws when the query file is empty', async () => {
-    vi.mocked(readFile).mockResolvedValue('   \n' as never)
-    await expect(resolveBulkOperationQuery({queryFile: './empty.graphql'})).rejects.toThrow(AbortError)
+  test('throws when the query file is empty', async ({tempDir}) => {
+    const queryFile = joinPath(tempDir, 'empty.graphql')
+    await writeFile(queryFile, '   \n')
+    await expect(resolveBulkOperationQuery({queryFile})).rejects.toThrow(AbortError)
   })
 
   test('throws a BugError when neither query nor query file is provided', async () => {

packages/cli-kit/src/public/node/context/local.test.ts

@@ -11,13 +11,14 @@ import {
   getThemeKitAccessDomain,
   opentelemetryDomain,
 } from './local.js'
-import {fileExists} from '../fs.js'
+import {inTemporaryDirectory, writeFile} from '../fs.js'
+import {joinPath} from '../path.js'
+import {pathConstants} from '../../../private/node/constants.js'
 import {exec} from '../system.js'
 import {afterEach, expect, describe, vi, test} from 'vitest'
 import * as os from 'os'
 import type {NetworkInterfaceInfo} from 'os'
 
-vi.mock('../fs.js')
 vi.mock('../system.js')
 vi.mock('../environment.js')
 vi.mock('os', async (importOriginal) => {
@@ -132,11 +133,16 @@ describe('isShopify', () => {
   })
 
   test('returns true when dev is installed', async () => {
-    // Given
-    vi.mocked(fileExists).mockResolvedValue(true)
-
-    // When
-    await expect(isShopify()).resolves.toBe(true)
+    await inTemporaryDirectory(async (tempDir) => {
+      const originalDevPath = pathConstants.executables.dev
+      pathConstants.executables.dev = joinPath(tempDir, 'dev')
+      try {
+        await writeFile(pathConstants.executables.dev, '')
+        await expect(isShopify({})).resolves.toBe(true)
+      } finally {
+        Object.assign(pathConstants.executables, {dev: originalDevPath})
+      }
+    })
   })
 })
 

packages/cli-kit/src/public/node/doctor/framework.test.ts

@@ -2,7 +2,6 @@ import {DoctorSuite} from './framework.js'
 import {describe, expect, test, vi, beforeEach} from 'vitest'
 import type {DoctorContext} from './types.js'
 
-vi.mock('../fs.js')
 vi.mock('../system.js')
 
 /**

packages/cli/src/cli/commands/docs/generate.test.ts

@@ -1,8 +1,7 @@
-import {CommandData, CommandWithMarkdown, extractCommandData, writeCommandFlagInterface} from './generate.js'
-import {writeFile} from '@shopify/cli-kit/node/fs'
-import {describe, test, vi, expect} from 'vitest'
-
-vi.mock('@shopify/cli-kit/node/fs')
+import {CommandData, CommandWithMarkdown, extractCommandData} from './generate.js'
+import {testWithTempDir as test} from '@shopify/cli-kit/node/testing/test-with-temp-dir'
+import {joinPath} from '@shopify/cli-kit/node/path'
+import {describe, vi, expect} from 'vitest'
 
 const testCommand: CommandWithMarkdown = {
   aliases: [],
@@ -45,11 +44,17 @@ describe('extractCommandData', () => {
 })
 
 describe('writeCommandFlagInterface', () => {
-  test('calls writeFile with the correct content', async () => {
+  test('calls writeFile with the correct content', async ({tempDir}) => {
+    vi.resetModules()
+    const path = await import('@shopify/cli-kit/node/path')
+    const fs = await import('@shopify/cli-kit/node/fs')
+    vi.spyOn(path, 'cwd').mockReturnValue(tempDir)
+    const writeFile = vi.spyOn(fs, 'writeFile')
+    const {writeCommandFlagInterface} = await import('./generate.js')
     await writeCommandFlagInterface(testCommand, commandData)
 
     expect(writeFile).toHaveBeenCalledWith(
-      expect.stringContaining('docs-shopify.dev/commands/interfaces/topic-test-command.interface.ts'),
+      joinPath(tempDir, 'docs-shopify.dev/commands/interfaces/topic-test-command.interface.ts'),
       `// This is an autogenerated file. Don't edit this file manually.
 /**
  * The following flags are available for the \`topic test-command\` command:
@@ -72,11 +77,17 @@ export interface topictestcommand {
     )
   })
 
-  test('calls writeFile with the correct content for a command with no flags', async () => {
+  test('calls writeFile with the correct content for a command with no flags', async ({tempDir}) => {
+    vi.resetModules()
+    const path = await import('@shopify/cli-kit/node/path')
+    const fs = await import('@shopify/cli-kit/node/fs')
+    vi.spyOn(path, 'cwd').mockReturnValue(tempDir)
+    const writeFile = vi.spyOn(fs, 'writeFile')
+    const {writeCommandFlagInterface} = await import('./generate.js')
     await writeCommandFlagInterface({...testCommand, flags: {}}, commandData)
 
     expect(writeFile).toHaveBeenCalledWith(
-      expect.stringContaining('docs-shopify.dev/commands/interfaces/topic-test-command.interface.ts'),
+      joinPath(tempDir, 'docs-shopify.dev/commands/interfaces/topic-test-command.interface.ts'),
       `// This is an autogenerated file. Don't edit this file manually.
 /**
  * The following flags are available for the \`topic test-command\` command:

packages/eslint-plugin-cli/config.js

@@ -12,6 +12,7 @@ const rules = {
   'command-conventional-flag-env': require('./rules/command-conventional-flag-env'),
   'command-reserved-flags': require('./rules/command-reserved-flags'),
   'command-json-output': require('./rules/command-json-output'),
+  'no-filesystem-auto-mocks': require('./rules/no-filesystem-auto-mocks'),
   'no-error-factory-functions': require('./rules/no-error-factory-functions'),
   'no-process-cwd': require('./rules/no-process-cwd'),
   'no-trailing-js-in-cli-kit-imports': require('./rules/no-trailing-js-in-cli-kit-imports'),
@@ -160,6 +161,7 @@ const baseRules = {
   '@shopify/cli/command-conventional-flag-env': 'error',
   '@shopify/cli/command-reserved-flags': 'error',
   '@shopify/cli/command-json-output': 'error',
+  '@shopify/cli/no-filesystem-auto-mocks': 'error',
   '@shopify/cli/no-error-factory-functions': 'error',
   '@shopify/cli/no-process-cwd': 'error',
   '@shopify/cli/no-trailing-js-in-cli-kit-imports': 'error',

packages/eslint-plugin-cli/index.js

@@ -8,6 +8,7 @@ const plugin = {
     'command-conventional-flag-env': require('./rules/command-conventional-flag-env'),
     'command-reserved-flags': require('./rules/command-reserved-flags'),
     'command-json-output': require('./rules/command-json-output'),
+    'no-filesystem-auto-mocks': require('./rules/no-filesystem-auto-mocks'),
     'no-error-factory-functions': require('./rules/no-error-factory-functions'),
     'no-process-cwd': require('./rules/no-process-cwd'),
     'no-trailing-js-in-cli-kit-imports': require('./rules/no-trailing-js-in-cli-kit-imports'),

packages/eslint-plugin-cli/rules/no-filesystem-auto-mocks.js

@@ -0,0 +1,56 @@
+const path = require('node:path')
+
+const filesystemModules = new Set(['fs', 'node:fs', 'fs/promises', 'node:fs/promises', '@shopify/cli-kit/node/fs'])
+
+module.exports = {
+  meta: {
+    type: 'problem',
+    docs: {description: 'use real filesystem fixtures instead of automatic filesystem mocks'},
+    schema: [],
+    messages: {filesystemMock: 'Use real files in a temporary directory instead of an automatic filesystem mock.'},
+  },
+  create(context) {
+    if (!/\.test\.[jt]sx?$/.test(context.filename)) return {}
+
+    return {
+      CallExpression(node) {
+        if (node.callee.type !== 'MemberExpression' || node.callee.computed) return
+        if (node.callee.object.name !== 'vi' || !['mock', 'doMock'].includes(node.callee.property.name)) return
+        if (node.arguments.length !== 1) return
+
+        const argument = node.arguments[0]
+        let moduleName
+        if (argument.type === 'Literal') moduleName = argument.value
+        else if (argument.type === 'ImportExpression') moduleName = argument.source.value
+        if (typeof moduleName !== 'string') return
+        const isFilesystemModule = moduleName.startsWith('.')
+          ? /\/packages\/cli-kit\/src\/public\/node\/fs\.[jt]s$/.test(
+              path.posix.resolve(path.posix.dirname(context.filename.replaceAll('\\', '/')), moduleName),
+            )
+          : filesystemModules.has(moduleName.replace(/\.js$/, ''))
+        if (!isFilesystemModule) return
+        const variable = findVariable(context.sourceCode.getScope(node), 'vi')
+        if (
+          variable &&
+          !variable.defs.some(
+            (definition) =>
+              definition.type === 'ImportBinding' &&
+              definition.parent.source.value === 'vitest' &&
+              definition.node.imported?.name === 'vi',
+          )
+        )
+          return
+
+        context.report({node, messageId: 'filesystemMock'})
+      },
+    }
+  },
+}
+
+function findVariable(scope, name) {
+  for (let current = scope; current; current = current.upper) {
+    const variable = current.set.get(name)
+    if (variable) return variable
+  }
+  return undefined
+}

packages/eslint-plugin-cli/rules/no-filesystem-auto-mocks.test.js

@@ -0,0 +1,61 @@
+const {RuleTester} = require('eslint')
+const typescriptParser = require('@typescript-eslint/parser')
+
+const rule = require('./no-filesystem-auto-mocks')
+
+const ruleTester = new RuleTester({languageOptions: {parser: typescriptParser}})
+const filename = '/repo/services/widgets.test.ts'
+
+ruleTester.run('no-filesystem-auto-mocks', rule, {
+  valid: [
+    {filename, code: 'vi.mock("./service")'},
+    {filename, code: 'vi.mock("../fs.js")'},
+    {filename, code: 'import {vi as v} from "vitest"; v.mock("fs")'},
+    {filename, code: 'import * as vitest from "vitest"; vitest.vi.mock("fs")'},
+    {filename, code: 'vi.mock(`fs`)'},
+    {filename: '/repo/services/widgets.spec.ts', code: 'vi.mock("fs")'},
+    {filename, code: 'vi.mock("fs", async (importOriginal) => ({...await importOriginal()}))'},
+    {filename: '/repo/packages/cli-kit/src/public/node/context/local.test.ts', code: 'vi.mock("../fs.js", () => ({}))'},
+    {filename: '/repo/packages/cli-kit/src/public/node/context/local.test.ts', code: 'vi.mock("./fs.js")'},
+    {
+      filename,
+      code: 'vi.mock("@shopify/cli-kit/node/fs", () => ({generateRandomNameForSubdirectory: () => "fixture"}))',
+    },
+    {filename, code: 'const vi = {mock() {}}; vi.mock("node:fs")'},
+    {filename, code: 'function check(vi) { vi.mock("node:fs") }'},
+    {filename, code: 'import {vi} from "another-library"; vi.mock("node:fs")'},
+    {filename, code: 'vi.mock(moduleName)'},
+    {filename: '/repo/services/widgets.ts', code: 'vi.mock("node:fs")'},
+  ],
+  invalid: [
+    {filename, code: 'vi.mock("node:fs")', errors: [{messageId: 'filesystemMock'}]},
+    {filename, code: 'vi.mock("@shopify/cli-kit/node/fs.js")', errors: [{messageId: 'filesystemMock'}]},
+    {filename, code: 'vi.mock("fs/promises.js")', errors: [{messageId: 'filesystemMock'}]},
+    {
+      filename: '/repo/packages/cli-kit/src/public/node/context/local.test.ts',
+      code: 'vi.mock("../fs.js")',
+      errors: [{messageId: 'filesystemMock'}],
+    },
+    {
+      filename: '/repo/packages/cli-kit/src/public/node/api/bulk-operations/query.test.ts',
+      code: 'vi.mock("../../fs.js")',
+      errors: [{messageId: 'filesystemMock'}],
+    },
+    {
+      filename: '/repo/packages/cli-kit/src/public/node/doctor/framework.test.ts',
+      code: 'vi.doMock(import("../fs.ts"))',
+      errors: [{messageId: 'filesystemMock'}],
+    },
+    {
+      filename: String.raw`C:\repo\packages\cli-kit\src\public\node\doctor\framework.test.ts`,
+      code: 'vi.mock("../fs.js")',
+      errors: [{messageId: 'filesystemMock'}],
+    },
+    {filename, code: 'vi.doMock("node:fs/promises")', errors: [{messageId: 'filesystemMock'}]},
+    {filename, code: 'vi.mock("fs")', errors: [{messageId: 'filesystemMock'}]},
+    {filename, code: 'vi.mock("fs/promises")', errors: [{messageId: 'filesystemMock'}]},
+    {filename, code: 'vi.mock("@shopify/cli-kit/node/fs")', errors: [{messageId: 'filesystemMock'}]},
+    {filename, code: 'import {vi} from "vitest"; vi.mock("fs")', errors: [{messageId: 'filesystemMock'}]},
+    {filename, code: 'vi.mock(import("node:fs"))', errors: [{messageId: 'filesystemMock'}]},
+  ],
+})