Implement `BrowserType.connect` #800

ka3de · 2023-02-27T13:59:48Z

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 · 2023-02-27T14:01:37Z

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 · 2023-02-27T16:38:52Z

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:

We change ErrInternal to ErrPermanent;
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 · 2023-02-28T08:08:50Z

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 grafana/k6#4403 ?

inancgumus · 2023-02-28T08:21:23Z

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 grafana/k6#4403 ?

@ka3de Agreed. Let's discuss this in grafana/k6#4403.

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 · 2023-02-28T13:54:09Z

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 grafana/k6#4406 should help as also test this better at some point.

inancgumus · 2023-02-28T14:10:50Z

@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 · 2023-02-28T14:24:32Z

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

Yep, sure, I understood the purpose 😄

ka3de · 2023-03-01T08:24:58Z

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.

common/browser.go

chromium/browser_type.go

common/browser_process_meta.go

chromium/browser_type.go

common/browser_test.go

tests/test_browser.go

inancgumus · 2023-03-01T08:27:44Z

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

👍

Define constants for the different modes for both flags so it improves readability. Resolves: #800 (comment). Resolves: #800 (comment).

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.

common/browser_options.go

ankur22 · 2023-03-01T10:31:02Z

common/browser_options.go


 // NewLaunchOptions returns a new LaunchOptions.
-func NewLaunchOptions(onCloud bool) *LaunchOptions {
+func NewLaunchOptions(onCloud, isRemoteBrowser bool) *LaunchOptions {


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

ka3de self-assigned this Feb 27, 2023

ka3de force-pushed the feat/17-browsertype-connect branch from 46ccdd1 to 09a53b1 Compare February 27, 2023 15:27

ka3de marked this pull request as ready for review February 28, 2023 13:16

ka3de requested review from ankur22 and inancgumus February 28, 2023 13:16

inancgumus reviewed Feb 28, 2023

View reviewed changes

inancgumus self-requested a review March 1, 2023 07:53

inancgumus reviewed Mar 1, 2023

View reviewed changes

ka3de added a commit that referenced this pull request Mar 1, 2023

Refactor cloud and remote browser flags

c421358

Define constants for the different modes for both flags so it improves readability. Resolves: #800 (comment). Resolves: #800 (comment).

ka3de force-pushed the feat/17-browsertype-connect branch from 7ee3d92 to c421358 Compare March 1, 2023 10:42

ankur22 approved these changes Mar 1, 2023

View reviewed changes

Implement BrowserType.connect #800

Implement BrowserType.connect #800

Uh oh!

Conversation

ka3de commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ka3de commented Feb 27, 2023

Uh oh!

ankur22 commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ka3de commented Feb 28, 2023

Uh oh!

inancgumus commented Feb 28, 2023

Uh oh!

inancgumus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ka3de commented Feb 28, 2023

Uh oh!

inancgumus commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ka3de commented Feb 28, 2023

Uh oh!

ka3de commented Mar 1, 2023

Uh oh!

inancgumus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

inancgumus commented Mar 1, 2023

Uh oh!

ankur22 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ankur22 Mar 1, 2023

Choose a reason for hiding this comment

Implement `BrowserType.connect` #800

Implement `BrowserType.connect` #800

ka3de commented Feb 27, 2023 •

edited

Loading

ankur22 commented Feb 27, 2023 •

edited

Loading

inancgumus left a comment •

edited

Loading

inancgumus commented Feb 28, 2023 •

edited

Loading

ankur22 left a comment •

edited

Loading