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 propagation of page timeout #1033

Merged
merged 3 commits into from
Sep 18, 2023
Merged

Fix propagation of page timeout #1033

merged 3 commits into from
Sep 18, 2023

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented Sep 14, 2023

What?

When working with the following APIs, the timeout was being set, but it wasn't then being propagated to the underlying frameManager and frames which meant that APIs the relied on the timeout were always working with the default 30 second timeout or the timeout set in the browserContext.

page.setDefaultTimeout()
page.setDefaultNavigationTimeout()

Why?

We want to be able to set the timeouts on the page and affect it's APIs, while also having a different timeout on its browserContext. This is the behaviour that we want, and at the moment the setting of timeouts through the page object does nothing.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

Closes: #940

The timeout to the underlying frame manager and frames that page
relied on was not being propagated if page.setDefaultTimeout or
page.setDefaultNavigationTimeout were being called.

This fix ensures that the correct timeout structure is passed to the
underlying objects that the page relies on.

Closes: #940
This tests ensures that the timeout that is set in page is propagated
to the underlying objects (frame manager and frame).

There is one issue which is that the timeout is set in milliseconds but
later it is translated into a second. This needs to be resolved in a
new PR.
This tests ensures that the timeout that is set in browserContext is
propagated to the underlying objects (frame manager and frame).
Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

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

I've tested this with the following script, and it worked 🥳

Suggestion 1: Can you merge the (almost) duplicated test code for better maintainability? Suggestion: Run the same test with page and browserContext in the same test.

Suggestion 2: Should we update the fillform example with this timeout setting and a comment on top of it?

import { browser } from 'k6/x/browser';

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

export default async function() {
  const context = browser.newContext();
  context.setDefaultTimeout(1);
  const page = context.newPage();
  try {
    // page.setDefaultTimeout(1);
    await page.goto('https://test.k6.io', { waitUntil: 'networkidle' });
    page.waitForSelector("div[class$='overlay-box']");
  } finally {
    page.close();
  }
}

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.

@ankur22 ankur22 changed the title Fix/940 propagate timeout Fix propagation of page timeout Sep 18, 2023
@ankur22
Copy link
Collaborator Author

ankur22 commented Sep 18, 2023

Suggestion 1: Can you merge the (almost) duplicated test code for better maintainability? Suggestion: Run the same test with page and browserContext in the same test.

I wanted to highlight that although they seem the same, they're in fact not the same. The tests for browserContext will have worked prior to the fix, whereas the tests for page only work after the fix. Happy to merge them though in a new PR once this and #1035 are merged in 👍

Suggestion 2: Should we update the fillform example with this timeout setting and a comment on top of it?

Happy to do this, just wondering what the hope is by doing that?

@ankur22 ankur22 merged commit d72aa0f into main Sep 18, 2023
12 checks passed
@ankur22 ankur22 deleted the fix/940-propagate-timeout branch September 18, 2023 10:19
@inancgumus
Copy link
Member

I wanted to highlight that although they seem the same, they're in fact not the same.

Ah, OK, makes sense 👍

Happy to do this, just wondering what the hope is by doing that?

I thought it might be helpful, but it's fine.

@ankur22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting the timeout with page.setDefaultTimeout doesn't then get used anywhere
3 participants