OpenLP

Merge lp:~tomasgroth/openlp/bug719514 into lp:openlp

  1. bug719514
  2. Merge into trunk
Proposed by Tomas Groth
Status: Merged
Merged at revision: 2410
Proposed branch: lp:~tomasgroth/openlp/bug719514
Merge into: lp:openlp
Diff against target: 155 lines (+85/-10)
4 files modified
openlp/core/ui/thememanager.py (+1/-1)
scripts/jenkins_script.py (+2/-1)
tests/functional/openlp_core_ui/test_thememanager.py (+80/-6)
tests/functional/openlp_plugins/songs/test_powerpraiseimport.py (+2/-2)
To merge this branch: bzr merge lp:~tomasgroth/openlp/bug719514
Reviewer Review Type Date Requested Status
Raoul Snyman Approve
Tim Bentley Approve
Samuel Mehrbrodt Pending
Review via email: mp+231727@code.launchpad.net

This proposal supersedes a proposal from 2014-08-04.

Description of the change

Fix bug 719514 + windows tests.

To post a comment you must log in.
Revision history for this message
Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal

lp:~tomasgroth/openlp/bug719514 (revision 2396)
[SUCCESS] http://ci.openlp.org/job/Branch-01-Pull/500/
[SUCCESS] http://ci.openlp.org/job/Branch-02-Functional-Tests/458/
[SUCCESS] http://ci.openlp.org/job/Branch-03-Interface-Tests/403/
[SUCCESS] http://ci.openlp.org/job/Branch-04-Windows_Tests/362/
[SUCCESS] http://ci.openlp.org/job/Branch-05a-Code_Analysis/241/
[SUCCESS] http://ci.openlp.org/job/Branch-05b-Test_Coverage/115/

Revision history for this message
Samuel Mehrbrodt (sam92) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

Some minor fixes to your tests, but otherwise looking good. Technically you also want to test the case where the files are the same (it should be simple to do if you just copy the current test and modify it where necessary)

review: Needs Fixing
Revision history for this message
Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal

lp:~tomasgroth/openlp/bug719514 (revision 2399)
[SUCCESS] http://ci.openlp.org/job/Branch-01-Pull/515/
[SUCCESS] http://ci.openlp.org/job/Branch-02-Functional-Tests/473/
[SUCCESS] http://ci.openlp.org/job/Branch-03-Interface-Tests/418/
[SUCCESS] http://ci.openlp.org/job/Branch-04-Windows_Tests/377/
[SUCCESS] http://ci.openlp.org/job/Branch-05a-Code_Analysis/246/
[SUCCESS] http://ci.openlp.org/job/Branch-05b-Test_Coverage/120/

Also fixed the tag test.

Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

Hi Tomas. My apologies, I reworked the way the bzr tag test works today and how we use it in CI. Could you merge from trunk and remove the 2.0.x tags please?

review: Needs Fixing
Revision history for this message
Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal

Merged with trunk and reverted changes to the tag test and updated jenkins script to match tests.

Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

Please re run CI tests

review: Needs Fixing
Revision history for this message
Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal

lp:~tomasgroth/openlp/bug719514 (revision 2402)
[SUCCESS] http://ci.openlp.org/job/Branch-01-Pull/552/
[SUCCESS] http://ci.openlp.org/job/Branch-02-Functional-Tests/505/
[SUCCESS] http://ci.openlp.org/job/Branch-03-Interface-Tests/449/
[SUCCESS] http://ci.openlp.org/job/Branch-04a-Windows_Functional_Tests/408/
[SUCCESS] http://ci.openlp.org/job/Branch-04b-Windows_Interface_Tests/21/
[SUCCESS] http://ci.openlp.org/job/Branch-05a-Code_Analysis/261/
[FAILURE] http://ci.openlp.org/job/Branch-05b-Test_Coverage/135/

Revision history for this message
Tim Bentley (trb143) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
Raoul Snyman (raoul-snyman) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

Needs to re roll due to conficts.
Sorry

review: Needs Fixing
Revision history for this message
Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal

Fixed the conflicts, but trunk now has the "2.2.2" tag again which also got into this branch while merging... So couldn't rerun tests on CI.

Revision history for this message
Jonathan Springer (springermac) wrote : Posted in a previous version of this proposal

You should be able to remove the extra tag by doing "bzr tag --delete 2.2.2" and then doing "bzr push --overwrite" to overwrite the branch on launchpad thus getting rid of the tag.

Revision history for this message
Tomas Groth (tomasgroth) wrote : Posted in a previous version of this proposal

> You should be able to remove the extra tag by doing "bzr tag --delete 2.2.2"
> and then doing "bzr push --overwrite" to overwrite the branch on launchpad
> thus getting rid of the tag.
I've tried that approach earlier without success...

Revision history for this message
Raoul Snyman (raoul-snyman) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

Why do we have a .moved file

review: Needs Fixing
Revision history for this message
Tomas Groth (tomasgroth) wrote :

