Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stabilize cookie handling #1008

Merged
merged 5 commits into from
Aug 23, 2023
Merged

Stabilize cookie handling #1008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ee1032
Move cookie methods closer
inancgumus Aug 22, 2023
a787622
Refactor addCookies
inancgumus Aug 22, 2023
7ae14a9
Refactor clearCookies
inancgumus Aug 22, 2023
0a83c65
Refactor AddCookies to return error
inancgumus Aug 22, 2023
92db064
Refactor clearCookies to return error
inancgumus Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/browser_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ import (

// BrowserContext is the public interface of a CDP browser context.
type BrowserContext interface {
AddCookies(cookies goja.Value)
AddCookies(cookies goja.Value) error
AddInitScript(script goja.Value, arg goja.Value) error
Browser() Browser
ClearCookies()
ClearCookies() error
ClearPermissions()
Close()
Cookies() ([]any, error) // TODO: make it []Cookie later on
Expand Down
97 changes: 42 additions & 55 deletions common/browser_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,17 +79,6 @@ func NewBrowserContext(
return &b, nil
}

// AddCookies adds cookies into this browser context.
// All pages within this context will have these cookies installed.
func (b *BrowserContext) AddCookies(cookies goja.Value) {
b.logger.Debugf("BrowserContext:AddCookies", "bctxid:%v", b.id)

err := b.addCookies(cookies)
if err != nil {
k6ext.Panic(b.ctx, "adding cookies: %w", err)
}
}

// AddInitScript adds a script that will be initialized on all new pages.
func (b *BrowserContext) AddInitScript(script goja.Value, arg goja.Value) error {
b.logger.Debugf("BrowserContext:AddInitScript", "bctxid:%v", b.id)
Expand Down Expand Up @@ -145,16 +134,6 @@ func (b *BrowserContext) Browser() api.Browser {
return b.browser
}

// ClearCookies clears cookies.
func (b *BrowserContext) ClearCookies() {
b.logger.Debugf("BrowserContext:ClearCookies", "bctxid:%v", b.id)

action := storage.ClearCookies().WithBrowserContextID(b.id)
if err := action.Do(b.ctx); err != nil {
k6ext.Panic(b.ctx, "clearing cookies: %w", err)
}
}

// ClearPermissions clears any permission overrides.
func (b *BrowserContext) ClearPermissions() {
b.logger.Debugf("BrowserContext:ClearPermissions", "bctxid:%v", b.id)
Expand All @@ -177,11 +156,6 @@ func (b *BrowserContext) Close() {
}
}

// Cookies is not implemented.
func (b *BrowserContext) Cookies() ([]any, error) {
return nil, fmt.Errorf("BrowserContext.cookies() has not been implemented yet: %w", k6error.ErrFatal)
}

// ExposeBinding is not implemented.
func (b *BrowserContext) ExposeBinding(name string, callback goja.Callable, opts goja.Value) {
k6ext.Panic(b.ctx, "BrowserContext.exposeBinding(name, callback, opts) has not been implemented yet")
Expand Down Expand Up @@ -463,48 +437,61 @@ func (b *BrowserContext) getSession(id target.SessionID) *Session {
return b.browser.conn.getSession(id)
}

func (b *BrowserContext) addCookies(cookies goja.Value) error {
// AddCookies adds cookies into this browser context.
// All pages within this context will have these cookies installed.
func (b *BrowserContext) AddCookies(cookies goja.Value) error {
b.logger.Debugf("BrowserContext:AddCookies", "bctxid:%v", b.id)

var cookieParams []network.CookieParam
if !gojaValueExists(cookies) {
return Error("cookies value is not set")
return Error("cookies argument must be set")
}

rt := b.vu.Runtime()
err := rt.ExportTo(cookies, &cookieParams)
if err != nil {
return fmt.Errorf("unable to export cookies value to cookieParams. %w", err)
if err := b.vu.Runtime().ExportTo(cookies, &cookieParams); err != nil {
return fmt.Errorf("cannot recognize cookie values: %w", err)
}

// Create new array of pointers to items in cookieParams
var cookieParamsPointers []*network.CookieParam
for i := 0; i < len(cookieParams); i++ {
cookieParam := cookieParams[i]
coookiesToSet := make([]*network.CookieParam, len(cookieParams))
for i, cookie := range cookieParams {
c := cookie

if cookieParam.Name == "" {
return fmt.Errorf("cookie name is not set. %#v", cookieParam)
if c.Name == "" {
return fmt.Errorf("cookie name must be set: %#v", c)
}

if cookieParam.Value == "" {
return fmt.Errorf("cookie value is not set. %#v", cookieParam)
if c.Value == "" {
return fmt.Errorf("cookie value must be set: %#v", c)
}

// if URL is not set, both Domain and Path must be provided
if cookieParam.URL == "" {
if cookieParam.Domain == "" || cookieParam.Path == "" {
return fmt.Errorf(
"if cookie url is not provided, both domain and path must be specified. %#v",
cookieParam,
)
}
if c.URL == "" && (c.Domain == "" || c.Path == "") {
const msg = "if cookie URL is not provided, both domain and path must be specified: %#v"
ankur22 marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf(msg, c)
}

cookieParamsPointers = append(cookieParamsPointers, &cookieParam)
coookiesToSet[i] = &c
}

action := storage.SetCookies(cookieParamsPointers).WithBrowserContextID(b.id)
if err := action.Do(cdp.WithExecutor(b.ctx, b.browser.conn)); err != nil {
return fmt.Errorf("unable to execute SetCookies action: %w", err)
setCookies := storage.
SetCookies(coookiesToSet).
WithBrowserContextID(b.id)
if err := setCookies.Do(cdp.WithExecutor(b.ctx, b.browser.conn)); err != nil {
return fmt.Errorf("cannot set cookies: %w", err)
}

return nil
}

// ClearCookies clears cookies.
func (b *BrowserContext) ClearCookies() error {
b.logger.Debugf("BrowserContext:ClearCookies", "bctxid:%v", b.id)

clearCookies := storage.
ClearCookies().
WithBrowserContextID(b.id)
if err := clearCookies.Do(b.ctx); err != nil {
return fmt.Errorf("clearing cookies: %w", err)
ankur22 marked this conversation as resolved.
Show resolved Hide resolved
}
return nil
}

// Cookies is not implemented.
func (b *BrowserContext) Cookies() ([]any, error) {
return nil, fmt.Errorf("BrowserContext.cookies() has not been implemented yet: %w", k6error.ErrFatal)
}
35 changes: 17 additions & 18 deletions tests/browser_context_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,22 +55,22 @@ func TestBrowserContextAddCookies(t *testing.T) {
errorTests := []struct {
description string
cookiesCmd string
shouldPanic bool
shouldFail bool
}{
{
description: "nil_cookies",
cookiesCmd: "",
shouldPanic: true,
shouldFail: true,
},
{
description: "goja_null_cookies",
cookiesCmd: "null;",
shouldPanic: true,
shouldFail: true,
},
{
description: "goja_undefined_cookies",
cookiesCmd: "undefined;",
shouldPanic: true,
shouldFail: true,
},
{
description: "goja_cookies_object",
Expand All @@ -81,12 +81,12 @@ func TestBrowserContextAddCookies(t *testing.T) {
url: "http://test.go",
});
`,
shouldPanic: true,
shouldFail: true,
},
{
description: "goja_cookies_string",
cookiesCmd: `"test_cookie_name=test_cookie_value"`,
shouldPanic: true,
shouldFail: true,
},
{
description: "cookie_missing_name",
Expand All @@ -96,7 +96,7 @@ func TestBrowserContextAddCookies(t *testing.T) {
url: "http://test.go",
}
];`,
shouldPanic: true,
shouldFail: true,
},
{
description: "cookie_missing_value",
Expand All @@ -106,7 +106,7 @@ func TestBrowserContextAddCookies(t *testing.T) {
url: "http://test.go",
}
];`,
shouldPanic: true,
shouldFail: true,
},
{
description: "cookie_missing_url",
Expand All @@ -116,7 +116,7 @@ func TestBrowserContextAddCookies(t *testing.T) {
value: "test_cookie_value",
}
];`,
shouldPanic: true,
shouldFail: true,
},
{
description: "cookies_missing_path",
Expand All @@ -127,7 +127,7 @@ func TestBrowserContextAddCookies(t *testing.T) {
domain: "http://test.go",
}
];`,
shouldPanic: true,
shouldFail: true,
},
{
description: "cookies_missing_domain",
Expand All @@ -138,7 +138,7 @@ func TestBrowserContextAddCookies(t *testing.T) {
path: "/to/page",
}
];`,
shouldPanic: true,
shouldFail: true,
},
{
description: "cookie_with_url",
Expand All @@ -149,7 +149,7 @@ func TestBrowserContextAddCookies(t *testing.T) {
url: "http://test.go",
}
];`,
shouldPanic: false,
shouldFail: false,
},
{
description: "cookie_with_domain_and_path",
Expand All @@ -161,10 +161,9 @@ func TestBrowserContextAddCookies(t *testing.T) {
path: "/to/page",
}
];`,
shouldPanic: false,
shouldFail: false,
},
}

for _, tt := range errorTests {
tt := tt
t.Run(tt.description, func(t *testing.T) {
Expand All @@ -182,11 +181,11 @@ func TestBrowserContextAddCookies(t *testing.T) {
bc, err := tb.NewContext(nil)
require.NoError(t, err)

if tt.shouldPanic {
assert.Panics(t, func() { bc.AddCookies(cookies) })
} else {
assert.NotPanics(t, func() { bc.AddCookies(cookies) })
if tt.shouldFail {
require.Error(t, bc.AddCookies(cookies))
ankur22 marked this conversation as resolved.
Show resolved Hide resolved
return
}
require.NoError(t, bc.AddCookies(cookies))
})
}
}
Loading