chore: update 'wait_for_sidekiq' function & use it more

JohnVillalovos · JohnVillalovos · commit fa2b2c10d73a · 2021-12-28T01:18:33.000-08:00
DRAFT: actually working on not using `wait_for_sidekiq`

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
@@ -101,7 +101,7 @@ def test_merge_request_merge(project):
 
 
 def test_merge_request_should_remove_source_branch(
-    project, merge_request, wait_for_sidekiq
+    project, merge_request, ensure_deleted
 ) -> None:
     """Test to ensure
     https://github.com/python-gitlab/python-gitlab/issues/1120 is fixed.
@@ -113,9 +113,6 @@ 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"
-
     # Wait until it is merged
     mr_iid = mr.iid
     for _ in range(60):
@@ -124,24 +121,16 @@ def test_merge_request_should_remove_source_branch(
             break
         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"
-
-    # Ensure we can NOT get the MR branch
-    with pytest.raises(gitlab.exceptions.GitlabGetError):
-        result = project.branches.get(source_branch)
-        # Help to debug in case the expected exception doesn't happen.
-        import pprint
 
-        print("mr:", pprint.pformat(mr))
-        print("mr.merged_at:", pprint.pformat(mr.merged_at))
-        print("result:", pprint.pformat(result))
+    # Ensure the MR branch is deleted
+    ensure_deleted(
+        pg_object=project.branches,
+        object_id=source_branch,
+        description="Project branch",
+    )
 
 
-def test_merge_request_large_commit_message(
-    project, merge_request, wait_for_sidekiq
-) -> None:
+def test_merge_request_large_commit_message(project, merge_request) -> None:
     """Test to ensure https://github.com/python-gitlab/python-gitlab/issues/1452
     is fixed.
     Bug reported that very long 'merge_commit_message' in mr.merge() would
@@ -156,9 +145,6 @@ 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"
-
     # Wait until it is merged
     mr_iid = mr.iid
     for _ in range(60):
@@ -182,7 +168,7 @@ def test_merge_request_merge_ref(merge_request) -> None:
 
 
 def test_merge_request_merge_ref_should_fail(
-    project, merge_request, wait_for_sidekiq
+    project, merge_request, ensure_raises_exception
 ) -> None:
     source_branch = "merge_ref_test2"
     mr = merge_request(source_branch=source_branch)
@@ -196,9 +182,14 @@ 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"
 
+    ensure_raises_exception(
+        func=mr.merge_ref,
+        exception=gitlab.exceptions.GitlabGetError,
+        args=[],
+        kwargs={},
+        description="mr.merge_ref()",
+    )
     # Check for non-existing merge_ref for MR with conflicts
     with pytest.raises(gitlab.exceptions.GitlabGetError):
         response = mr.merge_ref()
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
@@ -36,7 +36,7 @@ def test_block_user(gl, user):
     assert user in users
 
 
-def test_delete_user(gl, wait_for_sidekiq):
+def test_delete_user(gl, ensure_deleted):
     new_user = gl.users.create(
         {
             "email": "delete-user@test.com",
@@ -45,11 +45,11 @@ def test_delete_user(gl, wait_for_sidekiq):
             "password": "delete-user-pass",
         }
     )
+    new_user_id = new_user.id
 
     new_user.delete()
-    result = wait_for_sidekiq(timeout=60)
-    assert result is True, "sidekiq process should have terminated but did not"
 
+    ensure_deleted(pg_object=gl.users, object_id=new_user_id, description="User")
     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,8 +1,10 @@
+import dataclasses
 import tempfile
 import time
 import uuid
 from pathlib import Path
 from subprocess import check_output
+from typing import Any, Dict, List
 
 import pytest
 
@@ -30,6 +32,34 @@ def reset_gitlab(gl):
         if user.username != "root":
             user.delete(hard_delete=True)
 
+    sleep_interval = 0.1
+    timeout = 60
+    max_iterations = int(timeout / sleep_interval)
+
+    # Ensure everything is deleted.
+    start_time = time.perf_counter()
+
+    def ensure_list_empty(pg_object, description: str) -> None:
+        for _ in range(max_iterations):
+            if pg_object.list() == []:
+                break
+            time.sleep(sleep_interval)
+        assert (
+            pg_object.list() == []
+        ), f"Did not delete all {description}. Elapsed_time: {time.perf_counter() - start_time}"
+
+    ensure_list_empty(pg_object=gl.projects, description="projects")
+    ensure_list_empty(pg_object=gl.groups, description="groups")
+    ensure_list_empty(pg_object=gl.variables, description="variables")
+
+    for _ in range(max_iterations):
+        if len(gl.users.list()) <= 1:
+            break
+        time.sleep(sleep_interval)
+    assert (
+        len(gl.users.list()) <= 1
+    ), f"elapsed_time: {time.perf_counter() - start_time}"
+
 
 def set_token(container, fixture_dir):
     set_token_rb = fixture_dir / "set_token.rb"
