openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Raoul Snyman has proposed merging lp:~raoul-snyman/openlp/bug-1437771 into lp:openlp.

Requested reviews:
  OpenLP Core (openlp-core)
Related bugs:
  Bug #1437771 in OpenLP: "CCLISongSelect import saves multiple copies of same song"
  https://bugs.launchpad.net/openlp/+bug/1437771

For more details, see:
https://code.launchpad.net/~raoul-snyman/openlp/bug-1437771/+merge/254838

Fix bug #1437771: Clear the song after every import

- Set processed song to None after importing
- Update tests to check that song is None after importing
- Remove tests that were testing OptionParser, and replace with tests testing parse_options()
- Fix some docstring typos causing some tests to have a name of " (yes, a single double-quote)

Add this to your merge proposal:
--------------------------------
lp:~raoul-snyman/openlp/bug-1437771 (revision 2525)
[SUCCESS] https//ci.openlp.io/job/Branch-01-Pull/998/
[SUCCESS] https//ci.openlp.io/job/Branch-02-Functional-Tests/921/
[SUCCESS] https//ci.openlp.io/job/Branch-03-Interface-Tests/863/
[SUCCESS] https//ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/752/
[SUCCESS] https//ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/351/
[SUCCESS] https//ci.openlp.io/job/Branch-05a-Code_Analysis/486/
[SUCCESS] https//ci.openlp.io/job/Branch-05b-Test_Coverage/357/
-- 
Your team OpenLP Core is requested to review the proposed merge of lp:~raoul-snyman/openlp/bug-1437771 into lp:openlp.

=== modified file 'openlp/plugins/songs/forms/songselectform.py'
--- openlp/plugins/songs/forms/songselectform.py	2015-02-27 22:28:24 +0000
+++ openlp/plugins/songs/forms/songselectform.py	2015-03-31 21:06:21 +0000
@@ -367,6 +367,7 @@
         Import a song from SongSelect.
         """
         self.song_select_importer.save_song(self.song)
+        self.song = None
         if QtGui.QMessageBox.question(self, translate('SongsPlugin.SongSelectForm', 'Song Imported'),
                                       translate('SongsPlugin.SongSelectForm', 'Your song has been imported, would you '
                                                                               'like to import more songs?'),

=== modified file 'tests/functional/openlp_core_utils/test_utils.py'
--- tests/functional/openlp_core_utils/test_utils.py	2015-01-29 18:26:00 +0000
+++ tests/functional/openlp_core_utils/test_utils.py	2015-03-31 21:06:21 +0000
@@ -185,7 +185,7 @@
         self.assertEqual(wanted_name, result, 'The file name should not contain any special characters.')
 
     def delete_file_no_path_test(self):
-        """"
+        """
         Test the delete_file function when called with out a valid path
         """
         # GIVEN: A blank path
@@ -196,7 +196,7 @@
         self.assertFalse(result, "delete_file should return False when called with ''")
 
     def delete_file_path_success_test(self):
-        """"
+        """
         Test the delete_file function when it successfully deletes a file
         """
         # GIVEN: A mocked os which returns True when os.path.exists is called
@@ -209,7 +209,7 @@
             self.assertTrue(result, 'delete_file should return True when it successfully deletes a file')
 
     def delete_file_path_no_file_exists_test(self):
-        """"
+        """
         Test the delete_file function when the file to remove does not exist
         """
         # GIVEN: A mocked os which returns False when os.path.exists is called
@@ -222,7 +222,7 @@
             self.assertTrue(result, 'delete_file should return True when the file doesnt exist')
 
     def delete_file_path_exception_test(self):
