Skip to content

Commit

Permalink
Remove support for browser env option (#870)
Browse files Browse the repository at this point in the history
A decision was made to no longer support the env browser option as this
was not providing much value.
  • Loading branch information
ka3de authored May 3, 2023
1 parent 6615b02 commit af0f9f0
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 41 deletions.
10 changes: 3 additions & 7 deletions chromium/browser_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,6 @@ func (b *BrowserType) Launch(opts goja.Value) (_ api.Browser, browserProcessID i
func (b *BrowserType) launch(
ctx context.Context, opts *common.LaunchOptions, logger *log.Logger,
) (_ *common.Browser, pid int, _ error) {
envs := make([]string, 0, len(opts.Env))
for k, v := range opts.Env {
envs = append(envs, fmt.Sprintf("%s=%s", k, v))
}
flags, err := prepareFlags(opts, &(b.vu.State()).Options)
if err != nil {
return nil, 0, fmt.Errorf("%w", err)
Expand All @@ -204,7 +200,7 @@ func (b *BrowserType) launch(
<-c.Done()
}(ctx)

browserProc, err := b.allocate(ctx, opts, flags, envs, dataDir, logger)
browserProc, err := b.allocate(ctx, opts, flags, dataDir, logger)
if browserProc == nil {
return nil, 0, fmt.Errorf("launching browser: %w", err)
}
Expand Down Expand Up @@ -237,7 +233,7 @@ func (b *BrowserType) Name() string {
// allocate starts a new Chromium browser process and returns it.
func (b *BrowserType) allocate(
ctx context.Context, opts *common.LaunchOptions,
flags map[string]any, env []string, dataDir *storage.Dir,
flags map[string]any, dataDir *storage.Dir,
logger *log.Logger,
) (_ *common.BrowserProcess, rerr error) {
bProcCtx, bProcCtxCancel := context.WithTimeout(ctx, opts.Timeout)
Expand All @@ -257,7 +253,7 @@ func (b *BrowserType) allocate(
path = b.ExecutablePath()
}

return common.NewLocalBrowserProcess(bProcCtx, path, args, env, dataDir, bProcCtxCancel, logger) //nolint: wrapcheck
return common.NewLocalBrowserProcess(bProcCtx, path, args, dataDir, bProcCtxCancel, logger) //nolint: wrapcheck
}

// ExecutablePath returns the path where the extension expects to find the browser executable.
Expand Down
8 changes: 0 additions & 8 deletions common/browser_options.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
const (
optArgs = "args"
optDebug = "debug"
optEnv = "env"
optExecutablePath = "executablePath"
optHeadless = "headless"
optIgnoreDefaultArgs = "ignoreDefaultArgs"
Expand All @@ -35,7 +34,6 @@ type ProxyOptions struct {
type LaunchOptions struct {
Args []string
Debug bool
Env map[string]string
ExecutablePath string
Headless bool
IgnoreDefaultArgs []string
Expand All @@ -56,7 +54,6 @@ type LaunchPersistentContextOptions struct {
// NewLaunchOptions returns a new LaunchOptions.
func NewLaunchOptions() *LaunchOptions {
return &LaunchOptions{
Env: make(map[string]string),
Headless: true,
LogCategoryFilter: ".*",
Timeout: DefaultTimeout,
Expand All @@ -67,7 +64,6 @@ func NewLaunchOptions() *LaunchOptions {
// for a browser running in a remote machine.
func NewRemoteBrowserLaunchOptions() *LaunchOptions {
return &LaunchOptions{
Env: make(map[string]string),
Headless: true,
LogCategoryFilter: ".*",
Timeout: DefaultTimeout,
Expand All @@ -85,7 +81,6 @@ func (l *LaunchOptions) Parse(ctx context.Context, logger *log.Logger, opts goja
rt = k6ext.Runtime(ctx)
o = opts.ToObject(rt)
defaults = map[string]any{
optEnv: l.Env,
optHeadless: l.Headless,
optLogCategoryFilter: l.LogCategoryFilter,
optTimeout: l.Timeout,
Expand All @@ -109,8 +104,6 @@ func (l *LaunchOptions) Parse(ctx context.Context, logger *log.Logger, opts goja
err = exportOpt(rt, k, v, &l.Args)
case optDebug:
l.Debug, err = parseBoolOpt(k, v)
case optEnv:
err = exportOpt(rt, k, v, &l.Env)
case optExecutablePath:
l.ExecutablePath, err = parseStrOpt(k, v)
case optHeadless:
Expand Down Expand Up @@ -141,7 +134,6 @@ func (l *LaunchOptions) shouldIgnoreIfBrowserIsRemote(opt string) bool {

shouldIgnoreIfBrowserIsRemote := map[string]struct{}{
optArgs: {},
optEnv: {},
optExecutablePath: {},
optHeadless: {},
optIgnoreDefaultArgs: {},
Expand Down
18 changes: 0 additions & 18 deletions common/browser_options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ func TestBrowserLaunchOptionsParse(t *testing.T) {
t.Parallel()

defaultOptions := &LaunchOptions{
Env: make(map[string]string),
Headless: true,
LogCategoryFilter: ".*",
Timeout: DefaultTimeout,
Expand Down Expand Up @@ -46,7 +45,6 @@ func TestBrowserLaunchOptionsParse(t *testing.T) {
opts: map[string]any{
// disallow changing the following opts
"args": []string{"any"},
"env": map[string]string{"some": "thing"},
"executablePath": "something else",
"headless": false,
"ignoreDefaultArgs": []string{"any"},
Expand All @@ -61,7 +59,6 @@ func TestBrowserLaunchOptionsParse(t *testing.T) {
tb.Helper()
assert.Equal(t, &LaunchOptions{
// disallowed:
Env: make(map[string]string),
Headless: true,
// allowed:
Debug: true,
Expand All @@ -75,15 +72,13 @@ func TestBrowserLaunchOptionsParse(t *testing.T) {
},
"nulls": { // don't override the defaults on `null`
opts: map[string]any{
"env": nil,
"headless": nil,
"logCategoryFilter": nil,
"timeout": nil,
},
assert: func(tb testing.TB, lo *LaunchOptions) {
tb.Helper()
assert.Equal(tb, &LaunchOptions{
Env: make(map[string]string),
Headless: true,
LogCategoryFilter: ".*",
Timeout: DefaultTimeout,
Expand Down Expand Up @@ -119,19 +114,6 @@ func TestBrowserLaunchOptionsParse(t *testing.T) {
opts: map[string]any{"debug": "true"},
err: "debug should be a boolean",
},
"env": {
opts: map[string]any{
"env": map[string]string{"key": "value"},
},
assert: func(tb testing.TB, lo *LaunchOptions) {
tb.Helper()
assert.Equal(t, map[string]string{"key": "value"}, lo.Env)
},
},
"env_err": {
opts: map[string]any{"env": 1},
err: "env should be a map",
},
"executablePath": {
opts: map[string]any{
"executablePath": "cmd/somewhere",
Expand Down
11 changes: 3 additions & 8 deletions common/browser_process.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ type BrowserProcess struct {
// NewLocalBrowserProcess starts a local browser process and
// returns a new BrowserProcess instance to interact with it.
func NewLocalBrowserProcess(
ctx context.Context, path string, args, env []string, dataDir *storage.Dir,
ctx context.Context, path string, args []string, dataDir *storage.Dir,
ctxCancel context.CancelFunc, logger *log.Logger,
) (*BrowserProcess, error) {
cmd, err := execute(ctx, ctxCancel, path, args, env, dataDir, logger)
cmd, err := execute(ctx, path, args, dataDir, logger)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -151,7 +151,7 @@ type command struct {
}

func execute(
ctx context.Context, ctxCancel func(), path string, args, env []string,
ctx context.Context, path string, args []string,
dataDir *storage.Dir, logger *log.Logger,
) (command, error) {
cmd := exec.CommandContext(ctx, path, args...)
Expand All @@ -166,11 +166,6 @@ func execute(
return command{}, fmt.Errorf("%w", err)
}

// Set up environment variable for process
if len(env) > 0 {
cmd.Env = append(os.Environ(), env...)
}

// We must start the cmd before calling cmd.Wait, as otherwise the two
// can run into a data race.
err = cmd.Start()
Expand Down

0 comments on commit af0f9f0

Please sign in to comment.