PythonRemoteServer silently changes default keyword arguments values

[fadimko]

It's easier to demonstrate the problem with a simple example. Let's create remote library py.py run it and use it in test.robot

py.py:

class py:
    def foo(self, a=None, b=2, c=None, d=None):
        print('{}: {}'.format(a, type(a)))
        print('{}: {}'.format(b, type(b)))
        print('{}: {}'.format(c, type(c)))
        print('{}: {}'.format(d, type(d)))

if __name__ == "__main__":
    import robotremoteserver
    robotremoteserver.RobotRemoteServer(py(), '0.0.0.0', 8270)

test.robot:

*** Settings ***
Library  Remote  127.0.0.1:8270  WITH NAME  py


*** Test Cases ***
Test
    py.Foo

Test2
    py.Foo  c=\

$ pybot -L TRACE test.robot generates this:

Robot remote server passes default values converted to strings to first 2 parameters (but not 4th!).

The biggest problem is that all parameters with default None values became strings, and all checks like

if function_parameter is not None:
    do_some_stuff()

will pass now.

I know, that robot framework converts almost everything to strings, but this behavior (silently converting to strings values of some parameters) is not obvious to users. Also if we include this library as local, nothing will be silently passed to python functions.

[fadimko]

$ pybot --version
Robot Framework 3.0.2 (Python 2.7.12 on linux2)

$ pip show robotremoteserver
Name: robotremoteserver
Version: 1.1
...

[pekkaklarck]

This is a known issue. The reason for the behavior is this:

• The remote interface is build on top of the dynamic library API.
• Originally the dynamic library API method run_keyword only accepted two arguments name (name of the keyword to execute) and args (arguments to that keyword).
• When the named argument syntax was introduced to Robot (i.e. being able pass arguments like name=value) we still needed to map those named arguments into positional arguments. With arguments that were not used we need to use their default values.
• When dynamic libraries tell what arguments certain keywords accept (get_keyword_arguments method), all information is just strings without any information about original default value types.

In practice the above means that, with your example, when you use c=xxx, we need to pass also all the arguments before c. The arguments are a=None, b=2, but because type information is lost the args passed to run_keywords is going to be ['None', '2', 'xxx'].

This is obviously annoying and affected also SeleniumLibrary when it switched to the dynamic API (robotframework/SeleniumLibrary#843). With SL we couldn't wait for possible fixes in the dynamic API and decided to workaround the issue with is_noney utility that considers also string 'NONE' (case-insensitively) to be None. Numbers we already handled with int() and float(). The benefit of this workaround is that users can use the keyword in the data like | Foo | a=2 | b=None | and the keyword will still work correctly. Without explicit conversion both 2 and None would be strings.

Although there is a workaround, it would be better to fix the underlying problem. Luckily that ought to be pretty easy. In order to support free **kwargs, the run_keyword dynamic method has optionally supported three arguments name, args, kwargs since RF 2.8.2 (robotframework/robotframework#1500). We could change the dynamic API to pass also named arguments via kwargs making it possible to omit arguments that weren't used altogether. With the c=xxx example this would result args being empty and kwargs being {'c': 'xxx'}. As a result the actual method would not get any value for a or b (i.e. they'd get their default values). This change ought to be pretty simple but all changes like that are potentially backwards incompatible. In my opinion it could be done in RF 3.1. The change should then also be tested with the remote interface.

[fadimko]

In my opinion it could be done in RF 3.1.

This would be cool!

With SL we couldn't wait for possible fixes in the dynamic API and decided to workaround the issue with is_noney utility that considers also string 'NONE' (case-insensitively) to be None.

It's easier for me to use @none_converter decorator that will convert all 'None' strings to None before actual function call, but this solution may be not flexible enough for big libraries.

[pekkaklarck]

This has been fixed in the soon-to-be-released Robot Framework 3.1. The fix is already in the latest preview releases, so they can be used even before the final release. See robotframework/robotframework#2930 for details.