DM-15402: Fix race conditions in tests and enhance Config test coverage

python/lsst/daf/butler/core/config.py

@@ -85,12 +85,12 @@ def include(self, node):
         elif isinstance(node, yaml.SequenceNode):
             result = []
             for filename in self.construct_sequence(node):
-                result += self.extractFile(filename)
+                result.append(self.extractFile(filename))
             return result
 
         elif isinstance(node, yaml.MappingNode):
             result = {}
-            for k, v in self.construct_mapping(node).iteritems():
+            for k, v in self.construct_mapping(node).items():
                 result[k] = self.extractFile(v)
             return result
 
@@ -142,10 +142,10 @@ def __init__(self, other=None):
         if other is None:
             return
 
-        if isinstance(other, collections.Mapping):
-            self.update(other)
-        elif isinstance(other, Config):
+        if isinstance(other, Config):
             self.data = copy.deepcopy(other.data)
+        elif isinstance(other, collections.Mapping):
+            self.update(other)
         elif isinstance(other, str):
             # if other is a string, assume it is a file path.
             self.__initFromFile(other)
@@ -217,7 +217,7 @@ def __getitem__(self, name):
         for key in name.split("."):
             if data is None:
                 return None
-            if key in data:
+            if key in data and isinstance(data, collections.Mapping):
                 data = data[key]
             else:
                 try:
@@ -250,21 +250,38 @@ def __contains__(self, key):
         d = self.data
         keys = key.split(".")
         last = keys.pop()
-        for k in keys:
-            if isinstance(d, collections.Sequence):
+
+        def checkNextItem(k, d):
+            """See if k is in d and if it is return the new child"""
+            nextVal = None
+            isThere = False
+            if d is None:
+                # We have gone past the end of the hierarchy
+                pass
+            elif isinstance(d, collections.Sequence):
                 # Check sequence first because for lists
                 # __contains__ checks whether value is found in list
                 # not whether the index exists in list. When we traverse
                 # the hierarchy we are interested in the index.
                 try:
-                    d = d[int(k)]
+                    nextVal = d[int(k)]
+                    isThere = True
                 except IndexError:
-                    return False
+                    pass
+                except ValueError:
+                    isThere = k in d
             elif k in d:
-                d = d[k]
-            else:
+                nextVal = d[k]
+                isThere = True
+            return nextVal, isThere
+
+        for k in keys:
+            d, isThere = checkNextItem(k, d)
+            if not isThere:
                 return False
-        return last in d
+
+        _, isThere = checkNextItem(last, d)
+        return isThere
 
     def update(self, other):
         """Like dict.update, but will add or modify keys in nested dicts,
@@ -284,25 +301,14 @@ def update(self, other):
               foo == {"a": {"b": 1, "c": 2}}
         """
         def doUpdate(d, u):
+            if not isinstance(u, collections.Mapping) or \
+                    not isinstance(d, collections.Mapping):
+                raise RuntimeError("Only call update with Mapping, not {}".format(type(d)))
             for k, v in u.items():
-                if isinstance(d, collections.Mapping):
-                    if isinstance(v, collections.Mapping):
-                        r = doUpdate(d.get(k, {}), v)
-                        d[k] = r
-                    else:
-                        d[k] = v
+                if isinstance(v, collections.Mapping):
+                    d[k] = doUpdate(d.get(k, {}), v)
                 else:
-                    if isinstance(d, collections.Sequence):
-                        try:
-                            intk = int(k)
-                        except ValueError:
-                            # This will overwrite the entire sequence
-                            d = {k: v}
-                        else:
-                            r = doUpdate(d[intk], v)
-                            d[intk] = r
-                    else:
-                        d = {k: v}
+                    d[k] = v
             return d
         doUpdate(self.data, other)
 
