Fix timeout counter synchronization

[sunilsurana]

in one of the story we got timeout-- first and then timeout++ as these were not awaited so fixing those.

[p01]

Can we also edit the private method checkIfPageIsBusy() to just check if the page is busy ?

  private async checkIfPageIsBusy(screenshotPath: string) {
    const timeout = Date.now() + 1000 * 8; // WHATEVER REASONABLE TIME WE DECIDE
    let isBusy: boolean;
    do {
      await this.page.waitForTimeout(300);
      isBusy = await this.isPageBusy();
    } while (isBusy && Date.now() < timeout);
    // For debugging purpose, log timeout exit of the loop
    if (isBusy) {
      console.log(`E2223 : Time out despite busy page for ${this.page.url()} Path = ${screenshotPath}`)
    }
  }

[sunilsurana]

Can we also edit the private method checkIfPageIsBusy() to just check if the page is busy ?

  private async checkIfPageIsBusy(screenshotPath: string) {
    const timeout = Date.now() + 1000 * 8; // WHATEVER REASONABLE TIME WE DECIDE
    let isBusy: boolean;
    do {
      await this.page.waitForTimeout(300);
      isBusy = await this.isPageBusy();
    } while (isBusy && Date.now() < timeout);
    // For debugging purpose, log timeout exit of the loop
    if (isBusy) {
      console.log(`E2223 : Time out despite busy page for ${this.page.url()} Path = ${screenshotPath}`)
    }
  }

@p01 You mean remove out the check for buffer on consecutive screen? In some cases even if there are no timeouts it takes some time to render screen and that part had solved lot of rendering issues. I'll anyways try that out and if that works will push in separate PR so that it will be easy to revert if we have to

[p01]

in one of the story we got timeout-- first and then timeout++ as these were not awaited so fixing those.

Ì wonder if the await fixed it all or if this is also because we call await window.__pwBusy__("timeouts++",timeoutId); AFTER the _setTimeout(..., delay);

[sunilsurana]

It resolved on my dev box as await window.pwBusy("timeouts--",timeoutId); is called inside settimout it will get invoked later. First await window.pwBusy("timeouts--",timeoutId); will get invoked

[p01]

Can we also edit the private method checkIfPageIsBusy() to just check if the page is busy ?

  private async checkIfPageIsBusy(screenshotPath: string) {
    const timeout = Date.now() + 1000 * 8; // WHATEVER REASONABLE TIME WE DECIDE
    let isBusy: boolean;
    do {
      await this.page.waitForTimeout(300);
      isBusy = await this.isPageBusy();
    } while (isBusy && Date.now() < timeout);
    // For debugging purpose, log timeout exit of the loop
    if (isBusy) {
      console.log(`E2223 : Time out despite busy page for ${this.page.url()} Path = ${screenshotPath}`)
    }
  }

@p01 You mean remove out the check for buffer on consecutive screen? In some cases even if there are no timeouts it takes some time to render screen and that part had solved lot of rendering issues. I'll anyways try that out and if that works will push in separate PR so that it will be easy to revert if we have to

With the current approach we take a minimum of 3 screenshots per screenshot requested for the component tests. That's easily an additional 100ms per screenshot. With ~2,000 component screenshots, this amounts to 200 seconds of CPU time, to spread in multiple tabs depending on the number of CPU cores available to run things in parallel, this will easily add ~1-1.5 minute on CI.

[sunilsurana]

Can we also edit the private method checkIfPageIsBusy() to just check if the page is busy ?

  private async checkIfPageIsBusy(screenshotPath: string) {
    const timeout = Date.now() + 1000 * 8; // WHATEVER REASONABLE TIME WE DECIDE
    let isBusy: boolean;
    do {
      await this.page.waitForTimeout(300);
      isBusy = await this.isPageBusy();
    } while (isBusy && Date.now() < timeout);
    // For debugging purpose, log timeout exit of the loop
    if (isBusy) {
      console.log(`E2223 : Time out despite busy page for ${this.page.url()} Path = ${screenshotPath}`)
    }
  }

@p01 You mean remove out the check for buffer on consecutive screen? In some cases even if there are no timeouts it takes some time to render screen and that part had solved lot of rendering issues. I'll anyways try that out and if that works will push in separate PR so that it will be easy to revert if we have to

With the current approach we take a minimum of 3 screenshots per screenshot requested for the component tests. That's easily an additional 100ms per screenshot. With ~2,000 component screenshots, this amounts to 200 seconds of CPU time, to spread in multiple tabs depending on the number of CPU cores available to run things in parallel, this will easily add ~1-1.5 minute on CI.

I agree the current approach of 3 screenshot is not very efficient in terms of speed and resource utilization. Would like to address that as part of separate PR.

[sunilsurana]

@p01 Also added change to update playwright to latest version. Can you please approve if it look ok.