Fix #1320: fix handling of futures in work-in-progress check

checks/remotesettings/work_in_progress.py

@@ -43,45 +43,43 @@ async def run(server: str, auth: str, max_age: int) -> CheckResult:
     ]
     results_metadata = await run_parallel(*futures)
 
-    futures_sources = []
-    futures_destination = []
-    for resource, resp in zip(resources, results_metadata):
-        metadata = resp["data"]
+    too_old = {}
+    for resource, collection_metadata in zip(resources, results_metadata):
+        metadata = collection_metadata["data"]
+
         # For this check, since we want to detect pending changes,
         # we also consider work-in-progress a pending request review.
         if metadata["status"] not in ("work-in-progress", "to-review"):
             continue
 
-        if last_edit_age(metadata) > max_age:
+        # Ignore collections changed recently.
+        if last_edit_age(metadata) <= max_age:
+            continue
+
+        # Ignore collections in WIP with no pending changes.
+        if metadata["status"] == "work-in-progress":
             # These collections are worth introspecting.
-            futures_sources.append(client.get_records(**resource["source"]))
-            futures_destination.append(client.get_records(**resource["destination"]))
+            source_records = await client.get_records(**resource["source"])
+            destination_records = await client.get_records(**resource["destination"])
+            if not compare_collections(source_records, destination_records):
+                continue
+
+        # Fetch list of editors, if necessary to contact them.
+        group = await client.get_group(
+            bucket=resource["source"]["bucket"],
+            id=resource["source"]["collection"] + "-editors",
+        )
+        editors = group["data"]["members"]
 
-    results_sources = await run_parallel(*futures_sources)
-    results_destination = await run_parallel(*futures_destination)
+        cid = "{bucket}/{collection}".format(**resource["destination"])
 
-    too_old = {}
-    for resource, collection_metadata, source_records, destination_records in zip(
-        resources, results_metadata, results_sources, results_destination
-    ):
-        diff = compare_collections(source_records, destination_records)
-        if diff:
-            # Fetch list of editors, if necessary to contact them.
-            group = await client.get_group(
-                bucket=resource["source"]["bucket"],
-                id=resource["source"]["collection"] + "-editors",
-            )
-            editors = group["data"]["members"]
-
-            cid = "{bucket}/{collection}".format(**resource["destination"])
-            metadata = collection_metadata["data"]
-            last_edit_by = metadata.get("last_edit_by", "N/A")
-            too_old[cid] = {
-                "age": last_edit_age(metadata),
-                "status": metadata["status"],
-                "last_edit_by": last_edit_by,
-                "editors": editors,
-            }
+        last_edit_by = metadata.get("last_edit_by", "N/A")
+        too_old[cid] = {
+            "age": last_edit_age(metadata),
+            "status": metadata["status"],
+            "last_edit_by": last_edit_by,
+            "editors": editors,
+        }
 
     """
     {

tests/checks/remotesettings/test_work_in_progress.py

@@ -131,7 +131,7 @@ async def test_negative(mock_responses):
         collection_url,
         payload={
             "data": {
-                "status": "to-review",
+                "status": "work-in-progress",
                 "last_edit_by": "ldap:mleplatre@mozilla.com",
                 "last_edit_date": (utcnow() - timedelta(days=10)).isoformat(),
             }
@@ -158,22 +158,10 @@ async def test_negative(mock_responses):
     # Add another failing collection, without last-edit
     group_url = server_url + GROUP_URL.format("bid", "cid-editors")
     collection_url = server_url + COLLECTION_URL.format("bid", "cid2")
-    mock_responses.get(collection_url, payload={"data": {"status": "work-in-progress"}})
+    mock_responses.get(collection_url, payload={"data": {"status": "to-review"}})
     mock_responses.get(
         group_url, payload={"data": {"members": ["ldap:user@mozilla.com"]}}
     )
-    mock_responses.get(
-        server_url + RECORD_URL.format("bid", "cid2"),
-        payload={
-            "data": [{"id": "record"}],
-        },
-    )
-    mock_responses.get(
-        server_url + RECORD_URL.format("main", "cid2"),
-        payload={
-            "data": [{"id": "record", "field": "diff"}],
-        },
-    )
 
     with patch_async(f"{MODULE}.fetch_signed_resources", return_value=RESOURCES):
         status, data = await run(server_url, FAKE_AUTH, max_age=5)
@@ -182,14 +170,59 @@ async def test_negative(mock_responses):
     assert data == {
         "main/cid": {
             "age": 10,
-            "status": "to-review",
+            "status": "work-in-progress",
             "last_edit_by": "ldap:mleplatre@mozilla.com",
             "editors": ["ldap:user@mozilla.com"],
         },
         "main/cid2": {
             "age": sys.maxsize,
-            "status": "work-in-progress",
+            "status": "to-review",
             "last_edit_by": "N/A",
             "editors": ["ldap:editor@mozilla.com"],
         },
     }
+
+
+async def test_negative_with_recent(mock_responses):
+    server_url = "http://fake.local/v1"
+
+    collection_url = server_url + COLLECTION_URL.format("bid", "cid")
+    mock_responses.get(
+        collection_url,
+        payload={
+            "data": {
+                "status": "signed",
+                "last_edit_date": (utcnow() - timedelta(days=3)).isoformat(),
+                "last_edit_by": "ldap:mleplatre@mozilla.com",
+            }
+        },
+    )
+
+    collection_url2 = server_url + COLLECTION_URL.format("bid", "cid2")
+    mock_responses.get(
+        collection_url2,
+        payload={
+            "data": {
+                "status": "to-review",
+                "last_edit_date": (utcnow() - timedelta(days=20)).isoformat(),
+                "last_edit_by": "ldap:mleplatre@mozilla.com",
+            }
+        },
+    )
+    mock_responses.get(
+        server_url + GROUP_URL.format("bid", "cid2-editors"),
+        payload={"data": {"members": ["ldap:editor@mozilla.com"]}},
+    )
+
+    with patch_async(f"{MODULE}.fetch_signed_resources", return_value=RESOURCES):
+        status, data = await run(server_url, FAKE_AUTH, max_age=15)
+
+    assert status is False
+    assert data == {
+        "main/cid2": {
+            "age": 20,
+            "editors": ["ldap:editor@mozilla.com"],
+            "last_edit_by": "ldap:mleplatre@mozilla.com",
+            "status": "to-review",
+        }
+    }