From 13eb461f5b9a167bc118b27ecbf0555ec07e1c2a Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Fri, 30 Sep 2016 12:01:10 +0200 Subject: [PATCH 1/3] Fix bug #1629079 and add some more tests --- openlp/plugins/songs/lib/songselect.py | 4 +- .../openlp_plugins/songs/test_songselect.py | 69 +++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/openlp/plugins/songs/lib/songselect.py b/openlp/plugins/songs/lib/songselect.py index eb9bdede4..9c4aab21a 100644 --- a/openlp/plugins/songs/lib/songselect.py +++ b/openlp/plugins/songs/lib/songselect.py @@ -114,7 +114,7 @@ class SongSelectImport(object): try: self.opener.open(LOGOUT_URL) except (TypeError, URLError) as error: - log.exception('Could not log of SongSelect, %s', error) + log.exception('Could not log out of SongSelect, %s', error) def search(self, search_text, max_results, callback=None): """ @@ -255,7 +255,7 @@ class SongSelectImport(object): topic = self.db_manager.get_object_filtered(Topic, Topic.name == topic_name) if not topic: topic = Topic.populate(name=topic_name) - db_song.add_topic(topic) + db_song.topics.append(topic) self.db_manager.save_object(db_song) return db_song diff --git a/tests/functional/openlp_plugins/songs/test_songselect.py b/tests/functional/openlp_plugins/songs/test_songselect.py index f0fcbdf51..149ec5d65 100644 --- a/tests/functional/openlp_plugins/songs/test_songselect.py +++ b/tests/functional/openlp_plugins/songs/test_songselect.py @@ -61,6 +61,30 @@ class TestSongSelectImport(TestCase, TestMixin): self.assertIsNotNone(importer.opener, 'There should be a valid opener object') self.assertEqual(1, mocked_build_opener.call_count, 'The build_opener method should have been called once') + @patch('openlp.plugins.songs.lib.songselect.build_opener') + @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') + def login_fails_type_error_test(self, MockedBeautifulSoup, mocked_build_opener): + """ + Test that when logging in to SongSelect fails due to a TypeError, the login method returns False + """ + # GIVEN: A bunch of mocked out stuff and an importer object + mocked_opener = MagicMock() + mocked_build_opener.return_value = mocked_opener + mocked_login_page = MagicMock() + mocked_login_page.find.side_effect = [{'value': 'blah'}, None] + MockedBeautifulSoup.side_effect = [mocked_login_page, TypeError('wrong type')] + mock_callback = MagicMock() + importer = SongSelectImport(None) + + # WHEN: The login method is called after being rigged to fail + result = importer.login('username', 'password', mock_callback) + + # THEN: callback was called 3 times, open was called twice, find was called twice, and False was returned + self.assertEqual(2, mock_callback.call_count, 'callback should have been called 3 times') + self.assertEqual(1, mocked_login_page.find.call_count, 'find should have been called twice') + self.assertEqual(2, mocked_opener.open.call_count, 'opener should have been called twice') + self.assertFalse(result, 'The login method should have returned False') + @patch('openlp.plugins.songs.lib.songselect.build_opener') @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') def login_fails_test(self, MockedBeautifulSoup, mocked_build_opener): @@ -143,6 +167,27 @@ class TestSongSelectImport(TestCase, TestMixin): self.assertEqual(1, mocked_opener.open.call_count, 'opener should have been called once') mocked_opener.open.assert_called_with(LOGOUT_URL) + @patch('openlp.plugins.songs.lib.songselect.build_opener') + @patch('openlp.plugins.songs.lib.songselect.log') + def logout_fails_test(self, mocked_log, mocked_build_opener): + """ + Test that when the logout method is called, it logs the user out of SongSelect + """ + # GIVEN: A bunch of mocked out stuff and an importer object + type_error = TypeError('wrong type') + mocked_opener = MagicMock() + mocked_opener.open.side_effect = type_error + mocked_build_opener.return_value = mocked_opener + importer = SongSelectImport(None) + + # WHEN: The login method is called after being rigged to fail + importer.logout() + + # THEN: The opener is called once with the logout url + self.assertEqual(1, mocked_opener.open.call_count, 'opener should have been called once') + mocked_opener.open.assert_called_with(LOGOUT_URL) + mocked_log.exception.assert_called_once_with('Could not log out of SongSelect, %s', type_error) + @patch('openlp.plugins.songs.lib.songselect.build_opener') @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') def search_returns_no_results_test(self, MockedBeautifulSoup, mocked_build_opener): @@ -168,6 +213,30 @@ class TestSongSelectImport(TestCase, TestMixin): mocked_results_page.find_all.assert_called_with('div', 'song-result') self.assertEqual([], results, 'The search method should have returned an empty list') + @patch('openlp.plugins.songs.lib.songselect.build_opener') + @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') + def search_returns_no_results_after_exception_test(self, MockedBeautifulSoup, mocked_build_opener): + """ + Test that when the search finds no results, it simply returns an empty list + """ + # GIVEN: A bunch of mocked out stuff and an importer object + mocked_opener = MagicMock() + mocked_build_opener.return_value = mocked_opener + mocked_results_page = MagicMock() + mocked_results_page.find_all.return_value = [] + MockedBeautifulSoup.side_effect = TypeError('wrong type') + mock_callback = MagicMock() + importer = SongSelectImport(None) + + # WHEN: The login method is called after being rigged to fail + results = importer.search('text', 1000, mock_callback) + + # THEN: callback was never called, open was called once, find_all was called once, an empty list returned + self.assertEqual(0, mock_callback.call_count, 'callback should not have been called') + self.assertEqual(1, mocked_opener.open.call_count, 'open should have been called once') + self.assertEqual(0, mocked_results_page.find_all.call_count, 'find_all should not have been called') + self.assertEqual([], results, 'The search method should have returned an empty list') + @patch('openlp.plugins.songs.lib.songselect.build_opener') @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') def search_returns_two_results_test(self, MockedBeautifulSoup, mocked_build_opener): From 358667356f49d58cc6ada36f9de98f1a987684db Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Fri, 30 Sep 2016 14:18:50 +0200 Subject: [PATCH 2/3] Bulk up the tests --- openlp/plugins/songs/lib/songselect.py | 1 - .../openlp_plugins/songs/test_songselect.py | 60 +++++++++++++++---- 2 files changed, 47 insertions(+), 14 deletions(-) diff --git a/openlp/plugins/songs/lib/songselect.py b/openlp/plugins/songs/lib/songselect.py index 9c4aab21a..222187ade 100644 --- a/openlp/plugins/songs/lib/songselect.py +++ b/openlp/plugins/songs/lib/songselect.py @@ -23,7 +23,6 @@ The :mod:`~openlp.plugins.songs.lib.songselect` module contains the SongSelect importer itself. """ import logging -import sys import random import re from http.cookiejar import CookieJar diff --git a/tests/functional/openlp_plugins/songs/test_songselect.py b/tests/functional/openlp_plugins/songs/test_songselect.py index 149ec5d65..fef4d4012 100644 --- a/tests/functional/openlp_plugins/songs/test_songselect.py +++ b/tests/functional/openlp_plugins/songs/test_songselect.py @@ -26,16 +26,16 @@ import os from unittest import TestCase from urllib.error import URLError +from bs4 import NavigableString from PyQt5 import QtWidgets -from tests.helpers.songfileimport import SongImportTestHelper from openlp.core import Registry from openlp.plugins.songs.forms.songselectform import SongSelectForm, SearchWorker from openlp.plugins.songs.lib import Song from openlp.plugins.songs.lib.songselect import SongSelectImport, LOGOUT_URL, BASE_URL -from openlp.plugins.songs.lib.importers.cclifile import CCLIFileImport from tests.functional import MagicMock, patch, call +from tests.helpers.songfileimport import SongImportTestHelper from tests.helpers.testmixin import TestMixin TEST_PATH = os.path.abspath( @@ -391,22 +391,47 @@ class TestSongSelectImport(TestCase, TestMixin): Test that the get_song() method returns the correct song details """ # GIVEN: A bunch of mocked out stuff and an importer object - mocked_song_page = MagicMock() - mocked_copyright = MagicMock() - mocked_copyright.find_all.return_value = [MagicMock(string='Copyright 1'), MagicMock(string='Copyright 2')] - mocked_song_page.find.side_effect = [ - mocked_copyright, - MagicMock(find=MagicMock(string='CCLI: 123456')) + mocked_ul_copyright = MagicMock() + mocked_ul_copyright.find.side_effect = [True, False] + mocked_ul_copyright.find_all.return_value = [ + 'Copyright:', + MagicMock(string='Copyright 1'), + MagicMock(string='Copyright 2') ] + + mocked_ul_themes = MagicMock() + mocked_ul_themes.find.side_effect = [False, True] + mocked_ul_themes.find_all.return_value = [ + 'Themes:', + MagicMock(string='Theme 1'), + MagicMock(string='Theme 2') + ] + + mocked_ccli = MagicMock(string='CCLI: 123456 ') + mocked_find_strong = MagicMock(return_value=mocked_ccli) + mocked_find_li = MagicMock(return_value=mocked_find_strong) + mocked_find_ul = MagicMock(return_value=mocked_find_li) + + mocked_song_page = MagicMock() + mocked_song_page.find_all.return_value = [ + mocked_ul_copyright, + mocked_ul_themes + ] + mocked_song_page.find.return_value = mocked_find_ul + mocked_lyrics_page = MagicMock() mocked_find_all = MagicMock() mocked_find_all.side_effect = [ [ MagicMock(contents='The Lord told Noah: there\'s gonna be a floody, floody'), - MagicMock(contents='So, rise and shine, and give God the glory, glory'), + MagicMock(contents=NavigableString('So, rise and shine, and give God the glory, glory')), MagicMock(contents='The Lord told Noah to build him an arky, arky') ], - [MagicMock(string='Verse 1'), MagicMock(string='Chorus'), MagicMock(string='Verse 2')] + [ + MagicMock(string='Verse 1'), + MagicMock(string='Chorus'), + MagicMock(string='Verse 2') + ] ] mocked_lyrics_page.find.return_value = MagicMock(find_all=mocked_find_all) MockedBeautifulSoup.side_effect = [mocked_song_page, mocked_lyrics_page] @@ -418,8 +443,13 @@ class TestSongSelectImport(TestCase, TestMixin): result = importer.get_song(fake_song, callback=mocked_callback) # THEN: The callback should have been called three times and the song should be returned + mocked_song_page.find_all.assert_called_once_with('ul', 'song-meta-list') + self.assertEqual(2, mocked_ul_copyright.find.call_count) + self.assertEqual(1, mocked_ul_copyright.find_all.call_count) + self.assertEqual(2, mocked_ul_themes.find.call_count) + self.assertEqual(1, mocked_ul_themes.find_all.call_count) self.assertEqual(3, mocked_callback.call_count, 'The callback should have been called twice') - self.assertIsNotNone(result, 'The get_song() method should have returned a song dictionary') + self.assertIsInstance(result, dict, 'The get_song() method should have returned a song dictionary') self.assertEqual(2, mocked_lyrics_page.find.call_count, 'The find() method should have been called twice') self.assertEqual(2, mocked_find_all.call_count, 'The find_all() method should have been called twice') self.assertEqual([call('div', 'song-viewer lyrics'), call('div', 'song-viewer lyrics')], @@ -428,7 +458,9 @@ class TestSongSelectImport(TestCase, TestMixin): self.assertEqual([call('p'), call('h3')], mocked_find_all.call_args_list, 'The find_all() method should have been called with the right arguments') self.assertIn('copyright', result, 'The returned song should have a copyright') + self.assertEqual('Copyright 1/Copyright 2', result['copyright']) self.assertIn('ccli_number', result, 'The returned song should have a CCLI number') + # self.assertEqual('123456', result['ccli_number'], result['ccli_number']) self.assertIn('verses', result, 'The returned song should have verses') self.assertEqual(3, len(result['verses']), 'Three verses should have been returned') @@ -444,7 +476,7 @@ class TestSongSelectImport(TestCase, TestMixin): 'authors': ['Public Domain'], 'verses': [ {'label': 'Verse 1', 'lyrics': 'The Lord told Noah: there\'s gonna be a floody, floody'}, - {'label': 'Chorus 1', 'lyrics': 'So, rise and shine, and give God the glory, glory'}, + {'label': 'Chorus', 'lyrics': 'So, rise and shine, and give God the glory, glory'}, {'label': 'Verse 2', 'lyrics': 'The Lord told Noah to build him an arky, arky'} ], 'copyright': 'Public Domain', @@ -520,7 +552,8 @@ class TestSongSelectImport(TestCase, TestMixin): {'label': 'Verse 2', 'lyrics': 'The Lord told Noah to build him an arky, arky'} ], 'copyright': 'Public Domain', - 'ccli_number': '123456' + 'ccli_number': '123456', + 'topics': ['Grace', 'Love'] } MockedAuthor.display_name.__eq__.return_value = False MockedTopic.name.__eq__.return_value = False @@ -540,6 +573,7 @@ class TestSongSelectImport(TestCase, TestMixin): MockedAuthor.populate.assert_called_with(first_name='Unknown', last_name='', display_name='Unknown') self.assertEqual(1, len(result.authors_songs), 'There should only be one author') + # self.assertEqual(2, len(result.topics), 'There should only be two topics') class TestSongSelectForm(TestCase, TestMixin): From b789a7809d8562cefe7332bcc2f5513fdd1ad9ad Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Sat, 1 Oct 2016 20:45:05 +0200 Subject: [PATCH 3/3] Fix up the topics problem and split it into its own test --- openlp/plugins/songs/lib/songselect.py | 1 + .../openlp_plugins/songs/test_songselect.py | 49 ++++++++++++++++--- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/openlp/plugins/songs/lib/songselect.py b/openlp/plugins/songs/lib/songselect.py index 222187ade..eddd38f6a 100644 --- a/openlp/plugins/songs/lib/songselect.py +++ b/openlp/plugins/songs/lib/songselect.py @@ -250,6 +250,7 @@ class SongSelectImport(object): last_name = name_parts[1] author = Author.populate(first_name=first_name, last_name=last_name, display_name=author_name) db_song.add_author(author) + db_song.topics = [] for topic_name in song.get('topics', []): topic = self.db_manager.get_object_filtered(Topic, Topic.name == topic_name) if not topic: diff --git a/tests/functional/openlp_plugins/songs/test_songselect.py b/tests/functional/openlp_plugins/songs/test_songselect.py index fef4d4012..aee729931 100644 --- a/tests/functional/openlp_plugins/songs/test_songselect.py +++ b/tests/functional/openlp_plugins/songs/test_songselect.py @@ -536,9 +536,8 @@ class TestSongSelectImport(TestCase, TestMixin): self.assertEqual(1, len(result.authors_songs), 'There should only be one author') @patch('openlp.plugins.songs.lib.songselect.clean_song') - @patch('openlp.plugins.songs.lib.songselect.Topic') @patch('openlp.plugins.songs.lib.songselect.Author') - def save_song_unknown_author_test(self, MockedAuthor, MockedTopic, mocked_clean_song): + def save_song_unknown_author_test(self, MockedAuthor, mocked_clean_song): """ Test that saving a song with an author name of only one word performs the correct actions """ @@ -552,6 +551,44 @@ class TestSongSelectImport(TestCase, TestMixin): {'label': 'Verse 2', 'lyrics': 'The Lord told Noah to build him an arky, arky'} ], 'copyright': 'Public Domain', + 'ccli_number': '123456' + } + MockedAuthor.display_name.__eq__.return_value = False + mocked_db_manager = MagicMock() + mocked_db_manager.get_object_filtered.return_value = None + importer = SongSelectImport(mocked_db_manager) + + # WHEN: The song is saved to the database + result = importer.save_song(song_dict) + + # THEN: The return value should be a Song class and the mocked_db_manager should have been called + self.assertIsInstance(result, Song, 'The returned value should be a Song object') + mocked_clean_song.assert_called_with(mocked_db_manager, result) + self.assertEqual(2, mocked_db_manager.save_object.call_count, + 'The save_object() method should have been called twice') + mocked_db_manager.get_object_filtered.assert_called_with(MockedAuthor, False) + MockedAuthor.populate.assert_called_with(first_name='Unknown', last_name='', + display_name='Unknown') + self.assertEqual(1, len(result.authors_songs), 'There should only be one author') + # self.assertEqual(2, len(result.topics), 'There should only be two topics') + + @patch('openlp.plugins.songs.lib.songselect.clean_song') + @patch('openlp.plugins.songs.lib.songselect.Topic') + @patch('openlp.plugins.songs.lib.songselect.Author') + def save_song_topics_test(self, MockedAuthor, MockedTopic, mocked_clean_song): + """ + Test that saving a song with topics performs the correct actions + """ + # GIVEN: A song to save, and some mocked out objects + song_dict = { + 'title': 'Arky Arky', + 'authors': ['Public Domain'], + 'verses': [ + {'label': 'Verse 1', 'lyrics': 'The Lord told Noah: there\'s gonna be a floody, floody'}, + {'label': 'Chorus', 'lyrics': 'So, rise and shine, and give God the glory, glory'}, + {'label': 'Verse 2', 'lyrics': 'The Lord told Noah to build him an arky, arky'} + ], + 'copyright': 'Public Domain', 'ccli_number': '123456', 'topics': ['Grace', 'Love'] } @@ -569,11 +606,9 @@ class TestSongSelectImport(TestCase, TestMixin): mocked_clean_song.assert_called_with(mocked_db_manager, result) self.assertEqual(2, mocked_db_manager.save_object.call_count, 'The save_object() method should have been called twice') - mocked_db_manager.get_object_filtered.assert_called_with(MockedAuthor, False) - MockedAuthor.populate.assert_called_with(first_name='Unknown', last_name='', - display_name='Unknown') - self.assertEqual(1, len(result.authors_songs), 'There should only be one author') - # self.assertEqual(2, len(result.topics), 'There should only be two topics') + mocked_db_manager.get_object_filtered.assert_called_with(MockedTopic, False) + self.assertEqual([call(name='Grace'), call(name='Love')], MockedTopic.populate.call_args_list) + self.assertEqual(2, len(result.topics), 'There should be two topics') class TestSongSelectForm(TestCase, TestMixin):