ronantakizawa/codereview-bench huggingface dataset:
Repository: facebook/jest
File: packages/jest-test-sequencer/src/index.ts (TypeScript)

Code before:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

import * as crypto from 'crypto';
import * as path from 'path';
import * as fs from 'graceful-fs';
import slash = require('slash');
import type {AggregatedResult, Test, TestContext} from '@jest/test-result';
import type {Config} from '@jest/types';
import HasteMap from 'jest-haste-map';

const FAIL = 0;
const SUCCESS = 1;

export type TestSequencerOptions = {
  contexts: Array<TestContext>;
  globalConfig: Config.GlobalConfig;
};

type Cache = {
  [key: string]:
    | [testStatus: typeof FAIL | typeof SUCCESS, testDuration: number]
    | undefined;
};

export type ShardOptions = {
  shardIndex: number;
  shardCount: number;
};

type ShardPositionOptions = ShardOptions & {
  suiteLength: number;
};

/**
 * The TestSequencer will ultimately decide which tests should run first.
 * It is responsible for storing and reading from a local cache
 * map that stores context information for a given test, such as how long it
 * took to run during the last run and if it has failed or not.
 * Such information is used on:
 * TestSequencer.sort(tests: Array<Test>)

Review comment (suggestion):
```suggestion
  contexts: ReadonlyArray<TestContext>;
```

Code after:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

import * as crypto from 'crypto';
import * as path from 'path';
import * as fs from 'graceful-fs';
import slash = require('slash');
import type {AggregatedResult, Test, TestContext} from '@jest/test-result';
import type {Config} from '@jest/types';
import HasteMap from 'jest-haste-map';

const FAIL = 0;
const SUCCESS = 1;

export type TestSequencerOptions = {
  contexts: ReadonlyArray<TestContext>;
  globalConfig: Config.GlobalConfig;
};

type Cache = {
  [key: string]:
    | [testStatus: typeof FAIL | typeof SUCCESS, testDuration: number]
    | undefined;
};

export type ShardOptions = {
  shardIndex: number;
  shardCount: number;
};

type ShardPositionOptions = ShardOptions & {
  suiteLength: number;
};

/**
 * The TestSequencer will ultimately decide which tests should run first.
 * It is responsible for storing and reading from a local cache
 * map that stores context information for a given test, such as how long it
 * took to run during the last run and if it has failed or not.
 * Such information is used on:
 * TestSequencer.sort(tests: Array<Test>)
Repository: facebook/jest
File: scripts/build.mjs (JavaScript)

Code before:
    // type only packages
    if (typeOnlyPackages.has(pkg.name)) {
      continue;
    }

    const entryPoint = require(entryPointFile);
    const exportStatements = Object.keys(entryPoint)
      .filter(name => name !== '__esModule' && name !== 'default')
      .map(name => `export const ${name} = entryPoint.${name};`);

    if (entryPoint.default) {
      exportStatements.push('export default entryPoint.default;');
    }

    if (exportStatements.length === 0) {
      throw new Error(`No exports found in package ${pkg.name}`);
    }

    const mjsEntryFile = entryPointFile.replace(/\.js$/, '.mjs');

    const esSource = dedent`
      import {createRequire} from 'module';

      const require = createRequire(import.meta.url);

      const entryPoint = require('./index.js');

      ${exportStatements.join('\n')}
    `;

    await fs.promises.writeFile(mjsEntryFile, `${esSource}\n`);
  }

  process.stdout.write(`${OK}\n`);
}

buildNodePackages().catch(error => {
  console.error(error);
  process.exitCode = 1;
});


Review comment (suggestion):
not sure how bundlers deal with `createRequire` calls... we might have to adjust if people say it breaks them 😀 

Maybe top level `await import('./index.js')` is better

Code after:
    if (typeOnlyPackages.has(pkg.name)) {
      continue;
    }

    // TODO: can we get exports from a file from webpack's `stats`?
    const cjsModule = require(entryPointFile);
    const exportStatements = Object.keys(cjsModule)
      .filter(name => name !== '__esModule' && name !== 'default')
      .map(name => `export const ${name} = cjsModule.${name};`);

    if (cjsModule.default) {
      exportStatements.push('export default cjsModule.default;');
    }

    if (exportStatements.length === 0) {
      throw new Error(`No exports found in package ${pkg.name}`);
    }

    const mjsEntryFile = entryPointFile.replace(/\.js$/, '.mjs');

    const esSource = dedent`
      import cjsModule from './index.js';

      ${exportStatements.join('\n')}
    `;

    await fs.promises.writeFile(mjsEntryFile, `${esSource}\n`);
  }

  process.stdout.write(`${OK}\n`);
}

buildNodePackages().catch(error => {
  console.error(error);
  process.exitCode = 1;
});
Repository: facebook/jest
File: e2e/custom-esm-test-sequencer/testSequencer.mjs (JavaScript)

Code before:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

import Sequencer from '@jest/test-sequencer';

export default class CustomSequencer extends Sequencer.default {
  sort(tests) {
    const copyTests = Array.from(tests);
    return copyTests.sort((testA, testB) => (testA.path > testB.path ? 1 : -1));
  }
}


Review comment (suggestion):
this is very nice, but also makes it a breaking change for people who have used Jest's modules in native ESM before. Which is probably _fine_, but we need to call this out in the upgrade guide

Code after:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

import Sequencer from '@jest/test-sequencer';

export default class CustomSequencer extends Sequencer {
  sort(tests) {
    const copyTests = Array.from(tests);
    return copyTests.sort((testA, testB) => (testA.path > testB.path ? 1 : -1));
  }
}
Repository: facebook/jest
File: packages/jest-config/src/readConfigFileAndSetRootDir.ts (TypeScript)

Code before:
  // In case the config is a function which imports more Typescript code
  if (typeof configObject === 'function') {
    configObject = await configObject();
  }

  registeredCompiler.enabled(false);

  return configObject;
};

let registeredCompilerPromise: Promise<Service>;

function getRegisteredCompiler() {
  // Cache the promise to avoid multiple registrations
  registeredCompilerPromise = registeredCompilerPromise ?? registerTsNode();
  return registeredCompilerPromise;
}

async function registerTsNode(): Promise<Service> {
  try {
    // Register TypeScript compiler instance
    const tsNode = await import(/* webpackIgnore: true */ 'ts-node');
    return tsNode.register({
      compilerOptions: {
        module: 'CommonJS',
        moduleResolution: 'Node',
      },
      moduleTypes: {
        '**': 'cjs',
      },
    });
  } catch (e: any) {
    if (e.code === 'ERR_MODULE_NOT_FOUND') {
      throw new Error(
        `Jest: 'ts-node' is required for the TypeScript configuration files. Make sure it is installed\nError: ${e.message}`,
      );
    }

    throw e;
  }
}


