openlp-core team mailing list archive

[Phill <phill.ridout@xxxxxxxxx>]

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.