It seems you are using only "name", and "stream" from body. Could you store it separately here like:

def __init__(self, ..., name, stream):
   ...
   self.name = name
   self.stream = stream

The reason is that we might support another messaging system later and we would then have to convert that message from another messaging system to fedmsg. We should also not pass real fedmsg messages out of the parser, because the format is not clear from the code - I don't know what is stored in the body.

That's just a nitpick, but it would be cool to set the msg to "bumped to rebuild because of %s update" % event.body.get('name').

I'm not sure if we want to use "fedpkg" here. Just git clone with the generated URL should be enough. In case of "fedpkg", we would have to make it configurable, so it could be executed with "rhpkg" later for example...

Same about "fedpkg" as above - you can just use git push here. You should not need to set --user here I think, because that's already set in the .git directory of cloned git repo.

Can you store this msg and also the one in "test_only_rebuild_latest_depending_modules_on_module_built_event" to "tests/fedmsgs" and use "tests.get_fedmsg" to load them. They seem to be the same and also the modulemd part is quite long. I think it would be better to keep that in separate file out of the test.

Any reason why this is everything is not stored in test_mbs.py together with that single test we have for MBS handler?

Makes sense, I was trying to keep message body as we may want to get some modulemd or something else in handler later, we can get necessary info from message in parser and assign it to event latest.

Good point.

Will update, and I was thinking about reusing MBS's scm.py, there is something we can share between several projects, it would be good to have a library package to provide some generate utilities. At this moment, we can stay with this.

I'd like to keep these test messages within the test cases, as the implementation changed, we can find and update the test message easily when there is a test case fails, I'll try to add a test message producer later, so that we can generate such test messages as wish, and won't get a big file that contains a lot of test messages.

I think we may want to have test_mbs_parser.py later, and actually planning to move the case from test_mbs.py to test_mbs_handler.py.

+1 then :)

Can you also please check the tests in jenkins? They seem to fail in that new test code.

Updated per comments, for the test failures, I'm still investigating, the cases pass on my local env with both:

FRESHMAKER_DEVELOPER_ENV=1 python tests/test_mbs_handler.py

and

FRESHMAKER_DEVELOPER_ENV=1 pytest tests/test_mbs_handler.py

But it fails with

tox -r -e py27,py35

Mocking the rebuild_module:

handler.rebuild_module = mock.Mock()

from the test cases don't take effect at all in tox env.

So the problem why tests fail is partly here. You overwrite the MBS.handle_module_built here globally for all the tests, so test_mbs_handler.py test fails then, because handle_module_built is this mocked object instead of the real method. You should not do it like this and instead do this:

diff --git a/tests/test_consumer.py b/tests/test_consumer.py
index 29436bf..beaf67c 100644
--- a/tests/test_consumer.py
+++ b/tests/test_consumer.py
@@ -33,7 +33,8 @@ class ConsumerTest(unittest.TestCase):
         pass
     @mock.patch("freshmaker.consumer.get_global_consumer")
-    def test_consumer_processing_message(self, global_consumer):
+    @mock.patch("freshmaker.handlers.mbs.MBS.handle_module_built")
+    def test_consumer_processing_message(self, handle_module_built, global_consumer):
         """
         Tests that consumer parses the message, forwards the event
         to proper handler and is able to get the further work from
@@ -55,9 +56,7 @@ class ConsumerTest(unittest.TestCase):
             }
         }}
-        from freshmaker.handlers.mbs import MBS
-        MBS.handle_module_built = mock.Mock()
-        MBS.handle_module_built.return_value = [freshmaker.events.TestingEvent("ModuleBuilt handled")]
+        handle_module_built.return_value = [freshmaker.events.TestingEvent("ModuleBuilt handled")]
         consumer.consume(msg)
         event = consumer.incoming.get()

There are more places in test_mbs_handler where you do this :).

So the problem why tests fail is partly here. You overwrite the MBS.handle_module_built here globally

Good catch, thanks. I've fixed the test failures and pushed the updated patch.

This docstring needs update as well.

Just a thought FYI, how about make this configurable?

If no need of tearDown, why not delete it?

Why not merge _run_command and _get_command_output? These two methods do many same things except return value.

For Python 2.7+, this set can be simplified

set((m.get('variant_name'), m.get('variant_version')) for m in depending_modules)

I'm also curious why need to remove duplicate pair of (name, version)?

Should this be :return: pdc_client.PDCClient instance?

I guess this if-else is required for detecting different versions of pdc_client. If yes, it would probably good to use pdc_client version rather than inspecting __init__. Because, that could be straightforward and easy to understand the purpose.

If my understand is incorrect, please correct me :)

What does it mean by these dots? :)

It is required. pdc_client 1.2.0 is New API here and pdc_client 1.1.0 is old API.

We need to get the unique result of (name, version) and then query PDC to the latest (name, version) to filter out any (name, version) in depending_modules that is not the latest one.

Thanks for all your comments, updated.

+1

This looks like an MIT license header, but freshmaker is actually GPLv2+ (like all our other projects...)

In fact it seems that all the files in freshmaker have the wrong license header, I guess you just copy-pasted this one from one of the others.