From 7d66115573c6c029ce6aa00e244f8bdfbb907e33 Mon Sep 17 00:00:00 2001
From: "John L. Villalovos" <john@sodarock.com>
Date: Sun, 23 May 2021 10:44:27 -0700
Subject: [PATCH 1/5] chore: add a functional test for issue #1120

Going to switch to putting parameters from in the query string to
having them in the 'data' body section. Add a functional test to make
sure that we don't break anything.

https://github.com/python-gitlab/python-gitlab/issues/1120
---
 tools/functional/api/test_merge_requests.py | 59 +++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/tools/functional/api/test_merge_requests.py b/tools/functional/api/test_merge_requests.py
index c5de5ebc5..80a650cf6 100644
--- a/tools/functional/api/test_merge_requests.py
+++ b/tools/functional/api/test_merge_requests.py
@@ -1,6 +1,9 @@
+import time
+
 import pytest
 
 import gitlab
+import gitlab.v4.objects
 
 
 def test_merge_requests(project):
@@ -95,3 +98,59 @@ def test_merge_request_merge(project):
     with pytest.raises(gitlab.GitlabMRClosedError):
         # Two merge attempts should raise GitlabMRClosedError
         mr.merge()
+
+
+def test_merge_request_should_remove_source_branch(
+    project: gitlab.v4.objects.Project, wait_for_sidekiq
+):
+    """Test to ensure https://github.com/python-gitlab/python-gitlab/issues/1120
+    is fixed"""
+
+    source_branch = "remove_source_branch"
+    project.branches.create({"branch": source_branch, "ref": "master"})
+
+    # NOTE(jlvillal): Must create a commit in the new branch before we can
+    # create an MR that will work.
+    project.files.create(
+        {
+            "file_path": f"README.{source_branch}",
+            "branch": source_branch,
+            "content": "Initial content",
+            "commit_message": "New commit in new branch",
+        }
+    )
+
+    mr = project.mergerequests.create(
+        {
+            "source_branch": source_branch,
+            "target_branch": "master",
+            "title": "Should remove source branch",
+            "remove_source_branch": True,
+        }
+    )
+
+    result = wait_for_sidekiq(timeout=60)
+    assert result is True, "sidekiq process should have terminated but did not"
+
+    mr_iid = mr.iid
+    for _ in range(60):
+        mr = project.mergerequests.get(mr_iid)
+        if mr.merge_status != "checking":
+            break
+        time.sleep(0.5)
+    assert mr.merge_status != "checking"
+
+    # Ensure we can get the MR branch
+    project.branches.get(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"
+
+    # Ensure we can NOT get the MR branch
+    with pytest.raises(gitlab.exceptions.GitlabGetError):
+        project.branches.get(source_branch)
+
+    mr = project.mergerequests.get(mr.iid)
+    assert mr.merged_at is not None  # Now is merged

From cd5993c9d638c2a10879d7e3ac36db06df867e54 Mon Sep 17 00:00:00 2001
From: "John L. Villalovos" <john@sodarock.com>
Date: Sun, 23 May 2021 14:39:55 -0700
Subject: [PATCH 2/5] chore: add functional test mr.merge() with long commit
 message

Functional test to show that
https://github.com/python-gitlab/python-gitlab/issues/1452 is fixed.

Added a functional test to ensure that we can use large commit message
(10_000+ bytes) in mr.merge()

Related to: #1452
---
 tools/functional/api/test_merge_requests.py | 59 +++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/tools/functional/api/test_merge_requests.py b/tools/functional/api/test_merge_requests.py
index 80a650cf6..abb166813 100644
--- a/tools/functional/api/test_merge_requests.py
+++ b/tools/functional/api/test_merge_requests.py
@@ -154,3 +154,62 @@ def test_merge_request_should_remove_source_branch(
 
     mr = project.mergerequests.get(mr.iid)
     assert mr.merged_at is not None  # Now is merged
+
+
+def test_merge_request_large_commit_message(
+    project: gitlab.v4.objects.Project, wait_for_sidekiq
+):
+    """Test to ensure https://github.com/python-gitlab/python-gitlab/issues/1452
+    is fixed"""
+    source_branch = "large_commit_message"
+    project.branches.create({"branch": source_branch, "ref": "master"})
+
+    # NOTE(jlvillal): Must create a commit in the new branch before we can
+    # create an MR that will work.
+    project.files.create(
+        {
+            "file_path": f"README.{source_branch}",
+            "branch": source_branch,
+            "content": "Initial content",
+            "commit_message": "New commit in new branch",
+        }
+    )
+
+    mr = project.mergerequests.create(
+        {
+            "source_branch": source_branch,
+            "target_branch": "master",
+            "title": "Large Commit Message",
+            "remove_source_branch": True,
+        }
+    )
+
+    result = wait_for_sidekiq(timeout=60)
+    assert result is True, "sidekiq process should have terminated but did not"
+
+    mr_iid = mr.iid
+    for _ in range(60):
+        mr = project.mergerequests.get(mr_iid)
+        if mr.merge_status != "checking":
+            break
+        time.sleep(0.5)
+    assert mr.merge_status != "checking"
+
+    # Ensure we can get the MR branch
+    project.branches.get(source_branch)
+
+    commit_message = "large_message\r\n" * 1_000
+    assert len(commit_message) > 10_000
+    assert mr.merged_at is None  # Not yet merged
+
+    mr.merge(merge_commit_message=commit_message, should_remove_source_branch=True)
+
+    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):
+        project.branches.get(source_branch)
+
+    mr = project.mergerequests.get(mr.iid)
+    assert mr.merged_at is not None  # Now is merged

From cb6a3c672b9b162f7320c532410713576fbd1cdc Mon Sep 17 00:00:00 2001
From: "John L. Villalovos" <john@sodarock.com>
Date: Sun, 23 May 2021 14:42:44 -0700
Subject: [PATCH 3/5] fix: change mr.merge() to use 'post_data'

MR https://github.com/python-gitlab/python-gitlab/pull/1121 changed
mr.merge() to use 'query_data'. This appears to have been wrong.

From the Gitlab docs they state it should be sent in a payload body
https://docs.gitlab.com/ee/api/README.html#request-payload since
mr.merge() is a PUT request.

  > Request Payload

  > API Requests can use parameters sent as query strings or as a
  > payload body. GET requests usually send a query string, while PUT
  > or POST requests usually send the payload body

Fixes: #1452
Related to: #1120
---
 gitlab/v4/objects/merge_requests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gitlab/v4/objects/merge_requests.py b/gitlab/v4/objects/merge_requests.py
index 9ff72f95a..3a878e257 100644
--- a/gitlab/v4/objects/merge_requests.py
+++ b/gitlab/v4/objects/merge_requests.py
@@ -342,7 +342,7 @@ def merge(
         if merge_when_pipeline_succeeds:
             data["merge_when_pipeline_succeeds"] = True
 
-        server_data = self.manager.gitlab.http_put(path, query_data=data, **kwargs)
+        server_data = self.manager.gitlab.http_put(path, post_data=data, **kwargs)
         self._update_attrs(server_data)
 
 

From df9b5f9226f704a603a7e49c78bc4543b412f898 Mon Sep 17 00:00:00 2001
From: "John L. Villalovos" <john@sodarock.com>
Date: Sun, 23 May 2021 17:38:21 -0700
Subject: [PATCH 4/5] chore: simplify functional tests

Add a helper function to have less code duplication in the functional
testing.
---
 tools/functional/api/test_merge_requests.py | 116 ++++++++++----------
 1 file changed, 57 insertions(+), 59 deletions(-)

diff --git a/tools/functional/api/test_merge_requests.py b/tools/functional/api/test_merge_requests.py
index abb166813..d6b1e9526 100644
--- a/tools/functional/api/test_merge_requests.py
+++ b/tools/functional/api/test_merge_requests.py
@@ -100,13 +100,21 @@ def test_merge_request_merge(project):
         mr.merge()
 
 
-def test_merge_request_should_remove_source_branch(
-    project: gitlab.v4.objects.Project, wait_for_sidekiq
+def merge_request_create_helper(
+    *,
+    project: gitlab.v4.objects.Project,
+    source_branch: str,
+    wait_for_sidekiq,
+    branch_will_be_deleted: bool,
+    **kwargs,
 ):
-    """Test to ensure https://github.com/python-gitlab/python-gitlab/issues/1120
-    is fixed"""
+    # Wait for processes to be done before we start...
+    # 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"
 
-    source_branch = "remove_source_branch"
     project.branches.create({"branch": source_branch, "ref": "master"})
 
     # NOTE(jlvillal): Must create a commit in the new branch before we can
@@ -143,73 +151,63 @@ def test_merge_request_should_remove_source_branch(
     # Ensure we can get the MR branch
     project.branches.get(source_branch)
 
-    mr.merge(should_remove_source_branch=True)
+    mr.merge(**kwargs)
 
     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):
-        project.branches.get(source_branch)
+    # Wait until it is merged
+    mr_iid = mr.iid
+    for _ in range(60):
+        mr = project.mergerequests.get(mr_iid)
+        if mr.merged_at is not None:
+            break
+        time.sleep(0.5)
+    assert mr.merged_at is not None
+    time.sleep(0.5)
 
-    mr = project.mergerequests.get(mr.iid)
-    assert mr.merged_at is not None  # Now is merged
+    if branch_will_be_deleted:
+        # Ensure we can NOT get the MR branch
+        with pytest.raises(gitlab.exceptions.GitlabGetError):
+            project.branches.get(source_branch)
 
 
-def test_merge_request_large_commit_message(
+def test_merge_request_should_remove_source_branch(
     project: gitlab.v4.objects.Project, wait_for_sidekiq
 ):
-    """Test to ensure https://github.com/python-gitlab/python-gitlab/issues/1452
-    is fixed"""
-    source_branch = "large_commit_message"
-    project.branches.create({"branch": source_branch, "ref": "master"})
+    """Test to ensure
+    https://github.com/python-gitlab/python-gitlab/issues/1120 is fixed.
+    Bug reported that they could not use 'should_remove_source_branch' in
+    mr.merge() call"""
 
-    # NOTE(jlvillal): Must create a commit in the new branch before we can
-    # create an MR that will work.
-    project.files.create(
-        {
-            "file_path": f"README.{source_branch}",
-            "branch": source_branch,
-            "content": "Initial content",
-            "commit_message": "New commit in new branch",
-        }
-    )
+    source_branch = "remove_source_branch"
 
-    mr = project.mergerequests.create(
-        {
-            "source_branch": source_branch,
-            "target_branch": "master",
-            "title": "Large Commit Message",
-            "remove_source_branch": True,
-        }
+    merge_request_create_helper(
+        project=project,
+        source_branch=source_branch,
+        wait_for_sidekiq=wait_for_sidekiq,
+        branch_will_be_deleted=True,
+        should_remove_source_branch=True,
     )
 
-    result = wait_for_sidekiq(timeout=60)
-    assert result is True, "sidekiq process should have terminated but did not"
-
-    mr_iid = mr.iid
-    for _ in range(60):
-        mr = project.mergerequests.get(mr_iid)
-        if mr.merge_status != "checking":
-            break
-        time.sleep(0.5)
-    assert mr.merge_status != "checking"
 
-    # Ensure we can get the MR branch
-    project.branches.get(source_branch)
-
-    commit_message = "large_message\r\n" * 1_000
-    assert len(commit_message) > 10_000
-    assert mr.merged_at is None  # Not yet merged
-
-    mr.merge(merge_commit_message=commit_message, should_remove_source_branch=True)
-
-    result = wait_for_sidekiq(timeout=60)
-    assert result is True, "sidekiq process should have terminated but did not"
+def test_merge_request_large_commit_message(
+    project: gitlab.v4.objects.Project, wait_for_sidekiq
+):
+    """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
+    cause an error: 414 Request too large
+    """
+    source_branch = "large_commit_message"
 
-    # Ensure we can NOT get the MR branch
-    with pytest.raises(gitlab.exceptions.GitlabGetError):
-        project.branches.get(source_branch)
+    merge_commit_message = "large_message\r\n" * 1_000
+    assert len(merge_commit_message) > 10_000
 
-    mr = project.mergerequests.get(mr.iid)
-    assert mr.merged_at is not None  # Now is merged
+    merge_request_create_helper(
+        project=project,
+        source_branch=source_branch,
+        wait_for_sidekiq=wait_for_sidekiq,
+        branch_will_be_deleted=False,
+        merge_commit_message=merge_commit_message,
+    )

From 8be2838a9ee3e2440d066e2c4b77cb9b55fc3da2 Mon Sep 17 00:00:00 2001
From: "John L. Villalovos" <john@sodarock.com>
Date: Mon, 24 May 2021 15:30:08 -0700
Subject: [PATCH 5/5] chore: add a merge_request() pytest fixture and use it

Added a pytest.fixture for merge_request(). Use this fixture in
tools/functional/api/test_merge_requests.py
---
 tools/functional/api/test_merge_requests.py | 116 ++++++--------------
 tools/functional/conftest.py                |  71 ++++++++++++
 2 files changed, 105 insertions(+), 82 deletions(-)

diff --git a/tools/functional/api/test_merge_requests.py b/tools/functional/api/test_merge_requests.py
index d6b1e9526..e7682345e 100644
--- a/tools/functional/api/test_merge_requests.py
+++ b/tools/functional/api/test_merge_requests.py
@@ -100,58 +100,18 @@ def test_merge_request_merge(project):
         mr.merge()
 
 
-def merge_request_create_helper(
-    *,
-    project: gitlab.v4.objects.Project,
-    source_branch: str,
-    wait_for_sidekiq,
-    branch_will_be_deleted: bool,
-    **kwargs,
-):
-    # Wait for processes to be done before we start...
-    # 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"
-
-    project.branches.create({"branch": source_branch, "ref": "master"})
-
-    # NOTE(jlvillal): Must create a commit in the new branch before we can
-    # create an MR that will work.
-    project.files.create(
-        {
-            "file_path": f"README.{source_branch}",
-            "branch": source_branch,
-            "content": "Initial content",
-            "commit_message": "New commit in new branch",
-        }
-    )
-
-    mr = project.mergerequests.create(
-        {
-            "source_branch": source_branch,
-            "target_branch": "master",
-            "title": "Should remove source branch",
-            "remove_source_branch": True,
-        }
-    )
-
-    result = wait_for_sidekiq(timeout=60)
-    assert result is True, "sidekiq process should have terminated but did not"
-
-    mr_iid = mr.iid
-    for _ in range(60):
-        mr = project.mergerequests.get(mr_iid)
-        if mr.merge_status != "checking":
-            break
-        time.sleep(0.5)
-    assert mr.merge_status != "checking"
+def test_merge_request_should_remove_source_branch(
+    project, merge_request, wait_for_sidekiq
+) -> None:
+    """Test to ensure
+    https://github.com/python-gitlab/python-gitlab/issues/1120 is fixed.
+    Bug reported that they could not use 'should_remove_source_branch' in
+    mr.merge() call"""
 
-    # Ensure we can get the MR branch
-    project.branches.get(source_branch)
+    source_branch = "remove_source_branch"
+    mr = merge_request(source_branch=source_branch)
 
-    mr.merge(**kwargs)
+    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"
@@ -166,48 +126,40 @@ def merge_request_create_helper(
     assert mr.merged_at is not None
     time.sleep(0.5)
 
-    if branch_will_be_deleted:
-        # Ensure we can NOT get the MR branch
-        with pytest.raises(gitlab.exceptions.GitlabGetError):
-            project.branches.get(source_branch)
-
-
-def test_merge_request_should_remove_source_branch(
-    project: gitlab.v4.objects.Project, wait_for_sidekiq
-):
-    """Test to ensure
-    https://github.com/python-gitlab/python-gitlab/issues/1120 is fixed.
-    Bug reported that they could not use 'should_remove_source_branch' in
-    mr.merge() call"""
-
-    source_branch = "remove_source_branch"
-
-    merge_request_create_helper(
-        project=project,
-        source_branch=source_branch,
-        wait_for_sidekiq=wait_for_sidekiq,
-        branch_will_be_deleted=True,
-        should_remove_source_branch=True,
-    )
+    # Ensure we can NOT get the MR branch
+    with pytest.raises(gitlab.exceptions.GitlabGetError):
+        project.branches.get(source_branch)
 
 
 def test_merge_request_large_commit_message(
-    project: gitlab.v4.objects.Project, wait_for_sidekiq
-):
+    project, merge_request, wait_for_sidekiq
+) -> 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
     cause an error: 414 Request too large
     """
+
     source_branch = "large_commit_message"
+    mr = merge_request(source_branch=source_branch)
 
     merge_commit_message = "large_message\r\n" * 1_000
     assert len(merge_commit_message) > 10_000
 
-    merge_request_create_helper(
-        project=project,
-        source_branch=source_branch,
-        wait_for_sidekiq=wait_for_sidekiq,
-        branch_will_be_deleted=False,
-        merge_commit_message=merge_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):
+        mr = project.mergerequests.get(mr_iid)
+        if mr.merged_at is not None:
+            break
+        time.sleep(0.5)
+    assert mr.merged_at is not None
+    time.sleep(0.5)
+
+    # Ensure we can get the MR branch
+    project.branches.get(source_branch)
diff --git a/tools/functional/conftest.py b/tools/functional/conftest.py
index 89b3dda12..634713d81 100644
--- a/tools/functional/conftest.py
+++ b/tools/functional/conftest.py
@@ -201,6 +201,77 @@ def project(gl):
         print(f"Project already deleted: {e}")
 
 
+@pytest.fixture(scope="function")
+def merge_request(project, wait_for_sidekiq):
+    """Fixture used to create a merge_request.
+
+    It will create a branch, add a commit to the branch, and then create a
+    merge request against project.default_branch. The MR will be returned.
+
+    When finished any created merge requests and branches will be deleted.
+
+    NOTE: No attempt is made to restore project.default_branch to its previous
+    state. So if the merge request is merged then its content will be in the
+    project.default_branch branch.
+    """
+
+    to_delete = []
+
+    def _merge_request(*, source_branch: str):
+        # Wait for processes to be done before we start...
+        # 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"
+
+        project.refresh()  # Gets us the current default branch
+        project.branches.create(
+            {"branch": source_branch, "ref": project.default_branch}
+        )
+        # NOTE(jlvillal): Must create a commit in the new branch before we can
+        # create an MR that will work.
+        project.files.create(
+            {
+                "file_path": f"README.{source_branch}",
+                "branch": source_branch,
+                "content": "Initial content",
+                "commit_message": "New commit in new branch",
+            }
+        )
+        mr = project.mergerequests.create(
+            {
+                "source_branch": source_branch,
+                "target_branch": project.default_branch,
+                "title": "Should remove source branch",
+                "remove_source_branch": True,
+            }
+        )
+        result = wait_for_sidekiq(timeout=60)
+        assert result is True, "sidekiq process should have terminated but did not"
+
+        mr_iid = mr.iid
+        for _ in range(60):
+            mr = project.mergerequests.get(mr_iid)
+            if mr.merge_status != "checking":
+                break
+            time.sleep(0.5)
+        assert mr.merge_status != "checking"
+
+        to_delete.append((mr.iid, source_branch))
+        return mr
+
+    yield _merge_request
+
+    for mr_iid, source_branch in to_delete:
+        project.mergerequests.delete(mr_iid)
+        try:
+            project.branches.delete(source_branch)
+        except gitlab.exceptions.GitlabDeleteError:
+            # Ignore if branch was already deleted
+            pass
+
+
 @pytest.fixture(scope="module")
 def project_file(project):
     """File fixture for tests requiring a project with files and branches."""