Added execute_javascript_with_arguments keyword

[arnewohletz]

Added PR as discussed in
#323

[aaltat]

Each new functionality must be backed up with tests. I think this one needs few. But before that, I should read more about the JavaScript.

[aaltat]

I did read the Selenium API and I understand how that works. I agree that having separate keyword is easier in this phase but we should think what shall we do in the long run (not required in this PR).

I also noticed that async JavaScript method in Selenium API supports arguments: http://selenium-python.readthedocs.io/api.html#selenium.webdriver.remote.webdriver.WebDriver.execute_async_script For consistency reason we should create a similar keyword for http://robotframework.org/SeleniumLibrary/SeleniumLibrary.html#Execute%20Async%20Javascript
Also the PR must be backed up with tests. Would you @horsewithnoname1985 be willing to update the PR.

[emanlove]

Not sure if you have worked out a design but I was thinking that we could use "docstring" markup to delineate the JavaScript allowing for multi-line JavaScript and then have the arguments following the "docstring". This should allow for any JavaScript code and multiple lines. It might also be helpful to provide a warning if an argument is not called out in the JavaScript. I do like, at least in the interim, have keywords named Execute JavaScript With Arguments and Execute Async JavaScript With Arguments as a way to introduce these implementations.

[aaltat]

You mean something like Pekka did propose in the #323 that keyword signature is *code_and_args

** Keywords *** Highlight Element [Arguments] ${locator} ${element}= Get WebElement ${locator} Execute Javascript ... element = arguments[0]; ... original_style = element.getAttribute('style'); ... element.setAttribute('style', original_style + "; background: yellow;"); ... setTimeout(function(){ ... element.setAttribute('style', original_style); ... }, 300); ... ARGUMENTS ... ${element} 

That ARGUMENTS is a separator between the code and arguments?

[emanlove]

I need to review Pekka proposal but I was thinking something like

Execute JavaScript With Arguments """this = my_javascript_code;""" ${arg0} ${arg1} 

@horsewithnoname1985, what do you think?

[aaltat]

Here is the comment that I was referring: #323 (comment)

But it would be tempting to keep support for variable number of lines for the code and arguments.

[emanlove]

Taking that example what I was thinking the markup, if you will, be would be

*** Keywords *** Highlight Element [Arguments] ${locator} ${element}= Get WebElement ${locator} Execute Javascript """ ... element = arguments[0]; ... original_style = element.getAttribute('style'); ... element.setAttribute('style', original_style + "; background: yellow;"); ... setTimeout(function(){ ... element.setAttribute('style', original_style); ... }, 300);""" ... ${element} ${anotherarg} ${1} 

where the docstring style is used to delineate a single argument. This of course was wishful thinking on my part. Here is my unit test to see if what I was thinking may be part of the robot framework language or if this idea was possible

