Implement BrowserType.connect

[ka3de]

This PR implements the BrowserType.Connect method for chromium browser type in order to, instead of launching a browser from our implementation, establish the WS connection to a remote running browser.

The approach taken has been to minimize changes in the current implementation/flow. For that:

• A new interface BrowserProcessMetadata has been introduced in order to abstract the elements of the current BrowserProcess implementation that differ for a local/remote browser process [*].
• Refactor has been made in order to minimize the interactions with internal BrowserProcess properties from other components (mainly from Browser implementation).
• A new constructor has been added to comply with the differences for a new BrowserProcess struct that references a remote browser.
• Implementation of BrowserType.Connect method following behavior defined by the current launch method.
• Refactor in order to extract common behavior for launch and connect methods.

[*] There were alternatives other than creating a new interface to abstract these differences, for example to add a flag in BrowserProcess that would indicate if the os.Process property held by BrowserProcess was valid or not (due to being remote). But I believe the new interface definition abstracts this in a better way and prevents possible panics due to NPE, especially considering that there is not an abstraction in place for the whole BrowserProcess element, and this is reachable from other elements implementations, such as Browser.

Closes: #17.

[ka3de]

I have a comment about propagating the ErrInternal to the mapping layer, which I was trying to do for connect implementation here.
The problem is that we are extensively using the UserFriendlyError (sometimes not entirely correctly IMO), which is later on parsed from the k6ext.Panic implementation, but only one error can be wrapped with go v1.19, creating a "conflict" between ErrInternal to be handled in the mapping layer, and UserFriendlyError to be handled in the panic implementation.

Should ErrInternal replace the UserFriendlyError implementation? Should it "absorb" its fields (e.g.: timeout)?
Should we upgrade to go v1.20 which allows to wrap multiple errors?

WDYT @inancgumus @ankur22 ?

[ankur22]

Should ErrInternal replace the UserFriendlyError implementation? Should it "absorb" its fields (e.g.: timeout)?

I've been mulling over what makes an error internal again. Thinking back to how I might design errors in a backend service:

• if a user error (4**) occurred once, it would occur again the next time, so the user needs to change the input.
• If the error was unexpected then the chances are that the action can be retried, and it will work the next time, which are usually internal errors (5**).

So, it feels that we should be thinking about errors as whether they are temporary or permanent.

In xk6-browser:

• I think if a user sees a permanent error we should abort the whole test run, otherwise what's the point in a continuously failing test run where all the iterations are failing?
• If a user sees a temporary error then we should only end the iteration and not the whole test run.

So the question now is, are the UserFriendlyErrors temporary or not?

There are two actions from my viewpoint:

1. We change ErrInternal to ErrPermanent;
2. We don't worry about parsing UserFriendlyError if we detect a Permanent error. If errors are actually Permanent and we're first representing them as UserFriendlyError, maybe we simplify things and not wrap the error in UserFriendlyError, and instead just ensure we provide a user friendly string as the error.

Should we upgrade to go v1.20 which allows to wrap multiple errors?

I think we should get the conversation started and see if others agree.

[ka3de]

• I think if a user sees a permanent error we should abort the whole test run, otherwise what's the point in a continuously failing test run where all the iterations are failing?
• If a user sees a temporary error then we should only end the iteration and not the whole test run.

I agree on this approach, there can be cases where there is not a clear cut, but agree on the concept.

So the question now is, are the UserFriendlyErrors temporary or not?

I believe right now we are using it interchangeably. Personally I'm not sure the UserFriendlyError is bringing us much value. To me it seems like the UserFriendlyError was created to be a sort of "catch all" errors, but I don't like the idea of parsing the error in the "panic layer" and building the error message based on its different fields. I think it adds complexity, especially in an implementation where only one error can be wrapped.

Would it make sense to remove this error, and change the implementations to provide context to the errors where these are defined (as you would normally do)? We could then define specific errors for specific use cases, for example a timeout error. Or for example in our case a Fatal error, which is meant to end all iterations.

In regards of this PR, do you think it makes sense to take this discussion away from it, move forward to it and then tackle all this error/panic handling in a separate PR, possibly the one related with issue #688 ?

[inancgumus]

In regards of this PR, do you think it makes sense to take this discussion away from it, move forward to it and then tackle all this error/panic handling in a separate PR, possibly the one related with issue #688 ?

@ka3de Agreed. Let's discuss this in #688.

[inancgumus]

Hey, @ka3de, thanks for the PR, a long-awaited feature! 👏 I'll review this tomorrow 👍 Before that, I did the following and came up with panic: GoError: creating a new page: canceled errors. You might be interested.

