Skip to content

Commit ef88f48

Browse files
JohnVillalovosJohnVillalovos
committed
test(functional): replace merge-status sleeps with polling helper
Trying to make the tests less flaky. Have seen some issues with tests needing to be manually re-run due to failures. `test_accept_request_merge` used a blind `time.sleep(30)` before attempting to merge, which was flaky whenever GitLab hadn't finished evaluating mergeability by the time the sleep ended, causing intermittent CI failures with `GitlabMRClosedError: Branch cannot be merged`. Add `helpers.poll_until()`, a generic poll-until-condition-or-fail helper, and use it to replace the blind sleep with a check on `detailed_merge_status`, plus the duplicated `merged_at` polling loops in `test_merge_requests.py` and the `_make_merge_request` fixture in conftest.py. Assisted-by: Claude Sonnet 5
1 parent 39dc990 commit ef88f48

4 files changed

Lines changed: 66 additions & 23 deletions

File tree

‎tests/functional/api/test_merge_requests.py‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
import gitlab
77
import gitlab.v4.objects
8+
import tests.functional.helpers
89

910

1011
def test_merge_requests(project):
@@ -222,11 +223,16 @@ def test_merge_request_should_remove_source_branch(project, merge_request) -> No
222223
# Wait until it is merged
223224
mr = None
224225
mr_iid = merge_request.iid
225-
for _ in range(60):
226+
227+
def _merge_completed() -> bool:
228+
nonlocal mr
226229
mr = project.mergerequests.get(mr_iid)
227-
if mr.merged_at is not None:
228-
break
229-
time.sleep(0.5)
230+
return mr.merged_at is not None
231+
232+
tests.functional.helpers.poll_until(
233+
condition=_merge_completed,
234+
description=f"merge request {mr_iid} merged_at to be set",
235+
)
230236

231237
assert mr is not None
232238
assert mr.merged_at is not None
@@ -264,11 +270,16 @@ def test_merge_request_large_commit_message(project, merge_request) -> None:
264270
# Wait until it is merged
265271
mr = None
266272
mr_iid = merge_request.iid
267-
for _ in range(60):
273+
274+
def _merge_completed() -> bool:
275+
nonlocal mr
268276
mr = project.mergerequests.get(mr_iid)
269-
if mr.merged_at is not None:
270-
break
271-
time.sleep(0.5)
277+
return mr.merged_at is not None
278+
279+
tests.functional.helpers.poll_until(
280+
condition=_merge_completed,
281+
description=f"merge request {mr_iid} merged_at to be set",
282+
)
272283

273284
assert mr is not None
274285
assert mr.merged_at is not None

‎tests/functional/cli/test_cli_v4.py‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import datetime
22
import os
3-
import time
3+
4+
import tests.functional.helpers
45

56
branch = "BRANCH-cli-v4"
67

@@ -240,8 +241,18 @@ def test_accept_request_merge(gitlab_cli, project):
240241
"commit_message": "chore: test-cli-v4 change",
241242
}
242243
project.files.create(file_data)
243-
# Pause to let GL catch up (happens on hosted too, sometimes takes a while for server to be ready to merge)
244-
time.sleep(30)
244+
245+
def _merge_status_settled() -> bool:
246+
nonlocal mr
247+
mr = project.mergerequests.get(mr.iid)
248+
return mr.detailed_merge_status not in ("checking", "unchecked")
249+
250+
# Wait until GitLab has finished evaluating mergeability instead of a
251+
# blind sleep, so the test fails fast if it never becomes mergeable.
252+
tests.functional.helpers.poll_until(
253+
condition=_merge_status_settled,
254+
description=f"merge request {mr.iid} detailed_merge_status to settle",
255+
)
245256

246257
approve_cmd = [
247258
"project-merge-request",

‎tests/functional/conftest.py‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -413,19 +413,17 @@ def _make_merge_request(*, source_branch: str, create_pipeline: bool = False):
413413
}
414414
)
415415

416-
# Pause to let GL catch up (happens on hosted too, sometimes takes a while for server to be ready to merge)
417-
time.sleep(5)
418-
419416
mr_iid = mr.iid
420-
for _ in range(60):
417+
418+
def _merge_status_settled() -> bool:
419+
nonlocal mr
421420
mr = project.mergerequests.get(mr_iid)
422-
if (
423-
mr.detailed_merge_status == "checking"
424-
or mr.detailed_merge_status == "unchecked"
425-
):
426-
time.sleep(0.5)
427-
else:
428-
break
421+
return mr.detailed_merge_status not in ("checking", "unchecked")
422+
423+
helpers.poll_until(
424+
condition=_merge_status_settled,
425+
description=f"merge request {mr_iid} detailed_merge_status to settle",
426+
)
429427

430428
assert mr.detailed_merge_status != "checking"
431429
assert mr.detailed_merge_status != "unchecked"

‎tests/functional/helpers.py‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
import logging
44
import time
5-
from typing import TYPE_CHECKING
5+
from typing import Callable, TYPE_CHECKING
66

77
import pytest
88

@@ -29,6 +29,29 @@ def get_gitlab_plan(gl: gitlab.Gitlab) -> str | None:
2929
return license["plan"]
3030

3131

32+
def poll_until(
33+
*,
34+
condition: Callable[[], bool],
35+
description: str,
36+
timeout: float = TIMEOUT,
37+
interval: float = SLEEP_INTERVAL,
38+
) -> None:
39+
"""Repeatedly call `condition` until it returns truthy, sleeping `interval`
40+
seconds between attempts. Fails the test via `pytest.fail` if `timeout`
41+
seconds elapse first, so callers can replace a blind `time.sleep()` "let
42+
GitLab catch up" pause with a check that fails fast when something is
43+
actually wrong instead of always waiting the full duration.
44+
"""
45+
# Use a monotonic deadline rather than counting iterations so the timeout
46+
# is accurate even if `condition()` itself is slow (e.g. a slow API call).
47+
deadline = time.monotonic() + timeout
48+
while time.monotonic() < deadline:
49+
if condition():
50+
return
51+
time.sleep(interval)
52+
pytest.fail(f"Timed out after {timeout}s waiting for: {description}")
53+
54+
3255
def safe_delete(object: gitlab.base.RESTObject) -> None:
3356
"""Ensure the object specified can not be retrieved. If object still exists after
3457
timeout period, fail the test"""

0 commit comments

Comments
 (0)