robotframework / SeleniumLibrary

Web testing library for Robot Framework
Apache License 2.0
1.36k stars 751 forks source link

Execute JavaScript converts arguments to strings with robot==6.1 #1843

Closed ikseek closed 7 months ago

ikseek commented 1 year ago

Steps to reproduce the issue

*** Settings ***
Library    SeleniumLibrary

*** Test Cases ***
Test Print Dictionary Value
    Open Browser        browser=chrome
    &{ARGS}=            Create Dictionary  key=value
    Execute JavaScript  alert(JSON.stringify(arguments))    ARGUMENTS    ${ARGS}
    Sleep    5

Expected behavior and actual behavior

When I run this robot script using robot==6.1 I see the browser window with alert message box saying: {"0":"{'key': 'value'}"}

I was expecting to see a dictionary value that seleniumlibrary with robot==6.0.2 produced {"0":{"key":"value"}}

Notice that currently I see a string with python representation of the dict and previously it was a dict itself.

Hint: this happens because the type hint in this line insists on WebElements and strings: https://github.com/robotframework/SeleniumLibrary/blob/9823418feda556d48829a3137eefb962c44b14ff/src/SeleniumLibrary/keywords/javascript.py#L33 Robot 6.1 seems to use type hints for automatic type conversions.

Environment

Browser: Chrome (latest) Operating System: Mac OS 13.4.1 Libraries

Snooz82 commented 11 months ago

Without testing it, but reading the type hint of that keyword and its code, i would say, that the type hint of WebElement | str is simply wrong.

Python Selenium docs do say for execute_async_script for argument *args:

*args: Any applicable arguments for your JavaScript.

So there should not be any type hint it can be Union[Any, str] or so.

Ps: i am again really disappointed by the docs of Selenium itself. selenium.dev does not mention how to use args at all and even readthedocs is absolutely too lean here. And by reading the Selenium code that is implemeting this, i must say, that it is weird too. They have a weird special handling if one of the args is a list or tuple and contains EventFiringWebElement.

However: in the end they are just calling json.dumps on whatever you hand over to *args.

So if you hand over a string, it stays a string and if you hand over any json serializable object, like a dictionary in this example it will work.

Therefore the type hint must say, that args can be anything that is json-serializable. If SeleniumLibrary would require at least Robot Framework 5.0, you could introduce a new type hint with a custom converter.

That custom converter would try a json.dumps() on whatever it gets. and if it fails it would return a meaningful error that whatever is handed over could not be converted to a json object and therefor not be digested by Selenium.

See supported-conversions

class JsonSerializable(str):
    @classmethod
    def to_json_serializable(cls, value):
      """Any object that can be string serialised by `json.dumps()` function can be accepted.

      This may be one of `str, int, float, bool, None, Dict, List, Tuple` or a compound of those, like a list of int.
      Also custom types are possible as long as they are compatible with `json.dumps()` function."""
      try:
        json.dumps(value)
      except:
        raise ValueError(f"The given argument could not be serialized to json. Argument value: '{value}'")
      return cls(value)

ROBOT_LIBRARY_CONVERTERS = {JsonSerializable: JsonSerializable.to_json_serializable}

with this converter the type hint could just be JsonSerializable

pekkaklarck commented 11 months ago

As the mentioned already in the description, this keyword has wrong type hints. The reason it causes issues in RF 6.1 but not earlier is: https://github.com/robotframework/robotframework/issues/4648

Using Any would probably be the easiest fix. A custom converter that would report an error if args cannot be converted to JSON would be nice but I'm not sure would it be worth the effort.

Snooz82 commented 11 months ago

Its not only the error message, but more importantly the documentation of the types accepted.

emanlove commented 7 months ago

Fixed with #1866.