***Test Cases*** DocString As Argument ${docstring}= Set Variable """This is a single line""" Length Should Be ${docstring} 27 Should Not Be Empty ${docstring} Should Contain ${docstring} """ DocString With Double Space ${docstring}= Set Variable """First Name Last Name""" Length Should Be ${docstring} 27 Multi-Line DocString Using RF Syntax ${docstring}= Set Variable """This is the first line. ... This is the second line. ... This is the third line. ... This is the fourth and final line.""" Length Should Not Be ${docstring} 4 Multi-Line DocString Without RF Syntax ${docstring}= Set Variable """This is the first line. This is the second line. This is the third line. This is the fourth and final line.""" Length Should Not Be ${docstring} 4 

noting of course that Length Should Not Be is not a keyword itself.

This solution that I propose starts to take into consideration the concept of an object type - docstring - just as there are number variables or boolean variables.

[aaltat]

Now I see your idea. The basic idea is to have keyword which has one *vararg and keyword argument can present the JavaScript code and JavaScript arguments. Code and arguments are separated with a marker. In your and Pekka idea, the markers are different but the basic concept is pretty much same.

The more I think about the idea, the more I like it. Would have to agree only with the marker. It should be clear and something which is not already reserved by Robot Framework or by JavaScript.

[emanlove]

Yes, the same with a different marker. In my mind I do see a design where the triple docstring quotes would delineate into a single argument. So one could have whatever text in a docstring, like

Evalute DocString With Double Space ${docstring}= Evaluate return """First Name Last Name""" Log To Console ${docstring} 

This, support of docstrings in robot framework, of course may not be widely useful and this may be a "extractive idea".

[aaltat]

Hmmm. I would prefer a single marker, something similar that is used for Run Keyword If has for ELSE and ELSE IF. The biggest problem I see for the docstring is that I doesn't have support in the editor/IDE space. Although docstring is supported in the Python side, in Robot Framework data doesn't have support for it. It would be colored in same way as any other arguments and it doesn't stand out. Therefore sporting errors comes more difficult and it doesn't stand out as an separator. Any more good ideas for a separator, anyone?

[arnewohletz]

Sorry for the little mess I made with the recent commits here.
I added another keyword execute_async_javascript_with_arguments and also added a test (see here) for the execute_javascript_with_arguments. I borrowed the design from the scroll_into_view test, which, I hope, is OK. I am not sure on how to test the new asynchronous java scripts keyword, so I haven't added any test for this so far.

I like the idea of supporting multiline JavaScript code. Enabling this by putting the code inside a docstring seems very straightforward, so I support that. I’d still prefer a keyword with two arguments though, where the first argument is for the (multi-line) JavaScript code and the other's a varargs for all passed in arguments, instead of assigning the first of multiple arguments as the code:
Execute JavaScript with Arguments ${code} ${arg1} ${arg2] ${argX}

As for the delimiter: Maybe a multi-line JavaScript code can be defined in the *** Variables*** table using the SEPARATOR= \n attribute:

*** Variables *** ${my_multiline_js_code}= SEPARATOR=\n ... my_first.line_of_code; ... my_second.line_of_code; ... my_x.line.of_code; 

But I haven't tested that

[aaltat]

The down side of two arguments is that then we lose capability to define JavaScript inline. Example this is not anymore possible.

| Execute JavaScript | Some code | | ... | More code here | | ... | ARGUMENTS | | ... | ${variable1} | | ... | ${variable2} | 

Instead the code should be created as a list with a separate keyword and that is a down side. Example something like this:

| ${code} | Create List |Some code | | ... | More code here | | Execute JavaScript | ${code} | | ... | ${variable1} | | ... | ${variable2} | 

In long term, this causes a big backwards compatible issue and means that it's almost impossible to to join the two keywords together. Therefore I am now against the two argument implementation.

As a second issue, we should first decide one separator and not to support multiple different separators. Like the zen of python says: There should be one -- and preferably only one -- obvious way to do it. https://www.python.org/dev/peps/pep-0020/#id3

I am somewhat against the docstring, because the reasons I did explain above. Instead I would like to see something which is similar than what Run Keyword If uses for separating the evaluation statements. Something like ARGUMENTS feels natural for me, but I don't feel strongly what the actual text is.

[aaltat]

The problem with the commits is easy to fix. Are you able to fix it by yourself or do you need help with it? If you need help with with it, would you be able to do git remote -v from command line and post the output.

[arnewohletz]

Seems the error has resolved already.
I see your point about using two arguments in the keyword.

How about using the backslash, like in the FOR LOOP syntax, here:

Execute JavaScript ... :JAVASCRIPT ... \ element = arguments[0]; ... \ original_style = element.getAttribute('style'); ... \ element.setAttribute('style', original_style + "; background: yellow;"); ... \ setTimeout(function(){ ... \ element.setAttribute('style', original_style); ... \ }, 300);

If used with arguments, those could be integrated either via a 'ARGUMENTS' attribute like this

Highlight Element [Arguments] ${locator} ${unused_js_arg} ${element}= Get WebElement ${locator} Execute Javascript with arguments ... :JAVASCRIPT ... \ element = arguments[0]; ... \ original_style = element.getAttribute('style'); ... \ element.setAttribute('style', original_style + "; background: yellow;"); ... \ setTimeout(function(){ ... \ element.setAttribute('style', original_style); ... \ }, 300); ... ARGUMENTS ${element} ${unused_arg}

or preferably like this

Highlight Element [Arguments] ${locator} ${unused_js_arg} ${element}= Get WebElement ${locator} Execute Javascript with arguments ... :JAVASCRIPT ... \ element = ${element}; ... \ original_style = element.getAttribute('style'); ... \ element.setAttribute('style', original_style + "; background: yellow;"); ... \ setTimeout(function(){ ... \ element.setAttribute('style', original_style); ... \ }, 300); ... ARGUMENTS ${element} ${unused_js_arg}

or even automatically imported into the JavaScript

Highlight Element [Arguments] ${locator} ${unused_js_arg} ${element}= Get WebElement ${locator} Execute Javascript with arguments ... :JAVASCRIPT ... \ element = ${element}; ... \ original_style = element.getAttribute('style'); ... \ element.setAttribute('style', original_style + "; background: yellow;"); ... \ setTimeout(function(){ ... \ element.setAttribute('style', original_style); ... \ }, 300);

[aaltat]

I am on mobile and need to take a look with device that has a bigger screen.

[aaltat]

The first marker, JAVASCRIPT feels redundant when the JavaScript does not contain any arguments. Example like this:

Execute JavaScript ... JAVASCRIPT ... element = arguments[0]; ... original_style = element.getAttribute('style'); ... element.setAttribute('style', original_style + "; background: yellow;"); ... setTimeout(function(){ ... element.setAttribute('style', original_style); ... }, 300); 

But it would allow to change the order of the JavaScript code and arguments, example like this:

Execute Javascript with arguments ... ARGUMENTS ... ${element} ... ${unused_arg} ... JAVASCRIPT ... element = arguments[0]; ... original_style = element.getAttribute('style'); ... element.setAttribute('style', original_style + "; background: yellow;"); ... setTimeout(function(){ ... element.setAttribute('style', original_style); ... }, 300); 

Similar feature request was asked/proposed in the issue.

But I do not see benefit of supporting the FOR like syntax and using \ -character at start of the line, It just extra and not related characters which requires extra parsing and writing from users.

I am thinking, would this be good proposal.

1. Support current style:

Execute JavaScript ... element = arguments[0]; ... original_style = element.getAttribute('style'); ... element.setAttribute('style', original_style + "; background: yellow;"); ... setTimeout(function(){ ... element.setAttribute('style', original_style); ... }, 300); 

2. Support arguments in the end:

Execute Javascript ... element = arguments[0]; ... original_style = element.getAttribute('style'); ... element.setAttribute('style', original_style + "; background: yellow;"); ... setTimeout(function(){ ... element.setAttribute('style', original_style); ... }, 300); ... ARGUMENTS ... ${element} ${unused_arg} 

3. Support changing the order of arguments and code:

Execute Javascript ... ARGUMENTS ... ${element} ${unused_arg} ... JAVASCRIPT ... element = arguments[0]; ... original_style = element.getAttribute('style'); ... element.setAttribute('style', original_style + "; background: yellow;"); ... setTimeout(function(){ ... element.setAttribute('style', original_style); ... }, 300); 

4. Support defining the JAVASCRIPT at start (optional)

Execute Javascript ... JAVASCRIPT ... element = arguments[0]; ... original_style = element.getAttribute('style'); ... element.setAttribute('style', original_style + "; background: yellow;"); ... setTimeout(function(){ ... element.setAttribute('style', original_style); ... }, 300); ... ARGUMENTS ... ${element} ${unused_arg} 

The point 4 is optional and largely depends which way is the implementation better, with the support or without it.

What others think, @emanlove and @horsewithnoname1985?

[aaltat]

Did forget to say that in this way, we do not need a new keyword and the new feature can be implemented with the existing JavaScript keywords. The change is backwards incompatible but it is unlikely that someone would have ARGUMENTS and JAVASCRIPT as single arguments in their code.

[emanlove]

I agree with the idea that docstrings is a bad idea and I like yours and Pekka's idea for ARGUMENTS as a delimiter (No. 2).

[arnewohletz]

I also approve the ARGUMENTS marker and I also would very much appreciate the JAVASCRIPT marker to switch order, as it is more natural that arguments are defined before using them. @aaltat , I agree, that these feature are probably better implemented into the existing keywords instead of creating new ones.

[aaltat]

It looks like we have an agreement. Lets go with the ARGUMENTS and JAVASCRIPT markers. With JAVASCRIPT being optional and possibility to swap ARGUMENTS and JAVASCRIPT markers order. Would @horsewithnoname1985 be willing to implement this?

[aaltat]

The support for ApprovalTests is added. The ApprovalTests now works with pypy and for Jython the tests must be skipped. There is an example in the test_javascript.py file. Although the reporter might require some tuning, to make it more configurable for different OS and needs, the basic idea works.

Would you be willing to continue the work for the PR?

[BeniKeyserman]

@horsewithnoname1985 Is there any progress on this PR?
Do you need any help?

[arnewohletz]

@aaltat , sorry for my very late reply. I also had a holiday break. Yes, I will continue on this PR within these days. So I assume the unit tests using approvaltests for the new keyword are fine? Then I will work on the keyword implementation logic as you suggested on July 27th.
Thanks, @BeniKeyserman , I'll come back to you, if I need any help with this.

[aaltat]

No worries, holiday should be spend in the sun rather than coding.

I did also play with the ApprovalTest, just because I wanted to learn more about them and #1176 shows what I did have in mind. Please note that the _separate_code_and_args method is not ready for production. It needs some refactoring so that code easier to read and understand. Also the error messages should be more meaningful. But there are few differences in our approaches of testing.

I think you misunderstood how verify_all is used. Instead of passing a single result, one should pass list of results. When passing list of result, the results are easier to read with the reporter. The main idea of ApprovalTests is about visualizing the results in string format.

Your unit test do not have error testing. I did write some examples, how error handling can be tested by using ApprovalTests.

Also there are missing test, from me and you, like reading the JavaScript from file.

But because master has moved forward, there are few few conflicts which needs to be sorted out. Also your PR contains few commits which are already in master and should not be part of this PR. Are you able to sort out the problems or do you need help?

[arnewohletz]

Hi @aaltat , I see your point and will have a look on these tests next. I saw your _separate_code_and_argsmethod. We should bring this method, and the changes I just committed together.
One unit test currently fails, because I made changes in the _get_javascript_to_execute method. The unit test sends a single string, whereas RF tests would send a one string item tuple into the method for a one line javascript code. Since the users doesn't call that method directly, could the unit test be changed from:

def test_get_javascript(self): code, args = self.javascript._get_javascript_to_execute('code here') ...

to

 def test_get_javascript(self): code, args = self.javascript._get_javascript_to_execute(tuple(['code here'])) ...

?
Did something occur to the Travis CI? I did not see any test being executed after the commit.
Thanks for your support on the approvaltests. You're right, I misunderstood something. I will add your proposals to the PR next and see how it goes.

[aaltat]

Changing the unit test is fine, although I would change it like this: self.javascript._get_javascript_to_execute(('code here, ))`

The Travis problem is related to your PR: how you solving the conflicts. But that is actually a small problem compared to the problem that the commit history in this PR is mixed, meaning that commits which are already merged in master are somewhere in the between of your commits. In practice, this commit tries to rewrite the history of SeleniumLibrary and that is not acceptable, because it will lead to greater problems. Instead commit history should be: all commits which are in the master are in the bottom of the git log command. Then your commits are top of those commits. In this way git diff will actually show the files and lines which you have changed and doesn't show the files which someone else have changed. This will also ease the review process.

There is at least two ways to fix the problem: cherry-pick and rebase. By quickly looking, I would assume that cherry-pick might be the easiest way, but I am not sure. Are you able to solve the problem or do you want me to do it for you?

[aaltat]

Closing in favor of #1176

[arnewohletz]

Hi @Tattoo , sorry for not dealing with this issue in time. I see you moved this issue into a less confusing PR

[Tattoo]

@horsewithnoname1985 I'm not @aaltat :)

[aaltat]

No worries. I did try to recover the commit history from this PR, but it was actually quite a big task. Therefore I did decide to create a new one.