From a2090a0f6f3ea9ab6e869a2ecbfe30f42c5710f2 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 5 Oct 2020 11:03:04 +0200 Subject: [PATCH] chore: drop usage of `compileFunction` (#10586) --- CHANGELOG.md | 1 + ...consoleLogOutputWhenRunInBand.test.ts.snap | 2 +- .../__snapshots__/globals.test.ts.snap | 4 +- .../consoleLogOutputWhenRunInBand.test.ts | 16 +++++- e2e/__tests__/errorOnDeprecated.test.ts | 24 +++----- e2e/__tests__/failures.test.ts | 11 +--- .../jest-reporters/src/coverage_reporter.ts | 6 +- packages/jest-runtime/src/index.ts | 56 +++++++------------ packages/jest-test-result/src/index.ts | 1 + packages/jest-test-result/src/types.ts | 7 ++- packages/jest-types/src/Transform.ts | 2 +- 11 files changed, 61 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2f6fd665716..2de2ca13883e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ - `[jest-circus, jest-jasmine2]` Find correct location for `test.each` tests ([#10413](https://github.com/facebook/jest/pull/10413)) - `[jest-console]` Add `Console` constructor to `console` object ([#10502](https://github.com/facebook/jest/pull/10502)) - `[jest-globals]` Fix lifecycle hook function types ([#10480](https://github.com/facebook/jest/pull/10480)) +- `[jest-runtime]` Remove usage of `vm.compileFunction` due to a performance issue ([#10586](https://github.com/facebook/jest/pull/10586)) ### Chore & Maintenance diff --git a/e2e/__tests__/__snapshots__/consoleLogOutputWhenRunInBand.test.ts.snap b/e2e/__tests__/__snapshots__/consoleLogOutputWhenRunInBand.test.ts.snap index f4728989cc42..571dd419e639 100644 --- a/e2e/__tests__/__snapshots__/consoleLogOutputWhenRunInBand.test.ts.snap +++ b/e2e/__tests__/__snapshots__/consoleLogOutputWhenRunInBand.test.ts.snap @@ -20,6 +20,6 @@ exports[`prints console.logs when run with forceExit 3`] = ` console.log Hey - at Object.log (__tests__/a-banana.js:1:30) + at Object. (__tests__/a-banana.js:1:1) `; diff --git a/e2e/__tests__/__snapshots__/globals.test.ts.snap b/e2e/__tests__/__snapshots__/globals.test.ts.snap index a3262e166ca1..c8f6286b7360 100644 --- a/e2e/__tests__/__snapshots__/globals.test.ts.snap +++ b/e2e/__tests__/__snapshots__/globals.test.ts.snap @@ -24,9 +24,9 @@ FAIL __tests__/onlyConstructs.test.js Missing second argument. It must be a callback function. > 1 | describe('describe, no implementation'); - | ^ + | ^ - at Object.describe (__tests__/onlyConstructs.test.js:1:1) + at Object. (__tests__/onlyConstructs.test.js:1:10) `; exports[`cannot have describe with no implementation 2`] = ` diff --git a/e2e/__tests__/consoleLogOutputWhenRunInBand.test.ts b/e2e/__tests__/consoleLogOutputWhenRunInBand.test.ts index 618c48048ef6..3e505c906365 100644 --- a/e2e/__tests__/consoleLogOutputWhenRunInBand.test.ts +++ b/e2e/__tests__/consoleLogOutputWhenRunInBand.test.ts @@ -15,6 +15,8 @@ const DIR = path.resolve(__dirname, '../console-log-output-when-run-in-band'); beforeEach(() => cleanup(DIR)); afterAll(() => cleanup(DIR)); +const nodeMajorVersion = Number(process.versions.node.split('.')[0]); + test('prints console.logs when run with forceExit', () => { writeFiles(DIR, { '__tests__/a-banana.js': ` @@ -23,14 +25,26 @@ test('prints console.logs when run with forceExit', () => { 'package.json': '{}', }); - const {stderr, stdout, exitCode} = runJest(DIR, [ + const {stderr, exitCode, ...res} = runJest(DIR, [ '-i', '--ci=false', '--forceExit', ]); + let {stdout} = res; const {rest, summary} = extractSummary(stderr); + if (nodeMajorVersion < 12) { + expect(stdout).toContain( + 'at Object..test (__tests__/a-banana.js:1:1)', + ); + + stdout = stdout.replace( + 'at Object..test (__tests__/a-banana.js:1:1)', + 'at Object. (__tests__/a-banana.js:1:1)', + ); + } + expect(exitCode).toBe(0); expect(wrap(rest)).toMatchSnapshot(); expect(wrap(summary)).toMatchSnapshot(); diff --git a/e2e/__tests__/errorOnDeprecated.test.ts b/e2e/__tests__/errorOnDeprecated.test.ts index 7f8f448cf923..38353f73497b 100644 --- a/e2e/__tests__/errorOnDeprecated.test.ts +++ b/e2e/__tests__/errorOnDeprecated.test.ts @@ -43,24 +43,18 @@ testFiles.forEach(testFile => { expect(result.exitCode).toBe(1); let {rest} = extractSummary(result.stderr); - if (testFile === 'defaultTimeoutInterval.test.js') { + if ( + nodeMajorVersion < 12 && + testFile === 'defaultTimeoutInterval.test.js' + ) { const lineEntry = '(__tests__/defaultTimeoutInterval.test.js:10:3)'; - if (nodeMajorVersion < 10) { - expect(rest).toContain(`at Object..test ${lineEntry}`); + expect(rest).toContain(`at Object..test ${lineEntry}`); - rest = rest.replace( - `at Object..test ${lineEntry}`, - `at Object. ${lineEntry}`, - ); - } else if (nodeMajorVersion < 12) { - expect(rest).toContain(`at Object.test ${lineEntry}`); - - rest = rest.replace( - `at Object.test ${lineEntry}`, - `at Object. ${lineEntry}`, - ); - } + rest = rest.replace( + `at Object..test ${lineEntry}`, + `at Object. ${lineEntry}`, + ); } expect(wrap(rest)).toMatchSnapshot(); diff --git a/e2e/__tests__/failures.test.ts b/e2e/__tests__/failures.test.ts index 042749ca60e8..9eead97b8340 100644 --- a/e2e/__tests__/failures.test.ts +++ b/e2e/__tests__/failures.test.ts @@ -39,7 +39,7 @@ test('not throwing Error objects', () => { expect(wrap(cleanStderr(stderr))).toMatchSnapshot(); stderr = runJest(dir, ['duringTests.test.js']).stderr; - if (nodeMajorVersion < 10) { + if (nodeMajorVersion < 12) { const lineEntry = '(__tests__/duringTests.test.js:38:8)'; expect(stderr).toContain(`at Object..done ${lineEntry}`); @@ -48,15 +48,6 @@ test('not throwing Error objects', () => { `at Object..done ${lineEntry}`, `at Object. ${lineEntry}`, ); - } else if (nodeMajorVersion < 12) { - const lineEntry = '(__tests__/duringTests.test.js:38:8)'; - - expect(stderr).toContain(`at Object.done ${lineEntry}`); - - stderr = stderr.replace( - `at Object.done ${lineEntry}`, - `at Object. ${lineEntry}`, - ); } expect(wrap(cleanStderr(stderr))).toMatchSnapshot(); diff --git a/packages/jest-reporters/src/coverage_reporter.ts b/packages/jest-reporters/src/coverage_reporter.ts index 1faad089806e..9e1918bc8726 100644 --- a/packages/jest-reporters/src/coverage_reporter.ts +++ b/packages/jest-reporters/src/coverage_reporter.ts @@ -10,6 +10,7 @@ import * as fs from 'graceful-fs'; import type {Config} from '@jest/types'; import type { AggregatedResult, + RuntimeTransformResult, TestResult, V8CoverageResult, } from '@jest/test-result'; @@ -24,7 +25,6 @@ import Worker from 'jest-worker'; import glob = require('glob'); import v8toIstanbul = require('v8-to-istanbul'); import type {RawSourceMap} from 'source-map'; -import type {TransformResult} from '@jest/transform'; import BaseReporter from './base_reporter'; import type { Context, @@ -425,7 +425,7 @@ export default class CoverageReporter extends BaseReporter { this._v8CoverageResults.map(cov => ({result: cov.map(r => r.result)})), ); - const fileTransforms = new Map(); + const fileTransforms = new Map(); this._v8CoverageResults.forEach(res => res.forEach(r => { @@ -453,7 +453,7 @@ export default class CoverageReporter extends BaseReporter { const converter = v8toIstanbul( res.url, - 0, + fileTransform?.wrapperLength ?? 0, fileTransform && sourcemapContent ? { originalSource: fileTransform.originalCode, diff --git a/packages/jest-runtime/src/index.ts b/packages/jest-runtime/src/index.ts index 11f8ace43a99..c03500e3f758 100644 --- a/packages/jest-runtime/src/index.ts +++ b/packages/jest-runtime/src/index.ts @@ -16,7 +16,6 @@ import { Context as VMContext, // @ts-expect-error: experimental, not added to the types Module as VMModule, - compileFunction, } from 'vm'; import * as nativeModule from 'module'; import type {Config, Global} from '@jest/types'; @@ -35,12 +34,11 @@ import {escapePathForRegex} from 'jest-regex-util'; import { ScriptTransformer, ShouldInstrumentOptions, - TransformResult, TransformationOptions, handlePotentialSyntaxError, shouldInstrument, } from '@jest/transform'; -import type {V8CoverageResult} from '@jest/test-result'; +import type {RuntimeTransformResult, V8CoverageResult} from '@jest/test-result'; import {CoverageInstrumenter, V8Coverage} from 'collect-v8-coverage'; import * as fs from 'graceful-fs'; import jestMock = require('jest-mock'); @@ -170,7 +168,7 @@ class Runtime { private _shouldUnmockTransitiveDependenciesCache: BooleanMap; private _sourceMapRegistry: StringMap; private _scriptTransformer: ScriptTransformer; - private _fileTransforms: Map; + private _fileTransforms: Map; private _v8CoverageInstrumenter: CoverageInstrumenter | undefined; private _v8CoverageResult: V8Coverage | undefined; private _transitiveShouldMock: BooleanMap; @@ -830,7 +828,7 @@ class Runtime { }); } - // TODO - remove in Jest 26 + // TODO - remove in Jest 27 getSourceMapInfo(_coveredFiles: Set): Record { return {}; } @@ -1005,36 +1003,23 @@ class Runtime { let compiledFunction: ModuleWrapper | null = null; + const script = this.createScriptFromCode(transformedCode, filename); + + let runScript: RunScriptEvalResult | null = null; + // Use this if available instead of deprecated `JestEnvironment.runScript` if (typeof this._environment.getVmContext === 'function') { const vmContext = this._environment.getVmContext(); if (vmContext) { - try { - compiledFunction = compileFunction( - transformedCode, - this.constructInjectedModuleParameters(), - { - filename, - parsingContext: vmContext, - }, - ) as ModuleWrapper; - } catch (e) { - throw handlePotentialSyntaxError(e); - } + runScript = script.runInContext(vmContext, {filename}); } } else { - const script = this.createScriptFromCode(transformedCode, filename); - - const runScript = this._environment.runScript( - script, - ); + runScript = this._environment.runScript(script); + } - if (runScript === null) { - compiledFunction = null; - } else { - compiledFunction = runScript[EVAL_RESULT_VARIABLE]; - } + if (runScript !== null) { + compiledFunction = runScript[EVAL_RESULT_VARIABLE]; } if (compiledFunction === null) { @@ -1097,7 +1082,10 @@ class Runtime { source, ); - this._fileTransforms.set(filename, transformedFile); + this._fileTransforms.set(filename, { + ...transformedFile, + wrapperLength: this.constructModuleWrapperStart().length, + }); if (transformedFile.sourceMapPath) { this._sourceMapRegistry.set(filename, transformedFile.sourceMapPath); @@ -1602,15 +1590,13 @@ class Runtime { } private wrapCodeInModuleWrapper(content: string) { + return this.constructModuleWrapperStart() + content + '\n}});'; + } + + private constructModuleWrapperStart() { const args = this.constructInjectedModuleParameters(); - return ( - '({"' + - EVAL_RESULT_VARIABLE + - `":function(${args.join(',')}){` + - content + - '\n}});' - ); + return '({"' + EVAL_RESULT_VARIABLE + `":function(${args.join(',')}){`; } private constructInjectedModuleParameters(): Array { diff --git a/packages/jest-test-result/src/index.ts b/packages/jest-test-result/src/index.ts index 6f43aba84ce3..c149c10ee4b1 100644 --- a/packages/jest-test-result/src/index.ts +++ b/packages/jest-test-result/src/index.ts @@ -19,6 +19,7 @@ export type { FailedAssertion, FormattedTestResults, Milliseconds, + RuntimeTransformResult, SerializableError, SnapshotSummary, Status, diff --git a/packages/jest-test-result/src/types.ts b/packages/jest-test-result/src/types.ts index 25bcf43e892d..92c013a86586 100644 --- a/packages/jest-test-result/src/types.ts +++ b/packages/jest-test-result/src/types.ts @@ -10,8 +10,13 @@ import type {ConsoleBuffer} from '@jest/console'; import type {Config, TestResult, TransformTypes} from '@jest/types'; import type {V8Coverage} from 'collect-v8-coverage'; +export interface RuntimeTransformResult extends TransformTypes.TransformResult { + // TODO: Make mandatory in Jest 27 + wrapperLength?: number; +} + export type V8CoverageResult = Array<{ - codeTransformResult: TransformTypes.TransformResult | undefined; + codeTransformResult: RuntimeTransformResult | undefined; result: V8Coverage[number]; }>; diff --git a/packages/jest-types/src/Transform.ts b/packages/jest-types/src/Transform.ts index 2e60af1e489d..6371a226c201 100644 --- a/packages/jest-types/src/Transform.ts +++ b/packages/jest-types/src/Transform.ts @@ -9,6 +9,6 @@ export type TransformResult = { code: string; originalCode: string; - mapCoverage?: boolean; // TODO - Remove in Jest 26 + mapCoverage?: boolean; // TODO - Remove in Jest 27 sourceMapPath: string | null; };