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

Add 'expiry' to 'add cookie' keyword and add 'get cookie' keyword. #930

Merged
merged 9 commits into from
Oct 5, 2017
Merged

Add 'expiry' to 'add cookie' keyword and add 'get cookie' keyword. #930

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a242eae
Add 'expiry' to 'add cookie' keyword and add get cookie expiry keyword.
wappowers Sep 28, 2017
3938b59
Datetime format is now used for expiry setting. Get cookie keyword ad…
wappowers Sep 28, 2017
1e9b12a
Using Collections library for dictionary reliant acceptance test.
wappowers Sep 28, 2017
7f556b6
Merge branch 'master' of https://github.com/robotframework/SeleniumLi…
wappowers Sep 29, 2017
52f4db4
Refactor to have 'get cookie' return an extended variable rather than…
wappowers Sep 29, 2017
56730c3
Updated to use Robot Framework DateTime library, and PEP8 changes.
wappowers Oct 2, 2017
63f0209
Acceptance tests pass regardless of timezone.
wappowers Oct 2, 2017
6b57846
Added name to CHANGES.rst file.
wappowers Oct 3, 2017
dfa3fab
Added full_info option to CookieInformation class, and some code revi…
wappowers Oct 3, 2017
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
38 changes: 35 additions & 3 deletions src/SeleniumLibrary/keywords/cookie.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

from SeleniumLibrary.base import LibraryComponent, keyword
from SeleniumLibrary.utils import is_truthy
from datetime import datetime


class CookieKeywords(LibraryComponent):
Expand Down Expand Up @@ -53,11 +54,24 @@ def get_cookie_value(self, name):
raise ValueError("Cookie with name %s not found." % name)

@keyword
def add_cookie(self, name, value, path=None, domain=None, secure=None):
def get_cookie(self, name):
"""Returns the cookie found with `name` in extended variable format.

If no cookie is found with `name`, this keyword fails.
"""
cookie = self.browser.get_cookie(name)
if cookie is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Write: if cookie:

cookie_information = CookieInformation(cookie['name'], cookie['value'], cookie['domain'], cookie['path'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow pep8 in line length recommendation in new code. Only the keyword examples can normally exceed the line length.

cookie['expiry'], cookie['httpOnly'], cookie['secure'])
return cookie_information
raise ValueError("Cookie with name %s not found." % name)

@keyword
def add_cookie(self, name, value, path=None, domain=None, secure=None, expiry=None):
"""Adds a cookie to your current session.

"name" and "value" are required, "path", "domain" and "secure" are
optional
"name" and "value" are required, "path", "domain", "secure" and "expiry" are
optional. Expiry must be in datetime format for UTC time zone (Example: '2027-09-28 16:21:35').
"""
new_cookie = {'name': name, 'value': value}
if is_truthy(path):
Expand All @@ -67,4 +81,22 @@ def add_cookie(self, name, value, path=None, domain=None, secure=None):
# Secure should be True or False
if is_truthy(secure):
new_cookie['secure'] = secure
if is_truthy(expiry):
expiry_datetime = datetime.strptime(expiry, '%Y-%m-%d %H:%M:%S')
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to use Robot Framework DataTime library for better support different time formats.

Example something like this could work:

from robot.libraries.DateTime import convert_date

...

expiry_datetime = int(convert_date('20140528 12:05:03.111', result_format='epoch'))

Then the keyword documentation could say that expiry supports same formats as DateTime library,

expiry_epoch = (expiry_datetime - datetime(1970, 1, 1)).total_seconds()
new_cookie['expiry'] = int(expiry_epoch)
self.browser.add_cookie(new_cookie)


class CookieInformation(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

It is better ro write CookieInformation class like this:

class CookieInformation(object):

    def __init__(self, **cookie_dict):
        for key in cookie_dict:
            setattr(self, key, cookie_dict[key])

And write line 64 and 65 like this:
return CookieInformation(**cookie)

Because selenium cookie is a dictionary and at least in Windows get_cookie does not return expiry if it not defined.

Copy link
Member

Choose a reason for hiding this comment

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

First of all, such a generic __init__ would be even better implemented like this:

def __init__(self, data):
    self.__dict__.update(data)

I'm don't think that is a good idea in this context, though:

  • Explicit is better than implicit. We are saving few lines of code but hiding information. Being able to use this object you needed to know what info the Selenium method returned.
  • Making it sure we always return same info regardless what browsers return is important.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then something like this could work better:

class CookieInformation(object):

    def __init__(self, cookie):
        self.domain = cookie['domain'] if 'domain' in cookie else None
        self.expiry = cookie['expiry'] if 'expiry' in cookie else None
        self.httpOnly = cookie['httpOnly'] if 'httpOnly' in cookie else None
        self.name = cookie['name'] if 'name' in cookie else None
        self.path = cookie['path'] if 'path' in cookie else None
        self.secure = cookie['secure'] if 'secure' in cookie else None
        self.value = cookie['value'] if 'value' in cookie else None

    @property
    def full_info(self):
        return self.__dict__

def __init__(self, name, value, domain, path, expiry, http, secure):
self.name = name
self.value = value
self.domain = domain
self.path = path
self.expiry = expiry
self.http = http
self.secure = secure

def __str__(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

The __str__ method is not needed.

Copy link
Member

Choose a reason for hiding this comment

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

If actually is. Robot automatically logs return values from kws and without this method the message is useless. Should be discussed what info to show, though. Could only show name and value and keep the message short or return all the info. I'd probably keep __str__ simple but could add a separate property showing everything that could be used like

 Log    ${cookie.full_info}

if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did not know that it is needed. Most of the time, I am interested about the name and value, because for me they are the most important information which I need. I think we can leave it as is and Pekka idea about full_info could be useful.

return '{}={}'.format(self.name, self.value)
10 changes: 9 additions & 1 deletion test/acceptance/keywords/cookies.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Suite Teardown Delete All Cookies
Test Setup Add Cookies
Resource ../resource.robot


*** Test Cases ***
Get Cookies
[Documentation] Get Cookies
Expand Down Expand Up @@ -48,9 +49,16 @@ Get Cookies When There Are None
${cookies}= Get Cookies
Should Be Equal ${cookies} ${EMPTY}

Get Cookie Expiry Set By Selenium
Copy link
Contributor

Choose a reason for hiding this comment

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

FF indeed seems to have a problem with cookies, please add a tag to the test Known_Issue_Firefox and CI will automatically mark this test as noncritical for Firefox.

Did you happen to try with other browsers, like IE, Edge or Safari, that do they suffer from the same problem?

[Documentation] Get Cookie Value Set By Selenium
[Tags] Known Issue Firefox
${cookie_dict}= Get Cookie another
${expiry} = Convert To Integer 1822148495
Copy link
Contributor

Choose a reason for hiding this comment

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

One can not trust that expiry value is constant. Please set the cookie expiry first and then read it.

should be equal ${cookie_dict.expiry} ${expiry}

*** Keyword ***
Add Cookies
[Documentation] Add Cookies
Delete All Cookies
Add Cookie test seleniumlibrary
Add Cookie another value
Add Cookie another value expiry=2027-09-28 16:21:35