Repository navigation
Return value of Register keyword to run on failure cannot always be used to restore original state #176
Description
There seems to be an issue with the implementation of the "Register keyword to run on failure" keyword. The keyword returns the value of the previously registered keyword which works fine if this is set to a keyword e.g. "Capture Page Screenshot" but if the registered keyword is set to "None" the return value is the string "No keyword". If you then try to use the return value "No keyword" and pass this back into "Register keyword to run on failure" a warning is generated:
"Keyword 'No keyword' could not be run on failure: No keyword with name 'No keyword' found."
I guess this keyword should either be returning "Nothing" as a string rather than "No keyword" to be consistent with the current way of disabling the run-on-failure functionality or it could return None instead
Example test case (simplistic example):
*** Settings ***
Library Selenium2Library
*** Test Cases ***
Test Case 1
${previous_kw}= Register Keyword To Run On Failure Nothing
${previous_kw}= Register Keyword To Run On Failure Capture Page Screenshot
Register Keyword To Run On Failure ${previous_kw}
Open Browser http://www.google.com
Page Should Contain invalid_string
The implementation could be changed to the following which would return None from line 2 of the above test case and no warnings are observed on failure:
def register_keyword_to_run_on_failure(self, keyword):
"""Documentation here....
"""
old_keyword = self._run_on_failure_keyword
old_keyword_text = old_keyword if old_keyword is not None else "No keyword"
if keyword:
new_keyword = keyword if keyword.strip().lower() != "nothing" else None
else:
new_keyword = None
new_keyword_text = new_keyword if new_keyword is not None else "No keyword"
self._run_on_failure_keyword = new_keyword
self._info('%s will be run on failure.' % new_keyword_text)
return old_keyword
Activity
ombre42 commentedon Oct 5, 2014
I think that changing the return value is an unnecessary and a riskier change. When this was first implemented, it should have returned "nothing", but it seems too late to change that. If we consider "No keyword" and "nothing" to be synonymous, the warning will go away.
boakley commentedon Dec 7, 2015
I just stumbled on this today. This really ought to be fixed. I shouldn't have to add special case code in my test when saving and restoring the keyword. If changing the return value is unacceptable, perhaps changing it so that passing in "No Keyword" has the same effect as passing in "Nothing".
I shouldn't have to have special case code in my tests and keywords in case there is no keyword registered. In other words, the following should always return the system to the correct state:
aaltat commentedon Dec 8, 2015
Ugh
I agree, would you like to make a pull requests to fix it?
-Tatu
-----Original Message-----
From: "Bryan Oakley" notifications@github.com
Sent: 07/12/2015 18:44
To: "robotframework/Selenium2Library" Selenium2Library@noreply.github.com
Subject: Re: [Selenium2Library] "Register keyword to run on failure" shouldnot return the string "No keyword" (#176)
I just stumbled on this today. This really ought to be fixed. I shouldn't have to add special case code in my test when saving and restoring the keyword. If changing the return value is unacceptable, perhaps changing it so that passing in "No Keyword" has the same effect as passing in "Nothing".
I shouldn't have to have special case code in my tests and keywords in case there is no keyword registered. In other words, the following should always return the system to the correct state:
| | ${old}= | Register keyword to run on failure | Nothing
| | Register keyword to run on failure | ${old}
—
Reply to this email directly or view it on GitHub.
aaltat commentedon Dec 29, 2015
Perhaps easy fix could be change the
No KeywordtoNo Operation?aaltat commentedon Aug 30, 2017
This should have deprecated this in 3.0 Alpha release. We will make at least one release candidate, perhaps deprecation could be done in there. Adding this to 3.0.0 milestone.
aaltat commentedon Sep 2, 2017
We are not going to deprecate the old functionality, instead we are going to change the functionality.
Fixes and cleanup to running keyword on failure.
pekkaklarck commentedon Sep 2, 2017
My changes in #884 change the behavior like this:
In addition to the previously supported string
'Nothing'It's possible to use string'NONE'(case-insensitively) as well asNoneand anything else considered false in Python to disable the run-on-failure functionality.The keyword returns
Noneif the functionality was previously disabled.The change is to some extend backwards incompatible, because the earlier
'No keyword'return value is nowNone. I doubt this causes too big problems in practice and consider it safe enough in a major release. Please comment here if you feel otherwise![-]"Register keyword to run on failure" should not return the string "No keyword"[/-][+]Return value of `Register keyword to run on failure` cannot always be used to restore original state[/+]pekkaklarck commentedon Nov 5, 2017
On a second though I don't think it is useful to support any value considered false on Python when disabling this functionality. Supporting just
None, including stringsNONEandNOTHINGought to be enough (and easier to document).Cleanuo run on failure docs (robotframework#925).
Cleanuo run on failure docs (#925).