chore: update 'wait_for_sidekiq' function & use it more

JohnVillalovos · JohnVillalovos · commit abfcb9e408b4 · 2021-12-27T22:09:53.000-08:00
Make the `timeout` argument in `wait_for_sidekiq` be approximately the
seconds desired. Previously it was an iteration count so depending on
what the `step` argument was set to the time before the function would
time-out would vary.

Simpler for users to treat `timeout` as the (approximate) number of
seconds before `wait_for_sidekiq` will timeout.

Return new `WaitSidekiq` object which contains statistics from the
`wait_for_sidekiq` run.

Use `wait_for_sidekiq` in our fixtures defined in `conftest.py` after
they delete resources.  This will help increase the odds that we don't
have stray resources left-around when the next test starts. On my
local system the run-time of py_func_v4 went from 7m50s to 7m59s.
diff --git a/tests/functional/api/test_merge_requests.py b/tests/functional/api/test_merge_requests.py
@@ -113,8 +113,10 @@ def test_merge_request_should_remove_source_branch(
 
     mr.merge(should_remove_source_branch=True)
 
-    result = wait_for_sidekiq(timeout=60)
-    assert result is True, "sidekiq process should have terminated but did not"
+    result = wait_for_sidekiq()
+    assert (
+        result.success is True
+    ), f"sidekiq process should have terminated but did not: {result}"
 
     # Wait until it is merged
     mr_iid = mr.iid
@@ -125,8 +127,10 @@ def test_merge_request_should_remove_source_branch(
         time.sleep(0.5)
     assert mr.merged_at is not None
     time.sleep(0.5)
-    result = wait_for_sidekiq(timeout=60)
-    assert result is True, "sidekiq process should have terminated but did not"
+    result = wait_for_sidekiq()
+    assert (
+        result.success is True
+    ), f"sidekiq process should have terminated but did not: {result}"
 
     # Ensure we can NOT get the MR branch
     with pytest.raises(gitlab.exceptions.GitlabGetError):
@@ -156,8 +160,10 @@ def test_merge_request_large_commit_message(
 
     mr.merge(merge_commit_message=merge_commit_message)
 
-    result = wait_for_sidekiq(timeout=60)
-    assert result is True, "sidekiq process should have terminated but did not"
+    result = wait_for_sidekiq()
+    assert (
+        result.success is True
+    ), f"sidekiq process should have terminated but did not: {result}"
 
     # Wait until it is merged
     mr_iid = mr.iid
@@ -196,8 +202,10 @@ def test_merge_request_merge_ref_should_fail(
             "commit_message": "Another commit in main branch",
         }
     )
-    result = wait_for_sidekiq(timeout=60)
-    assert result is True, "sidekiq process should have terminated but did not"
+    result = wait_for_sidekiq()
+    assert (
+        result.success is True
+    ), f"sidekiq process should have terminated but did not: {result}"
 
     # Check for non-existing merge_ref for MR with conflicts
     with pytest.raises(gitlab.exceptions.GitlabGetError):
diff --git a/tests/functional/api/test_projects.py b/tests/functional/api/test_projects.py
@@ -329,3 +329,5 @@ def test_project_groups_list(gl, group):
     groups = project.groups.list()
     group_ids = set([x.id for x in groups])
     assert set((group.id, group2.id)) == group_ids
+    # NOTE: `group` will automatically be deleted by the fixture, but not `group2`
+    group2.delete()
diff --git a/tests/functional/api/test_users.py b/tests/functional/api/test_users.py
@@ -47,8 +47,10 @@ def test_delete_user(gl, wait_for_sidekiq):
     )
 
     new_user.delete()
-    result = wait_for_sidekiq(timeout=60)
-    assert result is True, "sidekiq process should have terminated but did not"
+    result = wait_for_sidekiq()
+    assert (
+        result.success is True
+    ), f"sidekiq process should have terminated but did not: {result}"
 
     assert new_user.id not in [user.id for user in gl.users.list()]
 
diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py
@@ -1,3 +1,4 @@
+import dataclasses
 import tempfile
 import time
 import uuid
@@ -105,25 +106,52 @@ def _check(container):
     return _check
 
 
+@dataclasses.dataclass
+class WaitSidekiq:
+    iterations: int
+    step: float
+    elapsed_time: float
+    success: bool
+
+
 @pytest.fixture
-def wait_for_sidekiq(gl):
+def wait_for_sidekiq(gl: gitlab.Gitlab):
     """
     Return a helper function to wait until there are no busy sidekiq processes.
 
     Use this with asserts for slow tasks (group/project/user creation/deletion).
     """
+    return _wait_for_sidekiq(gl=gl)
+
+
+def _wait_for_sidekiq(gl: gitlab.Gitlab):
+    """
+    Return a helper function to wait until there are no busy sidekiq processes.
 
-    def _wait(timeout=30, step=0.5):
-        for _ in range(timeout):
+    Use this with asserts for slow tasks (group/project/user creation/deletion).
+    """
+
+    def _wait(timeout: int = 60, step: float = 0.1) -> WaitSidekiq:
+        # timeout is approximately the timeout in seconds
+        max_iterations = int(timeout / step)
+        start_time = time.perf_counter()
+        success = False
+        for count in range(max_iterations):
             time.sleep(step)
             busy = False
             processes = gl.sidekiq.process_metrics()["processes"]
             for process in processes:
                 if process["busy"]:
                     busy = True
             if not busy:
-                return True
-        return False
+                success = True
+                break
+        return WaitSidekiq(
+            iterations=count,
+            step=step,
+            elapsed_time=time.perf_counter() - start_time,
+            success=success,
+        )
 
     return _wait
 
@@ -210,6 +238,10 @@ def group(gl):
         group.delete()
     except gitlab.exceptions.GitlabDeleteError as e:
         print(f"Group already deleted: {e}")
+    result = _wait_for_sidekiq(gl=gl)()
+    assert (
+        result.success is True
+    ), f"sidekiq process should have terminated but did not: {result}"
 
 
 @pytest.fixture(scope="module")
@@ -226,6 +258,10 @@ def project(gl):
         project.delete()
     except gitlab.exceptions.GitlabDeleteError as e:
         print(f"Project already deleted: {e}")
+    result = _wait_for_sidekiq(gl=gl)()
+    assert (
+        result.success is True
+    ), f"sidekiq process should have terminated but did not: {result}"
 
 
 @pytest.fixture(scope="function")
@@ -249,8 +285,10 @@ def _merge_request(*, source_branch: str):
         # NOTE(jlvillal): Sometimes the CI would give a "500 Internal Server
         # Error". Hoping that waiting until all other processes are done will
         # help with that.
-        result = wait_for_sidekiq(timeout=60)
-        assert result is True, "sidekiq process should have terminated but did not"
+        result = wait_for_sidekiq()
+        assert (
+            result.success is True
+        ), f"sidekiq process should have terminated but did not: {result}"
 
         project.refresh()  # Gets us the current default branch
         project.branches.create(
@@ -274,8 +312,10 @@ def _merge_request(*, source_branch: str):
                 "remove_source_branch": True,
             }
         )
-        result = wait_for_sidekiq(timeout=60)
-        assert result is True, "sidekiq process should have terminated but did not"
+        result = wait_for_sidekiq()
+        assert (
+            result.success is True
+        ), f"sidekiq process should have terminated but did not: {result}"
 
         mr_iid = mr.iid
         for _ in range(60):
@@ -297,6 +337,10 @@ def _merge_request(*, source_branch: str):
         except gitlab.exceptions.GitlabDeleteError:
             # Ignore if branch was already deleted
             pass
+    result = wait_for_sidekiq()
+    assert (
+        result.success is True
+    ), f"sidekiq process should have terminated but did not: {result}"
 
 
 @pytest.fixture(scope="module")
@@ -349,6 +393,10 @@ def user(gl):
         user.delete()
     except gitlab.exceptions.GitlabDeleteError as e:
         print(f"User already deleted: {e}")
+    result = _wait_for_sidekiq(gl=gl)()
+    assert (
+        result.success is True
+    ), f"sidekiq process should have terminated but did not: {result}"
 
 
 @pytest.fixture(scope="module")
