Bug 1354232 - Fix updating assert count when there's an exising value, r=maja_zf draft
authorJames Graham <james@hoppipolla.co.uk>
Thu, 07 Jun 2018 10:40:58 +0100
changeset 805844 f0d28f3db33f8185563f87abe882a02ce01d54f8
parent 805843 86a4ec11003b452919d71287619d53e062a3d472
child 805845 906452aa3563aaeefaa1bce2bf67050b4ef41032
push id112776
push userbmo:james@hoppipolla.co.uk
push dateFri, 08 Jun 2018 15:53:57 +0000
reviewersmaja_zf
bugs1354232
milestone62.0a1
Bug 1354232 - Fix updating assert count when there's an exising value, r=maja_zf In this case we want to take the existing value into account, and update to 1 more than the new value (in the max-asserts case). MozReview-Commit-ID: 1RtJ2gU1ZbH
testing/web-platform/tests/tools/wptrunner/wptrunner/manifestexpected.py
testing/web-platform/tests/tools/wptrunner/wptrunner/manifestupdate.py
--- a/testing/web-platform/tests/tools/wptrunner/wptrunner/manifestexpected.py
+++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/manifestexpected.py
@@ -297,16 +297,17 @@ def get_manifest(metadata_root, test_pat
             return static.compile(f,
                                   run_info,
                                   data_cls_getter=data_cls_getter,
                                   test_path=test_path,
                                   url_base=url_base)
     except IOError:
         return None
 
+
 def get_dir_manifest(path, run_info):
     """Get the ExpectedManifest for a particular test path, or None if there is no
     metadata stored for that test path.
 
     :param path: Full path to the ini file
     :param run_info: Dictionary of properties of the test run for which the expectation
                      values should be computed.
     """
--- a/testing/web-platform/tests/tools/wptrunner/wptrunner/manifestupdate.py
+++ b/testing/web-platform/tests/tools/wptrunner/wptrunner/manifestupdate.py
@@ -342,17 +342,19 @@ class PropertyUpdate(object):
                                 if result.value != unconditional_value)
 
         # It is an invariant that nothing in new matches an existing
         # condition except for the default condition
         if self.new:
             update_default, new_default_value = self.update_default()
             if update_default:
                 if new_default_value != self.default_value:
-                    self.node.set(self.property_name, self.update_value(None, new_default_value), condition=None)
+                    self.node.set(self.property_name,
+                                  self.update_value(unconditional_value, new_default_value),
+                                  condition=None)
             else:
                 self.add_new(unconditional_value)
 
         # Remove cases where the value matches the default
         if (self.property_name in self.node._data and
             len(self.node._data[self.property_name]) > 0 and
             self.node._data[self.property_name][-1].condition_node is None and
             self.node._data[self.property_name][-1].value_as(self.value_type) == self.default_value):
@@ -424,84 +426,86 @@ class ExpectedUpdate(PropertyUpdate):
 
 
 class MaxAssertsUpdate(PropertyUpdate):
     property_name = "max-asserts"
     cls_default_value = 0
     value_type = int
 
     def update_value(self, old_value, new_value):
+        new_value = self.value_type(new_value)
         if old_value is not None:
             old_value = self.value_type(old_value)
-        if old_value and old_value < new_value:
-            return new_value
+        if old_value is not None and old_value < new_value:
+            return new_value + 1
         if old_value is None:
-            return new_value
+            return new_value + 1
         return old_value
 
     def update_default(self):
         """For asserts we always update the default value and never add new conditionals.
         The value we set as the default is the maximum the current default or one more than the
         number of asserts we saw in any configuration."""
         # Current values
         values = []
         current_default = None
         if self.property_name in self.node._data:
             current_default = [item for item in
                                self.node._data[self.property_name]
                                if item.condition_node is None]
             if current_default:
                 values.append(int(current_default[0].value))
-        values.extend(item.value + 1 for item in self.new)
-        values.extend(item.value + 1 for item in
+        values.extend(item.value for item in self.new)
+        values.extend(item.value for item in
                       itertools.chain.from_iterable(results for _, results in self.updated))
         new_value = max(values)
         return True, new_value
 
 
 class MinAssertsUpdate(PropertyUpdate):
     property_name = "min-asserts"
     cls_default_value = 0
     value_type = int
 
     def update_value(self, old_value, new_value):
+        new_value = self.value_type(new_value)
         if old_value is not None:
             old_value = self.value_type(old_value)
-        if old_value and new_value < old_value:
+        if old_value is not None and new_value < old_value:
             return 0
         if old_value is None:
             # If we are getting some asserts for the first time, set the minimum to 0
-            return 0
+            return new_value
         return old_value
 
     def update_default(self):
         """For asserts we always update the default value and never add new conditionals.
         This is either set to the current value or one less than the number of asserts
         we saw, whichever is lower."""
         values = []
         current_default = None
         if self.property_name in self.node._data:
             current_default = [item for item in
                                self.node._data[self.property_name]
                                if item.condition_node is None]
         if current_default:
             values.append(current_default[0].value_as(self.value_type))
-        values.extend(max(0, item.value - 1) for item in self.new)
-        values.extend(max(0, item.value - 1) for item in
+        values.extend(max(0, item.value) for item in self.new)
+        values.extend(max(0, item.value) for item in
                       itertools.chain.from_iterable(results for _, results in self.updated))
         new_value = min(values)
         return True, new_value
 
 
 class LsanUpdate(PropertyUpdate):
     property_name = "lsan-allowed"
     cls_default_value = None
 
     def get_value(self, result):
-        # IF we have an allowed_match that matched, return None
+        # If we have an allowed_match that matched, return None
         # This value is ignored later (because it matches the default)
         # We do that because then if we allow a failure in foo/__dir__.ini
         # we don't want to update foo/bar/__dir__.ini with the same rule
         if result[1]:
             return None
         # Otherwise return the topmost stack frame
         # TODO: there is probably some improvement to be made by looking for a "better" stack frame
         return result[0][0]