Review: Needs Fixing
I bet you're beginning to wished you black balled me! ;-)
A few issues. 1st inline comment is just a comment only, I'm not expecting you
to take action on this case.
A few other inline comments after that are minor edits
Diff comments:
>
> === modified file 'tests/functional/openlp_core/lib/test_exceptions.py'
> --- tests/functional/openlp_core/lib/test_exceptions.py 2017-12-04
> 20:49:59 +0000
> +++ tests/functional/openlp_core/lib/test_exceptions.py 2017-12-17
> 21:08:00 +0000
> @@ -42,4 +42,4 @@
>
> # THEN: Then calling str on the error should return the correct text
> and it should be an instance of `Exception`
> assert str(error) == 'Test ValidationError'
> - assert isinstance(error, Exception)
> + assert isinstance(error, Exception) is True
Strictly is True is not required here! Let me explain my reasoning why its not
required here, but I've been moaning about the others.
The other 'assert is True/False/None' that I have mentioned before are because
we are checking the return value of some code under test. If the code is
supposed to return True under a set of conditions, then any other truthy values
would clearly be wrong. A return value such as string representation of True
('True') would pass the test.
Here however, we are not testing 'isinstance', so we can assume (the
documentation states so) that it will only return True or False. Whilst 'assert
isinstance(...) is True' is not wrong, neither is 'assert isinstance(...)'
>
> === modified file 'tests/functional/openlp_core/lib/test_lib.py'
> --- tests/functional/openlp_core/lib/test_lib.py 2017-11-21 07:23:02
> +0000
> +++ tests/functional/openlp_core/lib/test_lib.py 2017-12-17 21:08:00
> +0000
> @@ -286,16 +285,16 @@
> pass
>
> # Only continue when the thumb does not exist.
> - self.assertFalse(thumb_path.exists(), 'Test was not run, because the
> thumb already exists.')
> + assert thumb_path.exists() is False, 'Test was not run, because the
> thumb already exists.'
>
> # WHEN: Create the thumb.
> icon = create_thumb(image_path, thumb_path, size=thumb_size)
>
> # THEN: Check if the thumb was created and scaled to the given size.
> self.assertTrue(thumb_path.exists(), 'Test was not ran, because the
> thumb already exists')
> - self.assertIsInstance(icon, QtGui.QIcon, 'The icon should be a
> QIcon')
> - self.assertFalse(icon.isNull(), 'The icon should not be null')
> - self.assertEqual(thumb_size,
> QtGui.QImageReader(str(thumb_path)).size(), 'The thumb should have the given
> size')
> + assert isinstance(icon, QtGui.QIcon), 'The icon should be a QIcon'
> + assert icon.isNull()is False, 'The icon should not be null'
Spacing between isNull() and is
> + assert thumb_size == QtGui.QImageReader(str(thumb_path)).size(),
> 'The thumb should have the given size'
>
> # Remove the thumb so that the test actually tests if the thumb will
> be created.
> try:
>
> === modified file 'tests/functional/openlp_core/lib/test_serviceitem.py'
> --- tests/functional/openlp_core/lib/test_serviceitem.py 2017-10-10
> 07:08:44 +0000
> +++ tests/functional/openlp_core/lib/test_serviceitem.py 2017-12-17
> 21:08:00 +0000
> @@ -99,7 +113,7 @@
> service_item.set_from_service(line)
>
> # THEN: We should get back a valid service item
> - self.assertTrue(service_item.is_valid, 'The new service item should
> be valid')
> + assert service_item.is_valid, 'The new service item should be valid'
is True here please
> assert_length(0, service_item._display_frames, 'The service item
> should have no display frames')
> assert_length(5, service_item.capabilities, 'There should be 5
> default custom item capabilities')
>
> @@ -326,22 +334,22 @@
> service_item.set_from_service(line, '/test/')
>
> # THEN: We should get back a valid service item
> - self.assertTrue(service_item.is_valid, 'The new service item should
> be valid')
> - assert_length(0, service_item._display_frames, 'The service item
> should have no display frames')
> - assert_length(7, service_item.capabilities, 'There should be 7
> default custom item capabilities')
> + assert service_item.is_valid, 'The new service item should be valid'
'is True' here please
> + assert 0 == len(service_item._display_frames), 'The service item
> should have no display frames'
> + assert 7 == len(service_item.capabilities), 'There should be 7
> default custom item capabilities'
>
> # WHEN: We render the frames of the service item
> service_item.render(True)
>
> # THEN: The frames should also be valid
> - self.assertEqual('Amazing Grace', service_item.get_display_title(),
> 'The title should be "Amazing Grace"')
> - self.assertEqual(CLEANED_VERSE[:-1],
> service_item.get_frames()[0]['text'],
> - 'The returned text matches the input, except the
> last line feed')
> - self.assertEqual(RENDERED_VERSE.split('\n', 1)[0],
> service_item.get_rendered_frame(1),
> - 'The first line has been returned')
> - self.assertEqual('Amazing Grace! how sweet the s',
> service_item.get_frame_title(0),
> - '"Amazing Grace! how sweet the s" has been returned
> as the title')
> - self.assertEqual('’Twas grace that taught my hea',
> service_item.get_frame_title(1),
> - '"’Twas grace that taught my hea" has been returned
> as the title')
> - self.assertEqual('/test/amazing_grace.mp3',
> service_item.background_audio[0],
> - '"/test/amazing_grace.mp3" should be in the
> background_audio list')
> + assert 'Amazing Grace' == service_item.get_display_title(), 'The
> title should be "Amazing Grace"'
> + assert CLEANED_VERSE[:-1] == service_item.get_frames()[0]['text'], \
> + 'The returned text matches the input, except the last line feed'
> + assert RENDERED_VERSE.split('\n', 1)[0] ==
> service_item.get_rendered_frame(1), \
> + 'The first line has been returned'
> + assert 'Amazing Grace! how sweet the s' ==
> service_item.get_frame_title(0), \
> + '"Amazing Grace! how sweet the s" has been returned as the title'
> + assert '’Twas grace that taught my hea' ==
> service_item.get_frame_title(1), \
> + '"’Twas grace that taught my hea" has been returned as the title'
> + assert '/test/amazing_grace.mp3' ==
> service_item.background_audio[0], \
> + '"/test/amazing_grace.mp3" should be in the background_audio
> list'
--
https://code.launchpad.net/~trb143/openlp/asserts/+merge/335298
Your team OpenLP Core is subscribed to branch lp:openlp.
_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openlp-core
More help : https://help.launchpad.net/ListHelp
