From 898979c2c035795d9deb028175f8b8083a5d5a61 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 21 Apr 2022 01:38:49 -0700 Subject: [PATCH 1/3] add s/const/var jest transformer --- jest/transformers/constReplacer.ts | 84 ++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 jest/transformers/constReplacer.ts diff --git a/jest/transformers/constReplacer.ts b/jest/transformers/constReplacer.ts new file mode 100644 index 000000000000..725aa9ac48c8 --- /dev/null +++ b/jest/transformers/constReplacer.ts @@ -0,0 +1,84 @@ +/** + * This is a transformer which `ts-jest` applies during the compilation process, which switches all of the `const`s out + * for `var`s. Unlike in our package builds, where we do the same substiution for bundle size reasons, here we do it + * because otherwise `const global = getGlobalObject()` throws an error about redifining `global`. (This didn't used to + * be a problem because our down-compilation did the `const`-`var` substitution for us, but now that we're ES6-only, we + * have to do it ourselves.) + * + * Note: If you ever have to change this, and are testing it locally in the process, be sure to call + * `yarn jest --clearCache` + * before each test run, as transformation results are cached between runs. + */ + +import { + createVariableDeclarationList, + getCombinedNodeFlags, + isVariableDeclarationList, + Node, + NodeFlags, + SourceFile, + TransformationContext, + Transformer, + TransformerFactory, + visitEachChild, + visitNode, + VisitResult, +} from 'typescript'; + +// These can be anything - they're just used to construct a cache key for the transformer returned by the factory below. +// This really only matters when you're testing the transformer itself, as changing these values gives you a quick way +// to invalidate the cache and ensure that changes you've made to the code here are immediately picked up on and used. +export const name = 'const-to-var'; +export const version = '1.0'; + +/** + * Check whether the given AST node represents a `const` token. + * + * This function comes from the TS compiler, and is copied here to get around the fact that it's not exported by the + * `typescript` package. + * + * @param node The node to check + * @returns A boolean indicating if the node is a `const` token. + */ +function isVarConst(node: Node): boolean { + // eslint-disable-next-line no-bitwise + return !!(getCombinedNodeFlags(node) & NodeFlags.Const); +} + +/** + * Return a set of nested factory functions, which ultimately creates an AST-node visitor function, which can modify + * each visited node as it sees fit, and uses it to walk the AST, returning the results. + * + * In our case, we're modifying all `const` variable declarations to use `var` instead. + */ +export function factory(): TransformerFactory { + // Create the transformer + function transformerFactory(context: TransformationContext): Transformer { + // Create a visitor function and use it to walk the AST + function transformer(sourceFile: SourceFile): SourceFile { + // This visitor function can either return a node, in which case the subtree rooted at the returned node is + // substituted for the subtree rooted at the visited node, or can use the recursive `visitEachChild` function + // provided by TS to continue traversing the tree. + function visitor(node: Node): VisitResult { + // If we've found a `const` declaration, return a `var` declaration in its place + if (isVariableDeclarationList(node) && isVarConst(node)) { + // A declaration list with a `None` flag defaults to using `var` + return createVariableDeclarationList(node.declarations, NodeFlags.None); + } + + // This wasn't a node we're interested in, so keep walking down the tree. + return visitEachChild(node, visitor, context); + } + + // Having defined our visitor, pass it to the TS-provided `visitNode` function, which will use it to walk the AST, + // and return the results of that walk. + return visitNode(sourceFile, visitor); + } + + // Back in the transformer factory, return the transformer we just created + return transformer; + } + + // Finally, we're back in `factory`, and can return the whole nested system + return transformerFactory; +} From 3157c2c780d4347e43f1a7881f692e4828ef169d Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 22 Apr 2022 08:10:38 -0700 Subject: [PATCH 2/3] reorganize test script and fix transformer use in node 8 --- scripts/test.ts | 165 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 113 insertions(+), 52 deletions(-) diff --git a/scripts/test.ts b/scripts/test.ts index 1232d9f0d104..1aa4dc6ac8b1 100644 --- a/scripts/test.ts +++ b/scripts/test.ts @@ -1,66 +1,127 @@ -import { spawnSync } from 'child_process'; -import { join } from 'path'; +import * as childProcess from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; -function run(cmd: string, cwd: string = '') { - const result = spawnSync(cmd, { shell: true, stdio: 'inherit', cwd: join(__dirname, '..', cwd || '') }); +const CURRENT_NODE_VERSION = process.version.replace('v', '').split('.')[0]; - if (result.status !== 0) { - process.exit(result.status || undefined); - } -} +// We run ember tests in their own job. +const DEFAULT_SKIP_TESTS_PACKAGES = ['@sentry/ember']; +// These packages don't support Node 8 for syntax or dependency reasons. +const NODE_8_SKIP_TESTS_PACKAGES = [ + ...DEFAULT_SKIP_TESTS_PACKAGES, + '@sentry-internal/eslint-plugin-sdk', + '@sentry/react', + '@sentry/wasm', + '@sentry/gatsby', + '@sentry/serverless', + '@sentry/nextjs', + '@sentry/angular', +]; -const nodeMajorVersion = parseInt(process.version.split('.')[0].replace('v', ''), 10); +// We have to downgrade some of our dependencies in order to run tests in Node 8 and 10. +const NODE_8_LEGACY_DEPENDENCIES = [ + 'jsdom@15.x', + 'jest@25.x', + 'jest-environment-jsdom@25.x', + 'jest-environment-node@25.x', + 'ts-jest@25.x', +]; +const NODE_10_LEGACY_DEPENDENCIES = ['jsdom@16.x']; -// Ember tests require dependency changes for each set of tests, making them quite slow. To compensate for this, in CI -// we run them in a separate, parallel job. -let ignorePackages = ['@sentry/ember']; +/** + * Run the given shell command, piping the shell process's `stdin`, `stdout`, and `stderr` to that of the current + * process. Returns contents of `stdout`. + */ +function run(cmd: string, options?: childProcess.ExecSyncOptions) { + return childProcess.execSync(cmd, { stdio: 'inherit', ...options }); +} -// install legacy versions of third-party packages whose current versions don't support node 8 or 10, and skip testing -// our own packages which don't support node 8 for various syntax or dependency reasons -if (nodeMajorVersion <= 10) { - let legacyDependencies; +/** + * Install the given legacy dependencies, for compatibility with tests run in older versions of Node. + */ +function installLegacyDeps(legacyDeps: string[] = []): void { + // Ignoring engines and scripts lets us get away with having incompatible things installed for SDK packages we're not + // testing in the current node version, and ignoring the root check lets us install things at the repo root. + run(`yarn add --dev --ignore-engines --ignore-scripts --ignore-workspace-root-check ${legacyDeps.join(' ')}`); +} - if (nodeMajorVersion === 8) { - legacyDependencies = [ - 'jsdom@15.x', - 'jest@25.x', - 'jest-environment-jsdom@25.x', - 'jest-environment-node@25.x', - 'ts-jest@25.x', - ]; +/** + * Add a tranformer to our jest config, to do the same `const`-to-`var` replacement as our rollup plugin does. + * + * This is needed because Node 8 doesn't like the way we shadow `global` (`const global = getGlobalObject()`). Changing + * it to a `var` solves this by making it redeclarable. + * + */ +function addTransformer(): void { + // Though newer `ts-jest` versions support transformers written in TS, the legacy version does not. + run('yarn tsc --skipLibCheck jest/transformers/constReplacer.ts'); - ignorePackages = [ - ...ignorePackages, - '@sentry-internal/eslint-plugin-sdk', - '@sentry/react', - '@sentry/wasm', - '@sentry/gatsby', - '@sentry/serverless', - '@sentry/nextjs', - '@sentry/angular', - ]; + // Loading the existing Jest config will error out unless the config file has an accompanying types file, so we have + // to create that before we can load it. + run('yarn tsc --allowJs --skipLibCheck --declaration --emitDeclarationOnly jest/jest.config.js'); + // eslint-disable-next-line @typescript-eslint/no-var-requires + const jestConfig = require('../jest/jest.config.js'); - // This is a hack, to deal the fact that the browser-based tests fail under Node 8, because of a conflict buried - // somewhere in the interaction between our current overall set of dependencies and the older versions of a small - // subset we're about to install below. Since they're browser-based, these tests are never going to be running in a - // node 8 environment in any case, so it's fine to skip them here. (In the long run, we should only run such tests - // against a single version of node, but in the short run, this at least allows us to not be blocked by the - // failures.) - run('rm -rf packages/tracing/test/browser'); - } - // Node 10 - else { - legacyDependencies = ['jsdom@16.x']; - } + // Inject the transformer + jestConfig.globals['ts-jest'].astTransformers = ['/../../jest/transformers/constReplacer.js']; - const legacyDepStr = legacyDependencies.join(' '); + // When we required the jest config file above, all expressions it contained were evaluated. Specifically, the + // `rootDir: process.cwd()` + // entry was replaced with + // `rootDir: ""`, + // Though it's a little brute-force-y, the easiest way to fix this is to just stringify the code and perform the + // substitution in reverse. + const stringifiedConfig = JSON.stringify(jestConfig, null, 2).replace( + `"rootDir": "${process.cwd()}"`, + 'rootDir: process.cwd()', + ); - // ignoring engines and scripts lets us get away with having incompatible things installed for packages we're not testing - run(`yarn add --dev --ignore-engines --ignore-scripts --ignore-workspace-root-check ${legacyDepStr}`); + // Now we just have to convert it back to a module and write it to disk + const code = `module.exports = ${stringifiedConfig}`; + fs.writeFileSync(path.resolve('jest/jest.config.js'), code); } -const ignoreFlags = ignorePackages.map(dep => `--ignore="${dep}"`).join(' '); +/** + * Skip tests which don't apply to Node and therefore don't need to run in older Node versions. + * + * TODO We're foreced to skip these tests for compatibility reasons (right now this function only gets called in Node + * 8), but we could be skipping a lot more tests in Node 8-14 - anything where compatibility with different Node + * versions is irrelevant - and only running them in Node 16. + */ +function skipNonNodeTests(): void { + run('rm -rf packages/tracing/test/browser'); +} -run(`yarn test ${ignoreFlags}`); +/** + * Run tests, ignoring the given packages + */ +function runWithIgnores(skipPackages: string[] = []): void { + const ignoreFlags = skipPackages.map(dep => `--ignore="${dep}"`).join(' '); + run(`yarn test ${ignoreFlags}`); +} + +/** + * Run the tests, accounting for compatibility problems in older versions of Node. + */ +function runTests(): void { + if (CURRENT_NODE_VERSION === '8') { + installLegacyDeps(NODE_8_LEGACY_DEPENDENCIES); + // Inject a `const`-to-`var` transformer, in order to stop Node 8 from complaining when we shadow `global` + addTransformer(); + // TODO Right now, this just skips incompatible tests, but it could be skipping more (hence the aspirational name), + // and not just in Node 8. See `skipNonNodeTests`'s docstring. + skipNonNodeTests(); + runWithIgnores(NODE_8_SKIP_TESTS_PACKAGES); + } + // + else if (CURRENT_NODE_VERSION === '10') { + installLegacyDeps(NODE_10_LEGACY_DEPENDENCIES); + runWithIgnores(DEFAULT_SKIP_TESTS_PACKAGES); + } + // + else { + runWithIgnores(DEFAULT_SKIP_TESTS_PACKAGES); + } +} -process.exit(0); +runTests(); From 650f2ca0d713c3a2077a3507178f2e2ef9bd4e55 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 22 Apr 2022 18:49:23 -0700 Subject: [PATCH 3/3] add compiled ts-jest transformers to .gitignore --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index ae301fadfde7..430a977178a7 100644 --- a/.gitignore +++ b/.gitignore @@ -12,6 +12,8 @@ scratch/ *.pyc *.tsbuildinfo scenarios/*/dist/ +# transpiled transformers +jest/transformers/*.js # logs yarn-error.log