@@ -324,6 +330,17 @@ def merge(self, other):
     def names(self, topLevelOnly=False):
         """Get the dot-delimited name of all the keys in the hierarchy.
 
+        Parameters
+        ----------
+        topLevelOnly : `bool`, optional
+            If False, the default, a full hierarchy of names is returned.
+            If True, only the top level are returned.
+
+        Returns
+        -------
+        names : `list`
+            List of all names present in the `Config`.
+
         Notes
         -----
         This is different than the built-in method `dict.keys`, which will
@@ -333,41 +350,45 @@ def names(self, topLevelOnly=False):
             return list(self.keys())
 
         def getKeys(d, keys, base):
-            for key in d:
+            if isinstance(d, collections.Sequence):
+                theseKeys = range(len(d))
+            else:
+                theseKeys = d.keys()
+            for key in theseKeys:
                 val = d[key]
-                levelKey = base + "." + key if base is not None else key
+                levelKey = f"{base}.{key}" if base is not None else key
                 keys.append(levelKey)
-                if isinstance(val, collections.Mapping):
+                if isinstance(val, (collections.Mapping, collections.Sequence)) and not isinstance(val, str):
                     getKeys(val, keys, levelKey)
         keys = []
         getKeys(self.data, keys, None)
         return keys
 
     def asArray(self, name):
-        """Get a value as an array. May contain one or more elements.
+        """Get a value as an array.
+
+        May contain one or more elements.
 
         Parameters
         ----------
         name : `str`
-            Key
+            Key to use to retrieve value.
+
+        Returns
+        -------
+        array : `collections.Sequence`
+            The value corresponding to name, but guaranteed to be returned
+            as a list with at least one element. If the value is a
+            `~collections.Sequence` (and not a `str`) the value itself will be
+            returned, else the value will be the first element.
         """
         val = self.get(name)
         if isinstance(val, str):
             val = [val]
-        elif not isinstance(val, collections.Container):
+        elif not isinstance(val, collections.Sequence):
             val = [val]
         return val
 
-    def __lt__(self, other):
-        if isinstance(other, Config):
-            other = other.data
-        return self.data < other
-
-    def __le__(self, other):
-        if isinstance(other, Config):
-            other = other.data
-        return self.data <= other
-
     def __eq__(self, other):
         if isinstance(other, Config):
             other = other.data
@@ -378,16 +399,6 @@ def __ne__(self, other):
             other = other.data
         return self.data != other
 
-    def __gt__(self, other):
-        if isinstance(other, Config):
-            other = other.data
-        return self.data > other
-
-    def __ge__(self, other):
-        if isinstance(other, Config):
-            other = other.data
-        return self.data >= other
-
     #######
     # i/o #
 
@@ -404,8 +415,7 @@ def dump(self, output):
         # After the expected/ordered keys are weritten to the stream the
         # remainder of the keys are written to the stream.
         data = copy.copy(self.data)
-        keys = ["defects", "needCalibRegistry", "levels", "defaultLevel", "defaultSubLevels", "camera",
-                "exposures", "calibrations", "datasets"]
+        keys = []
         for key in keys:
             try:
                 yaml.safe_dump({key: data.pop(key)}, output, default_flow_style=False)

tests/config/testConfigs/testinclude.yaml

@@ -4,3 +4,5 @@ comp2: !include testconfig.yaml
 comp3:
   - 52
   - !include viacls.yaml
+comp4: !include [viacls.yaml, testconfig.yaml]
+comp5: !include {comp6: viacls.yaml}

tests/datasetsHelper.py

