ronantakizawa/codereview-bench huggingface dataset:
Repository: facebook/jest
File: packages/jest-resolve/src/resolver.ts (TypeScript)

Code before:

  getModule(name: string): string | null {
    return this._moduleMap.getModule(
      name,
      this._options.defaultPlatform,
      this._supportsNativePlatform,
    );
  }

  getModulePath(from: string, moduleName: string): string {
    if (moduleName[0] !== '.' || path.isAbsolute(moduleName)) {
      return moduleName;
    }
    return path.normalize(`${path.dirname(from)}/${moduleName}`);
  }

  getPackage(name: string): string | null {
    return this._moduleMap.getPackage(
      name,
      this._options.defaultPlatform,
      this._supportsNativePlatform,
    );
  }

  getMockModule(from: string, name: string): string | null {
    const resolvedName = this.resolveStubModuleName(from, name) ?? name;
    const mock = this._moduleMap.getMockModule(resolvedName);
    return mock ?? null;
  }

  async getMockModuleAsync(from: string, name: string): Promise<string | null> {
    const mock = this._moduleMap.getMockModule(name);
    if (mock) {
      return mock;
    } else {
      const moduleName = await this.resolveStubModuleNameAsync(from, name);
      if (moduleName) {
        return this.getModule(moduleName) || moduleName;
      }
    }
    return null;
  }

  getModulePaths(from: string): Array<string> {
    const cachedModule = this._modulePathCache.get(from);
    if (cachedModule) {
      return cachedModule;
    }

    const moduleDirectory = this._options.moduleDirectories;
    const paths = nodeModulesPaths(from, {moduleDirectory});

Review comment (performance):
Isn’t it that the current logic is optimised for better performance? I mean, resolving can be expansive step, so isn’t it better to skip it if the module is already cached in the `_moduleMap`?

Code after:

  getModule(name: string): string | null {
    return this._moduleMap.getModule(
      name,
      this._options.defaultPlatform,
      this._supportsNativePlatform,
    );
  }

  getModulePath(from: string, moduleName: string): string {
    if (moduleName[0] !== '.' || path.isAbsolute(moduleName)) {
      return moduleName;
    }
    return path.normalize(`${path.dirname(from)}/${moduleName}`);
  }

  getPackage(name: string): string | null {
    return this._moduleMap.getPackage(
      name,
      this._options.defaultPlatform,
      this._supportsNativePlatform,
    );
  }

  getMockModule(
    from: string,
    name: string,
    options: Pick<ResolveModuleConfig, 'conditions'>,
  ): string | null {
    const mock = this._moduleMap.getMockModule(name);
    if (mock) {
      return mock;
    } else {
      const resolvedName = this.resolveStubModuleName(from, name, options);
      if (resolvedName) {
        return this._moduleMap.getMockModule(resolvedName) ?? null;
      }
    }
    return null;
  }

  async getMockModuleAsync(
    from: string,
    name: string,
    options: Pick<ResolveModuleConfig, 'conditions'>,
  ): Promise<string | null> {
    const mock = this._moduleMap.getMockModule(name);
    if (mock) {
      return mock;
    } else {
      const resolvedName = await this.resolveStubModuleNameAsync(
Repository: facebook/jest
File: packages/jest-reporters/src/__tests__/GitHubActionsReporter.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 {afterEach, describe, expect, jest, test} from '@jest/globals';
import type {
  AggregatedResult,
  AssertionResult,
  Status,
  Test,
  TestCaseResult,
  TestResult,
} from '@jest/test-result';
import type {Config} from '@jest/types';
import {normalizeIcons} from '../../../../e2e/Utils';
import GitHubActionsReporter from '../GitHubActionsReporter';

afterEach(() => {
  jest.clearAllMocks();
});

const oldLog = GitHubActionsReporter.prototype.log;

GitHubActionsReporter.prototype.log = (message: string) => {
  oldLog(normalizeIcons(message));
};

const mockedStderrWrite = jest
  .spyOn(process.stderr, 'write')
  .mockImplementation(() => true);

Review comment (suggestion):
No need to import. Something did not work?

```suggestion
```


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 type {
  AggregatedResult,
  AssertionResult,
  Status,
  Test,
  TestCaseResult,
  TestResult,
} from '@jest/test-result';
import {normalizeIcons} from '@jest/test-utils';
import type {Config} from '@jest/types';
import BaseGitHubActionsReporter from '../GitHubActionsReporter';

afterEach(() => {
  jest.clearAllMocks();
});

class GitHubActionsReporter extends BaseGitHubActionsReporter {
  override log(message: string): void {
    super.log(normalizeIcons(message));
  }
}

const mockedStderrWrite = jest
  .spyOn(process.stderr, 'write')
  .mockImplementation(() => true);
Repository: facebook/jest
File: packages/jest-reporters/src/GitHubActionsReporter.ts (TypeScript)

Code before:
  separateMessageFromStack,
} from 'jest-message-util';
import {specialChars} from 'jest-util';
import BaseReporter from './BaseReporter';

type AnnotationOptions = {
  file?: string;
  line?: number | string;
  message: string;
  title: string;
  type: 'error' | 'warning';
};

const titleSeparator = ' \u203A ';
const ICONS = specialChars.ICONS;

type PerformanceInfo = {
  end: number;
  runtime: number;
  slow: boolean;
  start: number;
};

type ResultTreeLeaf = {
  name: string;
  passed: Status;
  duration: number;
  children: Array<never>;
};

type ResultTreeNode = {
  name: string;
  passed: boolean;
  children: Array<ResultTreeNode | ResultTreeLeaf>;
};

type ResultTree = {
  children: Array<ResultTreeLeaf | ResultTreeNode>;
  name: string;
  passed: boolean;
  performanceInfo: PerformanceInfo;
};

export default class GitHubActionsReporter extends BaseReporter {
  static readonly filename = __filename;
  private readonly options: {silent: boolean};

  constructor(
    _globalConfig: Config.GlobalConfig,
    reporterOptions: {silent?: boolean} = {silent: true},
  ) {

Review comment (suggestion):
Perhaps this prop should be renamed to `status` or so?

This is only used internally, right? Just a detail, but makes it more understandable for readers.

Code after:
  separateMessageFromStack,
} from 'jest-message-util';
import {specialChars} from 'jest-util';
import BaseReporter from './BaseReporter';

type AnnotationOptions = {
  file?: string;
  line?: number | string;
  message: string;
  title: string;
  type: 'error' | 'warning';
};

const titleSeparator = ' \u203A ';
const ICONS = specialChars.ICONS;

type PerformanceInfo = {
  end: number;
  runtime: number;
  slow: boolean;
  start: number;
};

type ResultTreeLeaf = {
  name: string;
  status: Status;
  duration: number;
  children: Array<never>;
};

type ResultTreeNode = {
  name: string;
  passed: boolean;
  children: Array<ResultTreeNode | ResultTreeLeaf>;
};

type ResultTree = {
  children: Array<ResultTreeLeaf | ResultTreeNode>;
  name: string;
  passed: boolean;
  performanceInfo: PerformanceInfo;
};

export default class GitHubActionsReporter extends BaseReporter {
  static readonly filename = __filename;
  private readonly options: {silent: boolean};

  constructor(
    _globalConfig: Config.GlobalConfig,
    reporterOptions: {silent?: boolean} = {silent: true},
  ) {
Repository: facebook/jest
File: packages/jest-util/src/invariant.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.
 */

export default function invariant(
  condition: unknown,
  message?: string,
): asserts condition {
  if (!condition) {
    throw new Error(message);
  }
}


Review comment (bug):
It would be nice to make the message required. Also it would be nice to append "This is a bug in Jest...", like here:

https://github.com/jestjs/jest/blob/0fd5b1c37555f485c56a6ad2d6b010a72204f9f6/packages/jest-runtime/src/index.ts#L530

https://github.com/jestjs/jest/blob/0fd5b1c37555f485c56a6ad2d6b010a72204f9f6/packages/jest-runtime/src/index.ts#L546

`invariant()` is used in places there value is defined, but TypeScript cannot see that. So a failure is definitely a bug, that is why it makes sense to append "This is a bug in Jest..."

By the way, it could be good default message (instead of making the message required).

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.
 */

export default function invariant(
  condition: unknown,
  message = '',
): asserts condition {
  if (!condition) {
    throw new Error(message);
  }
}
Repository: facebook/jest
File: packages/expect-utils/src/utils.ts (TypeScript)

Code before:
};

// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
export const typeEquality = (a: any, b: any): boolean | undefined => {
  if (
    a == null ||
    b == null ||
    a.constructor === b.constructor ||
    // Since Jest globals are different from Node globals,
    // constructors are different even between arrays when comparing properties of mock objects.
    // Both of them should be able to compare correctly when they are array-to-array.
    // https://github.com/facebook/jest/issues/2549
    (Array.isArray(a) && Array.isArray(b))
  ) {
    return undefined;
  }

  return false;
};

export const arrayBufferEquality = (
  a: unknown,
  b: unknown,
): boolean | undefined => {
  let dataViewA = a as DataView;
  let dataViewB = b as DataView;

  if (!(a instanceof DataView && b instanceof DataView)) {
    if (!(a instanceof ArrayBuffer) || !(b instanceof ArrayBuffer))
      return undefined;

    dataViewA = new DataView(a);
    dataViewB = new DataView(b);
  }

  // Buffers are not equal when they do not have the same byte length
  if (dataViewA.byteLength !== dataViewB.byteLength) {
    return false;
  }

  // Check if every byte value is equal to each other
  for (let i = 0; i < dataViewA.byteLength; i++) {
    if (dataViewA.getUint8(i) !== dataViewB.getUint8(i)) {
      return false;
    }
  }

  return true;
};

export const sparseArrayEquality = (

Review comment (bug):
Hm.. I’m not happy to see this cast. Right now it works with the following logic, but if the logic will change TS will fail to catch any errors. Would be useful to rethink this part.

Code after:
};

// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
export const typeEquality = (a: any, b: any): boolean | undefined => {
  if (
    a == null ||
    b == null ||
    a.constructor === b.constructor ||
    // Since Jest globals are different from Node globals,
    // constructors are different even between arrays when comparing properties of mock objects.
    // Both of them should be able to compare correctly when they are array-to-array.
    // https://github.com/jestjs/jest/issues/2549
    (Array.isArray(a) && Array.isArray(b))
  ) {
    return undefined;
  }

  return false;
};

export const arrayBufferEquality = (
  a: unknown,
  b: unknown,
): boolean | undefined => {
  let dataViewA = a;
  let dataViewB = b;

  if (a instanceof ArrayBuffer && b instanceof ArrayBuffer) {
    dataViewA = new DataView(a);
    dataViewB = new DataView(b);
  }

  if (!(dataViewA instanceof DataView && dataViewB instanceof DataView)) {
    return undefined;
  }

  // Buffers are not equal when they do not have the same byte length
  if (dataViewA.byteLength !== dataViewB.byteLength) {
    return false;
  }

  // Check if every byte value is equal to each other
  for (let i = 0; i < dataViewA.byteLength; i++) {
    if (dataViewA.getUint8(i) !== dataViewB.getUint8(i)) {
      return false;
    }
  }

  return true;
};
Repository: facebook/jest
File: packages/expect-utils/src/utils.ts (TypeScript)

Code before:
    // Since Jest globals are different from Node globals,
    // constructors are different even between arrays when comparing properties of mock objects.
    // Both of them should be able to compare correctly when they are array-to-array.
    // https://github.com/facebook/jest/issues/2549
    (Array.isArray(a) && Array.isArray(b))
  ) {
    return undefined;
  }

  return false;
};

export const arrayBufferEquality = (
  a: unknown,
  b: unknown,
): boolean | undefined => {
  let dataViewA = a as DataView;
  let dataViewB = b as DataView;

  if (!(a instanceof DataView && b instanceof DataView)) {
    if (!(a instanceof ArrayBuffer) || !(b instanceof ArrayBuffer))
      return undefined;

    dataViewA = new DataView(a);
    dataViewB = new DataView(b);
  }

  // Buffers are not equal when they do not have the same byte length
  if (dataViewA.byteLength !== dataViewB.byteLength) {
    return false;
  }

  // Check if every byte value is equal to each other
  for (let i = 0; i < dataViewA.byteLength; i++) {
    if (dataViewA.getUint8(i) !== dataViewB.getUint8(i)) {
      return false;
    }
  }

  return true;
};

export const sparseArrayEquality = (
  a: unknown,
  b: unknown,
  customTesters: Array<Tester> = [],
): boolean | undefined => {
  if (!Array.isArray(a) || !Array.isArray(b)) {
    return undefined;
  }


Review comment (suggestion):
Perhaps simply like this (just a quick idea):

```ts
  let dataViewA: DataView;
  let dataViewB: DataView;

  if (a instanceof DataView && b instanceof DataView) {
    dataViewA = a;
    dataViewB = b;
  } else if (a instanceof ArrayBuffer && b instanceof ArrayBuffer) {
    dataViewA = new DataView(a);
    dataViewB = new DataView(b);
  } else {
    return undefined;
  }
```

Code after:
    // Since Jest globals are different from Node globals,
    // constructors are different even between arrays when comparing properties of mock objects.
    // Both of them should be able to compare correctly when they are array-to-array.
    // https://github.com/jestjs/jest/issues/2549
    (Array.isArray(a) && Array.isArray(b))
  ) {
    return undefined;
  }

  return false;
};

export const arrayBufferEquality = (
  a: unknown,
  b: unknown,
): boolean | undefined => {
  let dataViewA = a;
  let dataViewB = b;

  if (a instanceof ArrayBuffer && b instanceof ArrayBuffer) {
    dataViewA = new DataView(a);
    dataViewB = new DataView(b);
  }

  if (!(dataViewA instanceof DataView && dataViewB instanceof DataView)) {
    return undefined;
  }

  // Buffers are not equal when they do not have the same byte length
  if (dataViewA.byteLength !== dataViewB.byteLength) {
    return false;
  }

  // Check if every byte value is equal to each other
  for (let i = 0; i < dataViewA.byteLength; i++) {
    if (dataViewA.getUint8(i) !== dataViewB.getUint8(i)) {
      return false;
    }
  }

  return true;
};

export const sparseArrayEquality = (
  a: unknown,
  b: unknown,
  customTesters: Array<Tester> = [],
): boolean | undefined => {
  if (!Array.isArray(a) || !Array.isArray(b)) {
    return undefined;
  }
Repository: facebook/jest
File: packages/expect-utils/src/utils.ts (TypeScript)

Code before:
};

// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
export const typeEquality = (a: any, b: any): boolean | undefined => {
  if (
    a == null ||
    b == null ||
    a.constructor === b.constructor ||
    // Since Jest globals are different from Node globals,
    // constructors are different even between arrays when comparing properties of mock objects.
    // Both of them should be able to compare correctly when they are array-to-array.
    // https://github.com/facebook/jest/issues/2549
    (Array.isArray(a) && Array.isArray(b))
  ) {
    return undefined;
  }

  return false;
};

export const arrayBufferEquality = (
  a: unknown,
  b: unknown,
): boolean | undefined => {
  let dataViewA = a;
  let dataViewB = b;

  if (a instanceof ArrayBuffer && b instanceof ArrayBuffer) {
    dataViewA = new DataView(a);
    dataViewB = new DataView(b);
  }

  if (!(dataViewA instanceof DataView && dataViewB instanceof DataView)) {
    return undefined;
  }

  // Buffers are not equal when they do not have the same byte length
  if (dataViewA.byteLength !== dataViewB.byteLength) {
    return false;
  }

  // Check if every byte value is equal to each other
  for (let i = 0; i < dataViewA.byteLength; i++) {
    if (dataViewA.getUint8(i) !== dataViewB.getUint8(i)) {
      return false;
    }
  }

  return true;
};


Review comment (bug):
From semantic point of view it feels wrong to name a variable `dataView` at the moment it is still not clear if that is a `DataView` instance.

Also the shape of the logic is very close to: https://github.com/jestjs/jest/pull/14408#discussion_r1293140956. Did I miss something in that proposal?

Code after:
};

// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
export const typeEquality = (a: any, b: any): boolean | undefined => {
  if (
    a == null ||
    b == null ||
    a.constructor === b.constructor ||
    // Since Jest globals are different from Node globals,
    // constructors are different even between arrays when comparing properties of mock objects.
    // Both of them should be able to compare correctly when they are array-to-array.
    // https://github.com/jestjs/jest/issues/2549
    (Array.isArray(a) && Array.isArray(b))
  ) {
    return undefined;
  }

  return false;
};

export const arrayBufferEquality = (
  a: unknown,
  b: unknown,
): boolean | undefined => {
  let dataViewA = a;
  let dataViewB = b;

  if (a instanceof ArrayBuffer && b instanceof ArrayBuffer) {
    dataViewA = new DataView(a);
    dataViewB = new DataView(b);
  }

  if (!(dataViewA instanceof DataView && dataViewB instanceof DataView)) {
    return undefined;
  }

  // Buffers are not equal when they do not have the same byte length
  if (dataViewA.byteLength !== dataViewB.byteLength) {
    return false;
  }

  // Check if every byte value is equal to each other
  for (let i = 0; i < dataViewA.byteLength; i++) {
    if (dataViewA.getUint8(i) !== dataViewB.getUint8(i)) {
      return false;
    }
  }

  return true;
};
Repository: facebook/jest
File: packages/expect/src/asymmetricMatchers.ts (TypeScript)

Code before:
  override getExpectedType() {
    return 'array';
  }
}

