Evaluate move of slowMo and timeout browser options to browser context
#857
Comments
ka3de
changed the title
slowMo and timeout browser options to browser contextslowMo and timeout browser options to browser context
ka3de
added
team/k6browser
A clarification on browser
|
|
Great work analysing the two browser properties! DISCLAIMER Ignore this comment and read this comment. Timeout
So change this to an env var? I think it could still be useful to set a timeout depending on the machine that is running the browser. Although 30seconds (I think this is the default) should be enough in most cases.
I don't think this browser level timeout should be inherited to anywhere else in the application and definitely shouldn't affect the navigation. I think once we abstract away the browser, then the timeout that users should interact with are through the browserContext or other elements that users work with. I think the browser level timeout should only affect the browser specific actions, such as launching/connecting to a browser and creating a new context (so browserType and browser). If we allowed the browser level timeout to alter the timeout in browserContext, page, frame etc, then the behaviour of a test will differ depending on which environment that the test is ran in.
I like this idea. slowmo
I think delaying this is a good short term solution, and we could convert it to an env var. I think working with the go context to change the behaviour of the application is a bad smell, and also not what the context is supposed to be used for (IMO), so i would prefer a well thoughtout refactor where we avoid working with the go context where the option can be safely moved to the browserContext level.
Did you get a chance to try this out and see if it worked? Or are there lots more work to be done for this to work? |
|
We seem to have quite different opinions on this 😄 Timeout
Personally, following the criteria mentioned in #856 , I think it's best to keep the
I personally don't think there is a need for that, and as you said, keeping a default of 30s for that should be more than enough. But no strong opinion. What I don't like of the current implementation is that the
Completely agree with this 👍
I don't think this should be exposed to the user, but instead use sensible defaults. But no strong opinion. slowMo
We can delay the decision on this indeed, although I don't see this as an ENV var because I see it as an option that can have a direct impact in the test execution.
I didn't, it seemed a considerable refactor, but I can take a better look. I did try to implement it following the same approach as we are using now, through context, but for the diffdiff --git a/common/browser_context.go b/common/browser_context.go
index b98e8a6..1ed7986 100644
--- a/common/browser_context.go
+++ b/common/browser_context.go
@@ -52,13 +52,16 @@ func NewBrowserContext(
) (*BrowserContext, error) {
b := BrowserContext{
BaseEventEmitter: NewBaseEventEmitter(ctx),
- ctx: ctx,
- browser: browser,
- id: id,
- opts: opts,
- logger: logger,
- vu: k6ext.GetVU(ctx),
- timeoutSettings: NewTimeoutSettings(nil),
+ // TODO: Maybe there is no need to set all BC options
+ // and we could just set the slowMo param with a specific
+ // key in context
+ ctx: WithBrowserContextOptions(ctx, opts),
+ browser: browser,
+ id: id,
+ opts: opts,
+ logger: logger,
+ vu: k6ext.GetVU(ctx),
+ timeoutSettings: NewTimeoutSettings(nil),
}
if opts != nil && len(opts.Permissions) > 0 {
diff --git a/common/browser_context_options.go b/common/browser_context_options.go
index 74c65ef..449e34d 100644
--- a/common/browser_context_options.go
+++ b/common/browser_context_options.go
@@ -3,6 +3,7 @@ package common
import (
"context"
"fmt"
+ "time"
"github.com/grafana/xk6-browser/k6ext"
@@ -31,6 +32,8 @@ type BrowserContextOptions struct {
UserAgent string `js:"userAgent"`
VideosPath string `js:"videosPath"`
Viewport *Viewport `js:"viewport"`
+ SlowMo time.Duration `js:"slowMo"`
+ Timeout time.Duration `js:"timeout"`
}
// NewBrowserContextOptions creates a default set of browser context options.
@@ -45,6 +48,7 @@ func NewBrowserContextOptions() *BrowserContextOptions {
ReducedMotion: ReducedMotionNoPreference,
Screen: &Screen{Width: DefaultScreenWidth, Height: DefaultScreenHeight},
Viewport: &Viewport{Width: DefaultScreenWidth, Height: DefaultScreenHeight},
+ Timeout: DefaultTimeout,
}
}
@@ -127,6 +131,14 @@ func (b *BrowserContextOptions) Parse(ctx context.Context, opts goja.Value) erro
return err
}
b.Viewport = viewport
+ case "slowMo":
+ b.SlowMo, _ = parseTimeOpt(k, opts.Get(k))
+ case "timeout":
+ var err error
+ b.Timeout, err = parseTimeOpt(k, opts.Get(k))
+ if err != nil {
+ b.Timeout = DefaultTimeout
+ }
}
}
}
diff --git a/common/context.go b/common/context.go
index 8efe868..cc29008 100644
--- a/common/context.go
+++ b/common/context.go
@@ -10,6 +10,7 @@ const (
ctxKeyLaunchOptions ctxKey = iota
ctxKeyHooks
ctxKeyIterationID
+ ctxKeyBrowserCtxOptions
)
func WithHooks(ctx context.Context, hooks *Hooks) context.Context {
@@ -47,6 +48,18 @@ func GetLaunchOptions(ctx context.Context) *LaunchOptions {
return v.(*LaunchOptions)
}
+func WithBrowserContextOptions(ctx context.Context, opts *BrowserContextOptions) context.Context {
+ return context.WithValue(ctx, ctxKeyBrowserCtxOptions, opts)
+}
+
+func GetBrowserContextOptions(ctx context.Context) *BrowserContextOptions {
+ v := ctx.Value(ctxKeyBrowserCtxOptions)
+ if v == nil {
+ return nil
+ }
+ return v.(*BrowserContextOptions)
+}
+
// contextWithDoneChan returns a new context that is canceled either
// when the done channel is closed or ctx is canceled.
func contextWithDoneChan(ctx context.Context, done chan struct{}) context.Context {
diff --git a/common/hooks.go b/common/hooks.go
index abfa220..911e8ed 100644
--- a/common/hooks.go
+++ b/common/hooks.go
@@ -30,7 +30,9 @@ func applySlowMo(ctx context.Context) {
}
func defaultSlowMo(ctx context.Context) {
- sm := GetLaunchOptions(ctx).SlowMo
+ // Panic here due to NPE when trying to retrieve
+ // browserContext options from ctx
+ sm := GetBrowserContextOptions(ctx).SlowMo
if sm <= 0 {
return
} |
|
DISCLAIMER Ignore this comment and read this comment. Timeout
I still think having the timeout as an env var is valuable to the user. Although we can set a default timeout, it should be overridable since the environment that the user is running the browser in could require a large timeout than what we provide.
Agreed. Slowmo
Yeah, i agree with this. In that case we have two options:
In terms of |
|
I think this is a good example as to why setting a timeout at the browser level is confusing for users, since at the moment users can set it there and it will affect all other timeouts, this just looks confusing to me 🙂 |
Timeout
In this case, are you referring to the TO for "browser initialization" etc? Or to the one that applies to navigation? slowMo
I guess my question is. Do we need to support "individual" slowMo values per
That's something that I would like to rediscuss based on this evaluation. Personally I think it can make sense to keep both of this options at the scenario browser params, as default values for the whole test. Then, for example in the case of In any case, I'm open to other options, I just think this would be the best option to:
|
Aha! I think i've been misunderstanding how these properties are implemented. They're not passed to the actual chromium instance, but they're implemented in the xk6-browser go code in the This means that multiple The one that I'm not completely convinced on still is for
I still think that the this should only affect the timeout when making the connection against Chromium, and not the navigation. So at the moment I still think that there's a valid reason to move the
It's also worth noting that Playwright uses the |
|
Yep, I think you have convinced me with this last comment about the usage of
In regards of |
@ka3de, great analysis, and nice work, both this issue and the other one (#856). I want to add my two cents here about my observations. In my tests, while connecting to a single browser from multiple tests, I find out that supporting timeout at the browser level and navigation is critical. It's because the browser process may get overloaded at some point, and increasing the timeout allowed tests to continue and finish, although a bit later. |
|
Thanks for the input @inancgumus ! Considering that, the points that I think still need agreement are:
WDYT @ankur22 , @inancgumus ? |
How about
Agreed.
This one i'm not sure of either. I'd prefer to either remove it from |
|
However, if we will remove the browser from the users' access, it makes sense to me to have simply,
👍 And I would keep |
Conclusion:Based on all comments I believe we have reached a final consensus: timeout
slowMo
|
Based on #857, a decision was made to not expose slowMo browser option by now, and try to move it to the browser context level instead, which will provide more flexibility.
Based on #857, a decision was made to not expose slowMo browser option by now, and try to move it to the browser context level instead, which will provide more flexibility.
Based on #857, a decision was made to not expose slowMo browser option by now, and try to move it to the browser context level instead, which will provide more flexibility.
Based on #857, a decision was made to not expose slowMo browser option by now, and try to move it to the browser context level instead, which will provide more flexibility.
Based on #857, a decision was made to not expose slowMo browser option by now, and try to move it to the browser context level instead, which will provide more flexibility.
Based on #857, a decision was made to not expose slowMo browser option by now, and try to move it to the browser context level instead, which will provide more flexibility.
Based on #857, a decision was made to not expose slowMo browser option by now, and try to move it to the browser context level instead, which will provide more flexibility.
Based on #857, a decision was made to not expose slowMo browser option by now, and try to move it to the browser context level instead, which will provide more flexibility.
Based on #857, a decision was made to not expose slowMo browser option by now, and try to move it to the browser context level instead, which will provide more flexibility.
Based on #857, a decision was made to not expose slowMo browser option by now, and try to move it to the browser context level instead, which will provide more flexibility.
Based on the work done in #856 ,
slowMoandtimeoutare the parameters from the current browser options that have a direct impact in the test execution and therefore which its definition should be kept in the test script itself.At the same time, these options effect is not translated to the browser instance and instead is part of our own k6 browser implementation (handled in the Go code). Therefore the goal of this issue is to evaluate if
slowMoandtimeoutparameters are a better fit to be defined at thebrowserorbrowserContextlevel. This can include considerations on, but not limited to:browserandbrowserContext).Related: #856
The text was updated successfully, but these errors were encountered: