Fix from_json with control structures. #3902

Also add tests for to/from_json roundtrip.

src/robot/model/body.py

@@ -85,9 +85,9 @@ class BaseBody(ItemList):
     # Set using 'BaseBody.register' when these classes are created.
     keyword_class = None
     for_class = None
+    while_class = None
     if_class = None
     try_class = None
-    while_class = None
     return_class = None
     continue_class = None
     break_class = None
@@ -97,9 +97,16 @@ def __init__(self, parent=None, items=None):
         super().__init__(BodyItem, {'parent': parent}, items)
 
     def _item_from_dict(self, data):
-        # FIXME: This doesn't work with all objects!
-        class_name = data.get('type', BodyItem.KEYWORD).lower() + '_class'
-        return getattr(self, class_name).from_dict(data)
+        item_type = data.get('type', None)
+        if not item_type:
+            item_class = self.keyword_class
+        elif item_type == BodyItem.IF_ELSE_ROOT:
+            item_class = self.if_class
+        elif item_type == BodyItem.TRY_EXCEPT_ROOT:
+            item_class = self.try_class
+        else:
+            item_class = getattr(self, item_type.lower() + '_class')
+        return item_class.from_dict(data)
 
     @classmethod
     def register(cls, item_class):
@@ -225,5 +232,8 @@ def __init__(self, branch_class, parent=None, items=None):
         self.branch_class = branch_class
         super().__init__(parent, items)
 
+    def _item_from_dict(self, data):
+        return self.branch_class.from_dict(data)
+
     def create_branch(self, *args, **kwargs):
         return self.append(self.branch_class(*args, **kwargs))

src/robot/model/fixture.py

@@ -13,15 +13,16 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 
-def create_fixture(fixture, parent, type):
+from collections.abc import Mapping
+
+
+def create_fixture(fixture, parent, fixture_type):
     # TestCase and TestSuite have 'fixture_class', Keyword doesn't.
     fixture_class = getattr(parent, 'fixture_class', parent.__class__)
+    if isinstance(fixture, fixture_class):
+        return fixture.config(parent=parent, type=fixture_type)
+    if isinstance(fixture, Mapping):
+        return fixture_class.from_dict(fixture).config(parent=parent, type=fixture_type)
     if fixture is None:
-        fixture = fixture_class(None, parent=parent, type=type)
-    elif isinstance(fixture, fixture_class):
-        fixture.parent = parent
-        fixture.type = type
-    else:
-        raise TypeError("Only %s objects accepted, got %s."
-                        % (fixture_class.__name__, fixture.__class__.__name__))
-    return fixture
+        return fixture_class(None, parent=parent, type=fixture_type)
+    raise TypeError(f"Invalid fixture type '{type(fixture).__name__}'.")

src/robot/model/modelobject.py

@@ -29,7 +29,7 @@ def from_dict(cls, data):
             return cls().config(**data)
         except AttributeError as err:
             raise ValueError(f"Creating '{full_name(cls)}' object from dictionary "
-                             f"failed: {err}\nDictionary:\n{data}")
+                             f"failed: {err}")
 
     @classmethod
     def from_json(cls, data):
@@ -50,7 +50,15 @@ def config(self, **attributes):
         New in Robot Framework 4.0.
         """
         for name in attributes:
-            setattr(self, name, attributes[name])
+            try:
+                setattr(self, name, attributes[name])
+            except AttributeError:
+                # Ignore error setting attribute if the object already has it.
+                # Avoids problems with `to/from_dict` roundtrip with body items
+                # having unsettable `type` attribute that is needed in dict data.
+                if getattr(self, name, object()) == attributes[name]:
+                    continue
+                raise AttributeError
         return self
 
     def copy(self, **attributes):

utest/model/test_fixture.py

@@ -1,6 +1,6 @@
 import unittest
 
-from robot.utils.asserts import assert_equal, assert_raises
+from robot.utils.asserts import assert_equal, assert_raises_with_msg
 from robot.model import TestSuite, Keyword
 from robot.model.fixture import create_fixture
 
@@ -21,7 +21,10 @@ def test_sets_parent_and_type_correctly(self):
     def test_raises_type_error_when_wrong_fixture_type(self):
         suite = TestSuite()
         wrong_kw = object()
-        assert_raises(TypeError, create_fixture, wrong_kw, suite, Keyword.TEARDOWN)
+        assert_raises_with_msg(
+            TypeError, "Invalid fixture type 'object'.",
+            create_fixture, wrong_kw, suite, Keyword.TEARDOWN
+        )
 
     def _assert_fixture(self, fixture, exp_parent, exp_type,
                         exp_class=TestSuite.fixture_class):

utest/model/test_modelobject.py

@@ -1,4 +1,3 @@
-import re
 import unittest
 
 from robot.model.modelobject import ModelObject
@@ -49,12 +48,9 @@ def test_not_accepted_attribute(self):
         class X(ModelObject):
             __slots__ = ['a']
         assert_equal(X.from_dict({'a': 1}).a, 1)
-        err = assert_raises(ValueError, X.from_dict, {'b': 'bad'})
-        expected = (f"Creating '{__name__}.X' object from dictionary failed: .*\n"
-                    f"Dictionary:\n{{'b': 'bad'}}")
-        if not re.fullmatch(expected, str(err)):
-            raise AssertionError(f'Unexpected error message. Expected:\n{expected}\n\n'
-                                 f'Actual:\n{err}')
+        error = assert_raises(ValueError, X.from_dict, {'b': 'bad'})
+        assert_equal(str(error).split(':')[0],
+                     f"Creating '{__name__}.X' object from dictionary failed")
 
 
 if __name__ == '__main__':

utest/running/test_run_model.py

@@ -7,7 +7,6 @@
 
 from robot import api, model
 from robot.model.modelobject import ModelObject
-from robot.running import TestSuiteBuilder
 from robot.running.model import (Break, Continue, For, If, IfBranch, Keyword, Return,
                                  TestCase, TestSuite, Try, TryBranch, UserKeyword, While)
 from robot.utils.asserts import (assert_equal, assert_false, assert_not_equal,
@@ -129,8 +128,9 @@ def _verify_suite(self, suite, name='Test Run Model', rpa=False):
 
 class TestCopy(unittest.TestCase):
 
-    def setUp(self):
-        self.suite = TestSuiteBuilder().build(MISC_DIR)
+    @classmethod
+    def setUpClass(cls):
+        cls.suite = TestSuite.from_file_system(MISC_DIR)
 
     def test_copy(self):
         self.assert_copy(self.suite, self.suite.copy())
@@ -220,7 +220,7 @@ def _assert_lineno_and_source(self, item, lineno):
         assert_equal(item.lineno, lineno)
 
 
-class TestToDict(unittest.TestCase):
+class TestToFromDict(unittest.TestCase):
 
     def test_keyword(self):
         self._verify(Keyword(), name='')
@@ -340,9 +340,16 @@ def test_suite_structure(self):
                      suites=[{'name': 'Child',
                               'tests': [{'name': 'T2', 'body': []}]}])
 
+    def test_bigger_suite_structure(self):
+        suite = TestSuite.from_file_system(MISC_DIR)
+        self._verify(suite, **suite.to_dict())
+
     def _verify(self, obj, **expected):
-        assert_equal(obj.to_dict(), expected)
-        assert_equal(list(obj.to_dict()), list(expected))
+        data = obj.to_dict()
+        assert_equal(data, expected)
+        assert_equal(list(data), list(expected))
+        roundtrip = type(obj).from_dict(data).to_dict()
+        assert_equal(roundtrip, expected)
 
 
 if __name__ == '__main__':