Review comment (suggestion):
Would recommend going with `Node10`, which is the "new" name for this value.

Code after:

  let configObject = interopRequireDefault(require(configPath)).default;

  // In case the config is a function which imports more Typescript code
  if (typeof configObject === 'function') {
    configObject = await configObject();
  }

  registeredCompiler.enabled(false);

  return configObject;
};

let registeredCompilerPromise: Promise<Service>;

function getRegisteredCompiler() {
  // Cache the promise to avoid multiple registrations
  registeredCompilerPromise = registeredCompilerPromise ?? registerTsNode();
  return registeredCompilerPromise;
}

async function registerTsNode(): Promise<Service> {
  try {
    // Register TypeScript compiler instance
    const tsNode = await import(/* webpackIgnore: true */ 'ts-node');
    return tsNode.register({
      compilerOptions: {
        module: 'CommonJS',
        moduleResolution: 'Node10',
      },
      moduleTypes: {
        '**': 'cjs',
      },
    });
  } catch (error: any) {
    if (error.code === 'ERR_MODULE_NOT_FOUND') {
      throw new Error(
        `Jest: 'ts-node' is required for the TypeScript configuration files. Make sure it is installed\nError: ${error.message}`,
      );
    }

    throw error;
  }
}
Repository: facebook/jest
File: .eslintrc.cjs (JavaScript)