class ObjectContaining extends AsymmetricMatcher<
  Record<string | symbol, unknown>
> {
  constructor(sample: Record<string | symbol, unknown>, inverse = false) {
    super(sample, inverse);
  }

  asymmetricMatch(other: any) {
    if (typeof this.sample !== 'object') {
      throw new Error(
        `You must provide an object to ${this.toString()}, not '${typeof this
          .sample}'.`,
      );
    }

    let result = true;

    const matcherContext = this.getMatcherContext();
    const objectSymbolKeys = Object.getOwnPropertySymbols(this.sample);

    for (const property in this.sample) {
      if (
        !hasProperty(other, property) ||
        !equals(
          this.sample[property],
          other[property],
          matcherContext.customTesters,
        )
      ) {
        result = false;
        break;
      }
    }

    if (objectSymbolKeys.length > 0) {
      for (let i = 0; i < objectSymbolKeys.length; i++) {
        const property = objectSymbolKeys[i];
        if (
          !hasProperty(other, property) ||
          !equals(
            this.sample[property],
            !other[property],
            matcherContext.customTesters,
          )
        ) {
          result = false;

Review comment (refactor):
Perhaps `getObjectKeys()` from `@jest/expect-utils` package could be used to extract keys and loop through all of them in one go:

https://github.com/jestjs/jest/blob/25a8785584c9d54a05887001ee7f498d489a5441/packages/expect-utils/src/utils.ts#L50

Code after:

  override getExpectedType() {
    return 'array';
  }
}

class ObjectContaining extends AsymmetricMatcher<
  Record<string | symbol, unknown>
> {
  constructor(sample: Record<string | symbol, unknown>, inverse = false) {
    super(sample, inverse);
  }

  asymmetricMatch(other: any) {
    if (typeof this.sample !== 'object') {
      throw new Error(
        `You must provide an object to ${this.toString()}, not '${typeof this
          .sample}'.`,
      );
    }

    let result = true;

    const matcherContext = this.getMatcherContext();
    const objectKeys = getObjectKeys(this.sample);

    for (const key of objectKeys) {
      if (
        !hasProperty(other, key) ||
        !equals(this.sample[key], other[key], matcherContext.customTesters)
      ) {
        result = false;
        break;
      }
    }

    return this.inverse ? !result : result;
  }

  toString() {
    return `Object${this.inverse ? 'Not' : ''}Containing`;
  }

  override getExpectedType() {
    return 'object';
  }
}

class StringContaining extends AsymmetricMatcher<string> {
  constructor(sample: string, inverse = false) {
    if (!isA('String', sample)) {
Repository: facebook/jest
File: packages/create-jest/src/runCreate.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 path from 'path';
import chalk = require('chalk');
import exit = require('exit');
import * as fs from 'graceful-fs';
import prompts = require('prompts');
import yargs = require('yargs');
import {getVersion} from '@jest/core';
import {constants} from 'jest-config';
import {clearLine, tryRealpath} from 'jest-util';
import {MalformedPackageJsonError, NotFoundPackageJsonError} from './errors';
import generateConfigFile from './generateConfigFile';
import modifyPackageJson from './modifyPackageJson';
import defaultQuestions, {testScriptQuestion} from './questions';
import type {ProjectPackageJson, PromptsResults} from './types';

const {
  JEST_CONFIG_BASE_NAME,
  JEST_CONFIG_EXT_MJS,
  JEST_CONFIG_EXT_JS,
  JEST_CONFIG_EXT_TS,
  JEST_CONFIG_EXT_ORDER,
  PACKAGE_JSON,
} = constants;

const getConfigFilename = (ext: string) => JEST_CONFIG_BASE_NAME + ext;

export async function runCLI(): Promise<void> {
  try {
    const argv = await yargs(process.argv.slice(2))
      .usage('Usage: $0 [rootDir]')
      .version(getVersion())

Review comment (suggestion):
Just thinking out loud: it feels too much to ship `yargs` with this package. I think `process.argv[2]` would be to do the job. The value will be always `string | undefined`.

Currently this package is 15.5 kB (unpacked). `yargs` takes 292 kB (unpacked, without dependencies). That’s what npm gave to me.


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 path from 'path';
import chalk = require('chalk');
import exit = require('exit');
import * as fs from 'graceful-fs';
import prompts = require('prompts');
import {constants} from 'jest-config';
import {clearLine, tryRealpath} from 'jest-util';
import {MalformedPackageJsonError, NotFoundPackageJsonError} from './errors';
import generateConfigFile from './generateConfigFile';
import modifyPackageJson from './modifyPackageJson';
import defaultQuestions, {testScriptQuestion} from './questions';
import type {ProjectPackageJson, PromptsResults} from './types';

const {
  JEST_CONFIG_BASE_NAME,
  JEST_CONFIG_EXT_MJS,
  JEST_CONFIG_EXT_JS,
  JEST_CONFIG_EXT_TS,
  JEST_CONFIG_EXT_ORDER,
  PACKAGE_JSON,
} = constants;

const getConfigFilename = (ext: string) => JEST_CONFIG_BASE_NAME + ext;

export async function runCLI(): Promise<void> {
  try {
    const rootDir = process.argv[2];
    await runCreate(rootDir);
  } catch (error: unknown) {
    clearLine(process.stderr);
    clearLine(process.stdout);