diff --git a/tests/test_browser.go b/tests/test_browser.go
index eef335f..856f51a 100644
--- a/tests/test_browser.go
+++ b/tests/test_browser.go
@@ -118,6 +118,7 @@ func newTestBrowser(tb testing.TB, opts ...any) *testBrowser {
 	if !ok {
 		panic(fmt.Errorf("testBrowser: unexpected browser %T", b))
 	}
+	b = bt.Connect(cb.WsURL(), rt.ToValue(launchOpts))
 
 	tb.Cleanup(func() {
 		select {

[ka3de]

Before that, I did the following and came up with panic: GoError: creating a new page: canceled errors. You might be interested.

Thanks for the heads up! I didn't want to convert the BrowserType tests into "test everything", but that's a good way to try this without creating a bunch of new tests 👍 will take a look. Also I think the work for #675 should help as also test this better at some point.

[inancgumus]

@ka3de, sure! 👍 But I wasn't suggesting testing it like that and putting it into this PR 😅 I've shared the diff in case we discover some edge cases where connect might not work as expected since that's what people might experience (while running the browser remotely)—and also to harden our implementation if necessary when we see fit (this PR or another).

[ka3de]

But I wasn't suggesting testing it like that and putting it into this PR

Yep, sure, I understood the purpose 😄

[ka3de]

I did the following and came up with panic: GoError: creating a new page: canceled errors. You might be interested.

Hey @inancgumus , so I ran the tests with this "hack" to use the connect method in the test browser, but I was getting a different error. Basically I was getting a failure in the TestBrowserLogIterationID here.

I was taking a look and this is "expected" due to the hack. What is happening is that the "log hook" set up here for the test browser will end up collecting the logs for the "initial browser" created through launch method, with an "initial" iterationID created here; but it will also collect logs for the "second browser" that is created through the connect method, which will create a new iterationID here again, and that context will be the one set up, and override, the previous one in the BrowserType here and eventually in the testBrowser here.

If you check the logs, there are references for both generated iteration IDs, one for the "initial browser" through launch and one for the "second" browser through the connect. I don't think this requires a fix, as is due to the "hack", but let me know if I misunderstood something.

[inancgumus]

Nice work 👏 Overall, LGTM. I have some suggestions and mostly questions to consider.

[inancgumus]

I don't think this requires a fix, as is due to the "hack", but let me know if I misunderstood something.

👍

[ankur22]

Nicely done 🙂 I just have some minor suggestions.

The one refactor suggestion (which might be too late now) is that it could have been a good opportunity to move browserprocess into its own package like we tried here.

[ankur22]

Could create a new function NewRemoteBrowserLaunchOptions where it will set isRemoteBrowser to true.

[ka3de]

Is another option, personally I prefer to keep a single "generic" constructor to which pass the appropriate flags, but no strong opinion. If we merge both flags and create this new constructor we can get rid of the constants defined in e24732b. But again, I prefer to keep a single constructor.

[inancgumus]

+1 for adding another constructor: Clearer, more readable, and we can eliminate the constants. On the other hand, there is this, and it hiddenly implies it's better to have constants and a single constructor.

[ankur22]

I prefer a new constructor, but happy to leave it as is.

[ka3de]

I'm changing this, but the code in BrowserType is a little bit "uglier" when calling to init method due to the lack of constants, as we have to pass true or false values. Another alternative is to create the launchOptions in connect and launch methods and then provide that to init method so its modified in it. But don't really like it. Will push the first approach and we can discuss.

[ka3de]

See: 4d5a38f

[inancgumus]

If we want to go this way, I suggest moving option parsing to Connect and Launch and passing the parsed launch options to init(). This way:

1. We can eliminate the ugly local/remote bool.
2. More importantly. We'll move Goja stuff to the mapping layer (Separate Goja and k6 logic from the module logic #271), and I wouldn't bury it in init(). Soon, options parsing will hopefully be in the mapping layer as it will deal with Goja values. Consider leaving it at the exported methods while changing this part of the code, OR just skip this comment since you already dealt with this PR a lot :)—we can take care of it later as well.

[ka3de]

So, as discussed, I think this is a good idea, but it's probably a considerable refactor to include it in this PR, so probably better to tackle it separately once this is merged 👍

[ka3de]

The one suggestion (which might be too late now) is that it could have been a good opportunity to move browserprocess into its own package like we tried #755.

I guess this can be done.. But I would prefer to tackle it in a separate PR if we really want to do it 😅
In this sense, I tried to "decouple" a little bit more the Browser from the BrowserProcess, as there were some references from the first one to internal properties of the second one, but yes, it's not much decoupled.

[inancgumus]

Ankur: The one suggestion (which might be too late now) is that it could have been a good opportunity to move browserprocess into its own package like we tried here.

I wouldn't do that in this PR as it's unrelated to its purpose. Another PR is welcome if we export only a few fields, unlike before, and develop a nicer solution (no pun intended! :)). FYI: @grafana/k6-browser

[inancgumus]

LGTM, and again, nice work 👏