Browser: Don't panic on nil page.on handlers

[joanlopez]

What?

It handles nil page.on handlers at both layers:

• JS
• Go API

Why?

Because now it makes k6 to panic.

Checklist

• I have performed a self-review of my code.
• I have commented on my code, particularly in hard-to-understand areas.
• I have added tests for my changes.
• I have run linter and tests locally (make check) and all pass.

Checklist: Documentation (only for k6 maintainers and if relevant)

Please do not merge this PR until the following items are filled out.

• I have added the correct milestone and labels to the PR.
• I have updated the release notes: link
• I have updated or added an issue to the k6-documentation: grafana/k6-docs#NUMBER if applicable
• I have updated or added an issue to the TypeScript definitions: grafana/k6-DefinitelyTyped#NUMBER if applicable

Related PR(s)/Issue(s)

[ankur22]

LGMT 🚀

[inancgumus]

LGTM 🎉 with some suggestions.

[inancgumus]

Since page.on runs for every page.on event, I believe checking only one event (or not at all) is enough to test it, rather than going through every event name. Otherwise, we slow down the already slow tests and have to keep maintaining this test for every event type we add. It's not the end of the world, but I don't think it's worth it.

No strong opinion! 😄

[joanlopez]

The reason why I did so is because although for now it's a bit redundant (I know the same line is executed despite the event type), in case of refactoring code, that corner case would still be covered, at least for the types that exist now.

If we follow the same idea you suggested, arguably the "valid" test cases are also a bit redundant. So... do you want me to write a single test named something like TestPageOnWithNilHandler, with a single call to page.on? If that's enough, I'm more than happy. Indeed, that's what I wrote first, trying to quickly fix this panic, but then I thought that maybe it was better to have broader coverage 🤷🏻 If it feels useless, and has downsides as making the test suite slightly slower/take longer, then I'm happy to simplify this.

[ankur22]

I did a test to see the behaviour difference between a panic with a typed error as @joanlopez has implemented vs what we had for these cases when we wanted to abort the whole test run with k6ext.Abortf.

With panic with a typed error (and a simple browser test running 10 iterations with 5 VUs), the panic does not stop the test run but fails the iteration. The errors are pretty clear:

ERRO[0001] Uncaught (in promise) TypeError: The "listener" argument must be a function
running at go.k6.io/k6/internal/js/modules/k6/browser/browser.mapPage.mapPageOn.func67 (native)

With k6ext.Abortf with the same test we get a lot of noise which doesn't explain the issue at all, but the whole test does end early:

ERRO[0001] communicating with browser: websocket: close 1006 (abnormal closure): unexpected EOF  category=cdp elapsed="0 ms" source=browser
ERRO[0001] closing the browser: context canceled         category="Browser:Close" elapsed="0 ms" source=browser
ERRO[0001] process with PID 55218 unexpectedly ended: signal: killed  category=browser elapsed="9 ms" source=browser
ERRO[0001] communicating with browser: websocket: close 1006 (abnormal closure): unexpected EOF  category=cdp elapsed="0 ms" source=browser
ERRO[0001] communicating with browser: websocket: close 1006 (abnormal closure): unexpected EOF  category=cdp elapsed="0 ms" source=browser

I think having a clear message is more important. We probably need to revisit the panicIfFatalError (which is a wrapper around k6ext.Abortf), test and possibly fix it.

[inancgumus]

@ankur22 Wouldn't returning a non-fatal error (instead of a panic) lead to the same clear-error behavior and stop the iteration?

[ankur22]

@ankur22 Wouldn't returning a non-fatal error (instead of a panic) lead to the same clear-error behavior and stop the iteration?

It's almost identical. THe only difference is that it is presented as a go error instead of a typed error.

[inancgumus]

It's almost identical. THe only difference is that it is presented as a go error instead of a typed error.

That's because @joanlopez returns it as vu.Runtime().NewTypeError? I believe we can, instead of panicking, return an error of this type to maintain the same behavior.

[joanlopez]

It's almost identical. THe only difference is that it is presented as a go error instead of a typed error.

That's because @joanlopez returns it as vu.Runtime().NewTypeError? I believe we can, instead of panicking, return an error of this type to maintain the same behavior.

I'm not sure if I understand what you mean @inancgumus, but note that NewTypeError doesn't return a Go error, but a *Sobek.Object (see https://github.com/grafana/sobek/blob/main/runtime.go#L548).

Besides what we have discussed so far (UX-related), another part of the reason why I did it like this is also because the pattern: panic(r.NewTypeError(...)) is repeated over and over within Sobek codebase, which in some means that's "THE" way to go.. or at least, "A" way.

That said, note that I'd also prefer to have a more idiomatic way to return errors. Not to say that I think for other types of JS errors or scenarios, isn't as easy as calling a Sobek.Runtime method and panicking. For instance, you can see how other packages created their own adhoc wrappers or methods to deal with it, with is.. obviously not ideal.

So, I merged the branch to fix the panic and move on, but I added a new item to past team sync to discuss it, and try to set the guidelines to move forward around this topic. Unfortunately, we didn't have time to discuss it during the last meeting, but hopefully we will get to that point in one of the upcoming ones, and eventually do.

[inancgumus]

NewTypeError doesn't return a Go error, but a *Sobek.Object

Ah, OK. I thought it was returning an error! TIL.

I'd also prefer to have a more idiomatic way to return errors.
I merged the branch to fix the panic and move on

That's fine! :) My questions are not about this PR. I'm asking in general.