Removed the .moved file, and fixed windows tests:
lp:~tomasgroth/openlp/bug719514 (revision 2406)
[SUCCESS] http://ci.openlp.org/job/Branch-01-Pull/577/
[SUCCESS] http://ci.openlp.org/job/Branch-02-Functional-Tests/528/
[SUCCESS] http://ci.openlp.org/job/Branch-03-Interface-Tests/472/
[SUCCESS] http://ci.openlp.org/job/Branch-04a-Windows_Functional_Tests/432/
[SUCCESS] http://ci.openlp.org/job/Branch-04b-Windows_Interface_Tests/44/
[SUCCESS] http://ci.openlp.org/job/Branch-05a-Code_Analysis/266/
[SUCCESS] http://ci.openlp.org/job/Branch-05b-Test_Coverage/140/

Revision history for this message
Tim Bentley (trb143) wrote :

Looks Ok

review: Approve
Revision history for this message
Raoul Snyman (raoul-snyman) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'openlp/core/ui/thememanager.py'
2--- openlp/core/ui/thememanager.py 2014-06-30 12:36:35 +0000
3+++ openlp/core/ui/thememanager.py 2014-08-21 13:13:41 +0000
4@@ -660,7 +660,7 @@
5 finally:
6 if out_file:
7 out_file.close()
8- if image_from and image_from != image_to:
9+ if image_from and os.path.abspath(image_from) != os.path.abspath(image_to):
10 try:
11 encoding = get_filesystem_encoding()
12 shutil.copyfile(str(image_from).encode(encoding), str(image_to).encode(encoding))
13
14=== modified file 'scripts/jenkins_script.py'
15--- scripts/jenkins_script.py 2014-07-17 21:55:54 +0000
16+++ scripts/jenkins_script.py 2014-08-21 13:13:41 +0000
17@@ -67,7 +67,8 @@
18 Branch_PEP = 'Branch-05a-Code_Analysis'
19 Branch_Coverage = 'Branch-05b-Test_Coverage'
20
21- Jobs = [Branch_Pull, Branch_Functional, Branch_Interface, Branch_Windows_Functional, Branch_Windows_Interface, Branch_PEP, Branch_Coverage]
22+ Jobs = [Branch_Pull, Branch_Functional, Branch_Interface, Branch_Windows_Functional, Branch_Windows_Interface,
23+ Branch_PEP, Branch_Coverage]
24
25
26 class Colour(object):
27
28=== modified file 'tests/functional/openlp_core_ui/test_thememanager.py'
29--- tests/functional/openlp_core_ui/test_thememanager.py 2014-06-23 13:51:56 +0000
30+++ tests/functional/openlp_core_ui/test_thememanager.py 2014-08-21 13:13:41 +0000
31@@ -36,27 +36,101 @@
32 from tests.interfaces import MagicMock
33
34 from openlp.core.ui import ThemeManager
35+from openlp.core.common import Registry
36
37-RESOURCES_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', 'resources', 'themes'))
38+from tests.utils.constants import TEST_RESOURCES_PATH
39+from tests.interfaces import MagicMock, patch
40
41
42 class TestThemeManager(TestCase):
43
44+ def setUp(self):
45+ """
46+ Set up the tests
47+ """
48+ Registry.create()
49+
50 def export_theme_test(self):
51 """
52 Test exporting a theme .
53 """
54 # GIVEN: A new ThemeManager instance.
55 theme_manager = ThemeManager()
56- theme_manager.path = RESOURCES_PATH
57+ theme_manager.path = os.path.join(TEST_RESOURCES_PATH, 'themes')
58 zipfile.ZipFile.__init__ = MagicMock()
59 zipfile.ZipFile.__init__.return_value = None
60 zipfile.ZipFile.write = MagicMock()
61
62 # WHEN: The theme is exported
63- theme_manager._export_theme('/some/path', 'Default')
64+ theme_manager._export_theme(os.path.join('some', 'path'), 'Default')
65
66 # THEN: The zipfile should be created at the given path
67- zipfile.ZipFile.__init__.assert_called_with('/some/path/Default.otz', 'w')
68- zipfile.ZipFile.write.assert_called_with(os.path.join(RESOURCES_PATH, 'Default', 'Default.xml'),
69- 'Default/Default.xml')
70+ zipfile.ZipFile.__init__.assert_called_with(os.path.join('some', 'path', 'Default.otz'), 'w')
71+ zipfile.ZipFile.write.assert_called_with(os.path.join(TEST_RESOURCES_PATH, 'themes', 'Default', 'Default.xml'),
72+ os.path.join('Default', 'Default.xml'))
73+
74+ def initial_theme_manager_test(self):
75+ """
76+ Test the instantiation of theme manager.
77+ """
78+ # GIVEN: A new service manager instance.
79+ ThemeManager(None)
80+
81+ # WHEN: the default theme manager is built.
82+ # THEN: The the controller should be registered in the registry.
83+ self.assertIsNotNone(Registry().get('theme_manager'), 'The base theme manager should be registered')
84+
85+ def write_theme_same_image_test(self):
86+ """
87+ Test that we don't try to overwrite a theme background image with itself
88+ """
89+ # GIVEN: A new theme manager instance, with mocked builtins.open, shutil.copyfile,
90+ # theme, check_directory_exists and thememanager-attributes.
91+ with patch('builtins.open') as mocked_open, \
92+ patch('openlp.core.ui.thememanager.shutil.copyfile') as mocked_copyfile, \
93+ patch('openlp.core.ui.thememanager.check_directory_exists') as mocked_check_directory_exists:
94+ mocked_open.return_value = MagicMock()
95+ theme_manager = ThemeManager(None)
96+ theme_manager.old_background_image = None
97+ theme_manager.generate_and_save_image = MagicMock()
98+ theme_manager.path = ''
99+ mocked_theme = MagicMock()
100+ mocked_theme.theme_name = 'themename'
101+ mocked_theme.extract_formatted_xml = MagicMock()
102+ mocked_theme.extract_formatted_xml.return_value = 'fake_theme_xml'.encode()
103+
104+ # WHEN: Calling _write_theme with path to the same image, but the path written slightly different
105+ file_name1 = os.path.join(TEST_RESOURCES_PATH, 'church.jpg')
106+ # Do replacement from end of string to avoid problems with path start
107+ file_name2 = file_name1[::-1].replace(os.sep, os.sep + os.sep, 2)[::-1]
108+ theme_manager._write_theme(mocked_theme, file_name1, file_name2)
109+
110+ # THEN: The mocked_copyfile should not have been called
111+ self.assertFalse(mocked_copyfile.called, 'shutil.copyfile should not be called')
112+
113+ def write_theme_diff_images_test(self):
114+ """
115+ Test that we do overwrite a theme background image when a new is submitted
116+ """
117+ # GIVEN: A new theme manager instance, with mocked builtins.open, shutil.copyfile,
118+ # theme, check_directory_exists and thememanager-attributes.
119+ with patch('builtins.open') as mocked_open, \
120+ patch('openlp.core.ui.thememanager.shutil.copyfile') as mocked_copyfile, \
121+ patch('openlp.core.ui.thememanager.check_directory_exists') as mocked_check_directory_exists:
122+ mocked_open.return_value = MagicMock()
123+ theme_manager = ThemeManager(None)
124+ theme_manager.old_background_image = None
125+ theme_manager.generate_and_save_image = MagicMock()
126+ theme_manager.path = ''
127+ mocked_theme = MagicMock()
128+ mocked_theme.theme_name = 'themename'
129+ mocked_theme.extract_formatted_xml = MagicMock()
130+ mocked_theme.extract_formatted_xml.return_value = 'fake_theme_xml'.encode()
131+
132+ # WHEN: Calling _write_theme with path to different images
133+ file_name1 = os.path.join(TEST_RESOURCES_PATH, 'church.jpg')
134+ file_name2 = os.path.join(TEST_RESOURCES_PATH, 'church2.jpg')
135+ theme_manager._write_theme(mocked_theme, file_name1, file_name2)
136+
137+ # THEN: The mocked_copyfile should not have been called
138+ self.assertTrue(mocked_copyfile.called, 'shutil.copyfile should be called')
139
140=== modified file 'tests/functional/openlp_plugins/songs/test_powerpraiseimport.py'
141--- tests/functional/openlp_plugins/songs/test_powerpraiseimport.py 2014-07-07 16:21:45 +0000
142+++ tests/functional/openlp_plugins/songs/test_powerpraiseimport.py 2014-08-21 13:13:41 +0000
143@@ -50,7 +50,7 @@
144 """
145 Test that loading a PowerPraise file works correctly
146 """
147- self.file_import([os.path.join(TEST_PATH, 'Näher, mein Gott zu Dir.ppl')],
148- self.load_external_result_data(os.path.join(TEST_PATH, 'Näher, mein Gott zu Dir.json')))
149+ self.file_import([os.path.join(TEST_PATH, 'Naher, mein Gott zu Dir.ppl')],
150+ self.load_external_result_data(os.path.join(TEST_PATH, 'Naher, mein Gott zu Dir.json')))
151 self.file_import([os.path.join(TEST_PATH, 'You are so faithful.ppl')],
152 self.load_external_result_data(os.path.join(TEST_PATH, 'You are so faithful.json')))
153
154=== renamed file 'tests/resources/powerpraisesongs/Näher, mein Gott zu Dir.json' => 'tests/resources/powerpraisesongs/Naher, mein Gott zu Dir.json'
155=== renamed file 'tests/resources/powerpraisesongs/Näher, mein Gott zu Dir.ppl' => 'tests/resources/powerpraisesongs/Naher, mein Gott zu Dir.ppl'