openlp-core team mailing list archive

[Raoul Snyman <raoul.snyman@xxxxxxxxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing

Lots of commented out code in the tests makes it very difficult to read what is going on. If you need to comment it out, does it really need to be there?


693	+ assert (service_item.get_display_title()) == u'Test Custom', u'The custom title is correct'

The messages in your asserts should use the phrase "... should be ..." in most cases. It doesn't make sense when the test fails and the message is "The custom title is correct" - it clearly isn't if the test is failing!


I also see you're tending to create mock instance variables when you don't always need to:

701	+ mocked_add_icon = MagicMock()
702	+ service_item.add_icon = mocked_add_icon
703	+ mocked_renderer = MagicMock()
704	+ service_item.renderer = mocked_renderer

you can just write "service_item.renderer = MagicMock()" and then type things like, "service_item.renderer.render_text.return_value = False" wherever necessary.
-- 
https://code.launchpad.net/~trb143/openlp/media/+merge/144774
Your team OpenLP Core is subscribed to branch lp:openlp.