-        """"
+        """
         Test the delete_file function when os.remove raises an exception
         """
         # GIVEN: A mocked os which returns True when os.path.exists is called and raises an OSError when os.remove is

=== modified file 'tests/functional/openlp_plugins/songs/test_songselect.py'
--- tests/functional/openlp_plugins/songs/test_songselect.py	2015-02-27 23:02:19 +0000
+++ tests/functional/openlp_plugins/songs/test_songselect.py	2015-03-31 21:06:21 +0000
@@ -489,6 +489,7 @@
                                            'Your song has been imported, would you like to import more songs?',
                                            QtGui.QMessageBox.Yes | QtGui.QMessageBox.No, QtGui.QMessageBox.Yes)
         mocked_on_back_button_clicked.assert_called_with()
+        self.assertIsNone(ssform.song)
 
     @patch('openlp.plugins.songs.forms.songselectform.QtGui.QMessageBox.question')
     @patch('openlp.plugins.songs.forms.songselectform.translate')
@@ -514,6 +515,7 @@
                                            'Your song has been imported, would you like to import more songs?',
                                            QtGui.QMessageBox.Yes | QtGui.QMessageBox.No, QtGui.QMessageBox.Yes)
         mocked_done.assert_called_with(QtGui.QDialog.Accepted)
+        self.assertIsNone(ssform.song)
 
     def on_back_button_clicked_test(self):
         """

=== modified file 'tests/functional/test_init.py'
--- tests/functional/test_init.py	2015-02-05 17:22:31 +0000
+++ tests/functional/test_init.py	2015-03-31 21:06:21 +0000
@@ -22,17 +22,16 @@
 """
 Package to test the openlp.core.__init__ package.
 """
-from optparse import Values
 import os
-import sys
-
 from unittest import TestCase
-from unittest.mock import MagicMock, patch
+
 from PyQt4 import QtCore, QtGui
 
 from openlp.core import OpenLP, parse_options
 from openlp.core.common import Settings
+
 from tests.helpers.testmixin import TestMixin
+from tests.functional import MagicMock, patch, call
 
 
 TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', 'resources'))
@@ -115,72 +114,59 @@
             self.assertEqual(Settings().value('core/application version'), '2.2.0', 'Version should be upgraded!')
             self.assertEqual(mocked_question.call_count, 1, 'A question should have been asked!')
 
-    def parse_options_short_options_test(self):
-        """
-        Test that parse_options parses short options correctly
-        """
-        # GIVEN: A list of valid short options
+    @patch(u'openlp.core.OptionParser')
+    def parse_options_test(self, MockedOptionParser):
+        """
+        Test that parse_options sets up OptionParser correctly and parses the options given
+        """
+        # GIVEN: A list of valid options and a mocked out OptionParser object
         options = ['-e', '-l', 'debug', '-pd', '-s', 'style', 'extra', 'qt', 'args']
