Runner agent is modifying nested objects causing tests to fail

[ombre42]

RED 0.7.9
Robot Framework 3.0.1 (Python 2.7.10 on darwin)

I found that when I run in debug mode, parts of some JSON had type changed from int to the str representation of the int as soon as the JSON was assigned to a variable. This occurred in debug mode but not in run mode. This occurs even with no breakpoints set.

Here is a minimal test to reproduce the issue:

*** Test Cases ***
Modifies Data Structures
    ${obj}    Evaluate    {'inner': [42]}
    Should Be Equal    int    ${obj['inner'][0].__class__.__name__}

This passes in run mode but fails in debug mode with:
FAIL : int != str

I suspect the problem lies in TestRunnerAgent._check_changed_variable

[michalanglart]

Hi,

interesting case, thanks for spotting that! Debugger is going through major changes now in order to fix couple of major problems we've had with remote debugging. However we also had issues with changing value of variables during debugging, so the area of code which provides variables to RED as well as the area responsible for changing the values have changed quite a bit already.

From what I've checked now this problem is already fixed on our branch with reworked debugger, so you should see it released with next version.

[michalanglart]

Ok,

I've looked what is wrong and the problem is not in _check_changed_variable - this method is only called when execution is suspended and user requests variable value change through Variables view.

The problem lies in _fix_unicode function. Whenever agent sends variables to RED it passes all values through this function in order to:

a) dump all the non-serializable objects to str in order to be able to send it through JSON
b) encode unicode properly
c) also truncate long strings in order not to have OutOfMemory as raised in #23

This function recursively traverses through list/dictionaries and as you can see https://github.com/nokia/RED/blob/master/src/RobotFrameworkCore/org.robotframework.ide.core-functions/src/main/python/scripts/TestRunnerAgent.py in line 111 it makes string representation out of int type and unfortunately in line 107 it reassigns this string into the original object. In your case there is a list inside this dictionary and _fix_unicode replaces the int inside with its string representation.

As I stated above - this is already fixed in development version and will be available with next RED.

If you however want to fix it on your side you have to modify the source of TestRunnerAgent.py.

1. inside your RED installation there is a plugins/org.robotframework.ide.eclipse.main.plugin_0.7.9.xxxxxxxxxxxx.jar file
2. open it with any zip archiver
3. inside it there is another jar: lib/org.robotframework.ide.core-functions-0.0.1-SNAPSHOT.jar
4. once again open it with zip archiver
5. modify scripts/TestRunnerAgent.py; this is how _fix_unicode looks currently

def _fix_unicode(max_length, data):
    if sys.version_info < (3, 0, 0) and isinstance(data, unicode):
        return _truncate(max_length, data.encode('utf-8'))
    elif sys.version_info < (3, 0, 0) and isinstance(data, basestring):
        return _truncate(max_length, data.encode('unicode_escape'))
    elif sys.version_info >= (3, 0, 0) and isinstance(data, str):
        return _truncate(max_length, data)
    elif isinstance(data, Mapping):
        return dict((_fix_unicode(max_length, k), _fix_unicode(max_length, data[k])) for k in data)
    elif isinstance(data, tuple):
        return tuple(list(_fix_unicode(max_length, el) for el in data))
    elif isinstance(data, list):
        return list(_fix_unicode(max_length, el) for el in data)
    elif data is None:
        return _fix_unicode(max_length, 'None')
    else:
        return _fix_unicode(max_length, str(data))

6. please note that now we do not check if isinstance(data, dict) but isinstance(data, Mapping) in order to handle other dictionaries-like structures which RF uses internally. Because of that you also need to import Mapping (try-except because Mapping lies in different modules in py3 than in py2):

try:
    from collections.abc import Mapping
except:
    from collections import Mapping

7. save the file; put it back into lib/org.robotframework.ide.core-functions-0.0.1-SNAPSHOT.jar archive
8. save the latter and put it again into plugins/org.robotframework.ide.eclipse.main.plugin_0.7.9.xxxxxxxxxxxx.jar archive
9. you should have this problem fixed

Hope this helps! For proper fix please wait till next release.

[kormbrek-rally]

Thank you so much for the detailed reply and workaround.

Although the upcoming fix may have resolved the issue, have you considered using copy.deepcopy in the code instead of copy.copy?

I would think that would avoid accidental modification of data structures, however it would impact performance.

I looked into debugging this and got lost in doing maven builds with test as the goal failing due to unresolvable host wrbboxv01.emea.nsn-net.net.

[ombre42]

also can't help but point out that this code would lead to some unexpected behavior if people are using collections.OrderedDict:

elif isinstance(data, Mapping):
        return dict((_fix_unicode(max_length, k), _fix_unicode(max_length, data[k])) for k in data)

[michalanglart]

Although the upcoming fix may have resolved the issue, have you considered using copy.deepcopy in the code instead of copy.copy?

Good point - I was thinking about it. However given the fact that currently (in both 0.7.9 and next version) we are sending those variables pretty often which is already hitting performance I'd rather prefer to avoid copying and assure that this function will not hurt original data with usage of some suite of unit tests. On the other hand we're currently trying to change code a bit in order to send variables only when they are needed (i.e. when execution have been suspended) - if this will work we could deep copy them.

I looked into debugging this and got lost in doing maven builds with test as the goal failing due to unresolvable host wrbboxv01.emea.nsn-net.net.

Ooopss.. sorry for that, we'll fix it - this is internal mirror of eclipse update site. It should be changed to http://download.eclipse.org/releases/neon/ and http://download.eclipse.org/releases/oxygen/ for neon-profile and oxygen-profile respectively in org.robotframework.ide.eclipse.parent/pom.xml file.

also can't help but point out that this code would lead to some unexpected behavior if people are using collections.OrderedDict:

Thanks for spotting; someone who expect order in dictionary could be confused. We'll fix that too.

[michalanglart]

Fixed in 0.8.0