@@ -105,25 +135,59 @@ 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.
 
     Use this with asserts for slow tasks (group/project/user creation/deletion).
     """
 
-    def _wait(timeout=30, step=0.5):
-        for _ in range(timeout):
+    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):
+            print(f"_wait: count: {count}")
             time.sleep(step)
             busy = False
             processes = gl.sidekiq.process_metrics()["processes"]
             for process in processes:
+                print(f"_wait: process['busy']: {process['busy']}")
                 if process["busy"]:
                     busy = True
             if not busy:
-                return True
-        return False
+                success = True
+                break
+        result = WaitSidekiq(
+            iterations=count,
+            step=step,
+            elapsed_time=time.perf_counter() - start_time,
+            success=success,
+        )
+        print(f"_wait: {result}")
+        return result
 
     return _wait
 
@@ -194,6 +258,73 @@ def gitlab_runner(gl):
     check_output(docker_exec + unregister).decode()
 
 
+@pytest.fixture
+def ensure_raises_exception():
+    """
+    Return a helper function to wait until specified exception is raised
+
+    Use this with asserts for slow tasks (group/project/user creation/deletion).
+    """
+    return _ensure_raises_exception_wrapped()
+
+
+def _ensure_raises_exception(
+    *,
+    func,
+    exception: Exception,
+    args: List[Any],
+    kwargs: Dict[str, Any],
+    description: str,
+) -> None:
+    """Ensure the exception is specified when calling `func`. If no exception is raided
+    after timeout period, fail the test"""
+    sleep_interval = 0.1
+    timeout = 60
+    max_iterations = int(timeout / sleep_interval)
+
+    for _ in range(max_iterations):
+        try:
+            func(*args, **kwargs)
+        except exception:
+            return
+        time.sleep(sleep_interval)
+    pytest.fail(f"{description} did not raise exception {exception}")
+
+
+def _ensure_raises_exception_wrapped():
+    return _ensure_raises_exception
+
+
+@pytest.fixture
+def ensure_deleted():
+    """
+    Return a helper function to wait until specified object is deleted
+
+    Use this with asserts for slow tasks (group/project/user creation/deletion).
+    """
+    return _ensure_deleted_wrapped()
+
+
+def _ensure_deleted(*, pg_object, object_id: int, description: str) -> None:
+    """Ensure the object specified can not be retrieved. If object still exists after
+    timeout period, fail the test"""
+    sleep_interval = 0.1
+    timeout = 60
+    max_iterations = int(timeout / sleep_interval)
+
+    for _ in range(max_iterations):
+        try:
+            pg_object.get(object_id)
+        except gitlab.exceptions.GitlabGetError:
+            return
+        time.sleep(sleep_interval)
+    pytest.fail(f"{description} {object_id} was not deleted")
+
+
+def _ensure_deleted_wrapped():
+    return _ensure_deleted
+
+
 @pytest.fixture(scope="module")
 def group(gl):
     """Group fixture for group API resource tests."""
@@ -203,6 +334,7 @@ def group(gl):
         "path": f"group-{_id}",
     }
     group = gl.groups.create(data)
+    group_id = group.id
 
     yield group
 
@@ -211,6 +343,8 @@ def group(gl):
     except gitlab.exceptions.GitlabDeleteError as e:
         print(f"Group already deleted: {e}")
 
+    _ensure_deleted(pg_object=gl.groups, object_id=group_id, description="Group")
+
 
 @pytest.fixture(scope="module")
 def project(gl):
@@ -219,6 +353,7 @@ def project(gl):
     name = f"test-project-{_id}"
 
     project = gl.projects.create(name=name)
+    project_id = project.id
 
     yield project
 
@@ -227,6 +362,8 @@ def project(gl):
     except gitlab.exceptions.GitlabDeleteError as e:
         print(f"Project already deleted: {e}")
 
+    _ensure_deleted(pg_object=gl.projects, object_id=project_id, description="Project")
+
 
 @pytest.fixture(scope="function")
 def merge_request(project, wait_for_sidekiq):
@@ -249,8 +386,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 +413,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):
@@ -298,6 +439,18 @@ def _merge_request(*, source_branch: str):
             # Ignore if branch was already deleted
             pass
 
+    for mr_iid, source_branch in to_delete:
+        _ensure_deleted(
+            pg_object=project.mergerequests,
+            object_id=mr_iid,
+            description="Project mergerequest",
+        )
+        _ensure_deleted(
+            pg_object=project.branches,
+            object_id=source_branch,
+            description="Project branch",
+        )
+
 
 @pytest.fixture(scope="module")
 def project_file(project):
@@ -342,6 +495,7 @@ def user(gl):
     password = "fakepassword"
 
     user = gl.users.create(email=email, username=username, name=name, password=password)
+    user_id = user.id
 
     yield user
 
@@ -350,6 +504,8 @@ def user(gl):
     except gitlab.exceptions.GitlabDeleteError as e:
         print(f"User already deleted: {e}")
 
+    _ensure_deleted(pg_object=gl.users, object_id=user_id, description="User")
+
 
 @pytest.fixture(scope="module")
 def issue(project):

Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ def test_block_user(gl, user):`
`36`	`36`	`assert user in users`
`37`	`37`
`38`	`38`
`39`		`-def test_delete_user(gl, wait_for_sidekiq):`
	`39`	`+def test_delete_user(gl, ensure_deleted):`
`40`	`40`	`new_user = gl.users.create(`
`41`	`41`	`{`
`42`	`42`	`"email": "delete-user@test.com",`
`@@ -45,11 +45,11 @@ def test_delete_user(gl, wait_for_sidekiq):`
`45`	`45`	`"password": "delete-user-pass",`
`46`	`46`	`}`
`47`	`47`	`)`
	`48`	`+ new_user_id = new_user.id`
`48`	`49`
`49`	`50`	`new_user.delete()`
`50`		`- result = wait_for_sidekiq(timeout=60)`
`51`		`- assert result is True, "sidekiq process should have terminated but did not"`
`52`	`51`
	`52`	`+ ensure_deleted(pg_object=gl.users, object_id=new_user_id, description="User")`
`53`	`53`	`assert new_user.id not in [user.id for user in gl.users.list()]`
`54`	`54`
`55`	`55`