@@ -70,3 +70,44 @@ def makeDatasetRef(self, datasetTypeName, dataUnits, storageClass, dataId, id=No
             self.id += 1
             id = self.id
         return DatasetRef(datasetType, dataId, id=id)
+
+
+class DatastoreTestHelper:
+    """Helper methods for Datastore tests"""
+
+    def setUpDatastoreTests(self, registryClass, configClass):
+        """Shared setUp code for all Datastore tests"""
+        self.registry = registryClass()
+
+        # Need to keep ID for each datasetRef since we have no butler
+        # for these tests
+        self.id = 1
+
+        self.config = configClass(self.configFile)
+
+        # Some subclasses override the working root directory
+        if self.root is not None:
+            self.datastoreType.setConfigRoot(self.root, self.config, self.config.copy())
+
+    def makeDatastore(self, sub=None):
+        """Make a new Datastore instance of the appropriate type.
+
+        Parameters
+        ----------
+        sub : str, optional
+            If not None, the returned Datastore will be distinct from any
+            Datastore constructed with a different value of ``sub``.  For
+            PosixDatastore, for example, the converse is also true, and ``sub``
+            is used as a subdirectory to form the new root.
+
+        Returns
+        -------
+        datastore : `Datastore`
+            Datastore constructed by this routine using the supplied
+            optional subdirectory if supported.
+        """
+        config = self.config.copy()
+        if sub is not None and self.root is not None:
+            self.datastoreType.setConfigRoot(os.path.join(self.root, sub), config, self.config)
+            print(config)
+        return self.datastoreType(config=config, registry=self.registry)

tests/test_config.py

@@ -92,6 +92,40 @@ class ConfigTestCls(ConfigTest):
 
 class ConfigTestCase(unittest.TestCase):
     """Tests of simple Config"""
+
+    def testBadConfig(self):
+        for badArg in ([], "file.fits"):
+            with self.assertRaises(RuntimeError):
+                Config(badArg)
+
+    def testOperators(self):
+        c1 = Config({"a": {"b": 1}, "c": 2})
+        c2 = c1.copy()
+        self.assertEqual(c1, c2)
+        c2["a.b"] = 5
+        self.assertNotEqual(c1, c2)
+
+    def testUpdate(self):
+        c = Config({"a": {"b": 1}})
+        c.update({"a": {"c": 2}})
+        self.assertEqual(c["a.b"], 1)
+        self.assertEqual(c["a.c"], 2)
+        c.update({"a": {"d": [3, 4]}})
+        self.assertEqual(c["a.d.0"], 3)
+        c.update({"z": [5, 6, {"g": 2, "h": 3}]})
+        self.assertEqual(c["z.1"], 6)
+
+        # This is detached from parent
+        c2 = c["z.2"]
+        self.assertEqual(c2["g"], 2)
+        c2.update({"h": 4, "j": 5})
+        self.assertEqual(c2["h"], 4)
+        self.assertNotIn("z.2.j", c)
+        self.assertNotEqual(c["z.2.h"], 4)
+
+        with self.assertRaises(RuntimeError):
+            c.update([1, 2, 3])
+
     def testHierarchy(self):
         c = Config()
 
@@ -111,16 +145,36 @@ def testHierarchy(self):
         # Test that we can index into lists
         c["a.b.c"] = [1, "7", 3, {"1": 4, "5": "Five"}, "hello"]
         self.assertIn("a.b.c.3.5", c)
+        self.assertNotIn("a.b.c.10", c)
+        self.assertNotIn("a.b.c.10.d", c)
         self.assertEqual(c["a.b.c.3.5"], "Five")
         # Is the value in the list?
         self.assertIn("a.b.c.hello", c)
+        self.assertNotIn("a.b.c.hello.not", c)
 
         # And assign to an element in the list
         self.assertEqual(c["a.b.c.1"], "7")
         c["a.b.c.1"] = 8
         self.assertEqual(c["a.b.c.1"], 8)
         self.assertIsInstance(c["a.b.c"], collections.Sequence)
 
+        # Test we do get lists back from asArray
+        a = c.asArray("a.b.c")
+        self.assertIsInstance(a, list)
+
+        # Is it the *same* list as in the config
+        a.append("Sentinel")
+        self.assertIn("Sentinel", c["a.b.c"])
+        self.assertIn("a.b.c.Sentinel", c)
+
+        # Test we always get a list
+        for k in c.names():
+            a = c.asArray(k)
+            self.assertIsInstance(a, list)
+
+        # Check we get the same top level keys
+        self.assertEqual(set(c.names(topLevelOnly=True)), set(c.data.keys()))
+
         # Check that lists still work even if assigned a dict
         c = Config({"cls": "lsst.daf.butler",
                     "datastores": [{"datastore": {"cls": "datastore1"}},
@@ -130,6 +184,14 @@ def testHierarchy(self):
         self.assertEqual(c["datastores.1.datastore.cls"], "datastore2modified")
         self.assertIsInstance(c["datastores"], collections.Sequence)
 
+        # Test that we can get all the listed names.
+        # and also that they are marked as "in" the Config
+        # Two distinct loops
+        for n in c.names():
+            val = c[n]
+            self.assertIsNotNone(val)
+            self.assertIn(n, c)
+
 
 class ConfigSubsetTestCase(unittest.TestCase):
     """Tests for ConfigSubset
@@ -268,10 +330,19 @@ def testClassDerived(self):
     def testInclude(self):
         """Read a config that has an include directive"""
         c = Config(os.path.join(self.configDir, "testinclude.yaml"))
-        print(c.ppprint())
         self.assertEqual(c["comp1.item1"], 58)
         self.assertEqual(c["comp2.comp.item1"], 1)
         self.assertEqual(c["comp3.1.comp.item1"], "posix")
+        self.assertEqual(c["comp4.0.comp.item1"], "posix")
+        self.assertEqual(c["comp4.1.comp.item1"], 1)
+        self.assertEqual(c["comp5.comp6.comp.item1"], "posix")
+
+        # Test a specific name and then test that all
+        # returned names are "in" the config.
+        names = c.names()
+        self.assertIn("comp3.1.comp.item1", names)
+        for n in names:
+            self.assertIn(n, names)
 
 
 if __name__ == "__main__":