-
Notifications
You must be signed in to change notification settings - Fork 763
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
Add 'expiry' to 'add cookie' keyword and add 'get cookie' keyword. #930
Conversation
Firefox specific issue is causing test failure. I will look into this. |
It looks like this is failing because of a known issue with how Firefox gets the expiry from cookies. It is slated to be fixed in Firefox 56. |
@@ -53,11 +53,22 @@ 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_expiry(self, name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't exactly right now remember what the Selenium get_cookie
returns, but if it's a dictionary (like it looks like) it might be feasible to write a keyword which return the whole cookie object. Or write a keyword which will return specific key from the cookie. Example like:
| Get Cookie | name |
Get Cookie Value | name | key |
The later example is enhancing existing keyword and therefore it would be good keep the backwards compatible as much as possible. There by default it should return the cookie value key.
@@ -48,9 +48,14 @@ Get Cookies When There Are None | |||
${cookies}= Get Cookies | |||
Should Be Equal ${cookies} ${EMPTY} | |||
|
|||
Get Cookie Expiry Set By Selenium |
There was a problem hiding this comment.
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?
Adding the expiry argument looks good, but for the new keyword I think we should create more generic solution. See my comments for more detailed response |
On a quick look the PR look very good. Few comments and questions related to it below:
It seems @aaltat already commented about these topics. Next time we need to synchronize. =) |
I discussed this with @aaltat and we agreed that support for setting expiry would be great in SL 3.0. As a result I assigned issue #891 into SL 3.0 rc 1 scope. We plan to get the rc 1 out next week. Do you @wappowers have time to work with this issue before that? I also submitted separate issue #932 about getting all cookie information. I think that's better than adding separate def get_cookie_value(self, name, info='value'):
cookie = self.browser.get_cookie(name)
if cookie is not None:
return cookie[info]
raise ValueError("Cookie with name %s not found." % name) |
@aaltat @pekkaklarck Thanks for the feedback! I will definitely have time to work on this in the next few days, so getting this in shape by the end of the weekend is no problem. The Firefox bug is only affecting retrieving the cookie expiry, it looks like it is setting it correctly. I have tested this on Safari as well, and the time is being set correctly, but Safari stores the time in a milliseconds format, so the acceptance test is failing. I will see if there is something that can be done with this, I imagine there is. I have not checked this on the Microsoft browsers yet. Since I need some way to get the cookie expiry value, if I try to address #932 as well, would it be ok to keep that in this PR? I will also look at allowing the expiry settings to be more flexible using |
Sounds great! I'm fine either keeping implementation of #932 in this PR or having it in another one. In general it's cleaner to have one feature per PR, but this time it's pretty hard to test one without the other. Did I get it right that Safari stores expiry as milliseconds but others use seconds? That's pretty strange. I think we should always return it in the same format, but then there's a question which format to use. I'll add a separate comment about that into #932. |
So, I proposed using Python's standard |
I made a few changes to try and get this ready. The new 'Get Cookie' keyword returns a dictionary of the named cookie, and this could address #932. However, the acceptance test is failing when using Robot 2.8.7 because the cookie is a dictionary, and it look like they dictionary support was added to Builtin with 2.9.0. I can update the acceptance test to use the Collections library, or another way if there is a better approach. The other change was to have the expiry set using a datetime format. However, the Selenium 'get cookie' command is still returning the cookie in epoch time. I opted not to reformat that before returning it to the user, since altering the data returned felt off to me. However, I can do that if it is the preferred choice. I don't love the way I handled this, but it was the best I could come up with. The other failure in the acceptance tests was a random one with javascript in Firefox. I cannot imagine my changes affected that, but I will look into it to be sure. |
I don't have time to really look at this now but one comment: Instead of returning the cookie informaion as a dict, I think you should return a separate Cookie or CookieInformation object which has all the cookie information as its attributes. That would be much easier to handle in test data using the extended variable syntax and implementation wouldn't be too hard: class CookieInformation(object):
def __init__(self, name, value, expiry, other, stuff, ...):
self.name = name
self.value = value
...
def __str__(self):
return '{}={}'.format(self.name, self.value) |
Thanks @pekkaklarck, I made the change you suggested, and it solved some of my other problems, like needing to use the collections library for dictionary compatibility. I could not find another example of a keyword file with more than one 'class' in it. So if implemented this incorrectly I can fix it. |
Apparently no other keyword has yet needed such a model object, and I think they are used a lot less than they should in general. One good example is Robot's Process library that returns a handy result object. And yeah, having the class in the module that needs it is just fine. |
If you have time @aaltat, this is passing the acceptance tests, and I think it is ready to be looked at again. Thanks. |
If no cookie is found with `name`, this keyword fails. | ||
""" | ||
cookie = self.browser.get_cookie(name) | ||
if cookie is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Write: if cookie:
""" | ||
cookie = self.browser.get_cookie(name) | ||
if cookie is not None: | ||
cookie_information = CookieInformation(cookie['name'], cookie['value'], cookie['domain'], cookie['path'], |
There was a problem hiding this comment.
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.
self.browser.add_cookie(new_cookie) | ||
|
||
|
||
class CookieInformation(object): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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__
self.http = http | ||
self.secure = secure | ||
|
||
def __str__(self): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
[Documentation] Get Cookie Value Set By Selenium | ||
[Tags] Known Issue Firefox | ||
${cookie_dict}= Get Cookie another | ||
${expiry} = Convert To Integer 1822148495 |
There was a problem hiding this comment.
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.
@@ -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') |
There was a problem hiding this comment.
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,
The code looks generally OK, but there are some changes needed to cover some cases. |
@aaltat Thank you for the feedback, it was very helpful. I have made the changes you requested, except for those that you are discussing with @pekkaklarck. If those conversations result in additional changes being required I can definitely make them. I have my Firefox tests failing off and on because of the Confirm Action test. It may be related to #934. But since this is a javascript test, I do not see how my changes could be related. |
self.http = http | ||
self.secure = secure | ||
|
||
def __str__(self): |
There was a problem hiding this comment.
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.
self.browser.add_cookie(new_cookie) | ||
|
||
|
||
class CookieInformation(object): |
There was a problem hiding this comment.
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__
Github seems to hide the outdated diff discussion by default, which is not nice. To be sure, this is thing what I wanted to say for the Something like this could work then:
Other changes looks good to go. If you want to be mentioned when we publish next release, please add your name to the the https://github.com/robotframework/SeleniumLibrary/blob/master/CHANGES.rst file. |
This isn't a big deal, but in my opinion model objects like class CookieInformation(object):
def __init__(self, name, value, expiry=None, ...):
self.name = name
self.value = value
self.expiry = # code to handle expiry somehow
# ... This could then be used, for example, like: cookie = CookieInformation(data['name'], data['value'], data.get('expiry'), ...) Having |
I updated this with the changes @aaltat suggested and added a full_info property that returns a string as @pekkaklarck requested. |
This is to address #891. I also added a "get cookie expiry" keyword, which I needed for the acceptance testing, and seems useful in any case. However, if that is an issue let me know.