ronantakizawa/codereview-bench huggingface dataset:
Repository: facebook/jest
File: e2e/__tests__/testRetries.test.ts (TypeScript)

Code before:
import runJest from '../runJest';

skipSuiteOnJasmine();

describe('Test Retries', () => {
  const outputFileName = 'retries.result.json';
  const outputFilePath = path.join(
    process.cwd(),
    'e2e/test-retries/',
    outputFileName,
  );
  const logErrorsBeforeRetryErrorMessage =
    'Errors that caused Jest to retry test: ';

  afterAll(() => {
    fs.unlinkSync(outputFilePath);
  });

  it('retries failed tests', () => {
    const result = runJest('test-retries', ['e2e.test.js']);

    expect(result.exitCode).toEqual(0);
    expect(result.failed).toBe(false);
    expect(result.stdout.includes(logErrorsBeforeRetryErrorMessage)).toBe(
      false,
    );
  });

  it('logs error(s) before retry', () => {
    const result = runJest('test-retries', ['logErrorsBeforeRetries.test.js']);
    expect(result.exitCode).toEqual(0);
    expect(result.failed).toBe(false);
    expect(result.stdout.includes(logErrorsBeforeRetryErrorMessage)).toBe(true);
  });

  it('reporter shows more than 1 invocation if test is retried', () => {
    let jsonResult;

    const reporterConfig = {
      reporters: [
        ['<rootDir>/reporters/RetryReporter.js', {output: outputFilePath}],
      ],
    };

    runJest('test-retries', [
      '--config',
      JSON.stringify(reporterConfig),
      'retry.test.js',
    ]);

    const testOutput = fs.readFileSync(outputFilePath, 'utf8');

Review comment (suggestion):
```suggestion
    expect(result.stdout).not.toContain(logErrorsBeforeRetryErrorMessage);
```

Code after:
import {extractSummary} from '../Utils';
import runJest from '../runJest';

skipSuiteOnJasmine();

describe('Test Retries', () => {
  const outputFileName = 'retries.result.json';
  const outputFilePath = path.join(
    process.cwd(),
    'e2e/test-retries/',
    outputFileName,
  );
  const logErrorsBeforeRetryErrorMessage = 'LOGGING RETRY ERRORS';

  afterAll(() => {
    fs.unlinkSync(outputFilePath);
  });

  it('retries failed tests', () => {
    const result = runJest('test-retries', ['e2e.test.js']);

    expect(result.exitCode).toEqual(0);
    expect(result.failed).toBe(false);
    expect(result.stderr).not.toContain(logErrorsBeforeRetryErrorMessage);
  });

  it('logs error(s) before retry', () => {
    const result = runJest('test-retries', ['logErrorsBeforeRetries.test.js']);
    expect(result.exitCode).toEqual(0);
    expect(result.failed).toBe(false);
    expect(result.stderr).toContain(logErrorsBeforeRetryErrorMessage);
    expect(extractSummary(result.stderr).rest).toMatchSnapshot();
  });

  it('reporter shows more than 1 invocation if test is retried', () => {
    let jsonResult;

    const reporterConfig = {
      reporters: [
        ['<rootDir>/reporters/RetryReporter.js', {output: outputFilePath}],
      ],
    };

    runJest('test-retries', [
      '--config',
      JSON.stringify(reporterConfig),
      'retry.test.js',
    ]);

    const testOutput = fs.readFileSync(outputFilePath, 'utf8');
Repository: facebook/jest
File: e2e/__tests__/testRetries.test.ts (TypeScript)

Code before:
    process.cwd(),
    'e2e/test-retries/',
    outputFileName,
  );
  const logErrorsBeforeRetryErrorMessage =
    'Errors that caused Jest to retry test: ';

  afterAll(() => {
    fs.unlinkSync(outputFilePath);
  });

  it('retries failed tests', () => {
    const result = runJest('test-retries', ['e2e.test.js']);

    expect(result.exitCode).toEqual(0);
    expect(result.failed).toBe(false);
    expect(result.stdout.includes(logErrorsBeforeRetryErrorMessage)).toBe(
      false,
    );
  });

  it('logs error(s) before retry', () => {
    const result = runJest('test-retries', ['logErrorsBeforeRetries.test.js']);
    expect(result.exitCode).toEqual(0);
    expect(result.failed).toBe(false);
    expect(result.stdout.includes(logErrorsBeforeRetryErrorMessage)).toBe(true);
  });

  it('reporter shows more than 1 invocation if test is retried', () => {
    let jsonResult;

    const reporterConfig = {
      reporters: [
        ['<rootDir>/reporters/RetryReporter.js', {output: outputFilePath}],
      ],
    };

    runJest('test-retries', [
      '--config',
      JSON.stringify(reporterConfig),
      'retry.test.js',
    ]);

    const testOutput = fs.readFileSync(outputFilePath, 'utf8');

    try {
      jsonResult = JSON.parse(testOutput);
    } catch (err: any) {
      throw new Error(
        `Can't parse the JSON result from ${outputFileName}, ${err.toString()}`,
      );

Review comment (suggestion):
```suggestion
    expect(result.stdout).toContain(logErrorsBeforeRetryErrorMessage);
```

Code after:
  const outputFilePath = path.join(
    process.cwd(),
    'e2e/test-retries/',
    outputFileName,
  );
  const logErrorsBeforeRetryErrorMessage = 'LOGGING RETRY ERRORS';

  afterAll(() => {
    fs.unlinkSync(outputFilePath);
  });

  it('retries failed tests', () => {
    const result = runJest('test-retries', ['e2e.test.js']);

    expect(result.exitCode).toEqual(0);
    expect(result.failed).toBe(false);
    expect(result.stderr).not.toContain(logErrorsBeforeRetryErrorMessage);
  });

  it('logs error(s) before retry', () => {
    const result = runJest('test-retries', ['logErrorsBeforeRetries.test.js']);
    expect(result.exitCode).toEqual(0);
    expect(result.failed).toBe(false);
    expect(result.stderr).toContain(logErrorsBeforeRetryErrorMessage);
    expect(extractSummary(result.stderr).rest).toMatchSnapshot();
  });

  it('reporter shows more than 1 invocation if test is retried', () => {
    let jsonResult;

    const reporterConfig = {
      reporters: [
        ['<rootDir>/reporters/RetryReporter.js', {output: outputFilePath}],
      ],
    };

    runJest('test-retries', [
      '--config',
      JSON.stringify(reporterConfig),
      'retry.test.js',
    ]);

    const testOutput = fs.readFileSync(outputFilePath, 'utf8');

    try {
      jsonResult = JSON.parse(testOutput);
    } catch (err: any) {
      throw new Error(
        `Can't parse the JSON result from ${outputFileName}, ${err.toString()}`,
      );
    }
Repository: facebook/jest
File: packages/jest-changed-files/src/hg.ts (TypeScript)

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

import * as path from 'path';
import execa = require('execa');
import type {Config} from '@jest/types';
import type {SCMAdapter} from './types';

const env = {...process.env, HGPLAIN: '1'};

const adapter: SCMAdapter = {
  findChangedFiles: async (cwd, options) => {
    const includePaths: Array<Config.Path> =
      (options && options.includePaths) || [];

    const args = ['status', '-amnu'];
    if (options && options.withAncestor) {
      args.push('--rev', `min((!public() & ::.)+.)`);
    } else if (options && options.changedSince) {
      args.push('--rev', `ancestor(., ${options.changedSince})`);
    } else if (options && options.lastCommit === true) {
      args.push('--change', '.');
    }
    args.push(...includePaths);

    let result: execa.ExecaReturnValue;

    try {
      result = await execa('hg', args, {cwd, env});
    } catch (e: any) {
      // TODO: Should we keep the original `message`?
      e.message = e.stderr;

      throw e;
    }

    return result.stdout
      .split('\n')
      .filter(s => s !== '')
      .map(changedPath => path.resolve(cwd, changedPath));
  },

  getRoot: async cwd => {

Review comment (bug):
interesting! I've never used mercurial before, but since the test assert on this I guess the change is correct? 🙂 

Was set to use `^` in #7880. I seem to have complained there about failing test, tho 🙈 

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

import * as path from 'path';
import execa = require('execa');
import type {SCMAdapter} from './types';

const env = {...process.env, HGPLAIN: '1'};

const adapter: SCMAdapter = {
  findChangedFiles: async (cwd, options) => {
    const includePaths = options.includePaths ?? [];

    const args = ['status', '-amnu'];
    if (options.withAncestor) {
      args.push('--rev', 'first(min(!public() & ::.)^+.^)');
    } else if (options.changedSince) {
      args.push('--rev', `ancestor(., ${options.changedSince})`);
    } else if (options.lastCommit === true) {
      args.push('--change', '.');
    }
    args.push(...includePaths);

    let result: execa.ExecaReturnValue;

    try {
      result = await execa('hg', args, {cwd, env});
    } catch (e: any) {
      // TODO: Should we keep the original `message`?
      e.message = e.stderr;

      throw e;
    }

    return result.stdout
      .split('\n')
      .filter(s => s !== '')
      .map(changedPath => path.resolve(cwd, changedPath));
  },

  getRoot: async cwd => {
    try {
      const result = await execa('hg', ['root'], {cwd, env});
Repository: facebook/jest
File: packages/jest-changed-files/src/hg.ts (TypeScript)

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

import * as path from 'path';
import execa = require('execa');
import type {Config} from '@jest/types';
import type {SCMAdapter} from './types';

const env = {...process.env, HGPLAIN: '1'};

const adapter: SCMAdapter = {
  findChangedFiles: async (cwd, options) => {
    const includePaths: Array<Config.Path> =
      (options && options.includePaths) || [];

    const args = ['status', '-amnu'];
    if (options && options.withAncestor) {
      args.push('--rev', 'min((not public() and ancestors(.))^ or .^)');
    } else if (options && options.changedSince) {
      args.push('--rev', `ancestor(., ${options.changedSince})`);
    } else if (options && options.lastCommit === true) {
      args.push('--change', '.');
    }
    args.push(...includePaths);

    let result: execa.ExecaReturnValue;

    try {
      result = await execa('hg', args, {cwd, env});
    } catch (e: any) {
      // TODO: Should we keep the original `message`?
      e.message = e.stderr;

      throw e;
    }

    return result.stdout
      .split('\n')
      .filter(s => s !== '')
      .map(changedPath => path.resolve(cwd, changedPath));
  },

  getRoot: async cwd => {

Review comment (suggestion):
The `^` is left in place, but not at the end. Also I replaced `!`, `&`, `+` with `not`, `and`, `or`. Might be these are less problematic.

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

import * as path from 'path';
import execa = require('execa');
import type {SCMAdapter} from './types';

const env = {...process.env, HGPLAIN: '1'};

const adapter: SCMAdapter = {
  findChangedFiles: async (cwd, options) => {
    const includePaths = options.includePaths ?? [];

    const args = ['status', '-amnu'];
    if (options.withAncestor) {
      args.push('--rev', 'first(min(!public() & ::.)^+.^)');
    } else if (options.changedSince) {
      args.push('--rev', `ancestor(., ${options.changedSince})`);
    } else if (options.lastCommit === true) {
      args.push('--change', '.');
    }
    args.push(...includePaths);

    let result: execa.ExecaReturnValue;

    try {
      result = await execa('hg', args, {cwd, env});
    } catch (e: any) {
      // TODO: Should we keep the original `message`?
      e.message = e.stderr;

      throw e;
    }

    return result.stdout
      .split('\n')
      .filter(s => s !== '')
      .map(changedPath => path.resolve(cwd, changedPath));
  },

  getRoot: async cwd => {
    try {
      const result = await execa('hg', ['root'], {cwd, env});
Repository: facebook/jest
File: packages/babel-jest/src/index.ts (TypeScript)

Code before:
        transformOptions.config.cwd,
        sourcePath,
        mergeBabelTransformOptions(sourcePath, transformOptions),
        transformOptions,
      );

      const transformResult = await babelTransformAsync(
        sourceText,
        babelOptions,
      );

      if (transformResult) {
        const {code, map} = transformResult;
        if (typeof code === 'string') {
          return {code, map};
        }
      }

      return sourceText;
    },
  };
};

const transformer: SyncTransformer<TransformOptions> = createTransformer();

export default transformer;


Review comment (suggestion):
One thing we need to check is that `{"\\.js$": ['babel-jest', {rootMode: "upward"}]}` still works, i.e. that we call. `createTransformer` on `babel-jest`.

`requireOrImportModule` will get `default` by... default, so `createTransformer` might be ignored for `babel-jest` if not present on the export

Code after:
        transformOptions.config.cwd,
        sourcePath,
        mergeBabelTransformOptions(sourcePath, transformOptions),
        transformOptions,
      );

      const transformResult = await babelTransformAsync(
        sourceText,
        babelOptions,
      );

      if (transformResult) {
        const {code, map} = transformResult;
        if (typeof code === 'string') {
          return {code, map};
        }
      }

      return sourceText;
    },
  };
};

const transformerFactory = {
  // Assigned here, instead of as a separate export, due to limitations in Jest's
  // requireOrImportModule, requiring all exports to be on the `default` export
  createTransformer,
};

export default transformerFactory;
Repository: facebook/jest
File: packages/jest-runner/src/types.ts (TypeScript)

Code before:
) => Promise<TestResult>;

export type TestRunnerOptions = {
  serial: boolean;
};

// make sure all props here are present in the type below it as well
export type TestRunnerContext = {
  changedFiles?: Set<string>;
  sourcesRelatedToTestsInChangedFiles?: Set<string>;
};

export type TestRunnerSerializedContext = {
  changedFiles?: Array<string>;
  sourcesRelatedToTestsInChangedFiles?: Array<string>;
};

// TODO: Should live in `@jest/core` or `jest-watcher`
type WatcherState = {interrupted: boolean};
export interface TestWatcher extends Emittery<{change: WatcherState}> {
  state: WatcherState;
  setState(state: WatcherState): void;
  isInterrupted(): boolean;
  isWatchMode(): boolean;
}

abstract class BaseTestRunner {
  readonly isSerial?: boolean;
  abstract readonly supportsEventEmitters: boolean;

  constructor(
    protected readonly _globalConfig: Config.GlobalConfig,
    protected readonly _context: TestRunnerContext,
  ) {}
}

export abstract class TestRunner extends BaseTestRunner {
  readonly supportsEventEmitters = false;

  abstract runTests(
    tests: Array<Test>,
    watcher: TestWatcher,
    onStart: OnTestStart,
    onResult: OnTestSuccess,
    onFailure: OnTestFailure,
    options: TestRunnerOptions,
  ): Promise<void>;
}

export abstract class EmittingTestRunner extends BaseTestRunner {
  readonly supportsEventEmitters = true;

Review comment (suggestion):
Here `#` does not work. It turns these props into `private`, but they must be `protected`, so that derived classes could inherit them.

Code after:
) => Promise<TestResult>;

export type TestRunnerOptions = {
  serial: boolean;
};

// make sure all props here are present in the type below it as well
export type TestRunnerContext = {
  changedFiles?: Set<string>;
  sourcesRelatedToTestsInChangedFiles?: Set<string>;
};

export type TestRunnerSerializedContext = {
  changedFiles?: Array<string>;
  sourcesRelatedToTestsInChangedFiles?: Array<string>;
};

export type UnsubscribeFn = () => void;

abstract class BaseTestRunner {
  readonly isSerial?: boolean;
  abstract readonly supportsEventEmitters: boolean;

  constructor(
    protected readonly _globalConfig: Config.GlobalConfig,
    protected readonly _context: TestRunnerContext,
  ) {}
}

export abstract class CallbackTestRunner extends BaseTestRunner {
  readonly supportsEventEmitters = false;

  abstract runTests(
    tests: Array<Test>,
    watcher: TestWatcher,
    onStart: OnTestStart,
    onResult: OnTestSuccess,
    onFailure: OnTestFailure,
    options: TestRunnerOptions,
  ): Promise<void>;
}

export abstract class EmittingTestRunner extends BaseTestRunner {
  readonly supportsEventEmitters = true;

  abstract runTests(
    tests: Array<Test>,
    watcher: TestWatcher,
    options: TestRunnerOptions,
  ): Promise<void>;
Repository: facebook/jest
File: packages/jest-runner/__typetests__/jest-runner.test.ts (TypeScript)

Code before:
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */

import type {UnsubscribeFn} from 'emittery';
import {expectType} from 'tsd-lite';
import type {Test, TestEvents} from '@jest/test-result';
import type {Config} from '@jest/types';
import {CallbackTestRunner, EmittingTestRunner} from 'jest-runner';
import type {
  OnTestFailure,
  OnTestStart,
  OnTestSuccess,
  TestRunnerContext,
  TestRunnerOptions,
  TestWatcher,
} from 'jest-runner';

const globalConfig = {} as Config.GlobalConfig;
const runnerContext = {} as TestRunnerContext;

class CallbackRunner extends CallbackTestRunner {
  async runTests(
    tests: Array<Test>,
    watcher: TestWatcher,
    onStart: OnTestStart,
    onResult: OnTestSuccess,
    onFailure: OnTestFailure,
    options: TestRunnerOptions,
  ): Promise<void> {
    return;
  }
}

const callbackRunner = new CallbackRunner(globalConfig, runnerContext);

expectType<boolean | undefined>(callbackRunner.isSerial);
expectType<false>(callbackRunner.supportsEventEmitters);

class EmittingRunner extends EmittingTestRunner {
  on<Name extends keyof TestEvents>(
    eventName: string,
    listener: (eventData: TestEvents[Name]) => void | Promise<void>,
  ): UnsubscribeFn {
    return () => {};
  }

  async runTests(
    tests: Array<Test>,
    watcher: TestWatcher,

Review comment (refactor):
I was hoping that `override` here in a test could catch some drift (hence #12648). Apparently `override` has no effect in this case, because methods are marked `abstract`.

In the other hand, `abstract` requires to implement those methods in a derived class. So to have this test passing is still good.

Code after:
 *
 * 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 {Test, TestEvents} from '@jest/test-result';
import type {Config} from '@jest/types';
import {CallbackTestRunner, EmittingTestRunner} from 'jest-runner';
import type {
  OnTestFailure,
  OnTestStart,
  OnTestSuccess,
  TestRunnerContext,
  TestRunnerOptions,
  TestWatcher,
  UnsubscribeFn,
} from 'jest-runner';

const globalConfig = {} as Config.GlobalConfig;
const runnerContext = {} as TestRunnerContext;

// CallbackRunner

class CallbackRunner extends CallbackTestRunner {
  async runTests(
    tests: Array<Test>,
    watcher: TestWatcher,
    onStart: OnTestStart,
    onResult: OnTestSuccess,
    onFailure: OnTestFailure,
    options: TestRunnerOptions,
  ): Promise<void> {
    expectType<Config.GlobalConfig>(this._globalConfig);
    expectType<TestRunnerContext>(this._context);

    return;
  }
}

const callbackRunner = new CallbackRunner(globalConfig, runnerContext);

expectType<boolean | undefined>(callbackRunner.isSerial);
expectType<false>(callbackRunner.supportsEventEmitters);

// EmittingRunner

class EmittingRunner extends EmittingTestRunner {
  async runTests(
    tests: Array<Test>,
    watcher: TestWatcher,
Repository: facebook/jest
File: packages/jest-core/src/TestScheduler.ts (TypeScript)

Code before:
      // If there is only one runner, don't partition the tests.
      return Object.assign(Object.create(null), {
        [tests[0].context.config.runner]: tests,
      });
    } else {
      return null;
    }
  }

  async _setupReporters() {
    const {collectCoverage, notify, reporters} = this._globalConfig;

    if (notify) {
      this.addReporter(
        new NotifyReporter(
          this._globalConfig,
          this._options.startRun,
          this._context,
        ),
      );
    }

    if (!reporters) {
      this._setupDefaultReporters(collectCoverage);
      return;
    }

    const reporterNames = reporters.map(
      reporter => this._getReporterProps(reporter).path,
    );

    const isDefault = reporterNames?.includes('default');
    const isGitHubActions =
      process.env.GITHUB_ACTIONS && reporterNames?.includes('github-actions');

    if (isDefault) {
      this._setupDefaultReporters(collectCoverage);
    }

    if (isGitHubActions) {
      this.addReporter(new GitHubActionsReporter());
    }

    if (!isDefault && collectCoverage) {
      this.addReporter(
        new CoverageReporter(this._globalConfig, {
          changedFiles: this._context?.changedFiles,
          sourcesRelatedToTestsInChangedFiles:
            this._context?.sourcesRelatedToTestsInChangedFiles,
        }),
      );

Review comment (suggestion):
Adding `NotifyReporter` first, allows to return early. Also later `reporters &&` checks become unnecessary. 

Code after:
      // If there is only one runner, don't partition the tests.
      return Object.assign(Object.create(null), {
        [tests[0].context.config.runner]: tests,
      });
    } else {
      return null;
    }
  }

  async _setupReporters() {
    const {collectCoverage, notify, reporters} = this._globalConfig;

    if (notify) {
      this.addReporter(
        new NotifyReporter(
          this._globalConfig,
          this._options.startRun,
          this._context,
        ),
      );
    }

    if (!reporters) {
      this._setupDefaultReporters(collectCoverage);
      return;
    }

    const reporterNames = reporters.map(
      reporter => this._getReporterProps(reporter).path,
    );

    const isDefault = reporterNames?.includes('default');
    const isGitHubActions =
      GITHUB_ACTIONS && reporterNames?.includes('github-actions');

    if (isDefault) {
      this._setupDefaultReporters(collectCoverage);
    }

    if (isGitHubActions) {
      this.addReporter(new GitHubActionsReporter());
    }

    if (!isDefault && collectCoverage) {
      this.addReporter(
        new CoverageReporter(this._globalConfig, {
          changedFiles: this._context?.changedFiles,
          sourcesRelatedToTestsInChangedFiles:
            this._context?.sourcesRelatedToTestsInChangedFiles,
        }),
      );