openlp-core team mailing list archive

[Phill <phill.ridout@xxxxxxxxx>]

Review: Needs Fixing

That's quite a diff!

See inline comments.
First couple need more info
rest need fixing

Diff comments:

> 
> === modified file 'tests/functional/openlp_core/ui/media/test_systemplayer.py'
> --- tests/functional/openlp_core/ui/media/test_systemplayer.py	2017-10-07 07:05:07 +0000
> +++ tests/functional/openlp_core/ui/media/test_systemplayer.py	2017-12-20 21:28:03 +0000
> @@ -96,15 +96,15 @@
>  
>          # THEN: The player should have a display widget
>          MockQVideoWidget.assert_called_once_with(mocked_display)
> -        self.assertEqual(mocked_video_widget, mocked_display.video_widget)
> +        assert mocked_video_widget, mocked_display.video_widget is True

Not quite sure these two lines are equivalent, is this intentional?

>          mocked_display.size.assert_called_once_with()
>          mocked_video_widget.resize.assert_called_once_with([1, 2, 3, 4])
>          MockQMediaPlayer.assert_called_with(mocked_display)
> -        self.assertEqual(mocked_media_player, mocked_display.media_player)
> +        assert mocked_media_player, mocked_display.media_player is True

same here

>          mocked_media_player.setVideoOutput.assert_called_once_with(mocked_video_widget)
>          mocked_video_widget.raise_.assert_called_once_with()
>          mocked_video_widget.hide.assert_called_once_with()
> -        self.assertTrue(player.has_own_widget)
> +        assert player.has_own_widget is True
>  
>      def test_disconnect_slots(self):
>          """
> 
> === modified file 'tests/functional/openlp_core/ui/media/test_vlcplayer.py'
> --- tests/functional/openlp_core/ui/media/test_vlcplayer.py	2017-12-04 20:32:02 +0000
> +++ tests/functional/openlp_core/ui/media/test_vlcplayer.py	2017-12-20 21:28:03 +0000
> @@ -95,8 +94,7 @@
>          get_vlc()
>  
>          # THEN: The extra environment variable should NOT be there
> -        self.assertNotIn('VLC_PLUGIN_PATH', os.environ,
> -                         'The plugin path should NOT be in the environment variables')
> +        assert 'VLC_PLUGIN_PATH'not in os.environ, 'The plugin path should NOT be in the environment variables'

space between 'VLC_PLUGIN_PATH'not please

>  
>      def test_init(self):
>          """
> @@ -953,12 +951,12 @@
>  
>          # THEN: Certain methods should be called
>          mocked_stop.assert_called_with(mocked_display)
> -        self.assertEqual(2, mocked_stop.call_count)
> +        assert 2, mocked_stop.call_count

should be: assert 2 == mocked_stop.call_count

>          mocked_display.vlc_media_player.get_time.assert_called_with()
>          mocked_set_visible.assert_called_with(mocked_display, False)
>          mocked_controller.seek_slider.setSliderPosition.assert_called_with(400000)
>          expected_calls = [call(True), call(False)]
> -        self.assertEqual(expected_calls, mocked_controller.seek_slider.blockSignals.call_args_list)
> +        assert expected_calls == mocked_controller.seek_slider.blockSignals.call_args_list
>  
>      @patch('openlp.core.ui.media.vlcplayer.get_vlc')
>      def test_update_ui_dvd(self, mocked_get_vlc):
> @@ -987,12 +985,12 @@
>  
>          # THEN: Certain methods should be called
>          mocked_stop.assert_called_with(mocked_display)
> -        self.assertEqual(2, mocked_stop.call_count)
> +        assert 2, mocked_stop.call_count

again: assert 2 == mocked_stop.call_count

>          mocked_display.vlc_media_player.get_time.assert_called_with()
>          mocked_set_visible.assert_called_with(mocked_display, False)
>          mocked_controller.seek_slider.setSliderPosition.assert_called_with(300)
>          expected_calls = [call(True), call(False)]
> -        self.assertEqual(expected_calls, mocked_controller.seek_slider.blockSignals.call_args_list)
> +        assert expected_calls == mocked_controller.seek_slider.blockSignals.call_args_list
>  
>      @patch('openlp.core.ui.media.vlcplayer.translate')
>      def test_get_info(self, mocked_translate):
> 
> === modified file 'tests/functional/openlp_core/ui/test_slidecontroller.py'
> --- tests/functional/openlp_core/ui/test_slidecontroller.py	2017-11-09 21:24:38 +0000
> +++ tests/functional/openlp_core/ui/test_slidecontroller.py	2017-12-20 21:28:03 +0000
> @@ -29,12 +29,18 @@
>  
>  from openlp.core.common.registry import Registry
>  from openlp.core.lib import ServiceItemAction
> -from openlp.core.ui import SlideController, LiveController, PreviewController
> +from openlp.core.ui.slidecontroller import SlideController, LiveController, PreviewController
>  from openlp.core.ui.slidecontroller import InfoLabel, WIDE_MENU, NON_TEXT_MENU

Can these be combined and alphabetized please?

>  
>  
>  class TestSlideController(TestCase):
>  
> +    def setUp(self):
> +        """
> +        Set up the components need for all tests.
> +        """
> +        Registry.create()
> +
>      def test_initial_slide_controller(self):
>          """
>          Test the initial slide controller state .


-- 
https://code.launchpad.net/~trb143/openlp/asserts/+merge/335477
Your team OpenLP Core is subscribed to branch lp:openlp.