Return value of Register keyword to run on failure cannot always be used to restore original state

[stevejefferies]

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

[ombre42]

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]

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}

[aaltat]

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]

Perhaps easy fix could be change the No Keyword to No Operation?

[aaltat]

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]

We are not going to deprecate the old functionality, instead we are going to change the functionality.

[pekkaklarck]

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 as None and anything else considered false in Python to disable the run-on-failure functionality.

• The keyword returns None if the functionality was previously disabled.

The change is to some extend backwards incompatible, because the earlier 'No keyword' return value is now None. 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!

[pekkaklarck]

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 strings NONE and NOTHING ought to be enough (and easier to document).