Skip to content

Commit 2702cf4

Browse files
sheremet-vasheremet-va
authored
fix: fix vm tests flakiness (#7741)
1 parent 12762ea commit 2702cf4

File tree

13 files changed

+124
-115
lines changed

13 files changed

+124
-115
lines changed

‎packages/runner/src/types/tasks.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -647,7 +647,7 @@ export interface TestContext {
647647
/**
648648
* Metadata of the current test
649649
*/
650-
task: Readonly<Task>
650+
task: Readonly<Test<TestContext>>
651651

652652
/**
653653
* Extract hooks on test failed

‎packages/vitest/src/runtime/external-executor.ts‎

Lines changed: 13 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,16 @@
1+
import type vm from 'node:vm'
12
import type { RuntimeRPC } from '../types/rpc'
23
import type { FileMap } from './vm/file-map'
3-
import type { VMModule, VMSyntheticModule } from './vm/types'
4+
import type { VMModule } from './vm/types'
45
import fs from 'node:fs'
56
import { dirname } from 'node:path'
67
import { fileURLToPath, pathToFileURL } from 'node:url'
7-
import vm from 'node:vm'
88
import { extname, join, normalize } from 'pathe'
99
import { getCachedData, isNodeBuiltin, setCacheData } from 'vite-node/utils'
1010
import { CommonjsExecutor } from './vm/commonjs-executor'
1111
import { EsmExecutor } from './vm/esm-executor'
12-
import { interopCommonJsModule } from './vm/utils'
1312
import { ViteExecutor } from './vm/vite-executor'
1413

15-
const SyntheticModule: typeof VMSyntheticModule = (vm as any).SyntheticModule
16-
1714
const { existsSync, statSync } = fs
1815

1916
// always defined when we use vm pool
@@ -64,6 +61,7 @@ export class ExternalModulesExecutor {
6461
context: this.context,
6562
importModuleDynamically: this.importModuleDynamically,
6663
fileMap: options.fileMap,
64+
interopDefault: options.interopDefault,
6765
})
6866
this.vite = new ViteExecutor({
6967
esmExecutor: this.esm,
@@ -156,52 +154,6 @@ export class ExternalModulesExecutor {
156154
return {}
157155
}
158156

159-
private wrapCoreSyntheticModule(
160-
identifier: string,
161-
exports: Record<string, unknown>,
162-
) {
163-
const moduleKeys = Object.keys(exports)
164-
const m = new SyntheticModule(
165-
[...moduleKeys, 'default'],
166-
() => {
167-
for (const key of moduleKeys) {
168-
m.setExport(key, exports[key])
169-
}
170-
m.setExport('default', exports)
171-
},
172-
{
173-
context: this.context,
174-
identifier,
175-
},
176-
)
177-
return m
178-
}
179-
180-
private wrapCommonJsSynteticModule(
181-
identifier: string,
182-
exports: Record<string, unknown>,
183-
) {
184-
// TODO: technically module should be parsed to find static exports, implement for strict mode in #2854
185-
const { keys, moduleExports, defaultExport } = interopCommonJsModule(
186-
this.options.interopDefault,
187-
exports,
188-
)
189-
const m = new SyntheticModule(
190-
[...keys, 'default'],
191-
() => {
192-
for (const key of keys) {
193-
m.setExport(key, moduleExports[key])
194-
}
195-
m.setExport('default', defaultExport)
196-
},
197-
{
198-
context: this.context,
199-
identifier,
200-
},
201-
)
202-
return m
203-
}
204-
205157
private getModuleInformation(identifier: string): ModuleInformation {
206158
if (identifier.startsWith('data:')) {
207159
return { type: 'data', url: identifier, path: identifier }
@@ -248,7 +200,7 @@ export class ExternalModulesExecutor {
248200
return { type, path: pathUrl, url: fileUrl }
249201
}
250202

251-
private async createModule(identifier: string): Promise<VMModule> {
203+
private createModule(identifier: string): VMModule | Promise<VMModule> {
252204
const { type, url, path } = this.getModuleInformation(identifier)
253205

254206
// create ERR_MODULE_NOT_FOUND on our own since latest NodeJS's import.meta.resolve doesn't throw on non-existing namespace or path
@@ -264,25 +216,21 @@ export class ExternalModulesExecutor {
264216

265217
switch (type) {
266218
case 'data':
267-
return await this.esm.createDataModule(identifier)
268-
case 'builtin': {
269-
const exports = this.require(identifier)
270-
return this.wrapCoreSyntheticModule(identifier, exports)
271-
}
219+
return this.esm.createDataModule(identifier)
220+
case 'builtin':
221+
return this.cjs.getCoreSyntheticModule(identifier)
272222
case 'vite':
273-
return await this.vite.createViteModule(url)
223+
return this.vite.createViteModule(url)
274224
case 'wasm':
275-
return await this.esm.createWebAssemblyModule(url, () =>
225+
return this.esm.createWebAssemblyModule(url, () =>
276226
this.fs.readBuffer(path))
277227
case 'module':
278-
return await this.esm.createEsModule(url, () =>
228+
return this.esm.createEsModule(url, () =>
279229
this.fs.readFileAsync(path))
280-
case 'commonjs': {
281-
const exports = this.require(path)
282-
return this.wrapCommonJsSynteticModule(identifier, exports)
283-
}
230+
case 'commonjs':
231+
return this.cjs.getCjsSyntheticModule(path, identifier)
284232
case 'network':
285-
return await this.esm.createNetworkModule(url)
233+
return this.esm.createNetworkModule(url)
286234
default: {
287235
const _deadend: never = type
288236
return _deadend

‎packages/vitest/src/runtime/vm/commonjs-executor.ts‎

Lines changed: 87 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
import type { FileMap } from './file-map'
2-
import type { ImportModuleDynamically, VMModule } from './types'
2+
import type { ImportModuleDynamically, VMSyntheticModule } from './types'
33
import { Module as _Module, createRequire } from 'node:module'
44
import vm from 'node:vm'
55
import { basename, dirname, extname } from 'pathe'
66
import { isNodeBuiltin } from 'vite-node/utils'
7+
import { interopCommonJsModule, SyntheticModule } from './utils'
78

89
interface CommonjsExecutorOptions {
910
fileMap: FileMap
11+
interopDefault?: boolean
1012
context: vm.Context
1113
importModuleDynamically: ImportModuleDynamically
1214
}
@@ -24,7 +26,7 @@ export class CommonjsExecutor {
2426
private requireCache = new Map<string, NodeJS.Module>()
2527
private publicRequireCache = this.createProxyCache()
2628

27-
private moduleCache = new Map<string, VMModule | Promise<VMModule>>()
29+
private moduleCache = new Map<string, VMSyntheticModule>()
2830
private builtinCache: Record<string, NodeJS.Module> = Object.create(null)
2931
private extensions: Record<
3032
string,
@@ -33,10 +35,12 @@ export class CommonjsExecutor {
3335

3436
private fs: FileMap
3537
private Module: typeof _Module
38+
private interopDefault: boolean | undefined
3639

3740
constructor(options: CommonjsExecutorOptions) {
3841
this.context = options.context
3942
this.fs = options.fileMap
43+
this.interopDefault = options.interopDefault
4044

4145
const primitives = vm.runInContext(
4246
'({ Object, Array, Error })',
@@ -132,7 +136,7 @@ export class CommonjsExecutor {
132136
static SourceMap = _Module.SourceMap
133137
static syncBuiltinESMExports = _Module.syncBuiltinESMExports
134138

135-
static _cache = executor.moduleCache
139+
static _cache = executor.publicRequireCache
136140
static _extensions = executor.extensions
137141

138142
static createRequire = (filename: string | URL) => {
@@ -256,6 +260,86 @@ export class CommonjsExecutor {
256260
return '.js'
257261
}
258262

263+
public getCoreSyntheticModule(identifier: string): VMSyntheticModule {
264+
if (this.moduleCache.has(identifier)) {
265+
return this.moduleCache.get(identifier)!
266+
}
267+
const exports = this.require(identifier)
268+
const keys = Object.keys(exports)
269+
const module = new SyntheticModule([...keys, 'default'], () => {
270+
for (const key of keys) {
271+
module.setExport(key, exports[key])
272+
}
273+
module.setExport('default', exports)
274+
}, { context: this.context, identifier })
275+
this.moduleCache.set(identifier, module)
276+
return module
277+
}
278+
279+
public getCjsSyntheticModule(path: string, identifier: string): VMSyntheticModule {
280+
if (this.moduleCache.has(identifier)) {
281+
return this.moduleCache.get(identifier)!
282+
}
283+
const exports = this.require(path)
284+
// TODO: technically module should be parsed to find static exports, implement for strict mode in #2854
285+
const { keys, moduleExports, defaultExport } = interopCommonJsModule(
286+
this.interopDefault,
287+
exports,
288+
)
289+
const module = new SyntheticModule([...keys, 'default'], function () {
290+
for (const key of keys) {
291+
this.setExport(key, moduleExports[key])
292+
}
293+
this.setExport('default', defaultExport)
294+
}, { context: this.context, identifier })
295+
this.moduleCache.set(identifier, module)
296+
return module
297+
}
298+
299+
// TODO: use this in strict mode, when available in #2854
300+
// private _getNamedCjsExports(path: string): Set<string> {
301+
// const cachedNamedExports = this.cjsNamedExportsMap.get(path)
302+
303+
// if (cachedNamedExports) {
304+
// return cachedNamedExports
305+
// }
306+
307+
// if (extname(path) === '.node') {
308+
// const moduleExports = this.require(path)
309+
// const namedExports = new Set(Object.keys(moduleExports))
310+
// this.cjsNamedExportsMap.set(path, namedExports)
311+
// return namedExports
312+
// }
313+
314+
// const code = this.fs.readFile(path)
315+
// const { exports, reexports } = parseCjs(code, path)
316+
// const namedExports = new Set(exports)
317+
// this.cjsNamedExportsMap.set(path, namedExports)
318+
319+
// for (const reexport of reexports) {
320+
// if (isNodeBuiltin(reexport)) {
321+
// const exports = this.require(reexport)
322+
// if (exports !== null && typeof exports === 'object') {
323+
// for (const e of Object.keys(exports)) {
324+
// namedExports.add(e)
325+
// }
326+
// }
327+
// }
328+
// else {
329+
// const require = this.createRequire(path)
330+
// const resolved = require.resolve(reexport)
331+
332+
// const exports = this._getNamedCjsExports(resolved)
333+
334+
// for (const e of exports) {
335+
// namedExports.add(e)
336+
// }
337+
// }
338+
// }
339+
340+
// return namedExports
341+
// }
342+
259343
public require(identifier: string): any {
260344
const ext = extname(identifier)
261345
if (ext === '.node' || isNodeBuiltin(identifier)) {

‎packages/vitest/src/runtime/vm/esm-executor.ts‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,9 @@ export class EsmExecutor {
6666
const code = await getCode()
6767
// TODO: should not be allowed in strict mode, implement in #2854
6868
if (fileURL.endsWith('.json')) {
69-
const m = new SyntheticModule(['default'], () => {
69+
const m = new SyntheticModule(['default'], function () {
7070
const result = JSON.parse(code)
71-
m.setExport('default', result)
71+
this.setExport('default', result)
7272
})
7373
this.moduleCache.set(fileURL, m)
7474
return m
@@ -148,15 +148,17 @@ export class EsmExecutor {
148148
}
149149
}
150150

151+
const evaluateModule = (module: VMModule) => this.evaluateModule(module)
152+
151153
const syntheticModule = new SyntheticModule(
152154
exports.map(({ name }) => name),
153-
async () => {
155+
async function () {
154156
const importsObject: WebAssembly.Imports = {}
155157
for (const { module, name } of imports) {
156158
if (!importsObject[module]) {
157159
importsObject[module] = {}
158160
}
159-
await this.evaluateModule(moduleLookup[module])
161+
await evaluateModule(moduleLookup[module])
160162
importsObject[module][name] = (moduleLookup[module].namespace as any)[
161163
name
162164
]
@@ -166,7 +168,7 @@ export class EsmExecutor {
166168
importsObject,
167169
)
168170
for (const { name } of exports) {
169-
syntheticModule.setExport(name, wasmInstance.exports[name])
171+
this.setExport(name, wasmInstance.exports[name])
170172
}
171173
},
172174
{ context: this.context, identifier },
@@ -229,9 +231,9 @@ export class EsmExecutor {
229231
if (mime === 'application/json') {
230232
const module = new SyntheticModule(
231233
['default'],
232-
() => {
234+
function () {
233235
const obj = JSON.parse(code)
234-
module.setExport('default', obj)
236+
this.setExport('default', obj)
235237
},
236238
{ context: this.context, identifier },
237239
)

‎packages/vitest/src/runtime/vm/vite-executor.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,9 +83,9 @@ export class ViteExecutor {
8383
const moduleKeys = Object.keys(stub)
8484
const module = new SyntheticModule(
8585
moduleKeys,
86-
() => {
86+
function () {
8787
moduleKeys.forEach((key) => {
88-
module.setExport(key, stub[key])
88+
this.setExport(key, stub[key])
8989
})
9090
},
9191
{ context: this.options.context, identifier },

‎test/cli/test/setup-files.test.ts‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,7 @@ import { promises as fs } from 'node:fs'
22
import { describe, expect, it, test } from 'vitest'
33
import { editFile, runVitest } from '../../test-utils'
44

5-
const [major] = process.version.slice(1).split('.').map(num => Number(num))
6-
75
test.each(['threads', 'vmThreads'])('%s: print stdout and stderr correctly when called in the setup file', async (pool) => {
8-
if (major >= 22 && pool === 'vmThreads') {
9-
return
10-
}
11-
126
const { stdout, stderr } = await runVitest({
137
root: 'fixtures/setup-files',
148
include: ['empty.test.ts'],

‎test/cli/test/stacktraces.test.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@ import { glob } from 'tinyglobby'
33
import { describe, expect, it } from 'vitest'
44
import { runVitest } from '../../test-utils'
55

6-
const [major] = process.version.slice(1).split('.').map(num => Number(num))
7-
86
// To prevent the warning coming up in snapshots
97
process.setMaxListeners(20)
108

@@ -92,7 +90,7 @@ describe('stacktrace in dependency package', () => {
9290
})
9391
})
9492

95-
it.runIf(major < 22)('stacktrace in vmThreads', async () => {
93+
it('stacktrace in vmThreads', async () => {
9694
const root = resolve(__dirname, '../fixtures/stacktraces')
9795
const testFile = resolve(root, './error-with-stack.test.js')
9896
const { stderr } = await runVitest({

‎test/cli/test/vm-threads.test.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,7 @@ import { expect, test } from 'vitest'
22

33
import { createFile, resolvePath, runVitest } from '../../test-utils'
44

5-
const [major] = process.version.slice(1).split('.').map(num => Number(num))
6-
7-
test.runIf(major < 22)('importing files in restricted fs works correctly', async () => {
5+
test('importing files in restricted fs works correctly', async () => {
86
createFile(
97
resolvePath(import.meta.url, '../fixtures/vm-threads/src/external/package-null/package-null.json'),
108
'null',

‎test/core/test/diff.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,7 @@ test('truncate large diff', () => {
322322
const diff = getErrorDiff(Array.from({ length: 500_000 }).fill(0), 1234)
323323
expect(diff.length).lessThan(200_000)
324324
expect(diff.trim()).toMatch(/\.\.\.$/)
325-
})
325+
}, 60_000)
326326

327327
test('diff default maxDepth', () => {
328328
function generateCycle(n: number) {

0 commit comments

Comments
 (0)