openlp-core team mailing list archive

[Tim Bentley <tim.bentley@xxxxxxxxx>]

Review: Needs Fixing

Very nice.
3 Minor questions.

Diff comments:

> 
> === modified file 'openlp/core/ui/mainwindow.py'
> --- openlp/core/ui/mainwindow.py	2017-12-29 09:15:48 +0000
> +++ openlp/core/ui/mainwindow.py	2018-01-07 05:37:57 +0000
> @@ -1063,8 +1074,8 @@
>          :param save_settings: Switch to prevent saving settings. Defaults to **True**.
>          """
>          self.image_manager.stop_manager = True
> -        while self.image_manager.image_thread.isRunning():
> -            time.sleep(0.1)
> +        # while self.image_manager.image_thread.isRunning():
> +        #     time.sleep(0.1)

Should this be here or removed?

>          if save_settings:
>              if Settings().value('advanced/save current plugin'):
>                  Settings().setValue('advanced/current media plugin', self.media_tool_box.currentIndex())
> 
> === added file 'tests/functional/openlp_core/test_threading.py'
> --- tests/functional/openlp_core/test_threading.py	1970-01-01 00:00:00 +0000
> +++ tests/functional/openlp_core/test_threading.py	2018-01-07 05:37:57 +0000
> @@ -0,0 +1,89 @@
> +# -*- coding: utf-8 -*-
> +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
> +
> +###############################################################################
> +# OpenLP - Open Source Lyrics Projection                                      #
> +# --------------------------------------------------------------------------- #
> +# Copyright (c) 2008-2017 OpenLP Developers                                   #

Wrong year!

> +# --------------------------------------------------------------------------- #
> +# This program is free software; you can redistribute it and/or modify it     #
> +# under the terms of the GNU General Public License as published by the Free  #
> +# Software Foundation; version 2 of the License.                              #
> +#                                                                             #
> +# This program is distributed in the hope that it will be useful, but WITHOUT #
> +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or       #
> +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for    #
> +# more details.                                                               #
> +#                                                                             #
> +# You should have received a copy of the GNU General Public License along     #
> +# with this program; if not, write to the Free Software Foundation, Inc., 59  #
> +# Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
> +###############################################################################
> +"""
> +Package to test the openlp.core.threading package.
> +"""
> +from unittest.mock import MagicMock, call, patch
> +
> +from openlp.core.version import run_thread
> +
> +
> +def test_run_thread_no_name():
> +    """
> +    Test that trying to run a thread without a name results in an exception being thrown
> +    """
> +    # GIVEN: A fake worker
> +    # WHEN: run_thread() is called without a name
> +    try:
> +        run_thread(MagicMock(), '')
> +        assert False, 'A ValueError should have been thrown to prevent blank names'
> +    except ValueError:
> +        # THEN: A ValueError should have been thrown
> +        assert True, 'A ValueError was correctly thrown'
> +
> +
> +@patch('openlp.core.threading.Registry')
> +def test_run_thread_exists(MockRegistry):
> +    """
> +    Test that trying to run a thread with a name that already exists will throw a KeyError
> +    """
> +    # GIVEN: A mocked registry with a main window object
> +    mocked_main_window = MagicMock()
> +    mocked_main_window.threads = {'test_thread': MagicMock()}
> +    MockRegistry.return_value.get.return_value = mocked_main_window
> +
> +    # WHEN: run_thread() is called
> +    try:
> +        run_thread(MagicMock(), 'test_thread')
> +        assert False, 'A KeyError should have been thrown to show that a thread with this name already exists'
> +    except KeyError:
> +        assert True, 'A KeyError was correctly thrown'
> +
> +
> +@patch('openlp.core.threading.QtCore.QThread')
> +@patch('openlp.core.threading.Registry')
> +def test_run_thread(MockRegistry, MockQThread):
> +    """
> +    Test that running a thread works correctly
> +    """
> +    # GIVEN: A mocked registry with a main window object
> +    mocked_main_window = MagicMock()
> +    mocked_main_window.threads = {}
> +    MockRegistry.return_value.get.return_value = mocked_main_window
> +
> +    # WHEN: run_thread() is called
> +    run_thread(MagicMock(), 'test_thread')
> +
> +    # THEN: The thread should be in the threads list and the correct methods should have been called
> +    assert len(mocked_main_window.threads.keys()) == 1, 'There should be 1 item in the list of threads'
> +    assert list(mocked_main_window.threads.keys()) == ['test_thread'], 'The test_thread item should be in the list'
> +    mocked_worker = mocked_main_window.threads['test_thread']['worker']
> +    mocked_thread = mocked_main_window.threads['test_thread']['thread']
> +    mocked_worker.moveToThread.assert_called_once_with(mocked_thread)
> +    mocked_thread.started.connect.assert_called_once_with(mocked_worker.start)
> +    expected_quit_calls = [call(mocked_thread.quit), call(mocked_worker.deleteLater)]
> +    assert mocked_worker.quit.connect.call_args_list == expected_quit_calls, \
> +        'The workers quit signal should be connected twice'
> +    assert mocked_thread.finished.connect.call_args_list[0] == call(mocked_thread.deleteLater), \
> +        'The threads finished signal should be connected to its deleteLater slot'
> +    assert mocked_thread.finished.connect.call_count == 2, 'The signal should have been connected twice'
> +    mocked_thread.start.assert_called_once_with()
> 
> === removed file 'tests/functional/openlp_core/ui/media/test_systemplayer.py'

Why? no comments as to why?



-- 
https://code.launchpad.net/~raoul-snyman/openlp/better-threading/+merge/335801
Your team OpenLP Core is subscribed to branch lp:openlp.