#1616 Add test_submit_module_build
Merged a month ago by mikem. Opened 6 months ago by jobrauer.
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"])

@@ -99,10 +99,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)




@@ -136,3 +136,6 @@ 


      module: testmodule

      branch: test-static-context

+   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

@@ -243,18 +248,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):
@@ -519,9 +529,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})
@@ -530,40 +541,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)
@@ -574,9 +570,10 @@ 

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

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


-         :attribute build_data (int|Build): build ID

+         :param int|Build build_data: build ID

          :return: module build object

          :rtype: Build

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


          build_id = self._get_build_id(build_data)

          url = f"{self._mbs_api}module-builds/{build_id}"
@@ -585,6 +582,29 @@ 


          return Build(self._mbs_api, r.json()["id"])


+     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()


      def wait_for_module_build(self, build_data, predicate_func, timeout=60, interval=5):

          """Wait for module build. Wait until the specified function returns True.


Add 'submit module build' test

rebased onto b788944b587f7a9b7bf9cb3cb1be55754b5f9c99

6 months ago

pretty please pagure-ci rebuild

6 months ago

rebased onto c6a0a80

6 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

5 months ago

1 new commit added

  • Rebase and adress comments
5 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
5 months ago

rebased onto 27bafd8fb607a2008cebd36113da4bef606e00f8

5 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

5 months ago

rebased onto 9357f5487338da226c7376db842992db70871083

5 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

5 months ago

rebased onto 13bb001a2f3d566eddf04e89ca320fd900280fa4

5 months ago

rebased onto e9903ee

4 months ago

rebased onto ec242a5

3 months 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.

rebased onto 3b2b22c

2 months ago

rebased onto 04cda52

a month ago

rebased onto 6385d46

a month ago

@breilly could you please look at this one as well? Thank you.

This looks much better.

I note that the rewrite of MBS.import_module() changes some error handling. I think the only reason to change this function was to pull out the krb auth code, and the only place it is called is unchanged. Seems like we'd want to preserve the behavior otherwise, right? Or is there a reason the try..except was dropped here?

I was a little surprised to see literal string interpolation used, since this feature is only in python3 and this project runs on both py2 and py3. However, looks like the integration tests use this feature heavily already, so I guess it must be ok.

A few of the changes seem like cleanup unrelated the work. I generally prefer for pull requests to stick to their topic, but I guess in this case it's fine.

@mikem thanks for the comments.

1) I feel that the error handling there is not consistent with the rest of the MBS object as more methods are being created. I'd probably put it back in the future in a more common/'polished' form. No other reason besides this. I can of course let it there for now.

2) Yeah, f strings were already heavily used in integrations, so I just carried on the tradition. Furthermore integration tests are executed separately from the rest of the project (it doesn't require the mbs code) and in a separate tox environment, which can be run only with py3 as the base python. I personally don't like integrations being apart like this - as a growing amount of code could be reused in unit tests (and vice versa), but at least for now it is what it is.

3) I can of course create separate PRs for test additions and cleanup/refactor, I just didn't want to spam with more PRs than necessary - given that it is 'just' test code. For the future: should I split the change into multiple PRs? Or more commits in one PR?

Thanks for the clarifications!

I feel that the error handling there is not consistent with the rest of the MBS object as more methods are being created

Works for me

Yeah, f strings were already heavily used in integrations, so I just carried on the tradition

I hadn't realized that the integration tests were py3 only. That should be fine. We're going to need to move the whole codebase to py3 eventually anyway.

I can of course create separate PRs for test additions and cleanup/refactor

No worries. It's fine for this one.

In general, I think that some unrelated cleanup in a PR makes sense when it's small, low-risk, and nearby the existing changes. I think it's good to keep it in a separate commit if feasible, just to make it clear what the change is about.

When someone is looking at the PR for review or later on to figure out what changed and why, it can be confusing to see unrelated changes. The reader could be wasting time wondering "what does this have to do with feature X" if feature X is all that is mentioned in the commit message(s). That situation can be improved by making the reason clear in the commit messages.

If cleanup becomes substantial, then it makes more sense to fully separate it out.

Commit 29c49dc fixes this pull-request

Pull-Request has been merged by mikem

a month ago