-
-        # WHEN: Calling parse_options
-        results = parse_options(options)
-
-        # THEN: A tuple should be returned with the parsed options and left over options
-        self.assertEqual(results, (Values({'no_error_form': True, 'dev_version': True, 'portable': True,
-                                           'style': 'style', 'loglevel': 'debug'}), ['extra', 'qt', 'args']))
-
-    def parse_options_valid_argv_short_options_test(self):
-        """
-        Test that parse_options parses valid short options correctly when passed through sys.argv
-        """
-        # GIVEN: A list of valid options
-        options = ['openlp.py', '-e', '-l', 'debug', '-pd', '-s', 'style', 'extra', 'qt', 'args']
-
-        # WHEN: Passing in the options through sys.argv and calling parse_options with None
-        with patch.object(sys, 'argv', options):
-            results = parse_options(None)
-
-        # THEN: parse_options should return a tuple of valid options and of left over options that OpenLP does not use
-        self.assertEqual(results, (Values({'no_error_form': True, 'dev_version': True, 'portable': True,
-                                           'style': 'style', 'loglevel': 'debug'}), ['extra', 'qt', 'args']))
-
-    def test_parse_options_valid_long_options(self):
-        """
-        Test that parse_options parses valid long options correctly
-        """
-        # GIVEN: A list of valid options
-        options = ['--no-error-form', 'extra', '--log-level', 'debug', 'qt', '--portable', '--dev-version', 'args',
-                   '--style=style']
-
-        # WHEN: Calling parse_options
-        results = parse_options(options)
-
-        # THEN: parse_options should return a tuple of valid options and of left over options that OpenLP does not use
-        self.assertEqual(results, (Values({'no_error_form': True, 'dev_version': True, 'portable': True,
-                                           'style': 'style', 'loglevel': 'debug'}), ['extra', 'qt', 'args']))
-
-    def test_parse_options_help_option(self):
-        """
-        Test that parse_options raises an SystemExit exception when called with invalid options
-        """
-        # GIVEN: The --help option
-        options = ['--help']
-
-        # WHEN: Calling parse_options
-        # THEN: parse_options should raise an SystemExit exception with exception code 0
-        with self.assertRaises(SystemExit) as raised_exception:
-            parse_options(options)
-        self.assertEqual(raised_exception.exception.code, 0)
-
-    def test_parse_options_invalid_option(self):
-        """
-        Test that parse_options raises an SystemExit exception when called with invalid options
-        """
-        # GIVEN: A list including invalid options
-        options = ['-t']
-
-        # WHEN: Calling parse_options
-        # THEN: parse_options should raise an SystemExit exception with exception code 2
-        with self.assertRaises(SystemExit) as raised_exception:
-            parse_options(options)
-        self.assertEqual(raised_exception.exception.code, 2)
+        mocked_parser = MagicMock()
+        MockedOptionParser.return_value = mocked_parser
+        expected_calls = [
+            call('-e', '--no-error-form', dest='no_error_form', action='store_true',
+                 help='Disable the error notification form.'),
+            call('-l', '--log-level', dest='loglevel', default='warning', metavar='LEVEL',
+                 help='Set logging to LEVEL level. Valid values are "debug", "info", "warning".'),
+            call('-p', '--portable', dest='portable', action='store_true',
+                 help='Specify if this should be run as a portable app, off a USB flash drive (not implemented).'),
+            call('-d', '--dev-version', dest='dev_version', action='store_true',
+                 help='Ignore the version file and pull the version directly from Bazaar'),
+            call('-s', '--style', dest='style', help='Set the Qt4 style (passed directly to Qt4).')
+        ]
+
+        # WHEN: Calling parse_options
+        parse_options(options)
+
+        # THEN: A tuple should be returned with the parsed options and left over options
+        MockedOptionParser.assert_called_with(usage='Usage: %prog [options] [qt-options]')
+        self.assertEquals(expected_calls, mocked_parser.add_option.call_args_list)
+        mocked_parser.parse_args.assert_called_with(options)
+
+    @patch(u'openlp.core.OptionParser')
+    def parse_options_from_sys_argv_test(self, MockedOptionParser):
+        """
+        Test that parse_options sets up OptionParser correctly and parses sys.argv
+        """
+        # GIVEN: A list of valid options and a mocked out OptionParser object
+        mocked_parser = MagicMock()
+        MockedOptionParser.return_value = mocked_parser
+        expected_calls = [
+            call('-e', '--no-error-form', dest='no_error_form', action='store_true',
+                 help='Disable the error notification form.'),
+            call('-l', '--log-level', dest='loglevel', default='warning', metavar='LEVEL',
+                 help='Set logging to LEVEL level. Valid values are "debug", "info", "warning".'),
+            call('-p', '--portable', dest='portable', action='store_true',
+                 help='Specify if this should be run as a portable app, off a USB flash drive (not implemented).'),
+            call('-d', '--dev-version', dest='dev_version', action='store_true',
+                 help='Ignore the version file and pull the version directly from Bazaar'),
+            call('-s', '--style', dest='style', help='Set the Qt4 style (passed directly to Qt4).')
+        ]
+
+        # WHEN: Calling parse_options
+        parse_options([])
+
+        # THEN: A tuple should be returned with the parsed options and left over options
+        MockedOptionParser.assert_called_with(usage='Usage: %prog [options] [qt-options]')
+        self.assertEquals(expected_calls, mocked_parser.add_option.call_args_list)
+        mocked_parser.parse_args.assert_called_with()