PR#13: Fix consumer's topics are not subscribed - freshmaker

freshmaker

#13 Fix consumer's topics are not subscribed

Closed 8 years ago by qwan. Opened 8 years ago by qwan.

qwan/freshmaker fix-setting-topics into master

Fix consumer's topics are not subscribed

Qixiang Wan • 8 years ago

dbeacc8

freshmaker/consumer.py

file modified

+2 -2

		`@@ -42,10 +42,10 @@`
		`config_key = 'freshmakerconsumer'`

		`def __init__(self, hub):`
		`- super(FreshmakerConsumer, self).__init__(hub)`
		`-`
		`+ # set topic before super, otherwise topic will not be subscribed`
		`self.handlers = list(freshmaker.handlers.load_handlers())`
		`self.register_parsers()`
		`+ super(FreshmakerConsumer, self).__init__(hub)`

		`# These two values are typically provided either by the unit tests or`
		`# by the local build command. They are empty in the production environ`

tests/test_consumer.py

file modified

+18 -7

		`@@ -22,19 +22,16 @@`
		`import mock`
		`import fedmsg.config`

		`- from mock import patch`
		`- from freshmaker.consumer import FreshmakerConsumer`
		`-`
		`-`
		`- @patch("freshmaker.consumer.get_global_consumer")`
		`- class TestPoller(unittest.TestCase):`
		`+ import freshmaker`

		`+ class ConsumerTest(unittest.TestCase):`
		`def setUp(self):`
		`pass`

		`def tearDown(self):`
		`pass`

		`+ @mock.patch("freshmaker.consumer.get_global_consumer")`
		`def test_consumer_processing_message(self, global_consumer):`
		`"""`
		`Tests that consumer parses the message, forwards the event`
		`@@ -43,7 +40,7 @@`
		`"""`
		`hub = mock.MagicMock()`
		`hub.config = fedmsg.config.load_config()`
		`- consumer = FreshmakerConsumer(hub)`
		`+ consumer = freshmaker.consumer.FreshmakerConsumer(hub)`
		`global_consumer.return_value = consumer`

		`msg = {'body': {`
		`@@ -61,3 +58,17 @@`

		`event = consumer.incoming.get()`
		`self.assertEqual(event.msg_id, "ModuleBuilt handled")`
		`+`
		`+ @mock.patch("freshmaker.consumer.get_global_consumer")`
		`+ def test_consumer_subscribe_topic(self, global_consumer):`
		`+ """`
		`+ Tests subscribe topics of consumer.`
		`+ """`
		`+ hub = mock.MagicMock()`
		`+ hub.config = fedmsg.config.load_config()`
		`+ consumer = freshmaker.consumer.FreshmakerConsumer(hub)`
		`+ global_consumer.return_value = consumer`
		`+ topics = freshmaker.events.BaseEvent.get_parsed_topics()`
		`+ callback = consumer._consume_json if consumer.jsonify else consumer.consume`
		`+ for topic in topics:`
		`+ self.assertIn(mock.call(topic, callback), hub.subscribe.call_args_list)`

qwan commented 8 years ago

freshmaker consumer's topic is an instance attribute, if it is not
set prior to call parent's (which is FedmsgConsumer) init(),
the consumer will not subscribe to its topics while instantiating.

rebased

8 years ago

dcallagh commented 8 years ago

:+1: fix seems okay, although I wish the test was clearer on what exactly it's covering. The description just says "Tests subscribe topics of consumer" which could be basically anything. I like to write my test names in the style of "Test that X does Y" and then structure the test with clear setup--execute--assert phases.

If the bug is that the thing does not subscribe to the right topic... can we write a test to prove that it does subscribe to the right topic? Maybe it is testing that with the mocks (they are a little hard to comprehend) but could we do it with an actual broker, so we have more confidence that it will actually do the right thing when it's executed for real?

Anyway feel free to ignore my rant if you want.

qwan commented 8 years ago

Thanks for reviewing, the description is bad :(, I updated it in another PR #17, and will look on improve the test cases and add more test cases for testing the consumer (include the topics are subscribed indeed) in separate patch.

I'm closing this, since #17 includes the commit.

Pull-Request has been closed by qwan

8 years ago