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.

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

fix: cookies regression: send Page commands for cookies handling #1534

Merged
merged 2 commits into from
Apr 1, 2023
Merged
Changes from 1 commit
Commits
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
Next Next commit
chore: use remote debugger for cookies
  • Loading branch information
KazuCocoa committed Mar 30, 2023
commit f3fc192797383e61bc797d7dfbfb0ace7f07605e
58 changes: 29 additions & 29 deletions lib/commands/web.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,25 +195,28 @@ const commands = {
* @this {XCUITestDriver}
*/
async getCookies() {
if (!this.isWebContext()) {
throw new errors.NotImplementedError();
}

this.log.debug('Retrieving all cookies');
if (!this.isWebContext()) {
throw new errors.NotImplementedError();
}

let script = 'return document.cookie';
let jsCookies = await this.executeAtom('execute_script', [script, []]);
// get the cookies from the remote debugger, or an empty object
const cookies = await this.remote.getCookies() || {cookies: []};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what remote.getCookies() returns if no cookies found?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{"cookies":[]} in my test, but not sure if it has a case that could return null for example since I haven't tested old versions.

This PR is from the previous deleted code as-is not to change anything right now. (Refactoring would be a separate PR)


let cookies = [];
try {
for (let [name, value] of _.toPairs(cookieUtils.createJWPCookie(undefined, jsCookies))) {
cookies.push({name, value});
// the value is URI encoded, so decode it safely
const decodedCookieValues = cookies.cookies.map((cookie) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we could destruct the object above, like const {cookies} =....

try {
return decodeURI(cookie.value);
} catch (error) {
this.log.debug(`Cookie ${cookie.name} was not decoded successfully. Cookie value: ${cookie.value}`);
this.log.warn(error);
return undefined;
}
return cookies;
} catch (err) {
this.log.error(err);
throw new errors.UnknownError(`Error parsing cookies from result: '${jsCookies}'`);
}
});

// zip cookies with decoded value, removing undefined cookie values
return _.zip(cookies.cookies, decodedCookieValues)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may decode and return in a single chain. I don't see a need for the intermediate decodedCookieValues variable

.filter(([, value]) => !_.isUndefined(value))
.map(([cookie, value]) => Object.assign({}, cookie, {value}));
},
/**
* @this {XCUITestDriver}
Expand Down Expand Up @@ -250,14 +253,15 @@ const commands = {
throw new errors.NotImplementedError();
}

// check cookie existence
let cookies = await this.getCookies();
if (_.indexOf(_.map(cookies, 'name'), cookieName) === -1) {
const cookies = await this.getCookies();
const cookie = cookies.find((cookie) => cookie.name === cookieName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be cookies.find(({name}) => name === cookieName)

if (!cookie) {
this.log.debug(`Cookie '${cookieName}' not found. Ignoring.`);
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be always return true?

}

return await this._deleteCookie(cookieName);
await this._deleteCookie(cookie);
return true;
},
/**
* @this {XCUITestDriver}
Expand All @@ -267,11 +271,9 @@ const commands = {
throw new errors.NotImplementedError();
}

let cookies = await this.getCookies();
if (cookies.length) {
for (let cookie of cookies) {
await this._deleteCookie(cookie.name);
}
const cookies = await this.getCookies();
for (const cookie of cookies) {
await this._deleteCookie(cookie);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may we use B.all here to speed up the process?

}
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we always return true?

},
Expand All @@ -282,10 +284,8 @@ const helpers = {
* @this {XCUITestDriver}
*/
async _deleteCookie(cookieName) {
this.log.debug(`Deleting cookie '${cookieName}'`);
let webCookie = cookieUtils.expireCookie(cookieName, {path: '/'});
let script = `document.cookie = ${JSON.stringify(webCookie)}`;
await this.executeAtom('execute_script', [script, []]);
const url = `http${cookie.secure ? 's' : ''}://${cookie.domain}${cookie.path}`;
return await this.remote.deleteCookie(cookie.name, url);
},
/**
* @this {XCUITestDriver}
Expand Down