Code before:
        'packages/jest-jasmine2/src/jasmine/**/*',
        'packages/expect-utils/src/jasmineUtils.ts',
      ],
      rules: {
        'eslint-comments/disable-enable-pair': 'off',
        'eslint-comments/no-unlimited-disable': 'off',
      },
    },
    {
      files: [
        'e2e/error-on-deprecated/__tests__/*',
        'e2e/jasmine-async/__tests__/*',
      ],
      globals: {
        fail: 'readonly',
        jasmine: 'readonly',
        pending: 'readonly',
      },
    },
    {
      files: [
        'e2e/**',
        'website/**',
        '**/__benchmarks__/**',
        '**/__tests__/**',
        'packages/jest-types/**/*',
        '.eslintplugin/**',
      ],
      rules: {
        'import/no-extraneous-dependencies': 'off',
      },
    },
    {
      files: ['**/__typetests__/**'],
      rules: {
        '@typescript-eslint/no-empty-function': 'off',
        '@typescript-eslint/no-invalid-void-type': 'off',
        '@typescript-eslint/no-useless-constructor': 'off',
      },
    },
    {
      env: {node: true},
      files: ['*.js', '*.jsx', '*.mjs', '*.cjs'],
    },
    {
      files: [
        'scripts/*',
        'packages/*/__benchmarks__/test.js',
        'packages/create-jest/src/runCreate.ts',
        'packages/jest-repl/src/cli/runtime-cli.ts',
      ],

Review comment (suggestion):
Strange to see `jest-types` here. Might be there was some limitation, but now lint is passing (at least locally).

It feels right adding `**/__typetests__/**` to the list, or?

Alternatively `tstyche` could be declared in each `package.json`, but that feels somewhat unnecessary. The `@jest/types` did not require that, because of the line I suggest to remove here. That’s why I though adding `**/__typetests__/**` here can be good idea.

Code after:
        'packages/jest-jasmine2/src/jasmine/**/*',
        'packages/expect-utils/src/jasmineUtils.ts',
      ],
      rules: {
        'eslint-comments/disable-enable-pair': 'off',
        'eslint-comments/no-unlimited-disable': 'off',
      },
    },
    {
      files: [
        'e2e/error-on-deprecated/__tests__/*',
        'e2e/jasmine-async/__tests__/*',
      ],
      globals: {
        fail: 'readonly',
        jasmine: 'readonly',
        pending: 'readonly',
      },
    },
    {
      files: [
        'e2e/**',
        'website/**',
        '**/__benchmarks__/**',
        '**/__tests__/**',
        '**/__typetests__/**',
        '.eslintplugin/**',
      ],
      rules: {
        'import/no-extraneous-dependencies': 'off',
      },
    },
    {
      files: ['**/__typetests__/**'],
      rules: {
        '@typescript-eslint/no-empty-function': 'off',
        '@typescript-eslint/no-invalid-void-type': 'off',
        '@typescript-eslint/no-useless-constructor': 'off',
      },
    },
    {
      env: {node: true},
      files: ['*.js', '*.jsx', '*.mjs', '*.cjs'],
    },
    {
      files: [
        'scripts/*',
        'packages/*/__benchmarks__/test.js',
        'packages/create-jest/src/runCreate.ts',
        'packages/jest-repl/src/cli/runtime-cli.ts',
      ],
Repository: facebook/jest
File: packages/jest/__typetests__/jest.test.ts (TypeScript)

Code before:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

import {expectType} from 'tsd-lite';
import type {Config as ConfigTypes} from '@jest/types';
import type {Config} from 'jest';

declare const config: Config;

expectType<ConfigTypes.InitialOptions>(config);


Review comment (suggestion):
By the way, TSTyche can compare two types just like that. No need of `declare const`.

Code after:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

import {describe, expect, test} from 'tstyche';
import type {Config} from 'jest';

describe('Config', () => {
  test('is a reexport of the `InitialOptions`', () => {
    type InitialOptions = import('@jest/types').Config.InitialOptions;

    expect<Config>().type.toEqual<InitialOptions>();
  });
});
Repository: facebook/jest
File: packages/jest-mock/src/__tests__/index.test.ts (TypeScript)

Code before:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 */

/* eslint-disable local/ban-types-eventually, local/prefer-rest-params-eventually */

import * as util from 'util';
import {type Context, createContext, runInContext, runInNewContext} from 'vm';
import {ModuleMocker, fn, mocked, spyOn} from '../';

if (!Symbol.dispose) {
  Object.defineProperty(Symbol, 'dispose', {
    get() {
      return Symbol.for('nodejs.dispose');
    },
  });
}

describe('moduleMocker', () => {
  let moduleMocker: ModuleMocker;
  let mockContext: Context;
  let mockGlobals: typeof globalThis;

  beforeEach(() => {
    mockContext = createContext();
    mockGlobals = runInNewContext('this', mockContext);
    moduleMocker = new ModuleMocker(mockGlobals);
  });

  describe('getMetadata', () => {
    it('returns the function `name` property', () => {
      function x() {}
      const metadata = moduleMocker.getMetadata(x);
      expect(x.name).toBe('x');
      expect(metadata!.name).toBe('x');
    });

    it('does not return broken name property', () => {
      class By {

Review comment (suggestion):
This is a bit weird, and the result of #14888 - usually, it should be possible to polyfill `Symbol.dispose` in any possible way, e.g. by doing `Symbol.dispose ??= Symbol.for('dispose')`, but because of #14888 it needs to explicitly be polyfilled with `Symbol.for('nodejs.dispose')`.  

I'm not sure if that change over there was a good idea, especially because that polyfill doesn't seem to be available in all scopes (that's why we need to polyfill here in the first place).

Code after:
/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 */

/* eslint-disable local/ban-types-eventually, local/prefer-rest-params-eventually */

import * as util from 'util';
import {type Context, createContext, runInContext, runInNewContext} from 'vm';
import {ModuleMocker, fn, mocked, spyOn} from '../';

describe('moduleMocker', () => {
  let moduleMocker: ModuleMocker;
  let mockContext: Context;
  let mockGlobals: typeof globalThis;

  beforeEach(() => {
    mockContext = createContext();
    mockGlobals = runInNewContext('this', mockContext);
    moduleMocker = new ModuleMocker(mockGlobals);
  });

  describe('getMetadata', () => {
    it('returns the function `name` property', () => {
      function x() {}
      const metadata = moduleMocker.getMetadata(x);
      expect(x.name).toBe('x');
      expect(metadata!.name).toBe('x');
    });

    it('does not return broken name property', () => {
      class By {
        // @ts-expect-error
        static name() {
          return 'this is not a name';
        }
      }
      const metadata = moduleMocker.getMetadata(By);
      expect(typeof By.name).toBe('function');
      expect(metadata).not.toHaveProperty('name');
Repository: facebook/jest
File: e2e/coverage-handlebars/transform-handlebars.js (JavaScript)

Code before:
/*
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

const {build} = require('@jridgewell/build-mapping');
const {encodedMap, AnyMap} = require('@jridgewell/trace-mapping');
const Handlebars = require('handlebars/dist/cjs/handlebars.js');
const dedent = require('string-dedent');

exports.process = (code, filename) => {
  const pc = Handlebars.precompile(code, {srcName: filename});
  const out = dedent(build)`
    const Handlebars = require("handlebars/dist/cjs/handlebars.runtime.js");
    module.exports = Handlebars.template(${pc});
  `;
  return {code: out.code, map: encodedMap(new AnyMap(out.map))};
};


Review comment (suggestion):
I just implemented a `normalize` function that will output a compatible source map without flattening (which should be much faster):
```suggestion
  return normalize(out);
```

Code after:
/*
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

const {build} = require('@jridgewell/build-mapping');
const Handlebars = require('handlebars/dist/cjs/handlebars.js');
const dedent = require('string-dedent');

exports.process = (code, filename) => {
  const pc = Handlebars.precompile(code, {srcName: filename});
  return dedent(build)`
    const Handlebars = require("handlebars/dist/cjs/handlebars.runtime.js");
    module.exports = Handlebars.template(${pc});
  `;
};
Repository: facebook/jest
File: e2e/coverage-handlebars/transform-handlebars.js (JavaScript)

Code before:
/*
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

const {build} = require('@jridgewell/build-mapping');
const {encodedMap, AnyMap} = require('@jridgewell/trace-mapping');
const Handlebars = require('handlebars/dist/cjs/handlebars.js');
const dedent = require('string-dedent');

exports.process = (code, filename) => {
  const pc = Handlebars.precompile(code, {srcName: filename});
  const out = dedent(build)`
    const Handlebars = require("handlebars/dist/cjs/handlebars.runtime.js");
    module.exports = Handlebars.template(${pc});
  `;
  return {code: out.code, map: encodedMap(new AnyMap(out.map))};
};


Review comment (suggestion):
```suggestion
const {build, normalize} = require('@jridgewell/build-mapping');
```

Code after:
/*
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

const {build} = require('@jridgewell/build-mapping');
const Handlebars = require('handlebars/dist/cjs/handlebars.js');
const dedent = require('string-dedent');

exports.process = (code, filename) => {
  const pc = Handlebars.precompile(code, {srcName: filename});
  return dedent(build)`
    const Handlebars = require("handlebars/dist/cjs/handlebars.runtime.js");
    module.exports = Handlebars.template(${pc});
  `;
};
Repository: facebook/jest
File: e2e/coverage-handlebars/transform-handlebars.js (JavaScript)

Code before:
/*
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

const {build, normalize} = require('@jridgewell/build-mapping');
const Handlebars = require('handlebars/dist/cjs/handlebars.js');
const dedent = require('string-dedent');

exports.process = (code, filename) => {
  const pc = Handlebars.precompile(code, {srcName: filename});
  const out = dedent(build)`
    const Handlebars = require("handlebars/dist/cjs/handlebars.runtime.js");
    module.exports = Handlebars.template(${pc});
  `;
  return normalize(out);
};


Review comment (suggestion):
Sorry to keep changing things. I decided to make `build` normalize by default (it's actually a bit faster this way, avoids a tree recursion). There's a separate `unnormalizedBuild` function, but since you want normalized you should just stick with `build`.
```suggestion
const {build} = require('@jridgewell/build-mapping');
```

Code after:
/*
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

const {build} = require('@jridgewell/build-mapping');
const Handlebars = require('handlebars/dist/cjs/handlebars.js');
const dedent = require('string-dedent');

exports.process = (code, filename) => {
  const pc = Handlebars.precompile(code, {srcName: filename});
  return dedent(build)`
    const Handlebars = require("handlebars/dist/cjs/handlebars.runtime.js");
    module.exports = Handlebars.template(${pc});
  `;
};
Repository: facebook/jest
File: packages/jest-circus/src/run.ts (TypeScript)

Code before:

    while (numRetriesAvailable > 0 && test.errors.length > 0) {
      // Clear errors so retries occur
      await dispatch({name: 'test_retry', test});

      if (waitBeforeRetry > 0) {
        await new Promise(resolve => setTimeout(resolve, waitBeforeRetry));
      }

      await _runTest(test, isSkipped);
      numRetriesAvailable--;
    }
  };

  for (const child of describeBlock.children) {
    switch (child.type) {
      case 'describeBlock': {
        await _runTestsForDescribeBlock(child, rng);
        break;
      }
      case 'test': {
        const hasErrorsBeforeTestRun = child.errors.length > 0;
        const hasRetryTimes = retryTimes > 0;
        await _runTest(child, isSkipped);

        //If immediate retry is set, we retry the test immediately after the first run
        if (
          retryImmediately &&
          hasErrorsBeforeTestRun === false &&
          hasRetryTimes
        ) {
          await rerunTest(child);
        }

        if (
          hasErrorsBeforeTestRun === false &&
          hasRetryTimes &&
          !retryImmediately
        ) {
          deferredRetryTests.push(child);
        }
        break;
      }
    }
  }

  // Re-run failed tests n-times if configured
  for (const test of deferredRetryTests) {
    await rerunTest(test);
  }


Review comment (suggestion):
```suggestion
        // If immediate retry is set, we retry the test immediately after the first run
```

Code after:

    while (numRetriesAvailable > 0 && test.errors.length > 0) {
      // Clear errors so retries occur
      await dispatch({name: 'test_retry', test});

      if (waitBeforeRetry > 0) {
        await new Promise(resolve => setTimeout(resolve, waitBeforeRetry));
      }

      await _runTest(test, isSkipped);
      numRetriesAvailable--;
    }
  };

  for (const child of describeBlock.children) {
    switch (child.type) {
      case 'describeBlock': {
        await _runTestsForDescribeBlock(child, rng);
        break;
      }
      case 'test': {
        const hasErrorsBeforeTestRun = child.errors.length > 0;
        const hasRetryTimes = retryTimes > 0;
        await _runTest(child, isSkipped);

        // If immediate retry is set, we retry the test immediately after the first run
        if (
          retryImmediately &&
          hasErrorsBeforeTestRun === false &&
          hasRetryTimes
        ) {
          await rerunTest(child);
        }

        if (
          hasErrorsBeforeTestRun === false &&
          hasRetryTimes &&
          !retryImmediately
        ) {
          deferredRetryTests.push(child);
        }
        break;
      }
    }
  }

  // Re-run failed tests n-times if configured
  for (const test of deferredRetryTests) {
    await rerunTest(test);
  }