#1616 Add test_submit_module_build
Opened 4 months ago by jobrauer. Modified a month ago
jobrauer/fm-orchestrator test_submit_build  into  master

@@ -33,7 +33,7 @@ 

      """Fixture to interact with the packaging utility


      :return: Packaging utility configured for the tests

-     :rtype: object of utils.PackagingUtility

+     :rtype: utils.PackagingUtility


      return utils.PackagingUtility(test_env["packaging_utility"], test_env["mbs_api"])


@@ -96,10 +96,7 @@ 

      builds = pkg_util.run("--optional", "rebuild_strategy=all")

      yield repo, builds

      for build in builds:

-         try:

-             pkg_util.cancel(build)

-         except sh.ErrorReturnCode:

-             pass  # we don't need to bother with clean-up errors

+         pkg_util.cancel(build, ignore_errors=True)




@@ -110,3 +110,6 @@ 

      branch: test-highest-version-selection

      stream_module: testmodule

      test_stream: test-stream-highest-version

+   rest_submit_module_build:

+     module: testmodule

+     branch: test-submit-module

tests/integration/test_rest_build_state.py tests/integration/test_rest_api.py
file renamed
file was moved with no change to the file

@@ -0,0 +1,48 @@ 

+ from requests import HTTPError



+ def assert_build_in_build_state(mbs, build):

+     """Assert build state was reached and then cancel the build using REST."""

+     try:

+         mbs.wait_for_module_build(build, lambda bld: bld.get("state") == 2, timeout=10)

+     finally:

+         mbs.cancel_module_build(build.id)



+ def test_rest_submit_module_build(pkg_util, scenario, repo, mbs):

+     """Test module build submission. Tests only whether or not

+     build gets accepted and transitions successfully to the build state.


+     Two variants:

+       * submit module build with modulemd yaml (test YAMLFileHandler)

+       * submit module build with scmurl (test SCMHandler)

+     ..are combined into one method to reuse 1 single test branch.


+     Steps:

+       * Submit module build using module's SCM URL (HTTP POST).

+       * Assert that build reaches 'build' state.

+       * Cancel the build (HTTP PATCH)

+     """


+     # 1) SCMURL submission

+     repo.bump()


+     scmurl = pkg_util.giturl().replace("#", "?#")

+     branch = scenario["branch"]

+     data = {"scmurl": scmurl, "branch": branch}


+     builds = mbs.submit_module_build(data)

+     assert len(builds) == 1

+     assert_build_in_build_state(mbs, builds[0])


+     # 2) YAML submission (might not be enabled, but if it is, let's test it)

+     repo.bump()


+     data = {"modulemd": str(repo.modulemd)}

+     try:

+         builds = mbs.submit_module_build(data)

+     except HTTPError as e:

+         if "YAML submission is not enabled" not in e.response.text:

+             raise

+     else:

+         assert_build_in_build_state(mbs, builds[0])

file modified
+51 -31

@@ -20,6 +20,13 @@ 

  from our_sh import Command, git, pushd  # noqa



+ def get_kerberos_auth():

+     """Get the 'default' Kerberos auth header field"""

+     # (!) User executing this request must be allowed to do so on the target MBS instance.

+     # MBS server does not support mutual auth, so make it optional (inspired by mbs-cli).

+     return requests_kerberos.HTTPKerberosAuth(mutual_authentication=requests_kerberos.OPTIONAL)



  class Koji:

      """Wrapper class to work with Koji


@@ -113,14 +120,12 @@ 


      :attribute string module_name: name of the module stored in this repo

      :attribute string branch: name of the branch, the repo is checked-out

-     :attribute string giturl: GIT URL of the repo/branch

      :attribute dict _modulemd: Modulemd file as read from the repo



      def __init__(self, module_name, branch):

          self.module_name = module_name

          self.branch = branch


          self._modulemd = None

          self._version = None


@@ -224,18 +229,23 @@ 


          return stdout


-     def cancel(self, build):

+     def cancel(self, build, ignore_errors=False):

          """Cancel the module build


          :param Build build: the Build object of the module build to be cancelled.

+         :param bool ignore_errors: ignore when command fails (ErrorReturnCode exception)

          :return: Standard output of the "module-build-cancel <build id=""> command

          :rtype: str


-         stdout = self._packaging_utility("module-build-cancel", build.id).stdout.decode(

-             "utf-8")

-         return stdout

+         cmd = "module-build-cancel"

+         try:

+             return self._packaging_utility(cmd, build.id).stdout.decode("utf-8")

+         except sh.ErrorReturnCode:

+             if not ignore_errors:

+                 raise


      def giturl(self):

+         """Get target URL of the current repository/branch"""

          return self._packaging_utility("giturl").stdout.decode("utf-8").strip()


      def clone(self, *args):

@@ -491,9 +501,10 @@ 

          :param str stream: Stream name

          :param str order_desc_by: Optional sorting parameter e.g. "version"

          :return: list of Build objects

-         :rtype: list

+         :rtype: list[Build]


          url = f"{self._mbs_api}module-builds/"


          payload = {'name': module, "stream": stream}

          if order_desc_by:

              payload.update({"order_desc_by": order_desc_by})

@@ -502,40 +513,25 @@ 

          return [Build(self._mbs_api, build["id"]) for build in r.json()["items"]]


      def import_module(self, scmurl):

-         """Import module from SCM URL (modulemd).

