`setDefaultTimeout` on `BrowserContext` isn't converting the time correctly

[ankur22]

Tested against: https://github.com/grafana/xk6-browser/commit/dbede120c63df43995813a847a25b0e66e289592

I was expecting setDefaultTimeout to timeout but the timeout didn't occur when I ran the following script:

In the below code it works against localhost:8080, which is a test server that can be found here with instructions on how to run it locally: https://github.com/ankur22/testserver.

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

export default function () {
  const browser = launcher.launch('chromium', {
      headless: false,
  });
  const context = browser.newContext();
  const page = context.newPage();
  context.setDefaultTimeout(1);
  const res = page.goto('http://localhost:8080/slow');

  sleep(10);
  browser.close();
}

In the documentation I found this comment which might give us a clue as to why:

const context = browser.newContext();
// FIXME: This is interpreted as 16m40s. Wrong JS->Go conversion.
context.setDefaultTimeout(1000); // 1s
const page = context.newPage();
page.click('h2'); // times out

[imiric]

Yeah, this is a problem in a few places :disappointed:

We should test this conversion everywhere we use duration arguments. Or, preferably, switch to using duration strings like k6 does, which is more explicit: "1s", "500ms", etc. We have a solid converter from that to time.Duration.

[mstoykov]

As I mention in https://github.com/grafana/xk6-browser/pull/574#discussion_r992035380

we can use the k6's https://github.com/grafana/k6/blob/fcbbcd54acb2ee58cf16d6a15c730809a541afe9/lib/types/types.go#L190-L211 but it will add support for "1s" , "5m" which is something playwright doesn't support.

Argaubly that is better for us ... but I would expect that if somebody writes "5m" in playwright they will get 5 milliseconds so it might be a thing we don't want to do :shrug:

[imiric]

I think it would be fine to break compatibility with Playwright and use duration strings everywhere. This aligns better with k6 and it's better UX than unitless integers, since it's always clear what the duration is, and you don't need to go looking in the docs or add a comment to explain it.

OTOH, it's unlikely scripts would need anything other than ms or s level precision for this use case, in which case settling on millisecond integers everywhere would be simpler.

[ankur22]

Hmm, i think being aligned with k6 would be the best, especially since the browser will one day be merged into k6.

[inancgumus]

Should we both support Playwright-like integers and k6-like human-readable ones to keep the compatibility promise? We can do that by wrapping the k6 one. I'm also OK with directly using the k6 one.

[mstoykov]

@inancgumus the k6 one already supports integers(and floats) and interpret them as milliseconds, which seems to be the de facto standard across the js world :shrug:

[ankur22]

This issue has been resolved by PR https://github.com/grafana/xk6-browser/pull/1035 which corrects the confusion over what the timeout was (ms or s) and uses time.Duration to store the timeout. The APIs (browserContext.setDefaultTimeout and browserContext.setDefaultNavigationTimeout) still expect a number such as 1000 and not duration strings like 1s or 500ms.