Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition in navigation (and lifecycle events) #943

Merged
merged 3 commits into from Jun 23, 2023

Conversation

inancgumus
Copy link
Member

@inancgumus inancgumus commented Jun 20, 2023

TLDR: Skip the initial blank page navigation/lifecycle event if we want to navigate to a non-blank page so that we can wait for the navigation to complete. This way, we can receive custom metrics like Web Vitals. Custom metrics would be randomly skipped without this.

In order to make sure we receive custom metrics like Web Vitals, it is necessary to skip the initial blank page navigation event when navigating to a non-blank page. By doing this, we can wait for the navigation to complete fully. Without skipping this event, custom metrics could be randomly skipped.

Before this fix, this test's Web Vital metrics were flaky. Sometimes they show up, and sometimes, they don't.
import { browser } from 'k6/x/browser';

export const options = {
  scenarios: {
    ui: {
      executor: 'shared-iterations',
      vus: 1,
      iterations: 1,
      options: {
        browser: {
          type: 'chromium',
        },
      },
    },
  },
};

export default async function() {
  const page = browser.newPage();
  try {
    await page.goto('https://test.k6.io', { waitUntil: 'networkidle' });
    // the above was returning prematurely and caused page.close to be
    // called before the navigation finished.
  } finally {
    page.close();
  } 
}

I'll go ahead and explain the problem using the provided script.

The issue of inconsistency arose because we were removing the Web Vital bindings using page.Close before the navigation had finished. The navigation process was prematurely interrupted due to the initial blank page lifecycle events. As a result, we were not able to receive all the events from the Web Vitals library since we didn't wait for the navigation to complete.

This pull request resolves the problem of unreliable behavior in emitting Web Vitals (or other custom metrics). However, it doesn't address the issue of displaying all Web Vital reports. According to Google's Web Vital documentation, most metrics are meant to be reported only occasionally.

P.S.: I attempted to create a test for this behavior, but I was unable to replicate it. Please let me know if you have any ideas. The existing tests locally pass!

Tip: I noticed that reloading the page just before closing it leads to more emitted metrics. Another pull request could tackle this aspect. Update: See #949.

Updates: #914.

This type will let us to know about the origin of the frame that emits
the lifecycle events. Using this type, we can see which events to
ignore or accept.
We skip the initial blank page if our goal is to navigate to a different
URL. Without this, the blank page would prematurely end the navigation
and our custom metrics (i.e. WebVitals) not be calculated.
@inancgumus inancgumus marked this pull request as ready for review June 22, 2023 13:42
@inancgumus inancgumus requested review from ankur22 and ka3de June 22, 2023 13:42
@inancgumus inancgumus self-assigned this Jun 22, 2023
@inancgumus inancgumus changed the title Fix lifecycle wait fix Fix race condition in navigation (and lifecycle events) Jun 22, 2023
@inancgumus inancgumus added the bug Something isn't working label Jun 22, 2023
@inancgumus inancgumus added this to the v0.11.0 milestone Jun 22, 2023
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @inancgumus LGTM 👍

@inancgumus inancgumus merged commit 9965947 into main Jun 23, 2023
9 of 13 checks passed
@inancgumus inancgumus deleted the fix/914-life-cycle-wait-fix branch June 23, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants