Prevent on-failure keyword from running more than once

[dpsfrishberg]

I would like to contribute a change to _runonfailure.py and keywordgroup.py to prevent multiple screenshots from being taken on failure. This could happen if one keyword calls another. The way Selenium2Library is currently implemented, this will not happen, but if another library extends Selenium2Library, and it defines a keyword that calls a Selenium2Library keyword, and the inner keyword fails, there will be a duplicate screenshot, because the decorator is applied to both methods. To solve this, I have added a flag that is set when a keyword begins executing.

At any rate, I need to write a test. I am not sure how to test that only one screenshot is taken, without calling the Robot test from a Python test and then checking the directory in which the test was run for multiple screenshot files. I could do this under the unit tests directory, but this is really an acceptance test that runs an inner scenario. What would be the recommended way of doing this? I would be happy to contribute a mechanism for "scenario"-type tests like this.

[ombre42]

We love contributions, especially with tests. Avoiding duplicate on failures is good. It sounds like your solution is only for when Selenium2Library is extended through inheritance. I'm thinking for extending the library by using a separate class, we could provide a ExtensionKeywordGroup people could inherit from that would provide on failure in the outside library and disable S2L's on failure during the execution of a keyword in the outside library. By default, the same keyword would run whether the failure occurred in the outside library or S2L. This is just me thinking - I'm not asking you to do that.

Registering a different keyword makes it easy to test how many times the on failure mechanism was triggered. Something like this:

On Failure Keyword Only Called Once
    [Setup]    Open Browser    http://www.google.com/    ie
    Set Test Variable    ${ON FAIL COUNT}    ${0}
    Register Keyword To Run On Failure    On Fail
    Run Keyword And Ignore Error    Custom Selenium Keyword    id=missing
    Should Be Equal    ${ON FAIL COUNT}    ${1}    On Failure Keyword called ${ON FAIL COUNT} times.
    [Teardown]    Close Browser

*** Keywords ***
On Fail
    ${count}=    Evaluate    ${ON FAIL COUNT} + 1
    Set Test Variable    ${ON FAIL COUNT}    ${count}

[dpsfrishberg]

Thanks! I will try registering a different keyword as you suggest. Do you see any downside to just keeping track, in the decorator in keywordgroup.py, of whether a screenshot has been taken yet? The code I am thinking of is this. In _runonfailure.py, add to __init__:

        self._in_keyword = False

And in keywordgroup.py:

def _run_on_failure_decorator(method, *args, **kwargs):
    self = args[0]
    in_keyword = self._in_keyword # If False, we are in the outermost keyword (or in `run_keyword`, if it's a dynamic library)
    self._in_keyword = True # Set a flag on the instance so that as we call keywords inside this call and this gets run again, we know we're at least one level in.
    try:
        return method(*args, **kwargs)
    except Exception, err:
        if hasattr(self, '_run_on_failure') and not self._has_run_on_failure:
            # If we're in an inner keyword, track the fact that we've already run on failure once
            self._has_run_on_failure = True
            self._run_on_failure()
        raise
    finally:
        if not in_keyword:
            # If we are in the outer call, reset the flags.
            self._in_keyword = False
            self._has_run_on_failure = False

[dpsfrishberg]

Also:

    Run Keyword And Ignore Error    Custom Selenium Keyword    id=missing

I am wondering where I should define "Custom Selenium Keyword". I could put it in a custom library that extends Selenium2Library, somewhere under the test/acceptance directory, and import it in my test, but the test runner needs to set the pythonpath accordingly, and it appears that that is already set to the src directory so that Selenium2Library can be imported.

[ombre42]

Overall, this looks good and will fix the issue. If the very first keyword invocation failed, and not self._has_run_on_failure: would cause AttributeError as it doesn't exist yet. You could do this before the try, avoiding changing the main class:

if not hasattr(self, '_has_run_on_failure'):
    self._has_run_on_failure = False

Normally creating instance attributes outside __init__ is considered a bad practice, but with decorators, I think separation of concerns takes precedence. It's probably debatable - @emanlove do you have an opinion on creating instance attributes from inside a decorator if there is no need to reference it outside the decorator?

The code would be easier to read if the local variable in_keyword (not the attribute) was renamed to already_in_keyword.

Would be cleaner to remove change to main class and instead do: already_in_keyword = getattr(self, '_in_keyword', False) We don't need quite so much comments in the final version.

This is what I think makes sense for adding test libraries: env.py : add TEST_LIBS_DIR = os.path.join(RESOURCES_DIR, "testlibs") run_tests: change to 'pythonpath': ':'.join((env.SRC_DIR, env.TEST_LIBS_DIR)) Put libraries in /test/resources/testlibs/ - we may want to use these libraries in unit tests at some point so here rather than under /test/acceptance/

The path need not be modified, you could do: Library ${CURDIR}${/}InheritedFromSelenium2Library.py but there will be more than one library so it makes sense to add the directory to pythonpath

[dpsfrishberg]

Great, thanks! Good catch re. hasattr; I will make that change and add a test.

[ombre42]

@frishberg quite by accident i discovered that the issue you brought up here can be solved by just setting the on-failure keyword to None before invoking the keyword. See: https://github.com/rtomac/robotframework-selenium2library/commit/3e233ac2fa5e96538f540723dfb9fa9ee0db76b4#diff-884eab145461eeab24866767b1b202a4R18

[dpsfrishberg]

Yes, that looks like a much simpler solution. Pardon my Git illiteracy, but is this commit part of a branch? I cannot check out the commit in detached head state when I clone the repo, and I cannot tell what branch it might belong to through the Github interface.

[ombre42]

The commit belongs to a branch in a fork that is not in master. I do not think you have to pull it down - its only a couple of lines and it belongs to a decorator specifically for user libraries. I'm a git novice myself.

[dpsfrishberg]

I do not think you have to pull it down - its only a couple of lines and it belongs to a decorator specifically for user libraries.

Sure, that makes sense. Just wasn't sure if this was something you were already planning on committing to the next version, in which case I wouldn't need to do my own contribution, I assume.

[dpsfrishberg]

Hi. So as I'm implementing this change, I am trying to run the tests against it. I would like to know how I can run only the tests in a specific file. I can limit it to a directory like this:

pybot --doc SeleniumSPacceptanceSPtestsSPwithSPfirefox --outputdir /path/to/se2lib/test/results --variable browser:firefox --escape space:SP --report none --log none --loglevel DEBUG --pythonpath /path/to/se2lib/test/../src:/path/to/se2lib/test/resources/testlibs /path/to/se2lib/test/acceptance/keywords, but it would be nice if I could limit it to a file within that "keywords" directory.

[ombre42]

You can pass options to RF after the browser name. For what you are trying to do, select the suite to run. For example: C:\>python run_tests.py python ff --suite textfields will run tests in acceptance/keywords/textfields.txt.

[dpsfrishberg]

Great, thanks!

I discovered that, using the approach you propose, the "Register Keyword To Run On Failure" keyword breaks. That keyword refers to self._run_on_failure_keyword. When I turn off running on failure by setting that attribute to None, the method ceases to return the old value.

Any objection to just using self._has_run_on_failure and already_in_keyword?

[ombre42]

Is there a valid use case for a Selenium2Library (or extension) keyword calling Register Keyword To Run On Failure?

[dpsfrishberg]

Is there a valid use case for a Selenium2Library (or extension) keyword calling Register Keyword To Run On Failure? I am not sure, but this happens even if Register Keyword To Run On Failure is called directly from a test, because when the try is reached in the decorator that calls the original method to run (in this case, the outer method, register_keyword_to_run_on_failure), self._run_on_failure_keyword has already been set to None.

[ombre42]

Oh. The bit about setting it to None was written from an external point of view. Go ahead and do your original fix. Register Keyword To Run On Failure should not be decorated, but its not really hurting anything except the scenario you just ran into.

[dpsfrishberg]

@ombre42, I have written the code and tests, and they pass, but I am seeing unrelated failures in master, both in my local (CentOS) environment, and on our Linux CI agents (all tests passed on the same agent in late August). Wondering if this makes any sense to you. The failures:

