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

Allow specifying path for storing cookies #547

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kasatani
Copy link

In current implementation of ghostdriver there is no way to load cookies when starting a session. Passing --cookies-file to phantomjs doesn't work because ghostdriver's session.js creates a new cookie jar without any argument, which doesn't allow us to load or save cookies.

I have added "phantomjs.cookies.path" capability that lets you specify the path for the cookie jar.

@jesg jesg self-requested a review July 21, 2017 01:53
Copy link
Collaborator

@jesg jesg left a comment

Choose a reason for hiding this comment

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

thanks for working on this. looks like a good addition to ghostdriver.

i need two changes to accept this feature.

  1. the default case should be backwards compatible

  2. basic unit test to verify this actually sets cookies

@@ -113,7 +113,8 @@ ghostdriver.Session = function(desiredCapabilities) {
},
_windows = {}, //< NOTE: windows are "webpage" in Phantom-dialect
_currentWindowHandle = null,
_cookieJar = require('cookiejar').create(),
_cookieJar,
_cookiePath = null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

the default should be an empty string to be consistent with the phantomjs source. though i'd prefer to be conservative and pass no arguments when this option is not used.

src/session.js Outdated
_cookiePath = desiredCapabilities['phantomjs.cookies.path'];
_negotiatedCapabilities['phantomjs.cookies.path'] = _cookiePath;
}
_cookieJar = require('cookiejar').create(_cookiePath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

the default case should be the same. require('cookiejar').create()

@kasatani
Copy link
Author

kasatani commented Oct 6, 2017

Thanks for the feedback. I've fixed the behavior when the path is not set, and added the test case.

@jesg
Copy link
Collaborator

jesg commented Oct 23, 2017

@kasatani the test ghostdriver.CookieStoreTest#shouldLoadCookies does not pass when i run it.

to run the test:

cd <ghostdriver project root>/test/java
./gradlew test -Dtest.single=CookieStoreTest  --debug

you may need to modify phantomjs_exec_path in <project root>/test/config.ini to phantomjs on your system.

@kasatani
Copy link
Author

CookieStoreTest passes in my environment. Which version of phantomjs should I use for the testing? I'm using the released phantomjs-2.1.1 on macOS Sierra.

@jesg
Copy link
Collaborator

jesg commented Nov 3, 2017

Which version of phantomjs should I use for the testing?

i normally use a patched version of 2.1.1 but i'm also seeing the error in the official 2.1.1 release. i run the tests on ubuntu 17.

i should be able to take a look on a mac sometime next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants