openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing

I think there's one instance you need to change. Please see my comments below for more guidance.

Diff comments:

> 
> === modified file 'tests/functional/openlp_plugins/bibles/test_bibleimport.py'
> --- tests/functional/openlp_plugins/bibles/test_bibleimport.py	2017-10-07 07:05:07 +0000
> +++ tests/functional/openlp_plugins/bibles/test_bibleimport.py	2017-10-11 06:59:39 +0000
> @@ -48,7 +48,7 @@
>              b'    <data><unsupported>Test<x>data</x><y>to</y>discard</unsupported></data>\n'
>              b'</root>'
>          )
> -        self.open_patcher = patch('builtins.open')
> +        self.open_patcher = patch.object(Path, 'open')

I don't see the import for this, which means it will fail. Also, you are not actually using the Path object as-is in your tests, so this is mocked out in the wrong place. You should be mocking it out where you import the object, not where the object is declared.

    self.open_patcher = patch('openlp.plugins.bibles.lib.bibleimporter.Path.open')

>          self.addCleanup(self.open_patcher.stop)
>          self.mocked_open = self.open_patcher.start()
>          self.critical_error_message_box_patcher = \
> 
> === modified file 'tests/functional/openlp_plugins/bibles/test_wordprojectimport.py'
> --- tests/functional/openlp_plugins/bibles/test_wordprojectimport.py	2017-04-24 05:17:55 +0000
> +++ tests/functional/openlp_plugins/bibles/test_wordprojectimport.py	2017-10-11 06:59:39 +0000
> @@ -48,19 +49,17 @@
>          self.addCleanup(self.manager_patcher.stop)
>          self.manager_patcher.start()
>  
> -    @patch('openlp.plugins.bibles.lib.importers.wordproject.os')
> -    @patch('openlp.plugins.bibles.lib.importers.wordproject.copen')
> -    def test_process_books(self, mocked_open, mocked_os):
> +    @patch.object(Path, 'read_text')

Presuming that the "read_text" method is attached to either the base_path or the file_path objects, then this is correct. If it's not, you should be patching it out where you import/use it.

> +    def test_process_books(self, mocked_read_text):
>          """
>          Test the process_books() method
>          """
>          # GIVEN: A WordProject importer and a bunch of mocked things
> -        importer = WordProjectBible(MagicMock(), path='.', name='.', filename='kj.zip')
> -        importer.base_dir = ''
> +        importer = WordProjectBible(MagicMock(), path='.', name='.', file_path=Path('kj.zip'))
> +        importer.base_path = Path()
>          importer.stop_import_flag = False
>          importer.language_id = 'en'
> -        mocked_open.return_value.__enter__.return_value.read.return_value = INDEX_PAGE
> -        mocked_os.path.join.side_effect = lambda *x: ''.join(x)
> +        mocked_read_text.return_value = INDEX_PAGE
>  
>          # WHEN: process_books() is called
>          with patch.object(importer, 'find_and_create_book') as mocked_find_and_create_book, \
> @@ -69,26 +68,22 @@
>              importer.process_books()
>  
>          # THEN: The right methods should have been called
> -        mocked_os.path.join.assert_called_once_with('', 'index.htm')
> -        mocked_open.assert_called_once_with('index.htm', encoding='utf-8', errors='ignore')
> +        mocked_read_text.assert_called_once_with(encoding='utf-8', errors='ignore')
>          assert mocked_find_and_create_book.call_count == 66, 'There should be 66 books'
>          assert mocked_process_chapters.call_count == 66, 'There should be 66 books'
>          assert mocked_session.commit.call_count == 66, 'There should be 66 books'
>  
> -    @patch('openlp.plugins.bibles.lib.importers.wordproject.os')
> -    @patch('openlp.plugins.bibles.lib.importers.wordproject.copen')
> -    def test_process_chapters(self, mocked_open, mocked_os):
> +    @patch.object(Path, 'read_text')

Same as my previous comment.

> +    def test_process_chapters(self, mocked_read_text):
>          """
>          Test the process_chapters() method
>          """
>          # GIVEN: A WordProject importer and a bunch of mocked things
> -        importer = WordProjectBible(MagicMock(), path='.', name='.', filename='kj.zip')
> -        importer.base_dir = ''
> +        importer = WordProjectBible(MagicMock(), path='.', name='.', file_path=Path('kj.zip'))
> +        importer.base_path = Path()
>          importer.stop_import_flag = False
>          importer.language_id = 'en'
> -        mocked_open.return_value.__enter__.return_value.read.return_value = CHAPTER_PAGE
> -        mocked_os.path.join.side_effect = lambda *x: ''.join(x)
> -        mocked_os.path.normpath.side_effect = lambda x: x
> +        mocked_read_text.return_value = CHAPTER_PAGE
>          mocked_db_book = MagicMock()
>          mocked_db_book.name = 'Genesis'
>          book_id = 1
> @@ -102,24 +97,21 @@
>          # THEN: The right methods should have been called
>          expected_set_current_chapter_calls = [call('Genesis', ch) for ch in range(1, 51)]
>          expected_process_verses_calls = [call(mocked_db_book, 1, ch) for ch in range(1, 51)]
> -        mocked_os.path.join.assert_called_once_with('', '01/1.htm')
> -        mocked_open.assert_called_once_with('01/1.htm', encoding='utf-8', errors='ignore')
> +        mocked_read_text.assert_called_once_with(encoding='utf-8', errors='ignore')
>          assert mocked_set_current_chapter.call_args_list == expected_set_current_chapter_calls
>          assert mocked_process_verses.call_args_list == expected_process_verses_calls
>  
> -    @patch('openlp.plugins.bibles.lib.importers.wordproject.os')
> -    @patch('openlp.plugins.bibles.lib.importers.wordproject.copen')
> -    def test_process_verses(self, mocked_open, mocked_os):
> +    @patch.object(Path, 'read_text')

Same as my previous comment.

> +    def test_process_verses(self, mocked_read_text):
>          """
>          Test the process_verses() method
>          """
>          # GIVEN: A WordProject importer and a bunch of mocked things
> -        importer = WordProjectBible(MagicMock(), path='.', name='.', filename='kj.zip')
> -        importer.base_dir = ''
> +        importer = WordProjectBible(MagicMock(), path='.', name='.', file_path=Path('kj.zip'))
> +        importer.base_path = Path()
>          importer.stop_import_flag = False
>          importer.language_id = 'en'
> -        mocked_open.return_value.__enter__.return_value.read.return_value = CHAPTER_PAGE
> -        mocked_os.path.join.side_effect = lambda *x: '/'.join(x)
> +        mocked_read_text.return_value = CHAPTER_PAGE
>          mocked_db_book = MagicMock()
>          mocked_db_book.name = 'Genesis'
>          book_number = 1


-- 
https://code.launchpad.net/~phill-ridout/openlp/pathlib9/+merge/332092
Your team OpenLP Core is subscribed to branch lp:openlp.