diff --git a/tests/functional/api/test_merge_requests.py b/tests/functional/api/test_merge_requests.py index f7a56721b..a3e17c767 100644 --- a/tests/functional/api/test_merge_requests.py +++ b/tests/functional/api/test_merge_requests.py @@ -5,6 +5,7 @@ import gitlab import gitlab.v4.objects +import tests.functional.helpers def test_merge_requests(project): @@ -229,11 +230,16 @@ def test_merge_request_should_remove_source_branch(project, merge_request) -> No # Wait until it is merged mr = None mr_iid = merge_request.iid - for _ in range(60): + + def _merge_completed() -> bool: + nonlocal mr mr = project.mergerequests.get(mr_iid) - if mr.merged_at is not None: - break - time.sleep(0.5) + return mr.merged_at is not None + + tests.functional.helpers.poll_until( + condition=_merge_completed, + description=f"merge request {mr_iid} merged_at to be set", + ) assert mr is not None assert mr.merged_at is not None @@ -271,11 +277,16 @@ def test_merge_request_large_commit_message(project, merge_request) -> None: # Wait until it is merged mr = None mr_iid = merge_request.iid - for _ in range(60): + + def _merge_completed() -> bool: + nonlocal mr mr = project.mergerequests.get(mr_iid) - if mr.merged_at is not None: - break - time.sleep(0.5) + return mr.merged_at is not None + + tests.functional.helpers.poll_until( + condition=_merge_completed, + description=f"merge request {mr_iid} merged_at to be set", + ) assert mr is not None assert mr.merged_at is not None diff --git a/tests/functional/cli/test_cli_v4.py b/tests/functional/cli/test_cli_v4.py index 189881207..916dbc21c 100644 --- a/tests/functional/cli/test_cli_v4.py +++ b/tests/functional/cli/test_cli_v4.py @@ -1,6 +1,9 @@ import datetime +import logging import os -import time + +import gitlab.const +import tests.functional.helpers branch = "BRANCH-cli-v4" @@ -240,8 +243,30 @@ def test_accept_request_merge(gitlab_cli, project): "commit_message": "chore: test-cli-v4 change", } project.files.create(file_data) - # Pause to let GL catch up (happens on hosted too, sometimes takes a while for server to be ready to merge) - time.sleep(30) + + def _mergeable() -> bool: + nonlocal mr + mr = project.mergerequests.get(mr.iid) + mr_status = mr.detailed_merge_status + assert isinstance(mr_status, str) + if mr_status != gitlab.const.DetailedMergeStatus.MERGEABLE: + logging.info( + f"merge request {mr.iid} not yet mergeable: " + f"detailed_merge_status={mr_status!r}" + ) + return False + return True + + # Wait until GitLab reports the MR as actually mergeable instead of a + # blind sleep. Other non-"checking"/"unchecked" statuses (e.g. + # "broken_status") can appear transiently before GitLab finishes + # re-evaluating mergeability after the commit above, so we wait for the + # positive result rather than merely "not still checking". + tests.functional.helpers.poll_until( + condition=_mergeable, + description=f"merge request {mr.iid} to become mergeable", + interval=1, + ) approve_cmd = [ "project-merge-request", @@ -253,7 +278,34 @@ def test_accept_request_merge(gitlab_cli, project): ] ret = gitlab_cli(approve_cmd) - assert ret.success + def _merge_succeeds() -> bool: + nonlocal ret + if not ret.success: + mr_after_failure = project.mergerequests.get(mr.iid) + logging.info( + f"merge request {mr.iid} merge attempt failed (will retry): " + f"stderr={ret.stderr!r}; state={mr_after_failure.state!r} " + f"merged_at={mr_after_failure.merged_at!r} " + f"detailed_merge_status={mr_after_failure.detailed_merge_status!r}" + ) + ret = gitlab_cli(approve_cmd) + return bool(ret.success) + + # Even once GitLab reports the MR as mergeable, the merge endpoint has + # returned 405 immediately afterward in CI. Retry a few times, logging + # the MR's actual state on each failure, so we can tell a transient + # rejection (still "opened") apart from something already merged/closed. + tests.functional.helpers.poll_until( + condition=_merge_succeeds, + description=f"merge request {mr.iid} merge command to succeed", + timeout=30, + interval=2, + ) + + assert ret.success, ( + f"merge request {mr.iid} failed to merge after retries: " + f"stdout={ret.stdout!r} stderr={ret.stderr!r}" + ) def test_create_project_label(gitlab_cli, project): diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py index 11330732d..ae8bb1002 100644 --- a/tests/functional/conftest.py +++ b/tests/functional/conftest.py @@ -413,22 +413,34 @@ def _make_merge_request(*, source_branch: str, create_pipeline: bool = False): } ) - # Pause to let GL catch up (happens on hosted too, sometimes takes a while for server to be ready to merge) - time.sleep(5) - mr_iid = mr.iid - for _ in range(60): + + def _merge_status_settled() -> bool: + nonlocal mr mr = project.mergerequests.get(mr_iid) - if ( - mr.detailed_merge_status == "checking" - or mr.detailed_merge_status == "unchecked" - ): - time.sleep(0.5) + mr_status = mr.detailed_merge_status + assert isinstance(mr_status, str) + if create_pipeline: + # The pipeline's `sleep 24h` never finishes, so this MR is + # never expected to become truly mergeable. Just wait until + # GitLab is done with its initial check. + settled = mr_status not in ("checking", "unchecked") else: - break - - assert mr.detailed_merge_status != "checking" - assert mr.detailed_merge_status != "unchecked" + # Wait for the positive result rather than merely "not still + # checking": other statuses (e.g. "broken_status") can appear + # transiently before GitLab finishes evaluating mergeability. + settled = mr_status == gitlab.const.DetailedMergeStatus.MERGEABLE + if not settled: + logging.info( + f"merge request {mr_iid} detailed_merge_status not yet " + f"settled: {mr_status!r}" + ) + return settled + + helpers.poll_until( + condition=_merge_status_settled, + description=f"merge request {mr_iid} detailed_merge_status to settle", + ) to_delete.extend([mr, mr_branch]) return mr diff --git a/tests/functional/helpers.py b/tests/functional/helpers.py index 9d313e540..493362194 100644 --- a/tests/functional/helpers.py +++ b/tests/functional/helpers.py @@ -2,7 +2,7 @@ import logging import time -from typing import TYPE_CHECKING +from typing import Callable, TYPE_CHECKING import pytest @@ -29,6 +29,29 @@ def get_gitlab_plan(gl: gitlab.Gitlab) -> str | None: return license["plan"] +def poll_until( + *, + condition: Callable[[], bool], + description: str, + timeout: float = TIMEOUT, + interval: float = SLEEP_INTERVAL, +) -> None: + """Repeatedly call `condition` until it returns truthy, sleeping `interval` + seconds between attempts. Fails the test via `pytest.fail` if `timeout` + seconds elapse first, so callers can replace a blind `time.sleep()` "let + GitLab catch up" pause with a check that fails fast when something is + actually wrong instead of always waiting the full duration. + """ + # Use a monotonic deadline rather than counting iterations so the timeout + # is accurate even if `condition()` itself is slow (e.g. a slow API call). + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if condition(): + return + time.sleep(interval) + pytest.fail(f"Timed out after {timeout}s waiting for: {description}") + + def safe_delete(object: gitlab.base.RESTObject) -> None: """Ensure the object specified can not be retrieved. If object still exists after timeout period, fail the test"""