+         """Import module from SCM URL.


          :param str scmurl:

          :return: requests response

          :rtype: requests response object


          url = f"{self._mbs_api}import-module/"

-         headers = {"Content-Type": "application/json"}

-         data = json.dumps({'scmurl': scmurl})


-         # (!) User executing this request must be allowed to do so on the target MBS instance.

-         # MBS server does not support mutual auth, so make it optional (inspired by mbs-cli).

-         auth = requests_kerberos.HTTPKerberosAuth(mutual_authentication=requests_kerberos.OPTIONAL)


-         response = requests.post(

-             url,

-             auth=auth,

-             headers=headers,

-             verify=False,

-             data=data

-         )

-         try:

-             response.raise_for_status()

-             return response

-         except requests.exceptions.HTTPError:

-             # response message contains useful information, which requests module omits

-             pytest.fail(response.text)

+         data = json.dumps({'scmurl': scmurl})

+         r = requests.post(url, auth=get_kerberos_auth(), verify=False, data=data)

+         r.raise_for_status()

+         return r


      def get_module_builds(self, **kwargs):

          """Query MBS API on module-builds endpoint


-         :attribute **kwargs: options for the HTTP GET

-         :return: list of Build objects

-         :rtype: list

+         :return: matched build entries

+         :rtype: list[Build]

+         :Keyword Arguments: passed directly to the request as HTTP params.


          url = f"{self._mbs_api}module-builds/"

          r = requests.get(url, params=kwargs)

@@ -546,9 +542,10 @@ 

      def get_module_build(self, build_id, **kwargs):

          """Query MBS API on module-builds endpoint for a specific build


-         :attribute build_id (int): build ID

+         :param int build_id: build ID

          :return: module build object

          :rtype: Build

+         :Keyword Arguments: passed directly to the request as HTTP params.


          url = f"{self._mbs_api}module-builds/{build_id}"

          r = requests.get(url, params=kwargs)

@@ -593,3 +590,26 @@ 


                  return False

          return self.wait_for_module_build(build_data, predicate, timeout, interval)


+     def submit_module_build(self, data):

+         """Submit a module build with arbitrary payload.


+         :param dict data: payload of the POST request

+             1) SCMURL submission: data = {scmurl, branch}

+             2) YAML submission: data = {modulemd: <str(modulemd as dict)>}

+         :return: submitted build(s)

+         :rtype: list[Build]

+         """

+         url = f"{self._mbs_api}module-builds/"


+         r = requests.post(url, verify=False, auth=get_kerberos_auth(), data=json.dumps(data))

+         r.raise_for_status()

+         return [Build(self._mbs_api, build["id"]) for build in r.json()]


+     def cancel_module_build(self, build_id):

+         """PATCH the state field of a module build to cancel it"""

+         url = f"{self._mbs_api}module-builds/{build_id}"


+         data = json.dumps({"state": "failed"})

+         response = requests.patch(url, auth=get_kerberos_auth(), verify=False, data=data)

+         response.raise_for_status()

Add 'submit module build' test

rebased onto b788944b587f7a9b7bf9cb3cb1be55754b5f9c99

4 months ago

pretty please pagure-ci rebuild

4 months ago

rebased onto c6a0a80

4 months ago

We should probably clarify (e.g. in a comment or docstring, perhap even a name change) how the purpose of this test is different from test_normal_build. This seems to be specifically testing the rest api directly, and cancels the build once it submits it.

I don't see much verification of the results here. Looks like we're just asserting that the length of the return is 1. Seems like we could query the state of the build and verify that it matches expectations. Seems like we could also verify the cancellation that we run.

Since this test seems to be focused on the api, I wonder if we should also use the api direction for the cancellation part.

I note you've defined an inner class. This isn't wrong, but it seems unusual in python. We don't really do that elsewhere in the code. While it is legal python, it just seems ... odd.

rebased onto 12cd38ba8471c101d5f352f83eae967c7b84c1de

3 months ago

1 new commit added

  • Rebase and adress comments
3 months ago

@mikem thanks for your comments. I tried to address all of them, however I'm not sure about how extensive the assertion should be. I guess having it reach 'build' state is good enough?

2 new commits added

  • Rebase and adress comments
  • Add test_submit_module_build
3 months ago

rebased onto 27bafd8fb607a2008cebd36113da4bef606e00f8

3 months ago

I'm not sure about how extensive the assertion should be. I guess having it reach 'build' state is good enough?

The earlier test you've grouped thing one with might serve as an example for the sorts of things you might assert. Though, now that I see these together, it seems like they are overlapping a bit. I'm sure there's a difference, but it might help to clarify that.

Also, this is breaking the intention from README.md of "each test case is implemented in a separate file".

Your refactor of the import_module() function seems to have lost the headers param that it was previously passing. Was that intentional?

Your update to the docstring for get_module_builds() has:

:param kwargs: options for the HTTP GET

I'm not sure this is the correct way to describe the function using **kwargs. This makes it sound like the user should pass a parameter named kwargs to the function call, which is not right.

rebased onto c4ec77d457ff589dbcfd68b1f3a0c85811457592

3 months ago

rebased onto 9357f5487338da226c7376db842992db70871083

3 months ago

@mikem thanks for your comments, please have a look at the latest version.

1) You are right, the two tests overlap significantly. I feel that this test does not need to do so extensive assertions, only that the build was submitted properly -> reduced.
2) Split into 2 separate files -> done.
3) Previously specified headers were not needed, the implicit ones from the library are the same.
4) I didn't find anything specific regarding the kwargs in docstring (elsewhere in the repo, they are simply omitted). Is it better like this? I can remove it altogether...

Build 9357f5487338da226c7376db842992db70871083 FAILED!
Rebase or make new commits to rebuild.

pretty please pagure-ci rebuild

3 months ago

rebased onto 13bb001a2f3d566eddf04e89ca320fd900280fa4

3 months ago

rebased onto e9903ee

2 months ago

rebased onto ec242a5

a month ago

@mikem please can you check this PR. It is open 3 months and 2 months without comment. We would like ti merge it. Thanks.