Skip to content

Commit 0bcfec0

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 b6e65dd commit 0bcfec0

4 files changed

Lines changed: 124 additions & 26 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):
@@ -229,11 +230,16 @@ def test_merge_request_should_remove_source_branch(project, merge_request) -> No
229230
# Wait until it is merged
230231
mr = None
231232
mr_iid = merge_request.iid
232-
for _ in range(60):
233+
234+
def _merge_completed() -> bool:
235+
nonlocal mr
233236
mr = project.mergerequests.get(mr_iid)
234-
if mr.merged_at is not None:
235-
break
236-
time.sleep(0.5)
237+
return mr.merged_at is not None
238+
239+
tests.functional.helpers.poll_until(
240+
condition=_merge_completed,
241+
description=f"merge request {mr_iid} merged_at to be set",
242+
)
237243

238244
assert mr is not None
239245
assert mr.merged_at is not None
@@ -271,11 +277,16 @@ def test_merge_request_large_commit_message(project, merge_request) -> None:
271277
# Wait until it is merged
272278
mr = None
273279
mr_iid = merge_request.iid
274-
for _ in range(60):
280+
281+
def _merge_completed() -> bool:
282+
nonlocal mr
275283
mr = project.mergerequests.get(mr_iid)
276-
if mr.merged_at is not None:
277-
break
278-
time.sleep(0.5)
284+
return mr.merged_at is not None
285+
286+
tests.functional.helpers.poll_until(
287+
condition=_merge_completed,
288+
description=f"merge request {mr_iid} merged_at to be set",
289+
)
279290

280291
assert mr is not None
281292
assert mr.merged_at is not None

‎tests/functional/cli/test_cli_v4.py‎

Lines changed: 56 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
import datetime
2+
import logging
23
import os
3-
import time
4+
5+
import gitlab.const
6+
import tests.functional.helpers
47

58
branch = "BRANCH-cli-v4"
69

@@ -240,8 +243,30 @@ def test_accept_request_merge(gitlab_cli, project):
240243
"commit_message": "chore: test-cli-v4 change",
241244
}
242245
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)
246+
247+
def _mergeable() -> bool:
248+
nonlocal mr
249+
mr = project.mergerequests.get(mr.iid)
250+
mr_status = mr.detailed_merge_status
251+
assert isinstance(mr_status, str)
252+
if mr_status != gitlab.const.DetailedMergeStatus.MERGEABLE:
253+
logging.info(
254+
f"merge request {mr.iid} not yet mergeable: "
255+
f"detailed_merge_status={mr_status!r}"
256+
)
257+
return False
258+
return True
259+
260+
# Wait until GitLab reports the MR as actually mergeable instead of a
261+
# blind sleep. Other non-"checking"/"unchecked" statuses (e.g.
262+
# "broken_status") can appear transiently before GitLab finishes
263+
# re-evaluating mergeability after the commit above, so we wait for the
264+
# positive result rather than merely "not still checking".
265+
tests.functional.helpers.poll_until(
266+
condition=_mergeable,
267+
description=f"merge request {mr.iid} to become mergeable",
268+
interval=1,
269+
)
245270

246271
approve_cmd = [
247272
"project-merge-request",
@@ -253,7 +278,34 @@ def test_accept_request_merge(gitlab_cli, project):
253278
]
254279
ret = gitlab_cli(approve_cmd)
255280

256-
assert ret.success
281+
def _merge_succeeds() -> bool:
282+
nonlocal ret
283+
if not ret.success:
284+
mr_after_failure = project.mergerequests.get(mr.iid)
285+
logging.info(
286+
f"merge request {mr.iid} merge attempt failed (will retry): "
287+
f"stderr={ret.stderr!r}; state={mr_after_failure.state!r} "
288+
f"merged_at={mr_after_failure.merged_at!r} "
289+
f"detailed_merge_status={mr_after_failure.detailed_merge_status!r}"
290+
)
291+
ret = gitlab_cli(approve_cmd)
292+
return bool(ret.success)
293+
294+
# Even once GitLab reports the MR as mergeable, the merge endpoint has
295+
# returned 405 immediately afterward in CI. Retry a few times, logging
296+
# the MR's actual state on each failure, so we can tell a transient
297+
# rejection (still "opened") apart from something already merged/closed.
298+
tests.functional.helpers.poll_until(
299+
condition=_merge_succeeds,
300+
description=f"merge request {mr.iid} merge command to succeed",
301+
timeout=30,
302+
interval=2,
303+
)
304+
305+
assert ret.success, (
306+
f"merge request {mr.iid} failed to merge after retries: "
307+
f"stdout={ret.stdout!r} stderr={ret.stderr!r}"
308+
)
257309

258310

259311
def test_create_project_label(gitlab_cli, project):

‎tests/functional/conftest.py‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -413,22 +413,34 @@ 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)
421+
mr_status = mr.detailed_merge_status
422+
assert isinstance(mr_status, str)
423+
if create_pipeline:
424+
# The pipeline's `sleep 24h` never finishes, so this MR is
425+
# never expected to become truly mergeable. Just wait until
426+
# GitLab is done with its initial check.
427+
settled = mr_status not in ("checking", "unchecked")
427428
else:
428-
break
429-
430-
assert mr.detailed_merge_status != "checking"
431-
assert mr.detailed_merge_status != "unchecked"
429+
# Wait for the positive result rather than merely "not still
430+
# checking": other statuses (e.g. "broken_status") can appear
431+
# transiently before GitLab finishes evaluating mergeability.
432+
settled = mr_status == gitlab.const.DetailedMergeStatus.MERGEABLE
433+
if not settled:
434+
logging.info(
435+
f"merge request {mr_iid} detailed_merge_status not yet "
436+
f"settled: {mr_status!r}"
437+
)
438+
return settled
439+
440+
helpers.poll_until(
441+
condition=_merge_status_settled,
442+
description=f"merge request {mr_iid} detailed_merge_status to settle",
443+
)
432444

433445
to_delete.extend([mr, mr_branch])
434446
return mr

‎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)