Test and refactor incremental results

spectacles/runner.py

@@ -1,8 +1,8 @@
 from typing import List, Dict, Any, Optional, NamedTuple
-from copy import deepcopy
+import itertools
 from spectacles.client import LookerClient
 from spectacles.validators import SqlValidator, DataTestValidator, ContentValidator
-from spectacles.utils import time_hash, hash_error
+from spectacles.utils import time_hash
 from spectacles.logger import GLOBAL_LOGGER as logger
 from spectacles.printer import print_header
 from spectacles.types import QueryMode
@@ -236,44 +236,46 @@ def validate_content(
         else:
             return results
 
+    @staticmethod
     def _incremental_results(
-        self, main: Dict[str, Any], additional: Dict[str, Any]
+        main: Dict[str, Any], additional: Dict[str, Any]
     ) -> Dict[str, Any]:
         """Returns a new result with only the additional errors in `additional`."""
-
-        def merge_test(main: Dict[str, Any], additional: Dict[str, Any]):
-            incremental = deepcopy(additional)
-            if not main["passed"] and not additional["passed"]:
-                incremental["passed"] = True
-            return incremental
-
-        def sort_tests(tested: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
-            return sorted(tested, key=lambda x: (x["model"], x["explore"]))
-
-        # Don't expect this to happen, but raise an exception here to avoid unexpected
-        # results from `zip` later
-        if len(main["tested"]) != len(additional["tested"]):
-            raise ValueError(
-                "Both main and additional results must "
-                "have the same number of tested explores."
-            )
-
-        incremental: Dict[str, Any] = {"validator": "content", "tested": []}
-        # Combine the tests, only marking `passed = False` if it didn't fail on main
-        for main_test, additional_test in zip(
-            sort_tests(main["tested"]), sort_tests(additional["tested"])
-        ):
-            incremental["tested"].append(merge_test(main_test, additional_test))
+        incremental: Dict[str, Any] = {
+            "validator": "content",
+            # Start with models and explores we know passed in `additional`
+            "tested": [test for test in additional["tested"] if test["passed"]],
+            "errors": [],
+        }
+
+        # Build a list of disputed tests where dupes by model and explore are allowed
+        tests = []
+        for error in additional["errors"]:
+            if error in main["errors"]:
+                passed = True
+            else:
+                passed = False
+                incremental["errors"].append(error)
+
+            test = dict(model=error["model"], explore=error["explore"], passed=passed)
+            tests.append(test)
+
+        def key_by(x):
+            return (x["model"], x["explore"])
+
+        if tests:
+            # Dedupe the list of tests, grouping by model and explore and taking the min
+            # To do this, we group by model and explore and sort by `passed`
+            tests = sorted(tests, key=lambda x: (x["model"], x["explore"], x["passed"]))
+            for key, group in itertools.groupby(tests, key=key_by):
+                items = list(group)
+                incremental["tested"].append(items[0])
+
+        # Re-sort the final list
+        incremental["tested"] = sorted(incremental["tested"], key=key_by)
 
         # Recompute the overall state of the test suite
         passed = min((test["passed"] for test in incremental["tested"]), default=True)
         incremental["status"] = "passed" if passed else "failed"
 
-        # Create a unique representation of each error
-        main_error_ids = [hash_error(error) for error in main["errors"]]
-        incremental["errors"] = [
-            error
-            for error in additional["errors"]
-            if hash_error(error) not in main_error_ids
-        ]
         return incremental

spectacles/utils.py

@@ -64,15 +64,3 @@ def time_hash() -> str:
     hash = hashlib.sha1()
     hash.update(str(time.time()).encode("utf-8"))
     return hash.hexdigest()[:10]
-
-
-def hash_error(error: Dict[str, Any]) -> int:
-    """Convert an error dictionary into a hash of the important keys."""
-    items = (
-        error["model"],
-        error["explore"],
-        error["message"],
-        *(v for v in error["metadata"].values() if v),
-    )
-    hashed = hash(items)
-    return hashed

tests/test_runner.py

@@ -0,0 +1,102 @@
+from spectacles.runner import Runner
+
+
+def build_validation(validator):
+    return {
+        "validator": validator,
+        "status": "failed",
+        "tested": [
+            dict(model="ecommerce", explore="orders", passed=True),
+            dict(model="ecommerce", explore="sessions", passed=True),
+            dict(model="ecommerce", explore="users", passed=False),
+        ],
+        "errors": [
+            dict(
+                model="ecommerce",
+                explore="users",
+                test=None,
+                message="An error occurred",
+                metadata={},
+            )
+        ],
+    }
+
+
+def test_incremental_same_results_should_not_have_errors():
+    main = build_validation("content")
+    additional = build_validation("content")
+    incremental = Runner._incremental_results(main, additional)
+    assert incremental["status"] == "passed"
+    assert incremental["errors"] == []
+    assert incremental["tested"] == [
+        dict(model="ecommerce", explore="orders", passed=True),
+        dict(model="ecommerce", explore="sessions", passed=True),
+        dict(model="ecommerce", explore="users", passed=True),
+    ]
+
+
+def test_incremental_with_fewer_errors_than_main():
+    main = build_validation("content")
+    additional = build_validation("content")
+    additional["tested"][2]["passed"] = True
+    additional["errors"] = []
+    incremental = Runner._incremental_results(main, additional)
+    assert incremental["status"] == "passed"
+    assert incremental["errors"] == []
+    assert incremental["tested"] == [
+        dict(model="ecommerce", explore="orders", passed=True),
+        dict(model="ecommerce", explore="sessions", passed=True),
+        dict(model="ecommerce", explore="users", passed=True),
+    ]
+
+
+def test_incremental_with_more_errors_than_main():
+    main = build_validation("content")
+    additional = build_validation("content")
+    additional["tested"][1]["passed"] = False
+    extra_errors = [
+        dict(
+            model="ecommerce",
+            explore="users",
+            test=None,
+            message="Another error occurred",
+            metadata={},
+        ),
+        dict(
+            model="ecommerce",
+            explore="sessions",
+            test=None,
+            message="An error occurred",
+            metadata={},
+        ),
+    ]
+    additional["errors"].extend(extra_errors)
+    incremental = Runner._incremental_results(main, additional)
+    assert incremental["status"] == "failed"
+    assert incremental["errors"] == extra_errors
+    assert incremental["tested"] == [
+        dict(model="ecommerce", explore="orders", passed=True),
+        dict(model="ecommerce", explore="sessions", passed=False),
+        dict(model="ecommerce", explore="users", passed=False),
+    ]
+
+
+def test_incremental_with_fewer_tested_explores_than_main():
+    main = build_validation("content")
+    additional = build_validation("content")
+    _ = additional["tested"].pop(0)
+    extra_error = dict(
+        model="ecommerce",
+        explore="users",
+        test=None,
+        message="Another error occurred",
+        metadata={},
+    )
+    additional["errors"].append(extra_error)
+    incremental = Runner._incremental_results(main, additional)
+    assert incremental["status"] == "failed"
+    assert incremental["errors"] == [extra_error]
+    assert incremental["tested"] == [
+        dict(model="ecommerce", explore="sessions", passed=True),
+        dict(model="ecommerce", explore="users", passed=False),
+    ]