From fb803019ffd3d8af473ce42280b3216358817fe2 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Tue, 7 Jan 2025 17:08:07 +1100 Subject: [PATCH 01/17] conftest: dos2unix conversion --- src/lando/tests/conftest.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/lando/tests/conftest.py b/src/lando/tests/conftest.py index c031804d..c9cd6cb0 100644 --- a/src/lando/tests/conftest.py +++ b/src/lando/tests/conftest.py @@ -1,13 +1,13 @@ -import pytest -from django.core.management import call_command - - -@pytest.fixture -def lando_version(): - # The version file may or may not exist in the testing environment. - # We'll explicitly generate it to ensure it's there. - call_command("generate_version_file") - - from lando.version import version - - return version +import pytest +from django.core.management import call_command + + +@pytest.fixture +def lando_version(): + # The version file may or may not exist in the testing environment. + # We'll explicitly generate it to ensure it's there. + call_command("generate_version_file") + + from lando.version import version + + return version From b4bd0ed93541ff845dc446e58e171ce041b6aaf2 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Mon, 6 Jan 2025 16:57:22 +1100 Subject: [PATCH 02/17] test-landings: add hg_repo_mc fixture --- src/lando/api/tests/test_landings.py | 239 +++++++++------------------ 1 file changed, 76 insertions(+), 163 deletions(-) diff --git a/src/lando/api/tests/test_landings.py b/src/lando/api/tests/test_landings.py index e984702b..f737f7ae 100644 --- a/src/lando/api/tests/test_landings.py +++ b/src/lando/api/tests/test_landings.py @@ -15,7 +15,7 @@ add_job_with_revisions, ) from lando.main.models.revision import Revision -from lando.main.scm import SCM_TYPE_HG, HgSCM +from lando.main.scm import SCM_TYPE_HG from lando.main.scm.hg import LostPushRace from lando.utils import HgPatchHelper @@ -226,6 +226,20 @@ def _create_patch_revision(number, patch=normal_patch_0): """.lstrip() +@pytest.fixture() +def hg_repo_mc(hg_server, hg_clone) -> Repo: + repo = Repo.objects.create( + scm_type=SCM_TYPE_HG, + name="mozilla-central", + required_permission=SCM_LEVEL_3, + url=hg_server, + push_path=hg_server, + pull_path=hg_server, + system_path=hg_clone.strpath, + ) + return repo + + @pytest.mark.parametrize( "revisions_params", [ @@ -238,23 +252,15 @@ def _create_patch_revision(number, patch=normal_patch_0): ) @pytest.mark.django_db def test_integrated_execute_job( - hg_server, - hg_clone, + hg_repo_mc, treestatusdouble, monkeypatch, create_patch_revision, revisions_params, ): - treestatusdouble.open_tree("mozilla-central") - repo = Repo.objects.create( - scm_type=SCM_TYPE_HG, - name="mozilla-central", - url=hg_server, - required_permission=SCM_LEVEL_3, - push_path=hg_server, - pull_path=hg_server, - system_path=hg_clone.strpath, - ) + repo = hg_repo_mc + treestatusdouble.open_tree(repo.name) + revisions = [ create_patch_revision(number, **kwargs) for number, kwargs in revisions_params ] @@ -285,23 +291,14 @@ def test_integrated_execute_job( @pytest.mark.django_db def test_integrated_execute_job_with_force_push( - hg_server, - hg_clone, + hg_repo_mc, treestatusdouble, monkeypatch, create_patch_revision, ): - treestatusdouble.open_tree("mozilla-central") - repo = Repo.objects.create( - scm_type=SCM_TYPE_HG, - name="mozilla-central", - url=hg_server, - required_permission=SCM_LEVEL_3, - push_path=hg_server, - pull_path=hg_server, - force_push=True, - system_path=hg_clone.strpath, - ) + repo = hg_repo_mc + repo.force_push = True + treestatusdouble.open_tree(repo.name) scm = repo.scm job_params = { "status": LandingJobStatus.IN_PROGRESS, @@ -325,29 +322,21 @@ def test_integrated_execute_job_with_force_push( assert scm.push.call_count == 1 assert len(scm.push.call_args) == 2 assert len(scm.push.call_args[0]) == 1 - assert scm.push.call_args[0][0] == hg_server + assert scm.push.call_args[0][0] == repo.url assert scm.push.call_args[1] == {"push_target": "", "force_push": True} @pytest.mark.django_db def test_integrated_execute_job_with_bookmark( - hg_server, - hg_clone, + hg_repo_mc, treestatusdouble, monkeypatch, create_patch_revision, ): - treestatusdouble.open_tree("mozilla-central") - repo = Repo.objects.create( - scm_type=SCM_TYPE_HG, - name="mozilla-central", - url=hg_server, - required_permission=SCM_LEVEL_3, - push_path=hg_server, - pull_path=hg_server, - push_target="@", - system_path=hg_clone.strpath, - ) + repo = hg_repo_mc + repo.push_target = "@" + treestatusdouble.open_tree(repo.name) + scm = repo.scm job_params = { "status": LandingJobStatus.IN_PROGRESS, @@ -371,28 +360,20 @@ def test_integrated_execute_job_with_bookmark( assert scm.push.call_count == 1 assert len(scm.push.call_args) == 2 assert len(scm.push.call_args[0]) == 1 - assert scm.push.call_args[0][0] == hg_server + assert scm.push.call_args[0][0] == repo.url assert scm.push.call_args[1] == {"push_target": "@", "force_push": False} @pytest.mark.django_db def test_no_diff_start_line( - hg_server, - hg_clone, + hg_repo_mc, treestatusdouble, create_patch_revision, caplog, ): - treestatusdouble.open_tree("mozilla-central") - repo = Repo.objects.create( - scm_type=SCM_TYPE_HG, - name="mozilla-central", - url=hg_server, - required_permission=SCM_LEVEL_3, - push_path=hg_server, - pull_path=hg_server, - system_path=hg_clone.strpath, - ) + repo = hg_repo_mc + treestatusdouble.open_tree(repo.name) + job_params = { "id": 1234, "status": LandingJobStatus.IN_PROGRESS, @@ -414,21 +395,13 @@ def test_no_diff_start_line( @pytest.mark.django_db def test_lose_push_race( monkeypatch, - hg_server, - hg_clone, + hg_repo_mc, treestatusdouble, create_patch_revision, ): - treestatusdouble.open_tree("mozilla-central") - repo = Repo.objects.create( - scm_type=SCM_TYPE_HG, - name="mozilla-central", - url=hg_server, - required_permission=SCM_LEVEL_3, - push_path=hg_server, - pull_path=hg_server, - system_path=hg_clone.strpath, - ) + repo = hg_repo_mc + treestatusdouble.open_tree(repo.name) + scm = repo.scm job_params = { "id": 1234, @@ -521,24 +494,16 @@ def test_merge_conflict( @pytest.mark.django_db def test_failed_landing_job_notification( - hg_server, - hg_clone, + hg_repo_mc, treestatusdouble, monkeypatch, create_patch_revision, ): """Ensure that a failed landings triggers a user notification.""" - treestatusdouble.open_tree("mozilla-central") - repo = Repo.objects.create( - scm_type=SCM_TYPE_HG, - name="mozilla-central", - required_permission=SCM_LEVEL_3, - push_path=hg_server, - pull_path=hg_server, - approval_required=True, - autoformat_enabled=False, - system_path=hg_clone.strpath, - ) + repo = hg_repo_mc + repo.approval_required = True + repo.autoformat_enabled = False + treestatusdouble.open_tree(repo.name) scm = repo.scm @@ -631,26 +596,16 @@ def test_landing_worker__extract_error_data(): @pytest.mark.django_db def test_format_patch_success_unchanged( - hg_server, - hg_clone, + hg_repo_mc, treestatusdouble, monkeypatch, create_patch_revision, normal_patch, ): """Tests automated formatting happy path where formatters made no changes.""" - tree = "mozilla-central" - treestatusdouble.open_tree(tree) - repo = Repo.objects.create( - scm_type=SCM_TYPE_HG, - name=tree, - url=hg_server, - push_path=hg_server, - pull_path=hg_server, - required_permission=SCM_LEVEL_3, - autoformat_enabled=True, - system_path=hg_clone.strpath, - ) + repo = hg_repo_mc + treestatusdouble.open_tree(repo.name) + repo.autoformat_enabled = True revisions = [ create_patch_revision(1, patch=PATCH_FORMATTING_PATTERN_PASS), @@ -687,38 +642,27 @@ def test_format_patch_success_unchanged( @pytest.mark.django_db def test_format_single_success_changed( - hg_server, - hg_clone, + hg_repo_mc, treestatusdouble, monkeypatch, create_patch_revision, ): """Test formatting a single commit via amending.""" - tree = "mozilla-central" - treestatusdouble.open_tree(tree) - repo = Repo.objects.create( - scm_type=SCM_TYPE_HG, - name=tree, - url=hg_server, - push_path=hg_server, - pull_path=hg_server, - required_permission=SCM_LEVEL_3, - autoformat_enabled=True, - system_path=hg_clone.strpath, - ) + repo = hg_repo_mc + repo.autoformat_enabled = True + treestatusdouble.open_tree(repo.name) # Push the `mach` formatting patch. - hgrepo = HgSCM(hg_clone.strpath) - with hgrepo.for_push("test@example.com"): + with repo.scm.for_push("test@example.com"): ph = HgPatchHelper(io.StringIO(PATCH_FORMATTING_PATTERN_PASS)) - hgrepo.apply_patch( + repo.scm.apply_patch( ph.get_diff(), ph.get_commit_description(), ph.get_header("User"), ph.get_header("Date"), ) - hgrepo.push(repo.push_path) - pre_landing_tip = hgrepo.run_hg(["log", "-r", "tip", "-T", "{node}"]).decode( + repo.scm.push(repo.push_path) + pre_landing_tip = repo.scm.run_hg(["log", "-r", "tip", "-T", "{node}"]).decode( "utf-8" ) @@ -750,19 +694,19 @@ def test_format_single_success_changed( mock_trigger_update.call_count == 1 ), "Successful landing should trigger Phab repo update." - with hgrepo.for_push(job.requester_email): - repo_root = hgrepo.run_hg(["root"]).decode("utf-8").strip() + with repo.scm.for_push(job.requester_email): + repo_root = repo.scm.run_hg(["root"]).decode("utf-8").strip() # Get the commit message. - desc = hgrepo.run_hg(["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8") + desc = repo.scm.run_hg(["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8") # Get the content of the file after autoformatting. - tip_content = hgrepo.run_hg( + tip_content = repo.scm.run_hg( ["cat", "--cwd", repo_root, "-r", "tip", "test.txt"] ) # Get the hash behind the tip commit. - hash_behind_current_tip = hgrepo.run_hg( + hash_behind_current_tip = repo.scm.run_hg( ["log", "-r", "tip^", "-T", "{node}"] ).decode("utf-8") @@ -779,27 +723,15 @@ def test_format_single_success_changed( @pytest.mark.django_db def test_format_stack_success_changed( - hg_server, - hg_clone, + hg_repo_mc, treestatusdouble, monkeypatch, create_patch_revision, ): """Test formatting a stack via an autoformat tip commit.""" - tree = "mozilla-central" - treestatusdouble.open_tree(tree) - repo = Repo.objects.create( - scm_type=SCM_TYPE_HG, - name=tree, - url=hg_server, - push_path=hg_server, - pull_path=hg_server, - required_permission=SCM_LEVEL_3, - autoformat_enabled=True, - system_path=hg_clone.strpath, - ) - - hgrepo = HgSCM(hg_clone.strpath) + repo = hg_repo_mc + repo.autoformat_enabled = (True,) + treestatusdouble.open_tree(repo.name) revisions = [ create_patch_revision(1, patch=PATCH_FORMATTING_PATTERN_PASS), @@ -831,14 +763,14 @@ def test_format_stack_success_changed( mock_trigger_update.call_count == 1 ), "Successful landing should trigger Phab repo update." - with hgrepo.for_push(job.requester_email): - repo_root = hgrepo.run_hg(["root"]).decode("utf-8").strip() + with repo.scm.for_push(job.requester_email): + repo_root = repo.scm.run_hg(["root"]).decode("utf-8").strip() # Get the commit message. - desc = hgrepo.run_hg(["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8") + desc = repo.scm.run_hg(["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8") # Get the content of the file after autoformatting. - rev3_content = hgrepo.run_hg( + rev3_content = repo.scm.run_hg( ["cat", "--cwd", repo_root, "-r", "tip", "test.txt"] ) @@ -857,25 +789,15 @@ def test_format_stack_success_changed( @pytest.mark.django_db def test_format_patch_fail( - hg_server, - hg_clone, + hg_repo_mc, treestatusdouble, monkeypatch, create_patch_revision, ): """Tests automated formatting failures before landing.""" - tree = "mozilla-central" - treestatusdouble.open_tree(tree) - repo = Repo.objects.create( - scm_type=SCM_TYPE_HG, - name=tree, - required_permission=SCM_LEVEL_3, - url=hg_server, - push_path=hg_server, - pull_path=hg_server, - autoformat_enabled=True, - system_path=hg_clone.strpath, - ) + repo = hg_repo_mc + repo.autoformat_enabled = True + treestatusdouble.open_tree(repo.name) revisions = [ create_patch_revision(1, patch=PATCH_FORMATTING_PATTERN_FAIL), @@ -915,24 +837,15 @@ def test_format_patch_fail( @pytest.mark.django_db def test_format_patch_no_landoini( - hg_server, - hg_clone, + hg_repo_mc, treestatusdouble, monkeypatch, create_patch_revision, ): """Tests behaviour of Lando when the `.lando.ini` file is missing.""" - treestatusdouble.open_tree("mozilla-central") - repo = Repo.objects.create( - scm_type=SCM_TYPE_HG, - name="mozilla-central", - required_permission=SCM_LEVEL_3, - url=hg_server, - push_path=hg_server, - pull_path=hg_server, - autoformat_enabled=True, - system_path=hg_clone.strpath, - ) + repo = hg_repo_mc + repo.autoformat_enabled = True + treestatusdouble.open_tree(repo.name) revisions = [ create_patch_revision(1), From e36a83958a102b6dd7b3a10d87c2f847b91824f4 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Tue, 7 Jan 2025 16:47:27 +1100 Subject: [PATCH 03/17] test_landings: add repo_mc parametrable factory --- src/lando/api/tests/test_landings.py | 75 +++++++++++++++++----------- 1 file changed, 46 insertions(+), 29 deletions(-) diff --git a/src/lando/api/tests/test_landings.py b/src/lando/api/tests/test_landings.py index f737f7ae..e28e6f22 100644 --- a/src/lando/api/tests/test_landings.py +++ b/src/lando/api/tests/test_landings.py @@ -1,7 +1,10 @@ import io +import pathlib import textwrap import unittest.mock as mock +from collections.abc import Callable +import py import pytest from lando.api.legacy.workers.landing_worker import ( @@ -227,7 +230,7 @@ def _create_patch_revision(number, patch=normal_patch_0): @pytest.fixture() -def hg_repo_mc(hg_server, hg_clone) -> Repo: +def hg_repo_mc(hg_server: str, hg_clone: py.path) -> Repo: repo = Repo.objects.create( scm_type=SCM_TYPE_HG, name="mozilla-central", @@ -240,25 +243,41 @@ def hg_repo_mc(hg_server, hg_clone) -> Repo: return repo +@pytest.fixture() +def repo_mc(hg_repo_mc: Repo) -> Callable: + def factory(scm_type: str) -> Repo: + # if scm_type == SCM_TYPE_GIT: + # return git_repo_mc + if scm_type == SCM_TYPE_HG: + return hg_repo_mc + raise Exception(f"Unknown SCM Type {scm_type=}") + + return factory + + @pytest.mark.parametrize( - "revisions_params", + "repo_type,revisions_params", [ - [ - (1, {}), - (2, {}), - ], - [(1, {"patch": LARGE_PATCH})], + ( + SCM_TYPE_HG, + [ + (1, {}), + (2, {}), + ], + ), + (SCM_TYPE_HG, [(1, {"patch": LARGE_PATCH})]), ], ) @pytest.mark.django_db def test_integrated_execute_job( - hg_repo_mc, + repo_mc: Repo, treestatusdouble, monkeypatch, create_patch_revision, + repo_type: str, revisions_params, ): - repo = hg_repo_mc + repo = repo_mc(repo_type) treestatusdouble.open_tree(repo.name) revisions = [ @@ -300,6 +319,7 @@ def test_integrated_execute_job_with_force_push( repo.force_push = True treestatusdouble.open_tree(repo.name) scm = repo.scm + job_params = { "status": LandingJobStatus.IN_PROGRESS, "requester_email": "test@example.com", @@ -336,8 +356,8 @@ def test_integrated_execute_job_with_bookmark( repo = hg_repo_mc repo.push_target = "@" treestatusdouble.open_tree(repo.name) - scm = repo.scm + job_params = { "status": LandingJobStatus.IN_PROGRESS, "requester_email": "test@example.com", @@ -401,8 +421,8 @@ def test_lose_push_race( ): repo = hg_repo_mc treestatusdouble.open_tree(repo.name) - scm = repo.scm + job_params = { "id": 1234, "status": LandingJobStatus.IN_PROGRESS, @@ -504,7 +524,6 @@ def test_failed_landing_job_notification( repo.approval_required = True repo.autoformat_enabled = False treestatusdouble.open_tree(repo.name) - scm = repo.scm # Mock `scm.update_repo` so we can force a failed landing. @@ -651,18 +670,19 @@ def test_format_single_success_changed( repo = hg_repo_mc repo.autoformat_enabled = True treestatusdouble.open_tree(repo.name) + scm = repo.scm # Push the `mach` formatting patch. - with repo.scm.for_push("test@example.com"): + with scm.for_push("test@example.com"): ph = HgPatchHelper(io.StringIO(PATCH_FORMATTING_PATTERN_PASS)) - repo.scm.apply_patch( + scm.apply_patch( ph.get_diff(), ph.get_commit_description(), ph.get_header("User"), ph.get_header("Date"), ) - repo.scm.push(repo.push_path) - pre_landing_tip = repo.scm.run_hg(["log", "-r", "tip", "-T", "{node}"]).decode( + scm.push(repo.push_path) + pre_landing_tip = scm.run_hg(["log", "-r", "tip", "-T", "{node}"]).decode( "utf-8" ) @@ -694,19 +714,17 @@ def test_format_single_success_changed( mock_trigger_update.call_count == 1 ), "Successful landing should trigger Phab repo update." - with repo.scm.for_push(job.requester_email): - repo_root = repo.scm.run_hg(["root"]).decode("utf-8").strip() + with scm.for_push(job.requester_email): + repo_root = scm.run_hg(["root"]).decode("utf-8").strip() # Get the commit message. - desc = repo.scm.run_hg(["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8") + desc = scm.run_hg(["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8") # Get the content of the file after autoformatting. - tip_content = repo.scm.run_hg( - ["cat", "--cwd", repo_root, "-r", "tip", "test.txt"] - ) + tip_content = scm.run_hg(["cat", "--cwd", repo_root, "-r", "tip", "test.txt"]) # Get the hash behind the tip commit. - hash_behind_current_tip = repo.scm.run_hg( + hash_behind_current_tip = scm.run_hg( ["log", "-r", "tip^", "-T", "{node}"] ).decode("utf-8") @@ -732,6 +750,7 @@ def test_format_stack_success_changed( repo = hg_repo_mc repo.autoformat_enabled = (True,) treestatusdouble.open_tree(repo.name) + scm = repo.scm revisions = [ create_patch_revision(1, patch=PATCH_FORMATTING_PATTERN_PASS), @@ -763,16 +782,14 @@ def test_format_stack_success_changed( mock_trigger_update.call_count == 1 ), "Successful landing should trigger Phab repo update." - with repo.scm.for_push(job.requester_email): - repo_root = repo.scm.run_hg(["root"]).decode("utf-8").strip() + with scm.for_push(job.requester_email): + repo_root = scm.run_hg(["root"]).decode("utf-8").strip() # Get the commit message. - desc = repo.scm.run_hg(["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8") + desc = scm.run_hg(["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8") # Get the content of the file after autoformatting. - rev3_content = repo.scm.run_hg( - ["cat", "--cwd", repo_root, "-r", "tip", "test.txt"] - ) + rev3_content = scm.run_hg(["cat", "--cwd", repo_root, "-r", "tip", "test.txt"]) assert ( rev3_content == TESTTXT_FORMATTED_2 From f1255042a668ab2322bc1f7529037c8c0cc49b69 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Tue, 7 Jan 2025 17:12:42 +1100 Subject: [PATCH 04/17] tests: add git_repo to landings tests --- src/lando/api/tests/conftest.py | 3 + src/lando/api/tests/test_landings.py | 224 ++++++++++++++++++++------- 2 files changed, 174 insertions(+), 53 deletions(-) diff --git a/src/lando/api/tests/conftest.py b/src/lando/api/tests/conftest.py index cfcc4e1a..cfece393 100644 --- a/src/lando/api/tests/conftest.py +++ b/src/lando/api/tests/conftest.py @@ -30,8 +30,11 @@ from lando.main.models import SCM_LEVEL_1, SCM_LEVEL_3, Profile, Repo from lando.main.scm import SCM_TYPE_HG from lando.main.support import LegacyAPIException +from lando.main.tests.conftest import git_repo from lando.utils.phabricator import PhabricatorClient +__all__ = ["git_repo"] + PATCH_NORMAL_1 = r""" # HG changeset patch # User Test User diff --git a/src/lando/api/tests/test_landings.py b/src/lando/api/tests/test_landings.py index e28e6f22..7dbfa236 100644 --- a/src/lando/api/tests/test_landings.py +++ b/src/lando/api/tests/test_landings.py @@ -19,7 +19,10 @@ ) from lando.main.models.revision import Revision from lando.main.scm import SCM_TYPE_HG -from lando.main.scm.hg import LostPushRace +from lando.main.scm.abstract_scm import AbstractSCM +from lando.main.scm.consts import SCM_TYPE_GIT +from lando.main.scm.git import GitSCM +from lando.main.scm.hg import HgSCM, LostPushRace from lando.utils import HgPatchHelper @@ -233,7 +236,7 @@ def _create_patch_revision(number, patch=normal_patch_0): def hg_repo_mc(hg_server: str, hg_clone: py.path) -> Repo: repo = Repo.objects.create( scm_type=SCM_TYPE_HG, - name="mozilla-central", + name="mozilla-central-hg", required_permission=SCM_LEVEL_3, url=hg_server, push_path=hg_server, @@ -244,10 +247,28 @@ def hg_repo_mc(hg_server: str, hg_clone: py.path) -> Repo: @pytest.fixture() -def repo_mc(hg_repo_mc: Repo) -> Callable: +@pytest.mark.django_db +def git_repo_mc(git_repo: pathlib.Path, tmp_path: pathlib.Path) -> Repo: + repos_dir = tmp_path / "repos" + repos_dir.mkdir() + repo = Repo.objects.create( + scm_type=SCM_TYPE_GIT, + name="mozilla-central-git", + required_permission=SCM_LEVEL_3, + url=str(git_repo), + push_path=str(git_repo), + system_path=repos_dir / "git_repo", + ) + repo.save() + repo.scm.prepare_repo(repo.pull_path) + return repo + + +@pytest.fixture() +def repo_mc(git_repo_mc: Repo, hg_repo_mc: Repo) -> Callable: def factory(scm_type: str) -> Repo: - # if scm_type == SCM_TYPE_GIT: - # return git_repo_mc + if scm_type == SCM_TYPE_GIT: + return git_repo_mc if scm_type == SCM_TYPE_HG: return hg_repo_mc raise Exception(f"Unknown SCM Type {scm_type=}") @@ -258,6 +279,16 @@ def factory(scm_type: str) -> Repo: @pytest.mark.parametrize( "repo_type,revisions_params", [ + # Git + ( + SCM_TYPE_GIT, + [ + (1, {}), + (2, {}), + ], + ), + (SCM_TYPE_GIT, [(1, {"patch": LARGE_PATCH})]), + # Hg ( SCM_TYPE_HG, [ @@ -270,7 +301,7 @@ def factory(scm_type: str) -> Repo: ) @pytest.mark.django_db def test_integrated_execute_job( - repo_mc: Repo, + repo_mc, treestatusdouble, monkeypatch, create_patch_revision, @@ -308,14 +339,22 @@ def test_integrated_execute_job( ), "Successful landing should trigger Phab repo update." +@pytest.mark.parametrize( + "repo_type", + [ + SCM_TYPE_GIT, + SCM_TYPE_HG, + ], +) @pytest.mark.django_db def test_integrated_execute_job_with_force_push( - hg_repo_mc, + repo_mc, treestatusdouble, monkeypatch, create_patch_revision, + repo_type: str, ): - repo = hg_repo_mc + repo = repo_mc(repo_type) repo.force_push = True treestatusdouble.open_tree(repo.name) scm = repo.scm @@ -346,14 +385,22 @@ def test_integrated_execute_job_with_force_push( assert scm.push.call_args[1] == {"push_target": "", "force_push": True} +@pytest.mark.parametrize( + "repo_type", + [ + SCM_TYPE_GIT, + SCM_TYPE_HG, + ], +) @pytest.mark.django_db def test_integrated_execute_job_with_bookmark( - hg_repo_mc, + repo_mc, treestatusdouble, monkeypatch, create_patch_revision, + repo_type: str, ): - repo = hg_repo_mc + repo = repo_mc(repo_type) repo.push_target = "@" treestatusdouble.open_tree(repo.name) scm = repo.scm @@ -384,14 +431,22 @@ def test_integrated_execute_job_with_bookmark( assert scm.push.call_args[1] == {"push_target": "@", "force_push": False} +@pytest.mark.parametrize( + "repo_type", + [ + SCM_TYPE_GIT, + SCM_TYPE_HG, + ], +) @pytest.mark.django_db def test_no_diff_start_line( - hg_repo_mc, + repo_mc, treestatusdouble, create_patch_revision, caplog, + repo_type: str, ): - repo = hg_repo_mc + repo = repo_mc(repo_type) treestatusdouble.open_tree(repo.name) job_params = { @@ -412,14 +467,22 @@ def test_no_diff_start_line( assert "Patch without a diff start line." in caplog.text +@pytest.mark.parametrize( + "repo_type", + [ + SCM_TYPE_GIT, + SCM_TYPE_HG, + ], +) @pytest.mark.django_db def test_lose_push_race( monkeypatch, - hg_repo_mc, + repo_mc, treestatusdouble, create_patch_revision, + repo_type: str, ): - repo = hg_repo_mc + repo = repo_mc(repo_type) treestatusdouble.open_tree(repo.name) scm = repo.scm @@ -449,26 +512,26 @@ def test_lose_push_race( assert job.status == LandingJobStatus.DEFERRED +@pytest.mark.parametrize( + "repo_type", + [ + # Bug 1940876 + # SCM_TYPE_GIT, + SCM_TYPE_HG, + ], +) @pytest.mark.django_db def test_merge_conflict( - hg_server, - hg_clone, + repo_mc, treestatusdouble, monkeypatch, create_patch_revision, - normal_patch, caplog, + repo_type: str, ): - treestatusdouble.open_tree("mozilla-central") - repo = Repo.objects.create( - scm_type=SCM_TYPE_HG, - name="mozilla-central", - url=hg_server, - required_permission=SCM_LEVEL_3, - push_path=hg_server, - pull_path=hg_server, - system_path=hg_clone.strpath, - ) + repo = repo_mc(repo_type) + treestatusdouble.open_tree(repo.name) + job_params = { "id": 1234, "status": LandingJobStatus.IN_PROGRESS, @@ -512,15 +575,24 @@ def test_merge_conflict( ), f"Empty or missing reject content for failed path {fp}" +@pytest.mark.parametrize( + "repo_type", + [ + # Bug 1940876 + # SCM_TYPE_GIT, + SCM_TYPE_HG, + ], +) @pytest.mark.django_db def test_failed_landing_job_notification( - hg_repo_mc, + repo_mc, treestatusdouble, monkeypatch, create_patch_revision, + repo_type: str, ): """Ensure that a failed landings triggers a user notification.""" - repo = hg_repo_mc + repo = repo_mc(repo_type) repo.approval_required = True repo.autoformat_enabled = False treestatusdouble.open_tree(repo.name) @@ -613,16 +685,24 @@ def test_landing_worker__extract_error_data(): assert rejects_paths == expected_rejects_paths +@pytest.mark.parametrize( + "repo_type", + [ + SCM_TYPE_GIT, + SCM_TYPE_HG, + ], +) @pytest.mark.django_db def test_format_patch_success_unchanged( - hg_repo_mc, + repo_mc, treestatusdouble, monkeypatch, create_patch_revision, normal_patch, + repo_type: str, ): """Tests automated formatting happy path where formatters made no changes.""" - repo = hg_repo_mc + repo = repo_mc(repo_type) treestatusdouble.open_tree(repo.name) repo.autoformat_enabled = True @@ -659,15 +739,23 @@ def test_format_patch_success_unchanged( ), "Autoformat making no changes should leave `formatted_replacements` empty." +@pytest.mark.parametrize( + "repo_type", + [ + SCM_TYPE_GIT, + SCM_TYPE_HG, + ], +) @pytest.mark.django_db def test_format_single_success_changed( - hg_repo_mc, + repo_mc, treestatusdouble, monkeypatch, create_patch_revision, + repo_type: str, ): """Test formatting a single commit via amending.""" - repo = hg_repo_mc + repo = repo_mc(repo_type) repo.autoformat_enabled = True treestatusdouble.open_tree(repo.name) scm = repo.scm @@ -682,9 +770,7 @@ def test_format_single_success_changed( ph.get_header("Date"), ) scm.push(repo.push_path) - pre_landing_tip = scm.run_hg(["log", "-r", "tip", "-T", "{node}"]).decode( - "utf-8" - ) + pre_landing_tip = scm.head_ref() # Upload a patch for formatting. job_params = { @@ -715,18 +801,14 @@ def test_format_single_success_changed( ), "Successful landing should trigger Phab repo update." with scm.for_push(job.requester_email): - repo_root = scm.run_hg(["root"]).decode("utf-8").strip() - # Get the commit message. - desc = scm.run_hg(["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8") + desc = _scm_get_last_commit_message(scm) # Get the content of the file after autoformatting. - tip_content = scm.run_hg(["cat", "--cwd", repo_root, "-r", "tip", "test.txt"]) + tip_content = scm.read_checkout_file("test.txt").encode("utf-8") # Get the hash behind the tip commit. - hash_behind_current_tip = scm.run_hg( - ["log", "-r", "tip^", "-T", "{node}"] - ).decode("utf-8") + hash_behind_current_tip = _scm_get_previous_hash(scm) assert tip_content == TESTTXT_FORMATTED_1, "`test.txt` is incorrect in base commit." @@ -739,15 +821,29 @@ def test_format_single_success_changed( ), "Autoformat via amending should only land a single commit." +def _scm_get_previous_hash(scm: AbstractSCM) -> str: + if scm.scm_type() == SCM_TYPE_HG: + return HgSCM.run_hg(scm, ["log", "-r", "tip^", "-T", "{node}"]).decode("utf-8") + return GitSCM._git_run("rev-parse", "HEAD^", cwd=scm.path) + + +@pytest.mark.parametrize( + "repo_type", + [ + SCM_TYPE_GIT, + SCM_TYPE_HG, + ], +) @pytest.mark.django_db def test_format_stack_success_changed( - hg_repo_mc, + repo_mc, treestatusdouble, monkeypatch, create_patch_revision, + repo_type: str, ): """Test formatting a stack via an autoformat tip commit.""" - repo = hg_repo_mc + repo = repo_mc(repo_type) repo.autoformat_enabled = (True,) treestatusdouble.open_tree(repo.name) scm = repo.scm @@ -783,13 +879,11 @@ def test_format_stack_success_changed( ), "Successful landing should trigger Phab repo update." with scm.for_push(job.requester_email): - repo_root = scm.run_hg(["root"]).decode("utf-8").strip() - # Get the commit message. - desc = scm.run_hg(["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8") + desc = _scm_get_last_commit_message(scm) # Get the content of the file after autoformatting. - rev3_content = scm.run_hg(["cat", "--cwd", repo_root, "-r", "tip", "test.txt"]) + rev3_content = scm.read_checkout_file("test.txt").encode("utf-8") assert ( rev3_content == TESTTXT_FORMATTED_2 @@ -804,15 +898,30 @@ def test_format_stack_success_changed( ), "Autoformat commit has incorrect commit message." +def _scm_get_last_commit_message(scm: AbstractSCM) -> str: + if scm.scm_type() == SCM_TYPE_HG: + return HgSCM.run_hg(scm, ["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8") + return GitSCM._git_run("log", "--pretty=%B", "HEAD^..", cwd=scm.path) + + +@pytest.mark.parametrize( + "repo_type", + [ + # Bug 1940876 + # SCM_TYPE_GIT, + SCM_TYPE_HG, + ], +) @pytest.mark.django_db def test_format_patch_fail( - hg_repo_mc, + repo_mc, treestatusdouble, monkeypatch, create_patch_revision, + repo_type: str, ): """Tests automated formatting failures before landing.""" - repo = hg_repo_mc + repo = repo_mc(repo_type) repo.autoformat_enabled = True treestatusdouble.open_tree(repo.name) @@ -852,15 +961,24 @@ def test_format_patch_fail( ), "User should be notified their landing was unsuccessful due to autoformat." +@pytest.mark.parametrize( + "repo_type", + [ + # Bug 1940876 + # SCM_TYPE_GIT, + SCM_TYPE_HG, + ], +) @pytest.mark.django_db def test_format_patch_no_landoini( - hg_repo_mc, + repo_mc, treestatusdouble, monkeypatch, create_patch_revision, + repo_type: str, ): """Tests behaviour of Lando when the `.lando.ini` file is missing.""" - repo = hg_repo_mc + repo = repo_mc(repo_type) repo.autoformat_enabled = True treestatusdouble.open_tree(repo.name) From 484658b8da53f668627535d80b6db80611404424 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Wed, 8 Jan 2025 16:09:50 +1100 Subject: [PATCH 05/17] test_git: initialise test_repo with seed patch --- src/lando/api/tests/conftest.py | 6 ++++-- src/lando/main/tests/conftest.py | 17 ++++++++++------- src/lando/main/tests/data/test-repo.patch | 23 +++++++++++++++++++++++ 3 files changed, 37 insertions(+), 9 deletions(-) create mode 100644 src/lando/main/tests/data/test-repo.patch diff --git a/src/lando/api/tests/conftest.py b/src/lando/api/tests/conftest.py index cfece393..58183067 100644 --- a/src/lando/api/tests/conftest.py +++ b/src/lando/api/tests/conftest.py @@ -30,10 +30,12 @@ from lando.main.models import SCM_LEVEL_1, SCM_LEVEL_3, Profile, Repo from lando.main.scm import SCM_TYPE_HG from lando.main.support import LegacyAPIException -from lando.main.tests.conftest import git_repo +from lando.main.tests.conftest import git_repo, git_repo_seed from lando.utils.phabricator import PhabricatorClient -__all__ = ["git_repo"] +# We need some local usage of those imported fixtures to satisfy the linters. +# This is it. +__all__ = ["git_repo", "git_repo_seed"] PATCH_NORMAL_1 = r""" # HG changeset patch diff --git a/src/lando/main/tests/conftest.py b/src/lando/main/tests/conftest.py index cc44bf02..8900acfb 100644 --- a/src/lando/main/tests/conftest.py +++ b/src/lando/main/tests/conftest.py @@ -1,11 +1,15 @@ -import pathlib import subprocess +from pathlib import Path import pytest @pytest.fixture -def git_repo(tmp_path: pathlib.Path): +def git_repo_seed(request) -> Path: + return Path(__file__).parent / "data" / "test-repo.patch" + +@pytest.fixture +def git_repo(tmp_path: Path, git_repo_seed: Path) -> Path: """ Creates a temporary Git repository for testing purposes. @@ -18,11 +22,10 @@ def git_repo(tmp_path: pathlib.Path): repo_dir = tmp_path / "git_repo" subprocess.run(["git", "init", repo_dir], check=True) subprocess.run(["git", "branch", "-m", "main"], check=True, cwd=repo_dir) - file = repo_dir / "first" - file.write_text("first file!") _git_setup_user(repo_dir) - subprocess.run(["git", "add", file.name], check=True, cwd=repo_dir) - subprocess.run(["git", "commit", "-m", "first commit"], check=True, cwd=repo_dir) + subprocess.run(["git", "am", str(git_repo_seed)], check=True, cwd=repo_dir) + subprocess.run(["git", "show"], check=True, cwd=repo_dir) + subprocess.run(["git", "branch"], check=True, cwd=repo_dir) return repo_dir @@ -31,7 +34,7 @@ def git_setup_user(): return _git_setup_user -def _git_setup_user(repo_dir): +def _git_setup_user(repo_dir: Path): """Configure the git user locally to repo_dir so as not to mess with the real user's configuration.""" subprocess.run(["git", "config", "user.name", "Py Test"], check=True, cwd=repo_dir) subprocess.run( diff --git a/src/lando/main/tests/data/test-repo.patch b/src/lando/main/tests/data/test-repo.patch new file mode 100644 index 00000000..8bf178df --- /dev/null +++ b/src/lando/main/tests/data/test-repo.patch @@ -0,0 +1,23 @@ +# HG changeset patch +# User Test User +# Date 1736311725 -39600 +# Wed Jan 08 15:48:45 2025 +1100 +# Node ID 4e379f8d3278a6645a5cf706cd8555da195b089e +# Parent 0000000000000000000000000000000000000000 +initial commit + + +add another file + +diff --git a/README b/README +new file mode 100644 +--- /dev/null ++++ b/README +@@ -0,0 +1,1 @@ ++HELLO +diff --git a/test.txt b/test.txt +new file mode 100644 +--- /dev/null ++++ b/test.txt +@@ -0,0 +1,1 @@ ++TEST From 599f2160d6c5504c8f7f65737e6c5ade41a02aec Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Thu, 9 Jan 2025 16:21:05 +1100 Subject: [PATCH 06/17] tests: don't rstrip diff fixtures (git requires a trailing newline) --- src/lando/api/tests/conftest.py | 6 +++--- src/lando/api/tests/test_landings.py | 14 +++++++------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/lando/api/tests/conftest.py b/src/lando/api/tests/conftest.py index 58183067..89a27f62 100644 --- a/src/lando/api/tests/conftest.py +++ b/src/lando/api/tests/conftest.py @@ -50,7 +50,7 @@ @@ -1,1 +1,2 @@ TEST +adding another line -""".strip() +""".lstrip() PATCH_NORMAL_2 = r""" # HG changeset patch @@ -66,7 +66,7 @@ TEST adding another line +adding one more line -""".strip() +""".lstrip() PATCH_NORMAL_3 = r""" # HG changeset patch @@ -87,7 +87,7 @@ +++ b/blah.txt @@ -0,0 +1,1 @@ +TEST -""".strip() +""".lstrip() @pytest.fixture diff --git a/src/lando/api/tests/test_landings.py b/src/lando/api/tests/test_landings.py index 7dbfa236..d719d96b 100644 --- a/src/lando/api/tests/test_landings.py +++ b/src/lando/api/tests/test_landings.py @@ -60,7 +60,7 @@ def _create_patch_revision(number, patch=normal_patch_0): @@ -1,1 +1,2 @@ TEST +{LARGE_UTF8_THING} -""".strip() +""".lstrip() PATCH_WITHOUT_STARTLINE = r""" # HG changeset patch @@ -74,7 +74,7 @@ def _create_patch_revision(number, patch=normal_patch_0): @@ -1,1 +1,2 @@ TEST +adding another line -""".strip() +""".lstrip() PATCH_PUSH_LOSER = r""" @@ -91,7 +91,7 @@ def _create_patch_revision(number, patch=normal_patch_0): @@ -1,1 +1,2 @@ TEST +adding one more line again -""".strip() +""".lstrip() PATCH_FORMATTING_PATTERN_PASS = r""" # HG changeset patch @@ -145,7 +145,7 @@ def _create_patch_revision(number, patch=normal_patch_0): + f.write("".join(stdout_content)) + sys.exit(0) -""".strip() +""".lstrip() PATCH_FORMATTING_PATTERN_FAIL = r""" # HG changeset patch @@ -178,7 +178,7 @@ def _create_patch_revision(number, patch=normal_patch_0): +sys.exit("MACH FAILED") + -""".strip() +""".lstrip() PATCH_FORMATTED_1 = r""" # HG changeset patch @@ -196,7 +196,7 @@ def _create_patch_revision(number, patch=normal_patch_0): + + +adding another line -""".strip() +""".lstrip() PATCH_FORMATTED_2 = r""" # HG changeset patch @@ -214,7 +214,7 @@ def _create_patch_revision(number, patch=normal_patch_0): adding another line +add one more line -""".strip() # noqa: W293 +""".lstrip() # noqa: W293 TESTTXT_FORMATTED_1 = b""" TeSt From a13da75a0832cda6593a379018e63ccad1f4517c Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Thu, 9 Jan 2025 16:21:39 +1100 Subject: [PATCH 07/17] *SCM.apply_patch: add suffix to temporary files --- src/lando/main/scm/git.py | 6 ++++-- src/lando/main/scm/hg.py | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/lando/main/scm/git.py b/src/lando/main/scm/git.py index 5fa2adb5..d39e4aad 100644 --- a/src/lando/main/scm/git.py +++ b/src/lando/main/scm/git.py @@ -145,8 +145,10 @@ def apply_patch( self, diff: str, commit_description: str, commit_author: str, commit_date: str ): """Apply the given patch to the current repository.""" - f_msg = tempfile.NamedTemporaryFile(encoding="utf-8", mode="w+") - f_diff = tempfile.NamedTemporaryFile(encoding="utf-8", mode="w+") + f_msg = tempfile.NamedTemporaryFile(encoding="utf-8", mode="w+", suffix=".msg") + f_diff = tempfile.NamedTemporaryFile( + encoding="utf-8", mode="w+", suffix=".diff" + ) with f_msg, f_diff: f_msg.write(commit_description) f_msg.flush() diff --git a/src/lando/main/scm/hg.py b/src/lando/main/scm/hg.py index 4d0ed89b..6cb89d68 100644 --- a/src/lando/main/scm/hg.py +++ b/src/lando/main/scm/hg.py @@ -232,8 +232,8 @@ def apply_patch( """Apply the given patch to the current repository.""" # Import the diff to apply the changes then commit separately to # ensure correct parsing of the commit message. - f_msg = tempfile.NamedTemporaryFile(encoding="utf-8", mode="w+") - f_diff = tempfile.NamedTemporaryFile(encoding="utf-8", mode="w+") + f_msg = tempfile.NamedTemporaryFile(encoding="utf-8", mode="w+", suffix="msg") + f_diff = tempfile.NamedTemporaryFile(encoding="utf-8", mode="w+", suffix="diff") with f_msg, f_diff: f_msg.write(commit_description) f_msg.flush() From 40b6d0cc4755929e11bde150aef11012a1f421ad Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Thu, 9 Jan 2025 16:44:05 +1100 Subject: [PATCH 08/17] test_landings: allow create_patch_revision to fetch normal patches by index --- src/lando/api/tests/test_landings.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/lando/api/tests/test_landings.py b/src/lando/api/tests/test_landings.py index d719d96b..745bf729 100644 --- a/src/lando/api/tests/test_landings.py +++ b/src/lando/api/tests/test_landings.py @@ -34,6 +34,13 @@ def create_patch_revision(normal_patch): normal_patch_0 = normal_patch(0) def _create_patch_revision(number, patch=normal_patch_0): + """Create revision number `number`, with patch text `patch`. + + `patch` will default to the first normal patch fixture if unspecified. However, + if it explicitely set to None, the `normal_patch` fixture will be used to get + normal patch number `number-1`.""" + if not patch: + patch = normal_patch(number - 1) revision = Revision() revision.revision_id = number revision.diff_id = number @@ -283,8 +290,8 @@ def factory(scm_type: str) -> Repo: ( SCM_TYPE_GIT, [ - (1, {}), - (2, {}), + (1, {"patch": None}), + (2, {"patch": None}), ], ), (SCM_TYPE_GIT, [(1, {"patch": LARGE_PATCH})]), @@ -292,8 +299,8 @@ def factory(scm_type: str) -> Repo: ( SCM_TYPE_HG, [ - (1, {}), - (2, {}), + (1, {"patch": None}), + (2, {"patch": None}), ], ), (SCM_TYPE_HG, [(1, {"patch": LARGE_PATCH})]), From 72f1cde488a15206aee346f97657acf3652340ec Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Thu, 9 Jan 2025 16:52:37 +1100 Subject: [PATCH 09/17] test: ignore denyCurrentBranch in git repo fixture --- src/lando/api/tests/test_landings.py | 1 + src/lando/main/tests/conftest.py | 15 +++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/lando/api/tests/test_landings.py b/src/lando/api/tests/test_landings.py index 745bf729..dcd54826 100644 --- a/src/lando/api/tests/test_landings.py +++ b/src/lando/api/tests/test_landings.py @@ -321,6 +321,7 @@ def test_integrated_execute_job( revisions = [ create_patch_revision(number, **kwargs) for number, kwargs in revisions_params ] + job_params = { "status": LandingJobStatus.IN_PROGRESS, "requester_email": "test@example.com", diff --git a/src/lando/main/tests/conftest.py b/src/lando/main/tests/conftest.py index 8900acfb..b8b383c2 100644 --- a/src/lando/main/tests/conftest.py +++ b/src/lando/main/tests/conftest.py @@ -8,6 +8,7 @@ def git_repo_seed(request) -> Path: return Path(__file__).parent / "data" / "test-repo.patch" + @pytest.fixture def git_repo(tmp_path: Path, git_repo_seed: Path) -> Path: """ @@ -23,6 +24,7 @@ def git_repo(tmp_path: Path, git_repo_seed: Path) -> Path: subprocess.run(["git", "init", repo_dir], check=True) subprocess.run(["git", "branch", "-m", "main"], check=True, cwd=repo_dir) _git_setup_user(repo_dir) + _git_ignore_denyCurrentBranch(repo_dir) subprocess.run(["git", "am", str(git_repo_seed)], check=True, cwd=repo_dir) subprocess.run(["git", "show"], check=True, cwd=repo_dir) subprocess.run(["git", "branch"], check=True, cwd=repo_dir) @@ -42,3 +44,16 @@ def _git_setup_user(repo_dir: Path): check=True, cwd=repo_dir, ) + + +def _git_ignore_denyCurrentBranch(repo_dir: Path): + """Disable error when pushing to this non-bare repo. + + This is a sane protection in general, but it gets in the way of the tests here, + where we just want a target, and don't care much about the final state of this + target after everything is done.""" + subprocess.run( + ["git", "config", "receive.denyCurrentBranch", "ignore"], + check=True, + cwd=repo_dir, + ) From 8375fc224709c597d950c59ded5bae38f935ab8d Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Thu, 9 Jan 2025 17:27:19 +1100 Subject: [PATCH 10/17] GitSCM: set Git user on local clone --- src/lando/main/scm/git.py | 15 ++++++++++++++- src/lando/main/tests/test_git.py | 2 +- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/lando/main/scm/git.py b/src/lando/main/scm/git.py index d39e4aad..164ade6d 100644 --- a/src/lando/main/scm/git.py +++ b/src/lando/main/scm/git.py @@ -14,6 +14,7 @@ from lando.main.scm.consts import SCM_TYPE_GIT from lando.main.scm.exceptions import SCMException +from lando.settings import LANDO_USER_EMAIL, LANDO_USER_NAME from .abstract_scm import AbstractSCM @@ -69,6 +70,12 @@ def clone(self, source: str): """Clone a repository from a source.""" # When cloning, self.path doesn't exist yet, so we need to use another CWD. self._git_run("clone", source, self.path, cwd="/") + self._git_setup_user() + + def _git_setup_user(self): + """Configure the git user locally to repo_dir so as not to mess with the real user's configuration.""" + self._git_run("config", "user.name", LANDO_USER_NAME, cwd=self.path) + self._git_run("config", "user.email", LANDO_USER_EMAIL, cwd=self.path) def push( self, @@ -231,7 +238,13 @@ def format_stack_amend(self) -> Optional[list[str]]: def format_stack_tip(self, commit_message: str) -> Optional[list[str]]: """Add an autoformat commit to the top of the patch stack.""" - self._git_run("commit", "--all", "--message", commit_message, cwd=self.path) + try: + self._git_run("commit", "--all", "--message", commit_message, cwd=self.path) + except SCMException as exc: + if "nothing to commit, working tree clean" in exc.out: + return [] + else: + raise exc return [self.get_current_node()] def get_current_node(self) -> str: diff --git a/src/lando/main/tests/test_git.py b/src/lando/main/tests/test_git.py index 5625b8e3..a879714d 100644 --- a/src/lando/main/tests/test_git.py +++ b/src/lando/main/tests/test_git.py @@ -56,7 +56,7 @@ def test_GitSCM_clone(git_repo: Path, tmp_path: Path, monkeypatch): scm.clone(str(git_repo)) - mock_git_run.assert_called_with("clone", str(git_repo), str(clone_path), cwd="/") + mock_git_run.assert_any_call("clone", str(git_repo), str(clone_path), cwd="/") assert clone_path.exists(), f"New git clone {clone_path} wasn't created" assert ( clone_path / ".git" From 2588f72c7d8e7ecb63098bb4111d478023afa2c3 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Fri, 10 Jan 2025 12:09:51 +1100 Subject: [PATCH 11/17] GitSCM.update_repo: use git fetch rather than pull --- src/lando/main/scm/git.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/lando/main/scm/git.py b/src/lando/main/scm/git.py index 164ade6d..64d96282 100644 --- a/src/lando/main/scm/git.py +++ b/src/lando/main/scm/git.py @@ -219,8 +219,17 @@ def update_repo(self, pull_path: str, target_cset: Optional[str] = None) -> str: """ branch = target_cset or self.default_branch self.clean_repo() - self._git_run("pull", "--prune", pull_path, cwd=self.path) - self._git_run("checkout", "--force", "-B", branch, cwd=self.path) + # Fetch all refs at the given pull_path, and overwrite the `origin` references. + self._git_run( + "fetch", + "--prune", + pull_path, + "+refs/heads/*:refs/remotes/origin/*", + cwd=self.path, + ) + self._git_run( + "checkout", "--force", "-B", branch, f"origin/{branch}", cwd=self.path + ) return self.head_ref() def clean_repo(self, *, strip_non_public_commits: bool = True): From a5cff9f04c215133735784af9432fc051b04c81c Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Mon, 13 Jan 2025 11:08:56 +1100 Subject: [PATCH 12/17] Update src/lando/api/tests/test_landings.py Co-authored-by: Connor Sheehan --- src/lando/api/tests/test_landings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lando/api/tests/test_landings.py b/src/lando/api/tests/test_landings.py index dcd54826..10076889 100644 --- a/src/lando/api/tests/test_landings.py +++ b/src/lando/api/tests/test_landings.py @@ -37,7 +37,7 @@ def _create_patch_revision(number, patch=normal_patch_0): """Create revision number `number`, with patch text `patch`. `patch` will default to the first normal patch fixture if unspecified. However, - if it explicitely set to None, the `normal_patch` fixture will be used to get + if explicitly set to None, the `normal_patch` fixture will be used to get normal patch number `number-1`.""" if not patch: patch = normal_patch(number - 1) From be12b481d236be8c23c2dac2bdf8fdf6ae20b1c2 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Mon, 13 Jan 2025 11:09:57 +1100 Subject: [PATCH 13/17] Update src/lando/main/tests/conftest.py Co-authored-by: Connor Sheehan --- src/lando/main/tests/conftest.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lando/main/tests/conftest.py b/src/lando/main/tests/conftest.py index b8b383c2..d01cb6cf 100644 --- a/src/lando/main/tests/conftest.py +++ b/src/lando/main/tests/conftest.py @@ -51,7 +51,8 @@ def _git_ignore_denyCurrentBranch(repo_dir: Path): This is a sane protection in general, but it gets in the way of the tests here, where we just want a target, and don't care much about the final state of this - target after everything is done.""" + target after everything is done. + """ subprocess.run( ["git", "config", "receive.denyCurrentBranch", "ignore"], check=True, From 9a7f1cd8da5eb1c25777bff135fc38a4bf8911dd Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Mon, 13 Jan 2025 11:18:12 +1100 Subject: [PATCH 14/17] fixup! landing_worker: add and use git scm support (bug 1895523) (#147) --- src/lando/main/tests/conftest.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/lando/main/tests/conftest.py b/src/lando/main/tests/conftest.py index d01cb6cf..c91babb6 100644 --- a/src/lando/main/tests/conftest.py +++ b/src/lando/main/tests/conftest.py @@ -5,7 +5,11 @@ @pytest.fixture -def git_repo_seed(request) -> Path: +def git_repo_seed() -> Path: + """ + Return the path to a diff file to apply on an empty repo to create a known base for + application of other patches as part of the tests. + """ return Path(__file__).parent / "data" / "test-repo.patch" From 4e8e277b2c51d7b83b7c04aeba5dcdb208975b10 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Mon, 13 Jan 2025 11:45:08 +1100 Subject: [PATCH 15/17] test_landings: update repo_mc factory to allow parameters --- src/lando/api/tests/test_landings.py | 117 +++++++++++++++++++-------- 1 file changed, 82 insertions(+), 35 deletions(-) diff --git a/src/lando/api/tests/test_landings.py b/src/lando/api/tests/test_landings.py index 10076889..f72130a9 100644 --- a/src/lando/api/tests/test_landings.py +++ b/src/lando/api/tests/test_landings.py @@ -239,32 +239,67 @@ def _create_patch_revision(number, patch=normal_patch_0): """.lstrip() -@pytest.fixture() -def hg_repo_mc(hg_server: str, hg_clone: py.path) -> Repo: +@pytest.mark.django_db +def hg_repo_mc( + hg_server: str, + hg_clone: py.path, + *, + approval_required: bool = False, + autoformat_enabled: bool = False, + force_push: bool = False, + push_target: str = "", +) -> Repo: + params = { + "required_permission": SCM_LEVEL_3, + "url": hg_server, + "push_path": hg_server, + "pull_path": hg_server, + "system_path": hg_clone.strpath, + # The option below can be overriden in the parameters + "approval_required": approval_required, + "autoformat_enabled": autoformat_enabled, + "force_push": force_push, + "push_target": push_target, + } repo = Repo.objects.create( scm_type=SCM_TYPE_HG, name="mozilla-central-hg", - required_permission=SCM_LEVEL_3, - url=hg_server, - push_path=hg_server, - pull_path=hg_server, - system_path=hg_clone.strpath, + **params, ) + repo.save() return repo -@pytest.fixture() @pytest.mark.django_db -def git_repo_mc(git_repo: pathlib.Path, tmp_path: pathlib.Path) -> Repo: +def git_repo_mc( + git_repo: pathlib.Path, + tmp_path: pathlib.Path, + *, + approval_required: bool = False, + autoformat_enabled: bool = False, + force_push: bool = False, + push_target: str = "", +) -> Repo: repos_dir = tmp_path / "repos" repos_dir.mkdir() + + params = { + "required_permission": SCM_LEVEL_3, + "url": str(git_repo), + "push_path": str(git_repo), + "pull_path": str(git_repo), + "system_path": repos_dir / "git_repo", + # The option below can be overriden in the parameters + "approval_required": approval_required, + "autoformat_enabled": autoformat_enabled, + "force_push": force_push, + "push_target": push_target, + } + repo = Repo.objects.create( scm_type=SCM_TYPE_GIT, name="mozilla-central-git", - required_permission=SCM_LEVEL_3, - url=str(git_repo), - push_path=str(git_repo), - system_path=repos_dir / "git_repo", + **params, ) repo.save() repo.scm.prepare_repo(repo.pull_path) @@ -272,12 +307,33 @@ def git_repo_mc(git_repo: pathlib.Path, tmp_path: pathlib.Path) -> Repo: @pytest.fixture() -def repo_mc(git_repo_mc: Repo, hg_repo_mc: Repo) -> Callable: - def factory(scm_type: str) -> Repo: +def repo_mc( + # Git + git_repo: pathlib.Path, + tmp_path: pathlib.Path, + # Hg + hg_server: str, + hg_clone: py.path, +) -> Callable: + def factory( + scm_type: str, + *, + approval_required: bool = False, + autoformat_enabled: bool = False, + force_push: bool = False, + push_target: str = "", + ) -> Repo: + params = { + "approval_required": approval_required, + "autoformat_enabled": autoformat_enabled, + "force_push": force_push, + "push_target": push_target, + } + if scm_type == SCM_TYPE_GIT: - return git_repo_mc - if scm_type == SCM_TYPE_HG: - return hg_repo_mc + return git_repo_mc(git_repo, tmp_path, **params) + elif scm_type == SCM_TYPE_HG: + return hg_repo_mc(hg_server, hg_clone, **params) raise Exception(f"Unknown SCM Type {scm_type=}") return factory @@ -362,8 +418,7 @@ def test_integrated_execute_job_with_force_push( create_patch_revision, repo_type: str, ): - repo = repo_mc(repo_type) - repo.force_push = True + repo = repo_mc(repo_type, force_push=True) treestatusdouble.open_tree(repo.name) scm = repo.scm @@ -408,8 +463,7 @@ def test_integrated_execute_job_with_bookmark( create_patch_revision, repo_type: str, ): - repo = repo_mc(repo_type) - repo.push_target = "@" + repo = repo_mc(repo_type, push_target="@") treestatusdouble.open_tree(repo.name) scm = repo.scm @@ -600,9 +654,7 @@ def test_failed_landing_job_notification( repo_type: str, ): """Ensure that a failed landings triggers a user notification.""" - repo = repo_mc(repo_type) - repo.approval_required = True - repo.autoformat_enabled = False + repo = repo_mc(repo_type, approval_required=True, autoformat_enabled=False) treestatusdouble.open_tree(repo.name) scm = repo.scm @@ -710,9 +762,8 @@ def test_format_patch_success_unchanged( repo_type: str, ): """Tests automated formatting happy path where formatters made no changes.""" - repo = repo_mc(repo_type) + repo = repo_mc(repo_type, autoformat_enabled=True) treestatusdouble.open_tree(repo.name) - repo.autoformat_enabled = True revisions = [ create_patch_revision(1, patch=PATCH_FORMATTING_PATTERN_PASS), @@ -763,8 +814,7 @@ def test_format_single_success_changed( repo_type: str, ): """Test formatting a single commit via amending.""" - repo = repo_mc(repo_type) - repo.autoformat_enabled = True + repo = repo_mc(repo_type, autoformat_enabled=True) treestatusdouble.open_tree(repo.name) scm = repo.scm @@ -851,8 +901,7 @@ def test_format_stack_success_changed( repo_type: str, ): """Test formatting a stack via an autoformat tip commit.""" - repo = repo_mc(repo_type) - repo.autoformat_enabled = (True,) + repo = repo_mc(repo_type, autoformat_enabled=True) treestatusdouble.open_tree(repo.name) scm = repo.scm @@ -929,8 +978,7 @@ def test_format_patch_fail( repo_type: str, ): """Tests automated formatting failures before landing.""" - repo = repo_mc(repo_type) - repo.autoformat_enabled = True + repo = repo_mc(repo_type, autoformat_enabled=True) treestatusdouble.open_tree(repo.name) revisions = [ @@ -986,8 +1034,7 @@ def test_format_patch_no_landoini( repo_type: str, ): """Tests behaviour of Lando when the `.lando.ini` file is missing.""" - repo = repo_mc(repo_type) - repo.autoformat_enabled = True + repo = repo_mc(repo_type, autoformat_enabled=True) treestatusdouble.open_tree(repo.name) revisions = [ From e7428e29cad91163a1dd4e63b2f02fa8cd081df2 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Fri, 17 Jan 2025 12:18:11 +1100 Subject: [PATCH 16/17] fixup! tests: add git_repo to landings tests --- src/lando/api/tests/test_landings.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/lando/api/tests/test_landings.py b/src/lando/api/tests/test_landings.py index f72130a9..01003ba3 100644 --- a/src/lando/api/tests/test_landings.py +++ b/src/lando/api/tests/test_landings.py @@ -880,7 +880,12 @@ def test_format_single_success_changed( def _scm_get_previous_hash(scm: AbstractSCM) -> str: - if scm.scm_type() == SCM_TYPE_HG: + """Get the second last commit hash. + + This is implementation-specific code, but is only used in the tests. Rather than + extending the AbstractSCM for the sole purpose of support test assertions, we keep + this in the test code instead.""" + if isinstance(scm, HgSCM): return HgSCM.run_hg(scm, ["log", "-r", "tip^", "-T", "{node}"]).decode("utf-8") return GitSCM._git_run("rev-parse", "HEAD^", cwd=scm.path) @@ -956,7 +961,12 @@ def test_format_stack_success_changed( def _scm_get_last_commit_message(scm: AbstractSCM) -> str: - if scm.scm_type() == SCM_TYPE_HG: + """Get the last commit message. + + This is implementation-specific code, but is only used in the tests. Rather than + extending the AbstractSCM for the sole purpose of support test assertions, we keep + this in the test code instead.""" + if isinstance(scm, HgSCM): return HgSCM.run_hg(scm, ["log", "-r", "tip", "-T", "{desc}"]).decode("utf-8") return GitSCM._git_run("log", "--pretty=%B", "HEAD^..", cwd=scm.path) From ab884d9e23c910469222be8671cc926cb9218888 Mon Sep 17 00:00:00 2001 From: Olivier Mehani Date: Fri, 17 Jan 2025 12:20:03 +1100 Subject: [PATCH 17/17] fixup! fixup! landing_worker: add and use git scm support (bug 1895523) (#147) --- src/lando/main/tests/conftest.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/lando/main/tests/conftest.py b/src/lando/main/tests/conftest.py index c91babb6..d001e23e 100644 --- a/src/lando/main/tests/conftest.py +++ b/src/lando/main/tests/conftest.py @@ -7,8 +7,10 @@ @pytest.fixture def git_repo_seed() -> Path: """ - Return the path to a diff file to apply on an empty repo to create a known base for - application of other patches as part of the tests. + Return the path to a patch to set up a base git repo for tests. + + The diff can apply on an empty repo to create a known base for application + of other patches as part of the tests. """ return Path(__file__).parent / "data" / "test-repo.patch"