[15:39:10][Step 2/3] Should Timeout If Script Does Not Invoke Callback | FAIL |
[15:39:10][Step 2/3] Expected error 'TimeoutException: Message: u'Timed out waiting for async script result after * ' but got 'TimeoutException: Message: Timed out waiting for async script result after 10000ms
[15:39:10][Step 2/3] Stacktrace:
[15:39:10][Step 2/3] at handleEvaluateEvent/timeoutId< (http://localhost:7000/html/javascript/dynamic_content.html:76:1)'.
[15:39:10][Step 2/3] ------------------------------------------------------------------------------
[15:39:20][Step 2/3] Should Timeout If Script Does Not Invoke Callback With A Zero Timeout | FAIL |
[15:39:20][Step 2/3] Expected error 'TimeoutException: Message: u'Timed out waiting for async script result after * ' but got 'TimeoutException: Message: Timed out waiting for async script result after 10001ms
[15:39:20][Step 2/3] Stacktrace:
[15:39:20][Step 2/3] at handleEvaluateEvent/timeoutId< (http://localhost:7000/html/javascript/dynamic_content.html:76:1)'.
[15:39:20][Step 2/3] ------------------------------------------------------------------------------
[15:39:20][Step 2/3] Should Not Timeout If Script Callsback Inside A Zero Timeout | PASS |
[15:39:20][Step 2/3] ------------------------------------------------------------------------------
[15:39:21][Step 2/3] Should Timeout If Script Does Not Invoke Callback With Long Timeout | FAIL |
[15:39:21][Step 2/3] Expected error 'TimeoutException: Message: u'Timed out waiting for async script result after * ' but got 'TimeoutException: Message: Timed out waiting for async script result after 501ms
[15:39:21][Step 2/3] Stacktrace:
[15:39:21][Step 2/3] at handleEvaluateEvent/timeoutId< (http://localhost:7000/html/javascript/dynamic_content.html:76:1)'.
[15:39:21][Step 2/3] ------------------------------------------------------------------------------
[15:39:21][Step 2/3] Should Detect Page Loads While Waiting On An Async Script And Retu... | FAIL |
[15:39:21][Step 2/3] Expected error 'WebDriverException: Message: u'Detected a page unload event; async script execution does not work across page loads' * ' but got 'WebDriverException: Message: Detected a page unload event; async script execution does not work across page loads
[15:39:21][Step 2/3] Stacktrace:
[15:39:21][Step 2/3] at onunload (http://localhost:7000/html/javascript/dynamic_content.html:55:7)'.
[15:39:21][Step 2/3] ------------------------------------------------------------------------------
[15:39:21][Step 2/3] Should Catch Errors When Executing Initial Script | FAIL |
[15:39:21][Step 2/3] Expected error 'WebDriverException: Message: u'you should catch this!' * ' but got 'WebDriverException: Message: you should catch this!
[15:39:21][Step 2/3] Stacktrace:
[15:39:21][Step 2/3] at anonymous (http://localhost:7000/html/javascript/dynamic_content.html line 68 > Function:1:1)
[15:39:21][Step 2/3] at handleEvaluateEvent (http://localhost:7000/html/javascript/dynamic_content.html:68:11)'.
[15:39:21][Step 2/3] ------------------------------------------------------------------------------
[15:39:21][Step 2/3] Acceptance.Keywords.Async Javascript | FAIL |
[15:39:21][Step 2/3] 12 critical tests, 7 passed, 5 failed
[15:39:21][Step 2/3] 12 tests total, 7 passed, 5 failed 

[15:39:46][Step 2/3] [ WARN ] Option(s) 'Tin Can Phone' not found within list 'preferred_channel'.
[15:39:46][Step 2/3] [ WARN ] Option(s) 'Tin Can Phone' not found within list 'preferred_channel'.
[15:39:46][Step 2/3] [ WARN ] Option(s) 'Smoke Signals' not found within list 'preferred_channel'.
[15:39:46][Step 2/3] Select Non-Existing Item From Single Selection List | FAIL |
[15:39:46][Step 2/3] Expected error 'NoSuchElementException: Message: u'Could not locate element with visible text: Tin Can Phone' ' but got 'NoSuchElementException: Message: Could not locate element with visible text: Tin Can Phone
[15:39:46][Step 2/3] '.
[15:39:46][Step 2/3] ------------------------------------------------------------------------------
[15:39:47][Step 2/3] Select Non-Existing Item From Multi-Selection List | PASS |
[15:39:47][Step 2/3] ------------------------------------------------------------------------------
[15:39:47][Step 2/3] Unselect Non-Existing Item From List :: LOG 3 Unselecting option(s... | PASS |
[15:39:47][Step 2/3] ------------------------------------------------------------------------------
[15:39:48][Step 2/3] Select From Multiselect List :: LOG 5 Selecting option(s) 'Direct ... | PASS |
[15:39:48][Step 2/3] ------------------------------------------------------------------------------
[15:39:49][Step 2/3] Select All From List :: LOG 2 Selecting all options from list 'int... | PASS |
[15:39:49][Step 2/3] ------------------------------------------------------------------------------
[15:39:50][Step 2/3] List Should Have No Selections :: LOG 2 Verifying list 'interests'... | PASS |
[15:39:50][Step 2/3] ------------------------------------------------------------------------------
[15:39:50][Step 2/3] Acceptance.Keywords.Lists | FAIL |
[15:39:50][Step 2/3] 18 critical tests, 17 passed, 1 failed
[15:39:50][Step 2/3] 18 tests total, 17 passed, 1 failed

[HelioGuilherme66]

Those tests need to be adjusted.

Please compare with my forked file prepared to PASS with different browsers.

Attached test/acceptance/keywords/async_javascript.robot

Full path is here: https://github.com/HelioGuilherme66/robotframework-selenium2library/blob/master/test/acceptance/keywords/async_javascript.robot

On Thu, Nov 6, 2014 at 5:23 PM, frishberg notifications@github.com wrote:

@ombre42 https://github.com/ombre42, I have written the code and tests, and they pass, but I am seeing unrelated failures in master, both in my local (CentOS) environment, and on our Linux CI agents (all tests passed on the same agent in late August). Wondering if this makes any sense to you. The failures:

[15:39:10][Step 2/3] Should Timeout If Script Does Not Invoke Callback | FAIL | [15:39:10][Step 2/3] Expected error 'TimeoutException: Message: u'Timed out waiting for async script result after * ' but got 'TimeoutException: Message: Timed out waiting for async script result after 10000ms [15:39:10][Step 2/3] Stacktrace: [15:39:10][Step 2/3] at handleEvaluateEvent/timeoutId< (http://localhost:7000/html/javascript/dynamic_content.html:76:1)'. [15:39:10][Step 2/3] ------------------------------------------------------------------------------ [15:39:20][Step 2/3] Should Timeout If Script Does Not Invoke Callback With A Zero Timeout | FAIL | [15:39:20][Step 2/3] Expected error 'TimeoutException: Message: u'Timed out waiting for async script result after * ' but got 'TimeoutException: Message: Timed out waiting for async script result after 10001ms [15:39:20][Step 2/3] Stacktrace: [15:39:20][Step 2/3] at handleEvaluateEvent/timeoutId< (http://localhost:7000/html/javascript/dynamic_content.html:76:1)'. [15:39:20][Step 2/3] ------------------------------------------------------------------------------ [15:39:20][Step 2/3] Should Not Timeout If Script Callsback Inside A Zero Timeout | PASS | [15:39:20][Step 2/3] ------------------------------------------------------------------------------ [15:39:21][Step 2/3] Should Timeout If Script Does Not Invoke Callback With Long Timeout | FAIL | [15:39:21][Step 2/3] Expected error 'TimeoutException: Message: u'Timed out waiting for async script result after * ' but got 'TimeoutException: Message: Timed out waiting for async script result after 501ms [15:39:21][Step 2/3] Stacktrace: [15:39:21][Step 2/3] at handleEvaluateEvent/timeoutId< (http://localhost:7000/html/javascript/dynamic_content.html:76:1)'. [15:39:21][Step 2/3] ------------------------------------------------------------------------------ [15:39:21][Step 2/3] Should Detect Page Loads While Waiting On An Async Script And Retu... | FAIL | [15:39:21][Step 2/3] Expected error 'WebDriverException: Message: u'Detected a page unload event; async script execution does not work across page loads' * ' but got 'WebDriverException: Message: Detected a page unload event; async script execution does not work across page loads [15:39:21][Step 2/3] Stacktrace: [15:39:21][Step 2/3] at onunload (http://localhost:7000/html/javascript/dynamic_content.html:55:7)'. [15:39:21][Step 2/3] ------------------------------------------------------------------------------ [15:39:21][Step 2/3] Should Catch Errors When Executing Initial Script | FAIL | [15:39:21][Step 2/3] Expected error 'WebDriverException: Message: u'you should catch this!' * ' but got 'WebDriverException: Message: you should catch this! [15:39:21][Step 2/3] Stacktrace: [15:39:21][Step 2/3] at anonymous (http://localhost:7000/html/javascript/dynamic_content.html line 68 > Function:1:1) [15:39:21][Step 2/3] at handleEvaluateEvent (http://localhost:7000/html/javascript/dynamic_content.html:68:11)'. [15:39:21][Step 2/3] ------------------------------------------------------------------------------ [15:39:21][Step 2/3] Acceptance.Keywords.Async Javascript | FAIL | [15:39:21][Step 2/3] 12 critical tests, 7 passed, 5 failed [15:39:21][Step 2/3] 12 tests total, 7 passed, 5 failed

[15:39:46][Step 2/3] [ WARN ] Option(s) 'Tin Can Phone' not found within list 'preferred_channel'. [15:39:46][Step 2/3] [ WARN ] Option(s) 'Tin Can Phone' not found within list 'preferred_channel'. [15:39:46][Step 2/3] [ WARN ] Option(s) 'Smoke Signals' not found within list 'preferred_channel'. [15:39:46][Step 2/3] Select Non-Existing Item From Single Selection List | FAIL | [15:39:46][Step 2/3] Expected error 'NoSuchElementException: Message: u'Could not locate element with visible text: Tin Can Phone' ' but got 'NoSuchElementException: Message: Could not locate element with visible text: Tin Can Phone [15:39:46][Step 2/3] '. [15:39:46][Step 2/3] ------------------------------------------------------------------------------ [15:39:47][Step 2/3] Select Non-Existing Item From Multi-Selection List | PASS | [15:39:47][Step 2/3] ------------------------------------------------------------------------------ [15:39:47][Step 2/3] Unselect Non-Existing Item From List :: LOG 3 Unselecting option(s... | PASS | [15:39:47][Step 2/3] ------------------------------------------------------------------------------ [15:39:48][Step 2/3] Select From Multiselect List :: LOG 5 Selecting option(s) 'Direct ... | PASS | [15:39:48][Step 2/3] ------------------------------------------------------------------------------ [15:39:49][Step 2/3] Select All From List :: LOG 2 Selecting all options from list 'int... | PASS | [15:39:49][Step 2/3] ------------------------------------------------------------------------------ [15:39:50][Step 2/3] List Should Have No Selections :: LOG 2 Verifying list 'interests'... | PASS | [15:39:50][Step 2/3] ------------------------------------------------------------------------------ [15:39:50][Step 2/3] Acceptance.Keywords.Lists | FAIL | [15:39:50][Step 2/3] 18 critical tests, 17 passed, 1 failed [15:39:50][Step 2/3] 18 tests total, 17 passed, 1 failed

— Reply to this email directly or view it on GitHub https://github.com/rtomac/robotframework-selenium2library/issues/341#issuecomment-62016450 .

This message has been scanned for viruses and dangerous content by MailScanner http://www.mailscanner.info/, and is believed to be clean.

[dpsfrishberg]

@ombre42, I am not seeing the file attachment.

[ombre42]

@emanlove has already addressed these failures in ef3f67dd978530b786ac75896b75207b775e33a5. Your fork/branch is behind master.

The failures are caused by a change in the very latest version of Selenium. The way exceptions are printed(str method of WebDriverException) changed: 2.44.0 exception_msg = "Message: %s\n" % self.msg 2.43.1 exception_msg = "Message: %s " % repr(self.msg) Putting a newline at the end is odd, but this change makes sense otherwise.

@HelioGuilherme66 - if you want to get them to pass with different webdriver implementations, I think the approach should be to match against TimeoutException: Message: \w.* rather than a specific message for every implementation. We are not trying to test Selenium here nor are we testing RF's ability to print exceptions. Perhaps testing with a strict pattern on FF and a looser one on other browsers. But that seems too complicated.

[emanlove]

@frishberg You should be able to update your fork/branch with something like

git remote add rtomac https://github.com/rtomac/robotframework-selenium2library.git
git pull --rebase rtomac

The "--rebase" will prevent alot of "Merge branch 'master' into ". Don't recommend this all the time but in this case where you have a fork/branch that is cloned from another and both yours and the other have been updated then you can merge in the remote changes "inline" with your own using rebase. Just make the commit log a little cleaner. You might need to do something like

git remote add --track master rtomac https://github.com/rtomac/robotframework-selenium2library.git

always forget how I add remote branches...

[dpsfrishberg]

OK. I followed your instructions and created #349.

[dpsfrishberg]

Tests are passing in #349. Does it look good to merge?

[dpsfrishberg]

Ping.

[aaltat]

It seems that this was just left hanging and we are sorry about that. Do you still fell that this is important for you and should be merged to the master? If you do, could rebase your commit with master and push it again?

If this is not important anymore or we do not hear from you for a month or so, this issue and the pull request will be closed.

[aaltat]

The current release 3,0,0b1 has had reasonable rework, there this might be fixed.

But in any case, closing due inactivity.