ronantakizawa/codereview-bench huggingface dataset:
Repository: puppeteer/puppeteer
File: tools/mochaRunner/src/test.ts (TypeScript)

Code before:
});

describe('getExpectationUpdates', () => {
  it('should generate an update for expectations if a test passed with a fail expectation', () => {
    const mochaResults = {
      stats: {tests: 1},
      pending: [],
      passes: [
        {
          fullTitle: 'Page Page.setContent should work',
          title: 'should work',
          file: 'page.spec.ts',
        },
      ],
      failures: [],
    };
    const expectations = [
      {
        testIdPattern: '[page.spec] Page Page.setContent should work',
        platforms: [os.platform()],
        parameters: ['test'],
        expectations: ['FAIL' as const],
      },
    ];
    const updates = getExpectationUpdates(mochaResults, expectations, {
      platforms: [os.platform()],
      parameters: ['test'],
    });
    assert.deepEqual(updates, [
      {
        action: 'remove',
        basedOn: {
          expectations: ['FAIL'],
          parameters: ['test'],
          platforms: ['darwin'],
          testIdPattern: '[page.spec] Page Page.setContent should work',
        },
        expectation: {
          expectations: ['FAIL'],
          parameters: ['test'],
          platforms: ['darwin'],
          testIdPattern: '[page.spec] Page Page.setContent should work',
        },
      },
    ]);
  });

  it('should not generate an update for successful retries', () => {
    const mochaResults = {
      stats: {tests: 1},
      pending: [],

Review comment (suggestion):
```suggestion
      platforms: ['darwin'] as Platform[],
```

Code after:

describe('getExpectationUpdates', () => {
  it('should generate an update for expectations if a test passed with a fail expectation', () => {
    const mochaResults = {
      stats: {tests: 1},
      pending: [],
      passes: [
        {
          fullTitle: 'Page Page.setContent should work',
          title: 'should work',
          file: 'page.spec.ts',
        },
      ],
      failures: [],
    };
    const expectations = [
      {
        testIdPattern: '[page.spec] Page Page.setContent should work',
        platforms: ['darwin'] as Platform[],
        parameters: ['test'],
        expectations: ['FAIL' as const],
      },
    ];
    const updates = getExpectationUpdates(mochaResults, expectations, {
      platforms: ['darwin'] as Platform[],
      parameters: ['test'],
    });
    assert.deepEqual(updates, [
      {
        action: 'remove',
        basedOn: {
          expectations: ['FAIL'],
          parameters: ['test'],
          platforms: ['darwin'],
          testIdPattern: '[page.spec] Page Page.setContent should work',
        },
        expectation: {
          expectations: ['FAIL'],
          parameters: ['test'],
          platforms: ['darwin'],
          testIdPattern: '[page.spec] Page Page.setContent should work',
        },
      },
    ]);
  });

  it('should not generate an update for successful retries', () => {
    const mochaResults = {
      stats: {tests: 1},
      pending: [],
      passes: [
Repository: puppeteer/puppeteer
File: test/src/locator.spec.ts (TypeScript)

Code before:
        const result = page
          .locator('::-p-text(test)')
          .setTimeout(5000)
          .expect(async element => {
            return element.getAttribute('clickable') === 'true';
          })
          .expect(element => {
            return element.getAttribute('clickable') === 'true';
          })
          .hover();
        clock.tick(2000);
        await page.evaluate(() => {
          document.querySelector('div')?.setAttribute('clickable', 'true');
        });
        clock.restore();
        await expect(result).resolves.toEqual(undefined);
      } finally {
        clock.restore();
      }
    });
  });

  describe('Locator.prototype.wait', () => {
    it('should work', async () => {
      const {page} = await getTestState();
      await page.setContent(`
        <script>
          setTimeout(() => {
            const element = document.createElement("div");
            element.innerText = "test2"
            document.body.append(element);
          }, 50);
        </script>
      `);
      await expect(page.locator('div').wait()).resolves.toStrictEqual({});
    });
  });
});


Review comment (suggestion):
```suggestion
     page.setContent(`
```

to be sure that the div is not there

Code after:
        const result = page
          .locator('::-p-text(test)')
          .setTimeout(5000)
          .expect(async element => {
            return element.getAttribute('clickable') === 'true';
          })
          .expect(element => {
            return element.getAttribute('clickable') === 'true';
          })
          .hover();
        clock.tick(2000);
        await page.evaluate(() => {
          document.querySelector('div')?.setAttribute('clickable', 'true');
        });
        clock.restore();
        await expect(result).resolves.toEqual(undefined);
      } finally {
        clock.restore();
      }
    });
  });

  describe('Locator.prototype.wait', () => {
    it('should work', async () => {
      const {page} = await getTestState();
      page.setContent(`
        <script>
          setTimeout(() => {
            const element = document.createElement("div");
            element.innerText = "test2"
            document.body.append(element);
          }, 50);
        </script>
      `);
      // This shouldn't throw.
      await page.locator('div').wait();
    });
  });
});
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/common/bidi/Sandbox.ts (TypeScript)

Code before:
/**
 * @internal
 */
export interface SandboxChart {
  [key: string]: Sandbox;
  [MAIN_SANDBOX]: Sandbox;
  [PUPPETEER_SANDBOX]: Sandbox;
}

/**
 * @internal
 */
export class Sandbox implements RealmBase {
  #realm: Realm;

  #timeoutSettings: TimeoutSettings;
  #taskManager = new TaskManager();

  constructor(context: Realm, timeoutSettings: TimeoutSettings) {
    this.#realm = context;
    this.#timeoutSettings = timeoutSettings;

    // TODO: we should track the correct realm
    // Temporary fix
    // This only works for the main context realm
    this.#realm.on(Bidi.ChromiumBidi.Script.EventNames.RealmCreated, () => {
      void this.#taskManager.rerunAll();
    });
  }

  dispose(): void {
    this.#taskManager.terminateAll(
      new Error('waitForFunction failed: frame got detached.')
    );
  }

  get taskManager(): TaskManager {
    return this.#taskManager;
  }

  async document(): Promise<ElementHandle<Document>> {
    // TODO(jrandolf): We should try to cache this because we need to dispose
    // this when it's unused.
    return await this.#realm.evaluateHandle(() => {
      return document;
    });
  }

  async $<Selector extends string>(
    selector: Selector
  ): Promise<ElementHandle<NodeFor<Selector>> | null> {

Review comment (suggestion):
There should be a better way to handle this, still looking over it, as it was part of an old stash,

Code after:

/**
 * @internal
 */
export interface SandboxChart {
  [key: string]: Sandbox;
  [MAIN_SANDBOX]: Sandbox;
  [PUPPETEER_SANDBOX]: Sandbox;
}

/**
 * @internal
 */
export class Sandbox implements RealmBase {
  #realm: Realm;

  #timeoutSettings: TimeoutSettings;
  #taskManager = new TaskManager();

  constructor(
    // TODO: We should split the Realm and BrowsingContext
    realm: Realm | BrowsingContext,
    timeoutSettings: TimeoutSettings
  ) {
    this.#realm = realm;
    this.#timeoutSettings = timeoutSettings;

    // TODO: Tack correct realm similar to BrowsingContexts
    this.#realm.connection.on(
      Bidi.ChromiumBidi.Script.EventNames.RealmCreated,
      () => {
        void this.#taskManager.rerunAll();
      }
    );
  }

  dispose(): void {
    this.#taskManager.terminateAll(
      new Error('waitForFunction failed: frame got detached.')
    );
  }

  get taskManager(): TaskManager {
    return this.#taskManager;
  }

  async document(): Promise<ElementHandle<Document>> {
    // TODO(jrandolf): We should try to cache this because we need to dispose
    // this when it's unused.
    return await this.#realm.evaluateHandle(() => {
      return document;
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/api/HTTPResponse.ts (TypeScript)

Code before:
   * `JSON.parse`.
   */
  async json(): Promise<any> {
    const content = await this.text();
    return JSON.parse(content);
  }

  /**
   * A matching {@link HTTPRequest} object.
   */
  request(): HTTPRequest {
    throw new Error('Not implemented');
  }

  /**
   * True if the response was served from either the browser's disk
   * cache or memory cache.
   */
  fromCache(): boolean {
    throw new Error('Not implemented');
  }

  /**
   * True if the response was served by a service worker.
   */
  fromServiceWorker(): boolean {
    return false;
  }

  /**
   * A {@link Frame} that initiated this response, or `null` if
   * navigating to error pages.
   */
  frame(): Frame | null {
    throw new Error('Not implemented');
  }
}


Review comment (refactor):
should we mark it as abstract to make sure we don't accidentially start returning false for everything?

Code after:
   * `JSON.parse`.
   */
  async json(): Promise<any> {
    const content = await this.text();
    return JSON.parse(content);
  }

  /**
   * A matching {@link HTTPRequest} object.
   */
  request(): HTTPRequest {
    throw new Error('Not implemented');
  }

  /**
   * True if the response was served from either the browser's disk
   * cache or memory cache.
   */
  fromCache(): boolean {
    throw new Error('Not implemented');
  }

  /**
   * True if the response was served by a service worker.
   */
  fromServiceWorker(): boolean {
    throw new Error('Not implemented');
  }

  /**
   * A {@link Frame} that initiated this response, or `null` if
   * navigating to error pages.
   */
  frame(): Frame | null {
    throw new Error('Not implemented');
  }
}
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/api/Page.ts (TypeScript)

Code before:
   * currently active device prompt.
   *
   * :::
   *
   * @example
   *
   * ```ts
   * const [devicePrompt] = Promise.all([
   *   page.waitForDevicePrompt(),
   *   page.click('#connect-bluetooth'),
   * ]);
   * await devicePrompt.select(
   *   await devicePrompt.waitForDevice(({name}) => name.includes('My Device'))
   * );
   * ```
   */
  waitForDevicePrompt(
    options?: WaitTimeoutOptions
  ): Promise<DeviceRequestPrompt>;
  waitForDevicePrompt(): Promise<DeviceRequestPrompt> {
    throw new Error('Not implemented');
  }
}

/**
 * @internal
 */
export const supportedMetrics = new Set<string>([
  'Timestamp',
  'Documents',
  'Frames',
  'JSEventListeners',
  'Nodes',
  'LayoutCount',
  'RecalcStyleCount',
  'LayoutDuration',
  'RecalcStyleDuration',
  'ScriptDuration',
  'TaskDuration',
  'JSHeapUsedSize',
  'JSHeapTotalSize',
]);

/**
 * @internal
 */
export const unitToPixels = {
  px: 1,
  in: 96,
  cm: 37.8,
  mm: 3.78,

Review comment (refactor):
I'd assume that when the page is disposing, it's already closed, and the only things needed is to clean up the state and remove the listeners. Using `close` here seems inappropriate, as `close` is intended to close the page. I'm sure that re-using should be either:
1. Other way around, and `close` should eventually call `dispose`, or
2. `close` should not call `dispose` at all, as the protocol event listeners will process corresponding event (`browsingContext.contextDestroyed` in vase of BiDi).

Code after:
   * This must be called before the device request is made. It will not return a
   * currently active device prompt.
   *
   * :::
   *
   * @example
   *
   * ```ts
   * const [devicePrompt] = Promise.all([
   *   page.waitForDevicePrompt(),
   *   page.click('#connect-bluetooth'),
   * ]);
   * await devicePrompt.select(
   *   await devicePrompt.waitForDevice(({name}) => name.includes('My Device'))
   * );
   * ```
   */
  waitForDevicePrompt(
    options?: WaitTimeoutOptions
  ): Promise<DeviceRequestPrompt>;
  waitForDevicePrompt(): Promise<DeviceRequestPrompt> {
    throw new Error('Not implemented');
  }

  [Symbol.dispose](): void {
    return void this.close().catch(debugError);
  }

  [Symbol.asyncDispose](): Promise<void> {
    return this.close();
  }
}

/**
 * @internal
 */
export const supportedMetrics = new Set<string>([
  'Timestamp',
  'Documents',
  'Frames',
  'JSEventListeners',
  'Nodes',
  'LayoutCount',
  'RecalcStyleCount',
  'LayoutDuration',
  'RecalcStyleDuration',
  'ScriptDuration',
  'TaskDuration',
  'JSHeapUsedSize',
  'JSHeapTotalSize',
]);
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/util/decorators.ts (TypeScript)

Code before:
      return asyncDispose.call(this);
    };
    hasDispose = true;
  }
  if (hasDispose) {
    Class.prototype.move = function (
      this: InstanceType<Class>
    ): InstanceType<Class> {
      instances.add(this);
      return this;
    };
  }
  return Class;
}

export function withIsolatedRealm<
  This extends ElementHandle<Node>,
  Args extends unknown[],
  Ret,
>(
  target: (this: This, ...args: Args) => Promise<Ret>,
  _: unknown
): typeof target {
  return async function (...args) {
    using adoptedThis = await this.frame.isolatedRealm().adoptHandle(this);
    const result = await target.call(adoptedThis, ...args);
    // If the function returns `adoptedThis`, then we return `this`.
    if ((result as unknown) === adoptedThis) {
      return this as unknown as Awaited<Ret>;
    }
    // If the function returns a handle, transfer it into the main realm.
    if (result instanceof JSHandle) {
      return await this.frame.mainRealm().transferHandle(result);
    }
    return result;
  };
}


Review comment (suggestion):
Note this `_: unknown` is required for TypeScript to understand this function is a decorator (it needs to be a function of two arguments: the target and context [which we don't type here]).

Code after:
    hasDispose = true;
  }
  if (hasDispose) {
    Class.prototype.move = function (
      this: InstanceType<Class>
    ): InstanceType<Class> {
      instances.add(this);
      return this;
    };
  }
  return Class;
}

export function throwIfDisposed<This extends Disposed>(
  message: (value: This) => string = value => {
    return `Attempted to use disposed ${value.constructor.name}.`;
  }
) {
  return (target: (this: This, ...args: any[]) => any, _: unknown) => {
    return function (this: This, ...args: any[]): any {
      if (this.disposed) {
        throw new Error(message(this));
      }
      return target.call(this, ...args);
    };
  };
}
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/common/ElementHandle.ts (TypeScript)

Code before:
  declare handle: CDPJSHandle<ElementType>;

  constructor(
    context: ExecutionContext,
    remoteObject: Protocol.Runtime.RemoteObject,
    frame: CDPFrame
  ) {
    super(new CDPJSHandle(context, remoteObject));
    this.#frame = frame;
  }

  /**
   * @internal
   */
  executionContext(): ExecutionContext {
    return this.handle.executionContext();
  }

  /**
   * @internal
   */
  get client(): CDPSession {
    return this.handle.client;
  }

  @throwIfDisposed()
  override remoteObject(): Protocol.Runtime.RemoteObject {
    return this.handle.remoteObject();
  }

  get #frameManager(): FrameManager {
    return this.#frame._frameManager;
  }

  get #page(): Page {
    return this.#frame.page();
  }

  override get frame(): CDPFrame {
    return this.#frame;
  }

  @throwIfDisposed()
  override async $<Selector extends string>(
    selector: Selector
  ): Promise<CDPElementHandle<NodeFor<Selector>> | null> {
    return super.$(selector) as Promise<CDPElementHandle<
      NodeFor<Selector>
    > | null>;
  }


Review comment (suggestion):
I think this might not need to throw. It does not make any calls and you might still want to have the remote object for the disposed handle.

Code after:
  declare handle: CDPJSHandle<ElementType>;

  constructor(
    context: ExecutionContext,
    remoteObject: Protocol.Runtime.RemoteObject,
    frame: CDPFrame
  ) {
    super(new CDPJSHandle(context, remoteObject));
    this.#frame = frame;
  }

  /**
   * @internal
   */
  executionContext(): ExecutionContext {
    return this.handle.executionContext();
  }

  /**
   * @internal
   */
  get client(): CDPSession {
    return this.handle.client;
  }

  override remoteObject(): Protocol.Runtime.RemoteObject {
    return this.handle.remoteObject();
  }

  get #frameManager(): FrameManager {
    return this.#frame._frameManager;
  }

  get #page(): Page {
    return this.#frame.page();
  }

  override get frame(): CDPFrame {
    return this.#frame;
  }

  @throwIfDisposed()
  override async $<Selector extends string>(
    selector: Selector
  ): Promise<CDPElementHandle<NodeFor<Selector>> | null> {
    return super.$(selector) as Promise<CDPElementHandle<
      NodeFor<Selector>
    > | null>;
  }

  @throwIfDisposed()
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/common/bidi/Frame.ts (TypeScript)

Code before:
    return this.#disposed;
  }

  [Symbol.dispose](): void {
    if (this.#disposed) {
      return;
    }
    this.#disposed = true;
    this.#abortDeferred.reject(new Error('Frame detached'));
    this.#context.dispose();
    this.sandboxes[MAIN_SANDBOX][Symbol.dispose]();
    this.sandboxes[PUPPETEER_SANDBOX][Symbol.dispose]();
  }

  #exposedFunctions = new Map<string, ExposeableFunction<never[], unknown>>();
  override async exposeFunction<Args extends unknown[], Ret>(
    name: string,
    apply: (...args: Args) => Awaitable<Ret>
  ): Promise<void> {
    if (this.#exposedFunctions.has(name)) {
      throw new Error(
        `Failed to add page binding with name ${name}: globalThis['${name}'] already exists!`
      );
    }
    const exposeable = new ExposeableFunction(this, name, apply);
    await exposeable.expose();
    this.#exposedFunctions.set(name, exposeable);
  }
}


Review comment (suggestion):
we should probably set it before we await `.expose` to avoid races

Code after:
    return this.#disposed;
  }

  [Symbol.dispose](): void {
    if (this.#disposed) {
      return;
    }
    this.#disposed = true;
    this.#abortDeferred.reject(new Error('Frame detached'));
    this.#context.dispose();
    this.sandboxes[MAIN_SANDBOX][Symbol.dispose]();
    this.sandboxes[PUPPETEER_SANDBOX][Symbol.dispose]();
  }

  #exposedFunctions = new Map<string, ExposeableFunction<never[], unknown>>();
  override async exposeFunction<Args extends unknown[], Ret>(
    name: string,
    apply: (...args: Args) => Awaitable<Ret>
  ): Promise<void> {
    if (this.#exposedFunctions.has(name)) {
      throw new Error(
        `Failed to add page binding with name ${name}: globalThis['${name}'] already exists!`
      );
    }
    const exposeable = new ExposeableFunction(this, name, apply);
    this.#exposedFunctions.set(name, exposeable);
    try {
      await exposeable.expose();
    } catch (error) {
      this.#exposedFunctions.delete(name);
      throw error;
    }
  }
}
Repository: puppeteer/puppeteer
File: packages/puppeteer-core/src/common/bidi/Page.ts (TypeScript)

Code before:
    return mainFrame;
  }

  override frames(): BidiFrame[] {
    return Array.from(this.#frameTree.frames());
  }

  frame(frameId?: string): BidiFrame | null {
    return this.#frameTree.getById(frameId ?? '') || null;
  }

  childFrames(frameId: string): BidiFrame[] {
    return this.#frameTree.childFrames(frameId);
  }

  #onFrameLoaded(info: Bidi.BrowsingContext.NavigationInfo): void {
    const frame = this.frame(info.context);
    if (frame) {
      if (this.mainFrame() === frame) {
        this.emit(PageEmittedEvents.Load);
      }
      this.emit(PageEmittedEvents.FrameNavigated, frame);
    }
  }

  #onFrameDOMContentLoaded(info: Bidi.BrowsingContext.NavigationInfo): void {
    const frame = this.frame(info.context);
    if (frame) {
      frame._hasStartedLoading = true;
      if (this.mainFrame() === frame) {
        this.emit(PageEmittedEvents.DOMContentLoaded);
      }
    }
  }

  #onContextCreated(context: BrowsingContext): void {
    if (
      !this.frame(context.id) &&
      (this.frame(context.parent ?? '') || !this.#frameTree.getMainFrame())
    ) {
      const frame = new BidiFrame(
        this,
        context,
        this.#timeoutSettings,
        context.parent
      );
      this.#frameTree.addFrame(frame);
      if (frame !== this.mainFrame()) {
        this.emit(PageEmittedEvents.FrameAttached, frame);
      }
    }

Review comment (suggestion):
This is not correct. Or at least there is better way of doing it then what we currently have and that is DomContentLoaded event, as that event is reported at a earlier stage then this.

Code after:

  override mainFrame(): BidiFrame {
    const mainFrame = this.#frameTree.getMainFrame();
    assert(mainFrame, 'Requesting main frame too early!');
    return mainFrame;
  }

  override frames(): BidiFrame[] {
    return Array.from(this.#frameTree.frames());
  }

  frame(frameId?: string): BidiFrame | null {
    return this.#frameTree.getById(frameId ?? '') || null;
  }

  childFrames(frameId: string): BidiFrame[] {
    return this.#frameTree.childFrames(frameId);
  }

  #onFrameLoaded(info: Bidi.BrowsingContext.NavigationInfo): void {
    const frame = this.frame(info.context);
    if (frame && this.mainFrame() === frame) {
      this.emit(PageEmittedEvents.Load);
    }
  }

  #onFrameFragmentNavigated(info: Bidi.BrowsingContext.NavigationInfo): void {
    const frame = this.frame(info.context);
    if (frame) {
      this.emit(PageEmittedEvents.FrameNavigated, frame);
    }
  }

  #onFrameDOMContentLoaded(info: Bidi.BrowsingContext.NavigationInfo): void {
    const frame = this.frame(info.context);
    if (frame) {
      frame._hasStartedLoading = true;
      if (this.mainFrame() === frame) {
        this.emit(PageEmittedEvents.DOMContentLoaded);
      }
      this.emit(PageEmittedEvents.FrameNavigated, frame);
    }
  }

  #onContextCreated(context: BrowsingContext): void {
    if (
      !this.frame(context.id) &&
      (this.frame(context.parent ?? '') || !this.#frameTree.getMainFrame())
    ) {
      const frame = new BidiFrame(
        this,