From a6324b6b7f3b6a0266eac81bd9e604fa6525be15 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Fri, 8 Sep 2017 22:19:22 -0700 Subject: [PATCH 01/13] Refactor version check threading --- openlp.py | 8 +- openlp/core/__init__.py | 40 ++--- openlp/core/lib/plugin.py | 4 +- openlp/core/threading.py | 55 ++++++ openlp/core/ui/aboutform.py | 4 +- openlp/core/ui/exceptionform.py | 4 +- openlp/core/ui/generaltab.py | 1 - openlp/core/ui/mainwindow.py | 17 +- .../{common/versionchecker.py => version.py} | 162 ++++++++---------- 9 files changed, 171 insertions(+), 124 deletions(-) create mode 100644 openlp/core/threading.py rename openlp/core/{common/versionchecker.py => version.py} (57%) diff --git a/openlp.py b/openlp.py index 7ede25519..68001f2d1 100755 --- a/openlp.py +++ b/openlp.py @@ -20,13 +20,17 @@ # with this program; if not, write to the Free Software Foundation, Inc., 59 # # Temple Place, Suite 330, Boston, MA 02111-1307 USA # ############################################################################### - -import sys +""" +The entrypoint for OpenLP +""" +import faulthandler import multiprocessing +import sys from openlp.core.common import is_win, is_macosx from openlp.core import main +faulthandler.enable() if __name__ == '__main__': """ diff --git a/openlp/core/__init__.py b/openlp/core/__init__.py index af29cddae..d7c21026e 100644 --- a/openlp/core/__init__.py +++ b/openlp/core/__init__.py @@ -26,21 +26,19 @@ The :mod:`core` module provides all core application functions All the core functions of the OpenLP application including the GUI, settings, logging and a plugin framework are contained within the openlp.core module. """ - import argparse import logging import os import shutil import sys import time -from pathlib import Path from traceback import format_exception from PyQt5 import QtCore, QtGui, QtWidgets from openlp.core.common import Registry, OpenLPMixin, AppLocation, LanguageManager, Settings, UiStrings, \ check_directory_exists, is_macosx, is_win, translate -from openlp.core.common.versionchecker import VersionThread, get_application_version +from openlp.core.version import check_for_update, get_version from openlp.core.lib import ScreenList from openlp.core.resources import qInitResources from openlp.core.ui import SplashScreen @@ -154,8 +152,8 @@ class OpenLP(OpenLPMixin, QtWidgets.QApplication): self.processEvents() if not has_run_wizard: self.main_window.first_time() - version = VersionThread(self.main_window) - version.start() + if Settings().value('core/update check'): + check_for_update(self.main_window) self.main_window.is_display_blank() self.main_window.app_startup() return self.exec() @@ -183,22 +181,18 @@ class OpenLP(OpenLPMixin, QtWidgets.QApplication): data_folder_path = str(AppLocation.get_data_path()) if not os.path.exists(data_folder_path): log.critical('Database was not found in: ' + data_folder_path) - status = QtWidgets.QMessageBox.critical(None, translate('OpenLP', 'Data Directory Error'), - translate('OpenLP', 'OpenLP data folder was not found in:\n\n{path}' - '\n\nThe location of the data folder was ' - 'previously changed from the OpenLP\'s ' - 'default location. If the data was stored on ' - 'removable device, that device needs to be ' - 'made available.\n\nYou may reset the data ' - 'location back to the default location, ' - 'or you can try to make the current location ' - 'available.\n\nDo you want to reset to the ' - 'default data location? If not, OpenLP will be ' - 'closed so you can try to fix the the problem.') - .format(path=data_folder_path), - QtWidgets.QMessageBox.StandardButtons(QtWidgets.QMessageBox.Yes | - QtWidgets.QMessageBox.No), - QtWidgets.QMessageBox.No) + status = QtWidgets.QMessageBox.critical( + None, translate('OpenLP', 'Data Directory Error'), + translate('OpenLP', 'OpenLP data folder was not found in:\n\n{path}\n\nThe location of the data ' + 'folder was previously changed from the OpenLP\'s default location. If the data was ' + 'stored on removable device, that device needs to be made available.\n\nYou may reset ' + 'the data location back to the default location, or you can try to make the current ' + 'location available.\n\nDo you want to reset to the default data location? If not, ' + 'OpenLP will be closed so you can try to fix the the problem.').format( + path=data_folder_path), + QtWidgets.QMessageBox.StandardButtons(QtWidgets.QMessageBox.Yes | QtWidgets.QMessageBox.No), + QtWidgets.QMessageBox.No + ) if status == QtWidgets.QMessageBox.No: # If answer was "No", return "True", it will shutdown OpenLP in def main log.info('User requested termination') @@ -239,7 +233,7 @@ class OpenLP(OpenLPMixin, QtWidgets.QApplication): :param can_show_splash: Should OpenLP show the splash screen """ data_version = Settings().value('core/application version') - openlp_version = get_application_version()['version'] + openlp_version = get_version()['version'] # New installation, no need to create backup if not has_run_wizard: Settings().setValue('core/application version', openlp_version) @@ -415,7 +409,7 @@ def main(args=None): Registry.create() Registry().register('application', application) Registry().set_flag('no_web_server', args.no_web_server) - application.setApplicationVersion(get_application_version()['version']) + application.setApplicationVersion(get_version()['version']) # Check if an instance of OpenLP is already running. Quit if there is a running instance and the user only wants one if application.is_already_running(): sys.exit() diff --git a/openlp/core/lib/plugin.py b/openlp/core/lib/plugin.py index b06e0fbd4..d06385864 100644 --- a/openlp/core/lib/plugin.py +++ b/openlp/core/lib/plugin.py @@ -27,7 +27,7 @@ import logging from PyQt5 import QtCore from openlp.core.common import Registry, RegistryProperties, Settings, UiStrings -from openlp.core.common.versionchecker import get_application_version +from openlp.core.version import get_version log = logging.getLogger(__name__) @@ -139,7 +139,7 @@ class Plugin(QtCore.QObject, RegistryProperties): if version: self.version = version else: - self.version = get_application_version()['version'] + self.version = get_version()['version'] self.settings_section = self.name self.icon = None self.media_item_class = media_item_class diff --git a/openlp/core/threading.py b/openlp/core/threading.py new file mode 100644 index 000000000..3eda2e436 --- /dev/null +++ b/openlp/core/threading.py @@ -0,0 +1,55 @@ +# -*- 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 # +# --------------------------------------------------------------------------- # +# 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 # +############################################################################### +""" +The :mod:`openlp.core.threading` module contains some common threading code +""" +from PyQt5 import QtCore + + +def run_thread(parent, worker, prefix='', auto_start=True): + """ + Create a thread and assign a worker to it. This removes a lot of boilerplate code from the codebase. + + :param object parent: The parent object so that the thread and worker are not orphaned. + :param QObject worker: A QObject-based worker object which does the actual work. + :param str prefix: A prefix to be applied to the attribute names. + :param bool auto_start: Automatically start the thread. Defaults to True. + """ + # Set up attribute names + thread_name = 'thread' + worker_name = 'worker' + if prefix: + thread_name = '_'.join([prefix, thread_name]) + worker_name = '_'.join([prefix, worker_name]) + # Create the thread and add the thread and the worker to the parent + thread = QtCore.QThread() + setattr(parent, thread_name, thread) + setattr(parent, worker_name, worker) + # Move the worker into the thread's context + worker.moveToThread(thread) + # Connect slots and signals + parent.version_thread.started.connect(parent.version_worker.start) + parent.version_worker.quit.connect(parent.version_thread.quit) + parent.version_worker.quit.connect(parent.version_worker.deleteLater) + parent.version_thread.finished.connect(parent.version_thread.deleteLater) + if auto_start: + parent.version_thread.start() diff --git a/openlp/core/ui/aboutform.py b/openlp/core/ui/aboutform.py index e1768b127..bed83785b 100644 --- a/openlp/core/ui/aboutform.py +++ b/openlp/core/ui/aboutform.py @@ -26,7 +26,7 @@ import webbrowser from PyQt5 import QtCore, QtWidgets -from openlp.core.common.versionchecker import get_application_version +from openlp.core.version import get_version from openlp.core.lib import translate from .aboutdialog import UiAboutDialog @@ -49,7 +49,7 @@ class AboutForm(QtWidgets.QDialog, UiAboutDialog): Set up the dialog. This method is mocked out in tests. """ self.setup_ui(self) - application_version = get_application_version() + application_version = get_version() about_text = self.about_text_edit.toPlainText() about_text = about_text.replace('', application_version['version']) if application_version['build']: diff --git a/openlp/core/ui/exceptionform.py b/openlp/core/ui/exceptionform.py index 349352d92..c8a1753c2 100644 --- a/openlp/core/ui/exceptionform.py +++ b/openlp/core/ui/exceptionform.py @@ -71,7 +71,7 @@ except ImportError: VLC_VERSION = '-' from openlp.core.common import Settings, UiStrings, translate -from openlp.core.common.versionchecker import get_application_version +from openlp.core.version import get_version from openlp.core.common import RegistryProperties, is_linux from .exceptiondialog import Ui_ExceptionDialog @@ -110,7 +110,7 @@ class ExceptionForm(QtWidgets.QDialog, Ui_ExceptionDialog, RegistryProperties): """ Create an exception report. """ - openlp_version = get_application_version() + openlp_version = get_version() description = self.description_text_edit.toPlainText() traceback = self.exception_text_edit.toPlainText() system = translate('OpenLP.ExceptionForm', 'Platform: {platform}\n').format(platform=platform.platform()) diff --git a/openlp/core/ui/generaltab.py b/openlp/core/ui/generaltab.py index dc084eb2b..d3c44ff6c 100644 --- a/openlp/core/ui/generaltab.py +++ b/openlp/core/ui/generaltab.py @@ -164,7 +164,6 @@ class GeneralTab(SettingsTab): self.startup_layout.addWidget(self.show_splash_check_box) self.check_for_updates_check_box = QtWidgets.QCheckBox(self.startup_group_box) self.check_for_updates_check_box.setObjectName('check_for_updates_check_box') - self.check_for_updates_check_box.setVisible(False) self.startup_layout.addWidget(self.check_for_updates_check_box) self.right_layout.addWidget(self.startup_group_box) # Logo diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 54f70ccb2..a8f7d91c7 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -40,7 +40,6 @@ from openlp.core.api.http import server from openlp.core.common import Registry, RegistryProperties, AppLocation, LanguageManager, Settings, UiStrings, \ check_directory_exists, translate, is_win, is_macosx, add_actions from openlp.core.common.actions import ActionList, CategoryOrder -from openlp.core.common.versionchecker import get_application_version from openlp.core.lib import Renderer, PluginManager, ImageManager, PluginStatus, ScreenList, build_icon from openlp.core.lib.ui import create_action from openlp.core.ui import AboutForm, SettingsForm, ServiceManager, ThemeManager, LiveController, PluginForm, \ @@ -51,6 +50,7 @@ from openlp.core.ui.printserviceform import PrintServiceForm from openlp.core.ui.projector.manager import ProjectorManager from openlp.core.ui.lib.dockwidget import OpenLPDockWidget from openlp.core.ui.lib.mediadockmanager import MediaDockManager +from openlp.core.version import get_version log = logging.getLogger(__name__) @@ -487,7 +487,6 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): """ The main window. """ - openlp_version_check = QtCore.pyqtSignal(QtCore.QVariant) log.info('MainWindow loaded') def __init__(self): @@ -561,7 +560,6 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): self.application.set_busy_cursor() # Simple message boxes Registry().register_function('theme_update_global', self.default_theme_changed) - self.openlp_version_check.connect(self.version_notice) Registry().register_function('config_screen_changed', self.screen_changed) Registry().register_function('bootstrap_post_set_up', self.bootstrap_post_set_up) # Reset the cursor @@ -587,6 +585,13 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): if saved_plugin_id != -1: self.media_tool_box.setCurrentIndex(saved_plugin_id) + def on_new_version_number(self, version_number): + """ + Called when the version check thread completes and we need to check the version number + + :param str version_number: The version number downloaded from the OpenLP server. + """ + def on_search_shortcut_triggered(self): """ Called when the search shortcut has been pressed. @@ -606,7 +611,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): if widget: widget.on_focus() - def version_notice(self, version): + def on_new_version(self, version): """ Notifies the user that a newer version of OpenLP is available. Triggered by delay thread and cannot display popup. @@ -616,7 +621,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): log.debug('version_notice') version_text = translate('OpenLP.MainWindow', 'Version {new} of OpenLP is now available for download (you are ' 'currently running version {current}). \n\nYou can download the latest version from ' - 'http://openlp.org/.').format(new=version, current=get_application_version()[u'full']) + 'http://openlp.org/.').format(new=version, current=get_version()[u'full']) QtWidgets.QMessageBox.question(self, translate('OpenLP.MainWindow', 'OpenLP Version Updated'), version_text) def show(self): @@ -973,7 +978,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): # Add a header section. # This is to insure it's our conf file for import. now = datetime.now() - application_version = get_application_version() + application_version = get_version() # Write INI format using Qsettings. # Write our header. export_settings.beginGroup(self.header_section) diff --git a/openlp/core/common/versionchecker.py b/openlp/core/version.py similarity index 57% rename from openlp/core/common/versionchecker.py rename to openlp/core/version.py index 6129ee2aa..3fa6c006c 100644 --- a/openlp/core/common/versionchecker.py +++ b/openlp/core/version.py @@ -20,24 +20,21 @@ # Temple Place, Suite 330, Boston, MA 02111-1307 USA # ############################################################################### """ -The :mod:`openlp.core.common` module downloads the version details for OpenLP. +The :mod:`openlp.core.version` module downloads the version details for OpenLP. """ import logging import os import platform import sys -import time -import urllib.error -import urllib.parse -import urllib.request from datetime import datetime from distutils.version import LooseVersion from subprocess import Popen, PIPE +import requests from PyQt5 import QtCore -from openlp.core.common import AppLocation, Registry, Settings -from openlp.core.common.httputils import ping +from openlp.core.common import AppLocation, Settings +from openlp.core.threading import run_thread log = logging.getLogger(__name__) @@ -46,42 +43,87 @@ CONNECTION_TIMEOUT = 30 CONNECTION_RETRIES = 2 -class VersionThread(QtCore.QThread): +class VersionWorker(QtCore.QObject): """ - A special Qt thread class to fetch the version of OpenLP from the website. - This is threaded so that it doesn't affect the loading time of OpenLP. + A worker class to fetch the version of OpenLP from the website. This is run from within a thread so that it + doesn't affect the loading time of OpenLP. """ - def __init__(self, main_window): - """ - Constructor for the thread class. + new_version = QtCore.pyqtSignal(dict) + no_internet = QtCore.pyqtSignal() + quit = QtCore.pyqtSignal() - :param main_window: The main window Object. + def __init__(self, last_check_date): """ - log.debug("VersionThread - Initialise") - super(VersionThread, self).__init__(None) - self.main_window = main_window + Constructor for the version check worker. - def run(self): + :param string last_check_date: The last day we checked for a new version of OpenLP """ - Run the thread. + log.debug('VersionWorker - Initialise') + super(VersionWorker, self).__init__(None) + self.last_check_date = last_check_date + + def start(self): """ - self.sleep(1) - log.debug('Version thread - run') - found = ping("openlp.io") - Registry().set_flag('internet_present', found) - update_check = Settings().value('core/update check') - if found: - Registry().execute('get_website_version') - if update_check: - app_version = get_application_version() - version = check_latest_version(app_version) - log.debug("Versions {version1} and {version2} ".format(version1=LooseVersion(str(version)), - version2=LooseVersion(str(app_version['full'])))) - if LooseVersion(str(version)) > LooseVersion(str(app_version['full'])): - self.main_window.openlp_version_check.emit('{version}'.format(version=version)) + Check the latest version of OpenLP against the version file on the OpenLP site. + + **Rules around versions and version files:** + + * If a version number has a build (i.e. -bzr1234), then it is a nightly. + * If a version number's minor version is an odd number, it is a development release. + * If a version number's minor version is an even number, it is a stable release. + """ + log.debug('VersionWorker - Start') + # I'm not entirely sure why this was here, I'm commenting it out until I hit the same scenario + # time.sleep(1) + current_version = get_version() + download_url = 'http://www.openlp.org/files/version.txt' + if current_version['build']: + download_url = 'http://www.openlp.org/files/nightly_version.txt' + elif int(current_version['version'].split('.')[1]) % 2 != 0: + download_url = 'http://www.openlp.org/files/dev_version.txt' + headers = { + 'User-Agent', 'OpenLP/{version} {system}/{release}; '.format(version=current_version['full'], + system=platform.system(), + release=platform.release()) + } + remote_version = None + retries = 0 + while retries < 3: + try: + response = requests.get(download_url, headers=headers) + remote_version = response.text + log.debug('New version found: %s', remote_version) + break + except requests.exceptions.ConnectionError: + log.exception('Unable to connect to OpenLP server to download version file') + self.no_internet.emit() + retries += 1 + except requests.exceptions.RequestException: + log.exception('Error occurred while connecting to OpenLP server to download version file') + retries += 1 + if remote_version and LooseVersion(remote_version) > LooseVersion(current_version['full']): + self.new_version.emit(remote_version) + self.quit.emit() -def get_application_version(): +def check_for_update(parent): + """ + Run a thread to download and check the version of OpenLP + + :param MainWindow parent: The parent object for the thread. Usually the OpenLP main window. + """ + last_check_date = Settings().value('core/last version test') + if datetime.date().strftime('%Y-%m-%d') <= last_check_date: + log.debug('Version check skipped, last checked today') + return + worker = VersionWorker(last_check_date) + worker.new_version.connect(parent.on_new_version) + # TODO: Use this to figure out if there's an Internet connection? + # worker.no_internet.connect(parent.on_no_internet) + run_thread(parent, worker, 'version') + + +def get_version(): """ Returns the application version of the running instance of OpenLP:: @@ -150,55 +192,3 @@ def get_application_version(): else: log.info('Openlp version {version}'.format(version=APPLICATION_VERSION['version'])) return APPLICATION_VERSION - - -def check_latest_version(current_version): - """ - Check the latest version of OpenLP against the version file on the OpenLP - site. - - **Rules around versions and version files:** - - * If a version number has a build (i.e. -bzr1234), then it is a nightly. - * If a version number's minor version is an odd number, it is a development release. - * If a version number's minor version is an even number, it is a stable release. - - :param current_version: The current version of OpenLP. - """ - version_string = current_version['full'] - # set to prod in the distribution config file. - settings = Settings() - settings.beginGroup('core') - last_test = settings.value('last version test') - this_test = str(datetime.now().date()) - settings.setValue('last version test', this_test) - settings.endGroup() - if last_test != this_test: - if current_version['build']: - req = urllib.request.Request('http://www.openlp.org/files/nightly_version.txt') - else: - version_parts = current_version['version'].split('.') - if int(version_parts[1]) % 2 != 0: - req = urllib.request.Request('http://www.openlp.org/files/dev_version.txt') - else: - req = urllib.request.Request('http://www.openlp.org/files/version.txt') - req.add_header('User-Agent', 'OpenLP/{version} {system}/{release}; '.format(version=current_version['full'], - system=platform.system(), - release=platform.release())) - remote_version = None - retries = 0 - while True: - try: - remote_version = str(urllib.request.urlopen(req, None, - timeout=CONNECTION_TIMEOUT).read().decode()).strip() - except (urllib.error.URLError, ConnectionError): - if retries > CONNECTION_RETRIES: - log.exception('Failed to download the latest OpenLP version file') - else: - retries += 1 - time.sleep(0.1) - continue - break - if remote_version: - version_string = remote_version - return version_string From 47f96a32860eaa328f756ced6df2da580a5f30e8 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Fri, 8 Sep 2017 22:57:06 -0700 Subject: [PATCH 02/13] Duh. Can't have the old names in there, now can we? --- openlp/core/threading.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/openlp/core/threading.py b/openlp/core/threading.py index 3eda2e436..36dfd481f 100644 --- a/openlp/core/threading.py +++ b/openlp/core/threading.py @@ -47,9 +47,9 @@ def run_thread(parent, worker, prefix='', auto_start=True): # Move the worker into the thread's context worker.moveToThread(thread) # Connect slots and signals - parent.version_thread.started.connect(parent.version_worker.start) - parent.version_worker.quit.connect(parent.version_thread.quit) - parent.version_worker.quit.connect(parent.version_worker.deleteLater) - parent.version_thread.finished.connect(parent.version_thread.deleteLater) + thread.started.connect(worker.start) + worker.quit.connect(thread.quit) + worker.quit.connect(worker.deleteLater) + thread.finished.connect(thread.deleteLater) if auto_start: - parent.version_thread.start() + thread.start() From d1a47036cf644828ef2cfa9ffddeca92dd877a32 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Mon, 11 Sep 2017 22:52:38 -0700 Subject: [PATCH 03/13] Remove unnecessary slot --- openlp/core/ui/mainwindow.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 4cee4a1f4..16328d63a 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -586,13 +586,6 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): if saved_plugin_id != -1: self.media_tool_box.setCurrentIndex(saved_plugin_id) - def on_new_version_number(self, version_number): - """ - Called when the version check thread completes and we need to check the version number - - :param str version_number: The version number downloaded from the OpenLP server. - """ - def on_search_shortcut_triggered(self): """ Called when the search shortcut has been pressed. From 2f8cdc81e03869ba72b5983e5848d9906cac8c56 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Tue, 12 Sep 2017 23:08:38 -0700 Subject: [PATCH 04/13] Fix up the version tests and remove old references to 'versionchecker' --- openlp/core/ui/mainwindow.py | 13 ++ openlp/core/version.py | 38 ++-- openlp/plugins/songs/lib/openlyricsxml.py | 4 +- scripts/check_dependencies.py | 7 +- tests/functional/openlp_core/test_version.py | 204 ++++++++++++++++++ .../openlp_core_common/test_versionchecker.py | 64 ------ .../openlp_core_ui/test_aboutform.py | 8 +- .../openlp_core_ui/test_exceptionform.py | 63 ++---- tests/functional/test_init.py | 12 +- 9 files changed, 273 insertions(+), 140 deletions(-) create mode 100644 tests/functional/openlp_core/test_version.py delete mode 100644 tests/functional/openlp_core_common/test_versionchecker.py diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 16328d63a..ae32c0347 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -496,6 +496,8 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): """ super(MainWindow, self).__init__() Registry().register('main_window', self) + self.version_thread = None + self.version_worker = None self.clipboard = self.application.clipboard() self.arguments = ''.join(self.application.args) # Set up settings sections for the main application (not for use by plugins). @@ -1009,6 +1011,17 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): if not self.application.is_event_loop_active: event.ignore() return + # Sometimes the version thread hasn't finished, let's wait for it + try: + if self.version_thread and self.version_thread.isRunning(): + wait_dialog = QtWidgets.QProgressDialog('Waiting for some things to finish...', '', 0, 0, self) + wait_dialog.setWindowModality(QtCore.Qt.WindowModal) + wait_dialog.setAutoClose(False) + self.version_thread.wait() + wait_dialog.close() + except RuntimeError: + # Ignore the RuntimeError that is thrown when Qt has already deleted the C++ thread object + pass # If we just did a settings import, close without saving changes. if self.settings_imported: self.clean_up(False) diff --git a/openlp/core/version.py b/openlp/core/version.py index 3fa6c006c..9a532fcd8 100644 --- a/openlp/core/version.py +++ b/openlp/core/version.py @@ -26,7 +26,8 @@ import logging import os import platform import sys -from datetime import datetime +import time +from datetime import date from distutils.version import LooseVersion from subprocess import Popen, PIPE @@ -52,7 +53,7 @@ class VersionWorker(QtCore.QObject): no_internet = QtCore.pyqtSignal() quit = QtCore.pyqtSignal() - def __init__(self, last_check_date): + def __init__(self, last_check_date, current_version): """ Constructor for the version check worker. @@ -61,6 +62,7 @@ class VersionWorker(QtCore.QObject): log.debug('VersionWorker - Initialise') super(VersionWorker, self).__init__(None) self.last_check_date = last_check_date + self.current_version = current_version def start(self): """ @@ -74,15 +76,14 @@ class VersionWorker(QtCore.QObject): """ log.debug('VersionWorker - Start') # I'm not entirely sure why this was here, I'm commenting it out until I hit the same scenario - # time.sleep(1) - current_version = get_version() + time.sleep(1) download_url = 'http://www.openlp.org/files/version.txt' - if current_version['build']: + if self.current_version['build']: download_url = 'http://www.openlp.org/files/nightly_version.txt' - elif int(current_version['version'].split('.')[1]) % 2 != 0: + elif int(self.current_version['version'].split('.')[1]) % 2 != 0: download_url = 'http://www.openlp.org/files/dev_version.txt' headers = { - 'User-Agent', 'OpenLP/{version} {system}/{release}; '.format(version=current_version['full'], + 'User-Agent': 'OpenLP/{version} {system}/{release}; '.format(version=self.current_version['full'], system=platform.system(), release=platform.release()) } @@ -94,18 +95,23 @@ class VersionWorker(QtCore.QObject): remote_version = response.text log.debug('New version found: %s', remote_version) break - except requests.exceptions.ConnectionError: + except IOError: log.exception('Unable to connect to OpenLP server to download version file') - self.no_internet.emit() retries += 1 - except requests.exceptions.RequestException: - log.exception('Error occurred while connecting to OpenLP server to download version file') - retries += 1 - if remote_version and LooseVersion(remote_version) > LooseVersion(current_version['full']): + else: + self.no_internet.emit() + if remote_version and LooseVersion(remote_version) > LooseVersion(self.current_version['full']): self.new_version.emit(remote_version) self.quit.emit() +def update_check_date(): + """ + Save when we last checked for an update + """ + Settings().setValue('core/last version test', date.today().strftime('%Y-%m-%d')) + + def check_for_update(parent): """ Run a thread to download and check the version of OpenLP @@ -113,11 +119,12 @@ def check_for_update(parent): :param MainWindow parent: The parent object for the thread. Usually the OpenLP main window. """ last_check_date = Settings().value('core/last version test') - if datetime.date().strftime('%Y-%m-%d') <= last_check_date: + if date.today().strftime('%Y-%m-%d') <= last_check_date: log.debug('Version check skipped, last checked today') return - worker = VersionWorker(last_check_date) + worker = VersionWorker(last_check_date, get_version()) worker.new_version.connect(parent.on_new_version) + worker.quit.connect(update_check_date) # TODO: Use this to figure out if there's an Internet connection? # worker.no_internet.connect(parent.on_no_internet) run_thread(parent, worker, 'version') @@ -132,6 +139,7 @@ def get_version(): global APPLICATION_VERSION if APPLICATION_VERSION: return APPLICATION_VERSION + print(sys.argv) if '--dev-version' in sys.argv or '-d' in sys.argv: # NOTE: The following code is a duplicate of the code in setup.py. Any fix applied here should also be applied # there. diff --git a/openlp/plugins/songs/lib/openlyricsxml.py b/openlp/plugins/songs/lib/openlyricsxml.py index 807ea5593..4819e61de 100644 --- a/openlp/plugins/songs/lib/openlyricsxml.py +++ b/openlp/plugins/songs/lib/openlyricsxml.py @@ -62,7 +62,7 @@ import re from lxml import etree, objectify from openlp.core.common import translate, Settings -from openlp.core.common.versionchecker import get_application_version +from openlp.core.version import get_version from openlp.core.lib import FormattingTags from openlp.plugins.songs.lib import VerseType, clean_song from openlp.plugins.songs.lib.db import Author, AuthorType, Book, Song, Topic @@ -234,7 +234,7 @@ class OpenLyrics(object): # Append the necessary meta data to the song. song_xml.set('xmlns', NAMESPACE) song_xml.set('version', OpenLyrics.IMPLEMENTED_VERSION) - application_name = 'OpenLP ' + get_application_version()['version'] + application_name = 'OpenLP ' + get_version()['version'] song_xml.set('createdIn', application_name) song_xml.set('modifiedIn', application_name) # "Convert" 2012-08-27 11:49:15 to 2012-08-27T11:49:15. diff --git a/scripts/check_dependencies.py b/scripts/check_dependencies.py index 253cb330c..1d8ffff3e 100755 --- a/scripts/check_dependencies.py +++ b/scripts/check_dependencies.py @@ -26,7 +26,7 @@ This script is used to check dependencies of OpenLP. It checks availability of required python modules and their version. To verify availability of Python modules, simply run this script:: - @:~$ ./check_dependencies.py + $ ./check_dependencies.py """ import os @@ -45,7 +45,7 @@ IS_MAC = sys.platform.startswith('dar') VERS = { - 'Python': '3.0', + 'Python': '3.4', 'PyQt5': '5.0', 'Qt5': '5.0', 'sqlalchemy': '0.5', @@ -97,7 +97,8 @@ MODULES = [ 'asyncio', 'waitress', 'six', - 'webob' + 'webob', + 'requests' ] diff --git a/tests/functional/openlp_core/test_version.py b/tests/functional/openlp_core/test_version.py new file mode 100644 index 000000000..c5790a9b6 --- /dev/null +++ b/tests/functional/openlp_core/test_version.py @@ -0,0 +1,204 @@ +# -*- 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 # +# --------------------------------------------------------------------------- # +# 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.version package. +""" +import sys +from datetime import date +from unittest.mock import MagicMock, patch + +from requests.exceptions import ConnectionError + +from openlp.core.version import VersionWorker, check_for_update, get_version, update_check_date + + +def test_worker_init(): + """Test the VersionWorker constructor""" + # GIVEN: A last check date and a current version + last_check_date = '1970-01-01' + current_version = '2.0' + + # WHEN: A worker is created + worker = VersionWorker(last_check_date, current_version) + + # THEN: The correct attributes should have been set + assert worker.last_check_date == last_check_date + assert worker.current_version == current_version + + +@patch('openlp.core.version.platform') +@patch('openlp.core.version.requests') +def test_worker_start(mock_requests, mock_platform): + """Test the VersionWorkder.start() method""" + # GIVEN: A last check date, current version, and an instance of worker + last_check_date = '1970-01-01' + current_version = {'full': '2.0', 'version': '2.0', 'build': None} + mock_platform.system.return_value = 'Linux' + mock_platform.release.return_value = '4.12.0-1-amd64' + mock_requests.get.return_value = MagicMock(text='2.4.6') + worker = VersionWorker(last_check_date, current_version) + + # WHEN: The worker is run + with patch.object(worker, 'new_version') as mock_new_version, \ + patch.object(worker, 'quit') as mock_quit: + worker.start() + + # THEN: The check completes and the signal is emitted + expected_download_url = 'http://www.openlp.org/files/version.txt' + expected_headers = {'User-Agent': 'OpenLP/2.0 Linux/4.12.0-1-amd64; '} + mock_requests.get.assert_called_once_with(expected_download_url, headers=expected_headers) + mock_new_version.emit.assert_called_once_with('2.4.6') + mock_quit.emit.assert_called_once_with() + + +@patch('openlp.core.version.platform') +@patch('openlp.core.version.requests') +def test_worker_start_dev_version(mock_requests, mock_platform): + """Test the VersionWorkder.start() method for dev versions""" + # GIVEN: A last check date, current version, and an instance of worker + last_check_date = '1970-01-01' + current_version = {'full': '2.1.3', 'version': '2.1.3', 'build': None} + mock_platform.system.return_value = 'Linux' + mock_platform.release.return_value = '4.12.0-1-amd64' + mock_requests.get.return_value = MagicMock(text='2.4.6') + worker = VersionWorker(last_check_date, current_version) + + # WHEN: The worker is run + with patch.object(worker, 'new_version') as mock_new_version, \ + patch.object(worker, 'quit') as mock_quit: + worker.start() + + # THEN: The check completes and the signal is emitted + expected_download_url = 'http://www.openlp.org/files/dev_version.txt' + expected_headers = {'User-Agent': 'OpenLP/2.1.3 Linux/4.12.0-1-amd64; '} + mock_requests.get.assert_called_once_with(expected_download_url, headers=expected_headers) + mock_new_version.emit.assert_called_once_with('2.4.6') + mock_quit.emit.assert_called_once_with() + + +@patch('openlp.core.version.platform') +@patch('openlp.core.version.requests') +def test_worker_start_nightly_version(mock_requests, mock_platform): + """Test the VersionWorkder.start() method for nightlies""" + # GIVEN: A last check date, current version, and an instance of worker + last_check_date = '1970-01-01' + current_version = {'full': '2.1-bzr2345', 'version': '2.1', 'build': '2345'} + mock_platform.system.return_value = 'Linux' + mock_platform.release.return_value = '4.12.0-1-amd64' + mock_requests.get.return_value = MagicMock(text='2.4.6') + worker = VersionWorker(last_check_date, current_version) + + # WHEN: The worker is run + with patch.object(worker, 'new_version') as mock_new_version, \ + patch.object(worker, 'quit') as mock_quit: + worker.start() + + # THEN: The check completes and the signal is emitted + expected_download_url = 'http://www.openlp.org/files/nightly_version.txt' + expected_headers = {'User-Agent': 'OpenLP/2.1-bzr2345 Linux/4.12.0-1-amd64; '} + mock_requests.get.assert_called_once_with(expected_download_url, headers=expected_headers) + mock_new_version.emit.assert_called_once_with('2.4.6') + mock_quit.emit.assert_called_once_with() + + +@patch('openlp.core.version.platform') +@patch('openlp.core.version.requests') +def test_worker_start_connection_error(mock_requests, mock_platform): + """Test the VersionWorkder.start() method when a ConnectionError happens""" + # GIVEN: A last check date, current version, and an instance of worker + last_check_date = '1970-01-01' + current_version = {'full': '2.0', 'version': '2.0', 'build': None} + mock_platform.system.return_value = 'Linux' + mock_platform.release.return_value = '4.12.0-1-amd64' + mock_requests.get.side_effect = ConnectionError('Could not connect') + worker = VersionWorker(last_check_date, current_version) + + # WHEN: The worker is run + with patch.object(worker, 'no_internet') as mocked_no_internet, \ + patch.object(worker, 'quit') as mocked_quit: + worker.start() + + # THEN: The check completes and the signal is emitted + expected_download_url = 'http://www.openlp.org/files/version.txt' + expected_headers = {'User-Agent': 'OpenLP/2.0 Linux/4.12.0-1-amd64; '} + mock_requests.get.assert_called_with(expected_download_url, headers=expected_headers) + assert mock_requests.get.call_count == 3 + mocked_no_internet.emit.assert_called_once_with() + mocked_quit.emit.assert_called_once_with() + + +@patch('openlp.core.version.Settings') +def test_update_check_date(MockSettings): + """Test that the update_check_date() function writes the correct date""" + # GIVEN: A mocked Settings object + mocked_settings = MagicMock() + MockSettings.return_value = mocked_settings + + # WHEN: update_check_date() is called + update_check_date() + + # THEN: The correct date should have been saved + mocked_settings.setValue.assert_called_once_with('core/last version test', date.today().strftime('%Y-%m-%d')) + + +@patch('openlp.core.version.Settings') +@patch('openlp.core.version.run_thread') +def test_check_for_update(mocked_run_thread, MockSettings): + """Test the check_for_update() function""" + # GIVEN: A mocked settings object + mocked_settings = MagicMock() + mocked_settings.value.return_value = '1970-01-01' + MockSettings.return_value = mocked_settings + + # WHEN: check_for_update() is called + check_for_update(MagicMock()) + + # THEN: The right things should have been called and a thread set in motion + assert mocked_run_thread.call_count == 1 + + +@patch('openlp.core.version.Settings') +@patch('openlp.core.version.run_thread') +def test_check_for_update_skipped(mocked_run_thread, MockSettings): + """Test that the check_for_update() function skips running if it already ran today""" + # GIVEN: A mocked settings object + mocked_settings = MagicMock() + mocked_settings.value.return_value = date.today().strftime('%Y-%m-%d') + MockSettings.return_value = mocked_settings + + # WHEN: check_for_update() is called + check_for_update(MagicMock()) + + # THEN: The right things should have been called and a thread set in motion + assert mocked_run_thread.call_count == 0 + + +def test_get_version_dev_version(): + """Test the get_version() function""" + # GIVEN: We're in dev mode + with patch.object(sys, 'argv', ['--dev-version']), \ + patch('openlp.core.version.APPLICATION_VERSION', None): + # WHEN: get_version() is run + version = get_version() + + # THEN: version is something + assert version diff --git a/tests/functional/openlp_core_common/test_versionchecker.py b/tests/functional/openlp_core_common/test_versionchecker.py deleted file mode 100644 index 022a5f06f..000000000 --- a/tests/functional/openlp_core_common/test_versionchecker.py +++ /dev/null @@ -1,64 +0,0 @@ -# -*- 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 # -# --------------------------------------------------------------------------- # -# 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.common.versionchecker package. -""" -from unittest import TestCase -from unittest.mock import MagicMock, patch - -from openlp.core.common.settings import Settings -from openlp.core.common.versionchecker import VersionThread - -from tests.helpers.testmixin import TestMixin - - -class TestVersionchecker(TestMixin, TestCase): - - def setUp(self): - """ - Create an instance and a few example actions. - """ - self.build_settings() - - def tearDown(self): - """ - Clean up - """ - self.destroy_settings() - - def test_version_thread_triggered(self): - """ - Test the version thread call does not trigger UI - :return: - """ - # GIVEN: a equal version setup and the data is not today. - mocked_main_window = MagicMock() - Settings().setValue('core/last version test', '1950-04-01') - # WHEN: We check to see if the version is different . - with patch('PyQt5.QtCore.QThread'),\ - patch('openlp.core.common.versionchecker.get_application_version') as mocked_get_application_version: - mocked_get_application_version.return_value = {'version': '1.0.0', 'build': '', 'full': '2.0.4'} - version_thread = VersionThread(mocked_main_window) - version_thread.run() - # THEN: If the version has changed the main window is notified - self.assertTrue(mocked_main_window.openlp_version_check.emit.called, - 'The main windows should have been notified') diff --git a/tests/functional/openlp_core_ui/test_aboutform.py b/tests/functional/openlp_core_ui/test_aboutform.py index 817c13c48..c30ef588e 100644 --- a/tests/functional/openlp_core_ui/test_aboutform.py +++ b/tests/functional/openlp_core_ui/test_aboutform.py @@ -47,13 +47,13 @@ class TestFirstTimeForm(TestCase, TestMixin): # THEN: A web browser is opened mocked_webbrowser.open_new.assert_called_with('http://openlp.org/en/contribute') - @patch('openlp.core.ui.aboutform.get_application_version') - def test_about_form_build_number(self, mocked_get_application_version): + @patch('openlp.core.ui.aboutform.get_version') + def test_about_form_build_number(self, mocked_get_version): """ Test that the build number is added to the about form """ - # GIVEN: A mocked out get_application_version function - mocked_get_application_version.return_value = {'version': '3.1.5', 'build': '3000'} + # GIVEN: A mocked out get_version function + mocked_get_version.return_value = {'version': '3.1.5', 'build': '3000'} # WHEN: The about form is created about_form = AboutForm(None) diff --git a/tests/functional/openlp_core_ui/test_exceptionform.py b/tests/functional/openlp_core_ui/test_exceptionform.py index 37a040aa6..4bc83dbcd 100644 --- a/tests/functional/openlp_core_ui/test_exceptionform.py +++ b/tests/functional/openlp_core_ui/test_exceptionform.py @@ -53,7 +53,7 @@ MAIL_ITEM_TEXT = ('**OpenLP Bug Report**\nVersion: Trunk Test\n\n--- Details of @patch("openlp.core.ui.exceptionform.Qt.qVersion") @patch("openlp.core.ui.exceptionform.QtGui.QDesktopServices.openUrl") -@patch("openlp.core.ui.exceptionform.get_application_version") +@patch("openlp.core.ui.exceptionform.get_version") @patch("openlp.core.ui.exceptionform.sqlalchemy") @patch("openlp.core.ui.exceptionform.bs4") @patch("openlp.core.ui.exceptionform.etree") @@ -64,18 +64,10 @@ class TestExceptionForm(TestMixin, TestCase): """ Test functionality of exception form functions """ - def __method_template_for_class_patches(self, - __PLACEHOLDER_FOR_LOCAL_METHOD_PATCH_DECORATORS_GO_HERE__, - mocked_python_version, - mocked_platform, - mocked_is_linux, - mocked_etree, - mocked_bs4, - mocked_sqlalchemy, - mocked_application_version, - mocked_openlurl, - mocked_qversion, - ): + def __method_template_for_class_patches(self, __PLACEHOLDER_FOR_LOCAL_METHOD_PATCH_DECORATORS_GO_HERE__, + mocked_python_version, mocked_platform, mocked_is_linux, + mocked_etree, mocked_bs4, mocked_sqlalchemy, mocked_get_version, + mocked_openlurl, mocked_qversion): """ Template so you don't have to remember the layout of class mock options for methods """ @@ -86,7 +78,7 @@ class TestExceptionForm(TestMixin, TestCase): mocked_platform.return_value = 'Nose Test' mocked_qversion.return_value = 'Qt5 test' mocked_is_linux.return_value = False - mocked_application_version.return_value = 'Trunk Test' + mocked_get_version.return_value = 'Trunk Test' def setUp(self): self.setup_application() @@ -107,22 +99,10 @@ class TestExceptionForm(TestMixin, TestCase): @patch("openlp.core.ui.exceptionform.QtCore.QUrl") @patch("openlp.core.ui.exceptionform.QtCore.QUrlQuery.addQueryItem") @patch("openlp.core.ui.exceptionform.Qt") - def test_on_send_report_button_clicked(self, - mocked_qt, - mocked_add_query_item, - mocked_qurl, - mocked_file_dialog, - mocked_ui_exception_dialog, - mocked_python_version, - mocked_platform, - mocked_is_linux, - mocked_etree, - mocked_bs4, - mocked_sqlalchemy, - mocked_application_version, - mocked_openlurl, - mocked_qversion, - ): + def test_on_send_report_button_clicked(self, mocked_qt, mocked_add_query_item, mocked_qurl, mocked_file_dialog, + mocked_ui_exception_dialog, mocked_python_version, mocked_platform, + mocked_is_linux, mocked_etree, mocked_bs4, mocked_sqlalchemy, + mocked_get_version, mocked_openlurl, mocked_qversion): """ Test send report creates the proper system information text """ @@ -134,10 +114,10 @@ class TestExceptionForm(TestMixin, TestCase): mocked_platform.return_value = 'Nose Test' mocked_qversion.return_value = 'Qt5 test' mocked_is_linux.return_value = False - mocked_application_version.return_value = 'Trunk Test' + mocked_get_version.return_value = 'Trunk Test' mocked_qt.PYQT_VERSION_STR = 'PyQt5 Test' mocked_is_linux.return_value = False - mocked_application_version.return_value = 'Trunk Test' + mocked_get_version.return_value = 'Trunk Test' test_form = exceptionform.ExceptionForm() test_form.file_attachment = None @@ -157,19 +137,10 @@ class TestExceptionForm(TestMixin, TestCase): @patch("openlp.core.ui.exceptionform.FileDialog.getSaveFileName") @patch("openlp.core.ui.exceptionform.Qt") - def test_on_save_report_button_clicked(self, - mocked_qt, - mocked_save_filename, - mocked_python_version, - mocked_platform, - mocked_is_linux, - mocked_etree, - mocked_bs4, - mocked_sqlalchemy, - mocked_application_version, - mocked_openlurl, - mocked_qversion, - ): + def test_on_save_report_button_clicked(self, mocked_qt, mocked_save_filename, mocked_python_version, + mocked_platform, mocked_is_linux, mocked_etree, mocked_bs4, + mocked_sqlalchemy, mocked_get_version, mocked_openlurl, + mocked_qversion): """ Test save report saves the correct information to a file """ @@ -181,7 +152,7 @@ class TestExceptionForm(TestMixin, TestCase): mocked_qversion.return_value = 'Qt5 test' mocked_qt.PYQT_VERSION_STR = 'PyQt5 Test' mocked_is_linux.return_value = False - mocked_application_version.return_value = 'Trunk Test' + mocked_get_version.return_value = 'Trunk Test' mocked_save_filename.return_value = (Path('testfile.txt'), 'filter') test_form = exceptionform.ExceptionForm() diff --git a/tests/functional/test_init.py b/tests/functional/test_init.py index 4cb6e5a76..3f30b253e 100644 --- a/tests/functional/test_init.py +++ b/tests/functional/test_init.py @@ -24,11 +24,11 @@ Package to test the openlp.core.__init__ package. """ import os from unittest import TestCase -from unittest.mock import MagicMock, patch, call +from unittest.mock import MagicMock, patch from PyQt5 import QtCore, QtWidgets -from openlp.core import OpenLP, parse_options +from openlp.core import OpenLP from openlp.core.common import Settings from tests.helpers.testmixin import TestMixin @@ -96,9 +96,9 @@ class TestInit(TestCase, TestMixin): 'build': 'bzr000' } Settings().setValue('core/application version', '2.2.0') - with patch('openlp.core.get_application_version') as mocked_get_application_version,\ + with patch('openlp.core.get_version') as mocked_get_version,\ patch('openlp.core.QtWidgets.QMessageBox.question') as mocked_question: - mocked_get_application_version.return_value = MOCKED_VERSION + mocked_get_version.return_value = MOCKED_VERSION mocked_question.return_value = QtWidgets.QMessageBox.No # WHEN: We check if a backup should be created @@ -122,9 +122,9 @@ class TestInit(TestCase, TestMixin): Settings().setValue('core/application version', '2.0.5') self.openlp.splash = MagicMock() self.openlp.splash.isVisible.return_value = True - with patch('openlp.core.get_application_version') as mocked_get_application_version,\ + with patch('openlp.core.get_version') as mocked_get_version, \ patch('openlp.core.QtWidgets.QMessageBox.question') as mocked_question: - mocked_get_application_version.return_value = MOCKED_VERSION + mocked_get_version.return_value = MOCKED_VERSION mocked_question.return_value = QtWidgets.QMessageBox.No # WHEN: We check if a backup should be created From 15c8023357adf9dd82a032298e5accfedf43b640 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Tue, 19 Sep 2017 09:48:34 -0700 Subject: [PATCH 05/13] Moving over to requests --- openlp/core/api/poll.py | 2 +- openlp/core/common/httputils.py | 177 +++++------------- openlp/core/ui/firsttimeform.py | 20 +- openlp/core/version.py | 1 - openlp/plugins/bibles/lib/importers/http.py | 28 +-- openlp/plugins/remotes/deploy.py | 4 +- .../openlp_core_api/test_websockets.py | 13 +- .../openlp_core_common/test_httputils.py | 30 +-- 8 files changed, 84 insertions(+), 191 deletions(-) diff --git a/openlp/core/api/poll.py b/openlp/core/api/poll.py index b8a29e1f2..c4b25848a 100644 --- a/openlp/core/api/poll.py +++ b/openlp/core/api/poll.py @@ -52,7 +52,7 @@ class Poller(RegistryProperties): 'isSecure': Settings().value('api/authentication enabled'), 'isAuthorised': False, 'chordNotation': Settings().value('songs/chord notation'), - 'isStagedActive': self.is_stage_active(), + 'isStageActive': self.is_stage_active(), 'isLiveActive': self.is_live_active(), 'isChordsActive': self.is_chords_active() } diff --git a/openlp/core/common/httputils.py b/openlp/core/common/httputils.py index b0c9c1b2f..0a5c962f8 100644 --- a/openlp/core/common/httputils.py +++ b/openlp/core/common/httputils.py @@ -25,17 +25,12 @@ The :mod:`openlp.core.utils` module provides the utility libraries for OpenLP. import hashlib import logging import os -import platform -import socket import sys -import subprocess import time -import urllib.error -import urllib.parse -import urllib.request -from http.client import HTTPException from random import randint +import requests + from openlp.core.common import Registry, trace_error_handler log = logging.getLogger(__name__ + '.__init__') @@ -69,33 +64,6 @@ CONNECTION_TIMEOUT = 30 CONNECTION_RETRIES = 2 -class HTTPRedirectHandlerFixed(urllib.request.HTTPRedirectHandler): - """ - Special HTTPRedirectHandler used to work around http://bugs.python.org/issue22248 - (Redirecting to urls with special chars) - """ - def redirect_request(self, req, fp, code, msg, headers, new_url): - # - """ - Test if the new_url can be decoded to ascii - - :param req: - :param fp: - :param code: - :param msg: - :param headers: - :param new_url: - :return: - """ - try: - new_url.encode('latin1').decode('ascii') - fixed_url = new_url - except Exception: - # The url could not be decoded to ascii, so we do some url encoding - fixed_url = urllib.parse.quote(new_url.encode('latin1').decode('utf-8', 'replace'), safe='/:') - return super(HTTPRedirectHandlerFixed, self).redirect_request(req, fp, code, msg, headers, fixed_url) - - def get_user_agent(): """ Return a user agent customised for the platform the user is on. @@ -107,7 +75,7 @@ def get_user_agent(): return browser_list[random_index] -def get_web_page(url, header=None, update_openlp=False): +def get_web_page(url, headers=None, update_openlp=False, proxies=None): """ Attempts to download the webpage at url and returns that page or None. @@ -116,71 +84,37 @@ def get_web_page(url, header=None, update_openlp=False): :param update_openlp: Tells OpenLP to update itself if the page is successfully downloaded. Defaults to False. """ - # TODO: Add proxy usage. Get proxy info from OpenLP settings, add to a - # proxy_handler, build into an opener and install the opener into urllib2. - # http://docs.python.org/library/urllib2.html if not url: return None - # This is needed to work around http://bugs.python.org/issue22248 and https://bugs.launchpad.net/openlp/+bug/1251437 - opener = urllib.request.build_opener(HTTPRedirectHandlerFixed()) - urllib.request.install_opener(opener) - req = urllib.request.Request(url) - if not header or header[0].lower() != 'user-agent': - user_agent = get_user_agent() - req.add_header('User-Agent', user_agent) - if header: - req.add_header(header[0], header[1]) + if headers and 'user-agent' not in [key.lower() for key in headers.keys()]: + headers['User-Agent'] = get_user_agent() log.debug('Downloading URL = %s' % url) retries = 0 - while retries <= CONNECTION_RETRIES: - retries += 1 - time.sleep(0.1) + while retries < CONNECTION_RETRIES: + # Put this at the bottom + # retries += 1 + # time.sleep(0.1) try: - page = urllib.request.urlopen(req, timeout=CONNECTION_TIMEOUT) - log.debug('Downloaded page {text}'.format(text=page.geturl())) + response = requests.get(url, headers=headers, proxies=proxies, timeout=float(CONNECTION_TIMEOUT)) + log.debug('Downloaded page {url}'.format(url=response.url)) break - except urllib.error.URLError as err: - log.exception('URLError on {text}'.format(text=url)) - log.exception('URLError: {text}'.format(text=err.reason)) - page = None + except IOError: + # For now, catch IOError. All requests errors inherit from IOError + log.exception('Unable to connect to {url}'.format(url=url)) + response = None if retries > CONNECTION_RETRIES: - raise - except socket.timeout: - log.exception('Socket timeout: {text}'.format(text=url)) - page = None - if retries > CONNECTION_RETRIES: - raise - except socket.gaierror: - log.exception('Socket gaierror: {text}'.format(text=url)) - page = None - if retries > CONNECTION_RETRIES: - raise - except ConnectionRefusedError: - log.exception('ConnectionRefused: {text}'.format(text=url)) - page = None - if retries > CONNECTION_RETRIES: - raise - break - except ConnectionError: - log.exception('Connection error: {text}'.format(text=url)) - page = None - if retries > CONNECTION_RETRIES: - raise - except HTTPException: - log.exception('HTTPException error: {text}'.format(text=url)) - page = None - if retries > CONNECTION_RETRIES: - raise + raise ConnectionError('Unable to connect to {url}, see log for details'.format(url=url)) except: # Don't know what's happening, so reraise the original + log.exception('Unknown error when trying to connect to {url}'.format(url=url)) raise if update_openlp: Registry().get('application').process_events() - if not page: - log.exception('{text} could not be downloaded'.format(text=url)) + if not response or not response.text: + log.error('{url} could not be downloaded'.format(url=url)) return None - log.debug(page) - return page + log.debug(response.text) + return response.text def get_url_file_size(url): @@ -192,19 +126,18 @@ def get_url_file_size(url): retries = 0 while True: try: - site = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT) - meta = site.info() - return int(meta.get("Content-Length")) - except urllib.error.URLError: + response = requests.head(url, timeout=float(CONNECTION_TIMEOUT), allow_redirects=True) + return int(response.headers['Content-Length']) + except IOError: if retries > CONNECTION_RETRIES: - raise + raise ConnectionError('Unable to download {url}'.format(url=url)) else: retries += 1 time.sleep(0.1) continue -def url_get_file(callback, url, f_path, sha256=None): +def url_get_file(callback, url, file_path, sha256=None): """" Download a file given a URL. The file is retrieved in chunks, giving the ability to cancel the download at any point. Returns False on download error. @@ -217,56 +150,42 @@ def url_get_file(callback, url, f_path, sha256=None): block_count = 0 block_size = 4096 retries = 0 - log.debug("url_get_file: " + url) - while True: + log.debug('url_get_file: %s', url) + while retries < CONNECTION_RETRIES: try: - filename = open(f_path, "wb") - url_file = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT) - if sha256: - hasher = hashlib.sha256() - # Download until finished or canceled. - while not callback.was_cancelled: - data = url_file.read(block_size) - if not data: - break - filename.write(data) + with open(file_path, 'wb') as saved_file: + response = requests.get(url, timeout=float(CONNECTION_TIMEOUT), stream=True) if sha256: - hasher.update(data) - block_count += 1 - callback._download_progress(block_count, block_size) - filename.close() + hasher = hashlib.sha256() + # Download until finished or canceled. + for chunk in response.iter_content(chunk_size=block_size): + if callback.was_cancelled: + break + saved_file.write(chunk) + if sha256: + hasher.update(chunk) + block_count += 1 + callback._download_progress(block_count, block_size) + response.close() if sha256 and hasher.hexdigest() != sha256: - log.error('sha256 sums did not match for file: {file}'.format(file=f_path)) - os.remove(f_path) + log.error('sha256 sums did not match for file %s, got %s, expected %s', file_path, hasher.hexdigest(), + sha256) + os.remove(file_path) return False - except (urllib.error.URLError, socket.timeout) as err: + break + except IOError: trace_error_handler(log) - filename.close() - os.remove(f_path) + os.remove(file_path) if retries > CONNECTION_RETRIES: return False else: retries += 1 time.sleep(0.1) continue - break # Delete file if cancelled, it may be a partial file. if callback.was_cancelled: - os.remove(f_path) + os.remove(file_path) return True -def ping(host): - """ - Returns True if host responds to a ping request - """ - # Ping parameters as function of OS - ping_str = "-n 1" if platform.system().lower() == "windows" else "-c 1" - args = "ping " + " " + ping_str + " " + host - need_sh = False if platform.system().lower() == "windows" else True - - # Ping - return subprocess.call(args, shell=need_sh) == 0 - - __all__ = ['get_web_page'] diff --git a/openlp/core/ui/firsttimeform.py b/openlp/core/ui/firsttimeform.py index 1ae923467..a44384a61 100644 --- a/openlp/core/ui/firsttimeform.py +++ b/openlp/core/ui/firsttimeform.py @@ -181,22 +181,16 @@ class FirstTimeForm(QtWidgets.QWizard, UiFirstTimeWizard, RegistryProperties): self.application.process_events() try: web_config = get_web_page('{host}{name}'.format(host=self.web, name='download.cfg'), - header=('User-Agent', user_agent)) - except (urllib.error.URLError, ConnectionError) as err: - msg = QtWidgets.QMessageBox() - title = translate('OpenLP.FirstTimeWizard', 'Network Error') - msg.setText('{title} {error}'.format(title=title, - error=err.code if hasattr(err, 'code') else '')) - msg.setInformativeText(translate('OpenLP.FirstTimeWizard', - 'There was a network error attempting to ' - 'connect to retrieve initial configuration information')) - msg.setStandardButtons(msg.Ok) - ans = msg.exec() + headers={'User-Agent': user_agent}) + except ConnectionError: + QtWidgets.QMessageBox.critical(self, translate('OpenLP.FirstTimeWizard', 'Network Error'), + translate('OpenLP.FirstTimeWizard', 'There was a network error attempting ' + 'to connect to retrieve initial configuration information'), + QtWidgets.QMessageBox.Ok) web_config = False if web_config: - files = web_config.read() try: - self.config.read_string(files.decode()) + self.config.read_string(web_config) self.web = self.config.get('general', 'base url') self.songs_url = self.web + self.config.get('songs', 'directory') + '/' self.bibles_url = self.web + self.config.get('bibles', 'directory') + '/' diff --git a/openlp/core/version.py b/openlp/core/version.py index 9a532fcd8..14ddc40ff 100644 --- a/openlp/core/version.py +++ b/openlp/core/version.py @@ -139,7 +139,6 @@ def get_version(): global APPLICATION_VERSION if APPLICATION_VERSION: return APPLICATION_VERSION - print(sys.argv) if '--dev-version' in sys.argv or '-d' in sys.argv: # NOTE: The following code is a duplicate of the code in setup.py. Any fix applied here should also be applied # there. diff --git a/openlp/plugins/bibles/lib/importers/http.py b/openlp/plugins/bibles/lib/importers/http.py index 1616ebcf7..c6dc90667 100644 --- a/openlp/plugins/bibles/lib/importers/http.py +++ b/openlp/plugins/bibles/lib/importers/http.py @@ -93,7 +93,7 @@ class BGExtract(RegistryProperties): NAME = 'BibleGateway' def __init__(self, proxy_url=None): - log.debug('BGExtract.init("{url}")'.format(url=proxy_url)) + log.debug('BGExtract.init(proxy_url="{url}")'.format(url=proxy_url)) self.proxy_url = proxy_url socket.setdefaulttimeout(30) @@ -285,15 +285,15 @@ class BGExtract(RegistryProperties): log.debug('BGExtract.get_books_from_http("{version}")'.format(version=version)) url_params = urllib.parse.urlencode({'action': 'getVersionInfo', 'vid': '{version}'.format(version=version)}) reference_url = 'http://www.biblegateway.com/versions/?{url}#books'.format(url=url_params) - page = get_web_page(reference_url) - if not page: + page_source = get_web_page(reference_url) + if not page_source: send_error_message('download') return None - page_source = page.read() - try: - page_source = str(page_source, 'utf8') - except UnicodeDecodeError: - page_source = str(page_source, 'cp1251') + # TODO: Is this even necessary anymore? + # try: + # page_source = str(page_source, 'utf8') + # except UnicodeDecodeError: + # page_source = str(page_source, 'cp1251') try: soup = BeautifulSoup(page_source, 'lxml') except Exception: @@ -759,7 +759,7 @@ class HTTPBible(BibleImport, RegistryProperties): return BiblesResourcesDB.get_verse_count(book_id, chapter) -def get_soup_for_bible_ref(reference_url, header=None, pre_parse_regex=None, pre_parse_substitute=None): +def get_soup_for_bible_ref(reference_url, headers=None, pre_parse_regex=None, pre_parse_substitute=None): """ Gets a webpage and returns a parsed and optionally cleaned soup or None. @@ -772,15 +772,15 @@ def get_soup_for_bible_ref(reference_url, header=None, pre_parse_regex=None, pre if not reference_url: return None try: - page = get_web_page(reference_url, header, True) + page_source = get_web_page(reference_url, headers, update_openlp=True) except Exception as e: - page = None - if not page: + log.exception('Unable to download Bible %s, unknown exception occurred', reference_url) + page_source = None + if not page_source: send_error_message('download') return None - page_source = page.read() if pre_parse_regex and pre_parse_substitute is not None: - page_source = re.sub(pre_parse_regex, pre_parse_substitute, page_source.decode()) + page_source = re.sub(pre_parse_regex, pre_parse_substitute, page_source) soup = None try: soup = BeautifulSoup(page_source, 'lxml') diff --git a/openlp/plugins/remotes/deploy.py b/openlp/plugins/remotes/deploy.py index d971499f0..d159c9b16 100644 --- a/openlp/plugins/remotes/deploy.py +++ b/openlp/plugins/remotes/deploy.py @@ -49,10 +49,10 @@ def download_sha256(): user_agent = 'OpenLP/' + Registry().get('application').applicationVersion() try: web_config = get_web_page('{host}{name}'.format(host='https://get.openlp.org/webclient/', name='download.cfg'), - header=('User-Agent', user_agent)) + headers={'User-Agent': user_agent}) except (urllib.error.URLError, ConnectionError) as err: return False - file_bits = web_config.read().decode('utf-8').split() + file_bits = web_config.split() return file_bits[0], file_bits[2] diff --git a/tests/functional/openlp_core_api/test_websockets.py b/tests/functional/openlp_core_api/test_websockets.py index 8f1356566..c66b73c64 100644 --- a/tests/functional/openlp_core_api/test_websockets.py +++ b/tests/functional/openlp_core_api/test_websockets.py @@ -70,7 +70,7 @@ class TestWSServer(TestCase, TestMixin): """ # GIVEN: A new httpserver # WHEN: I start the server - server = WebSocketServer() + WebSocketServer() # THEN: the api environment should have been created self.assertEquals(1, mock_qthread.call_count, 'The qthread should have been called once') @@ -93,7 +93,7 @@ class TestWSServer(TestCase, TestMixin): """ Test the poll function returns the correct JSON """ - # WHEN: the system is configured with a set of data + # GIVEN: the system is configured with a set of data mocked_service_manager = MagicMock() mocked_service_manager.service_id = 21 mocked_live_controller = MagicMock() @@ -105,8 +105,15 @@ class TestWSServer(TestCase, TestMixin): mocked_live_controller.desktop_screen.isChecked.return_value = False Registry().register('live_controller', mocked_live_controller) Registry().register('service_manager', mocked_service_manager) + # WHEN: The poller polls + with patch.object(self.poll, 'is_stage_active') as mocked_is_stage_active, \ + patch.object(self.poll, 'is_live_active') as mocked_is_live_active, \ + patch.object(self.poll, 'is_chords_active') as mocked_is_chords_active: + mocked_is_stage_active.return_value = True + mocked_is_live_active.return_value = True + mocked_is_chords_active.return_value = True + poll_json = self.poll.poll() # THEN: the live json should be generated and match expected results - poll_json = self.poll.poll() self.assertTrue(poll_json['results']['blank'], 'The blank return value should be True') self.assertFalse(poll_json['results']['theme'], 'The theme return value should be False') self.assertFalse(poll_json['results']['display'], 'The display return value should be False') diff --git a/tests/functional/openlp_core_common/test_httputils.py b/tests/functional/openlp_core_common/test_httputils.py index 26ac297dd..9cb32db25 100644 --- a/tests/functional/openlp_core_common/test_httputils.py +++ b/tests/functional/openlp_core_common/test_httputils.py @@ -28,7 +28,7 @@ import socket from unittest import TestCase from unittest.mock import MagicMock, patch -from openlp.core.common.httputils import get_user_agent, get_web_page, get_url_file_size, url_get_file, ping +from openlp.core.common.httputils import get_user_agent, get_web_page, get_url_file_size, url_get_file from tests.helpers.testmixin import TestMixin @@ -253,7 +253,7 @@ class TestHttpUtils(TestCase, TestMixin): fake_url = 'this://is.a.fake/url' # WHEN: The get_url_file_size() method is called - size = get_url_file_size(fake_url) + get_url_file_size(fake_url) # THEN: The correct methods are called with the correct arguments and a web page is returned mock_urlopen.assert_called_with(fake_url, timeout=30) @@ -272,29 +272,3 @@ class TestHttpUtils(TestCase, TestMixin): # THEN: socket.timeout should have been caught # NOTE: Test is if $tmpdir/tempfile is still there, then test fails since ftw deletes bad downloaded files self.assertFalse(os.path.exists(self.tempfile), 'FTW url_get_file should have caught socket.timeout') - - def test_ping_valid(self): - """ - Test ping for OpenLP - """ - # GIVEN: a valid url to test - url = "openlp.io" - - # WHEN: Attempt to check the url exists - url_found = ping(url) - - # THEN: It should be found - self.assertTrue(url_found, 'OpenLP.io is not found') - - def test_ping_invalid(self): - """ - Test ping for OpenLP - """ - # GIVEN: a valid url to test - url = "trb143.io" - - # WHEN: Attempt to check the url exists - url_found = ping(url) - - # THEN: It should be found - self.assertFalse(url_found, 'TRB143.io is found') From 99b7239bc0a743c1194cf7b1a2443da276f26fa9 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Tue, 19 Sep 2017 21:36:59 -0700 Subject: [PATCH 06/13] A little cleanup --- openlp/plugins/bibles/lib/importers/http.py | 5 ----- openlp/plugins/remotes/deploy.py | 14 +++++++------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/openlp/plugins/bibles/lib/importers/http.py b/openlp/plugins/bibles/lib/importers/http.py index c6dc90667..ffc2ddfbf 100644 --- a/openlp/plugins/bibles/lib/importers/http.py +++ b/openlp/plugins/bibles/lib/importers/http.py @@ -289,11 +289,6 @@ class BGExtract(RegistryProperties): if not page_source: send_error_message('download') return None - # TODO: Is this even necessary anymore? - # try: - # page_source = str(page_source, 'utf8') - # except UnicodeDecodeError: - # page_source = str(page_source, 'cp1251') try: soup = BeautifulSoup(page_source, 'lxml') except Exception: diff --git a/openlp/plugins/remotes/deploy.py b/openlp/plugins/remotes/deploy.py index d159c9b16..a56792f62 100644 --- a/openlp/plugins/remotes/deploy.py +++ b/openlp/plugins/remotes/deploy.py @@ -19,10 +19,11 @@ # with this program; if not, write to the Free Software Foundation, Inc., 59 # # Temple Place, Suite 330, Boston, MA 02111-1307 USA # ############################################################################### - +""" +Download and "install" the remote web client +""" import os -import zipfile -import urllib.error +from zipfile import ZipFile from openlp.core.common import AppLocation, Registry from openlp.core.common.httputils import url_get_file, get_web_page, get_url_file_size @@ -38,7 +39,7 @@ def deploy_zipfile(app_root, zip_name): :return: None """ zip_file = os.path.join(app_root, zip_name) - web_zip = zipfile.ZipFile(zip_file) + web_zip = ZipFile(zip_file) web_zip.extractall(app_root) @@ -48,9 +49,8 @@ def download_sha256(): """ user_agent = 'OpenLP/' + Registry().get('application').applicationVersion() try: - web_config = get_web_page('{host}{name}'.format(host='https://get.openlp.org/webclient/', name='download.cfg'), - headers={'User-Agent': user_agent}) - except (urllib.error.URLError, ConnectionError) as err: + web_config = get_web_page('https://get.openlp.org/webclient/download.cfg', headers={'User-Agent': user_agent}) + except ConnectionError: return False file_bits = web_config.split() return file_bits[0], file_bits[2] From c06cd39cab35e68ae0bfa5a865acfc08cc996baf Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 20 Sep 2017 09:55:21 -0700 Subject: [PATCH 07/13] Fix up all the tests --- openlp/core/common/httputils.py | 10 +- .../openlp_core_common/test_httputils.py | 208 ++++++++---------- .../openlp_core_ui/test_first_time.py | 26 +-- .../openlp_core_ui/test_firsttimeform.py | 25 +-- 4 files changed, 118 insertions(+), 151 deletions(-) diff --git a/openlp/core/common/httputils.py b/openlp/core/common/httputils.py index 0a5c962f8..d02626389 100644 --- a/openlp/core/common/httputils.py +++ b/openlp/core/common/httputils.py @@ -86,14 +86,13 @@ def get_web_page(url, headers=None, update_openlp=False, proxies=None): """ if not url: return None - if headers and 'user-agent' not in [key.lower() for key in headers.keys()]: + if not headers: + headers = {} + if 'user-agent' not in [key.lower() for key in headers.keys()]: headers['User-Agent'] = get_user_agent() log.debug('Downloading URL = %s' % url) retries = 0 while retries < CONNECTION_RETRIES: - # Put this at the bottom - # retries += 1 - # time.sleep(0.1) try: response = requests.get(url, headers=headers, proxies=proxies, timeout=float(CONNECTION_TIMEOUT)) log.debug('Downloaded page {url}'.format(url=response.url)) @@ -102,8 +101,9 @@ def get_web_page(url, headers=None, update_openlp=False, proxies=None): # For now, catch IOError. All requests errors inherit from IOError log.exception('Unable to connect to {url}'.format(url=url)) response = None - if retries > CONNECTION_RETRIES: + if retries >= CONNECTION_RETRIES: raise ConnectionError('Unable to connect to {url}, see log for details'.format(url=url)) + retries += 1 except: # Don't know what's happening, so reraise the original log.exception('Unknown error when trying to connect to {url}'.format(url=url)) diff --git a/tests/functional/openlp_core_common/test_httputils.py b/tests/functional/openlp_core_common/test_httputils.py index 9cb32db25..a32f19172 100644 --- a/tests/functional/openlp_core_common/test_httputils.py +++ b/tests/functional/openlp_core_common/test_httputils.py @@ -24,7 +24,6 @@ Functional tests to test the AppLocation class and related methods. """ import os import tempfile -import socket from unittest import TestCase from unittest.mock import MagicMock, patch @@ -67,7 +66,7 @@ class TestHttpUtils(TestCase, TestMixin): """ with patch('openlp.core.common.httputils.sys') as mocked_sys: - # GIVEN: The system is Linux + # GIVEN: The system is Windows mocked_sys.platform = 'win32' # WHEN: We call get_user_agent() @@ -82,7 +81,7 @@ class TestHttpUtils(TestCase, TestMixin): """ with patch('openlp.core.common.httputils.sys') as mocked_sys: - # GIVEN: The system is Linux + # GIVEN: The system is macOS mocked_sys.platform = 'darwin' # WHEN: We call get_user_agent() @@ -97,7 +96,7 @@ class TestHttpUtils(TestCase, TestMixin): """ with patch('openlp.core.common.httputils.sys') as mocked_sys: - # GIVEN: The system is Linux + # GIVEN: The system is something else mocked_sys.platform = 'freebsd' # WHEN: We call get_user_agent() @@ -119,156 +118,127 @@ class TestHttpUtils(TestCase, TestMixin): # THEN: None should be returned self.assertIsNone(result, 'The return value of get_web_page should be None') - def test_get_web_page(self): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.get_user_agent') + @patch('openlp.core.common.httputils.Registry') + def test_get_web_page(self, MockRegistry, mocked_get_user_agent, mocked_requests): """ Test that the get_web_page method works correctly """ - with patch('openlp.core.common.httputils.urllib.request.Request') as MockRequest, \ - patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent, \ - patch('openlp.core.common.Registry') as MockRegistry: - # GIVEN: Mocked out objects and a fake URL - mocked_request_object = MagicMock() - MockRequest.return_value = mocked_request_object - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - mock_get_user_agent.return_value = 'user_agent' - fake_url = 'this://is.a.fake/url' + # GIVEN: Mocked out objects and a fake URL + mocked_requests.get.return_value = MagicMock(text='text') + mocked_get_user_agent.return_value = 'user_agent' + fake_url = 'this://is.a.fake/url' - # WHEN: The get_web_page() method is called - returned_page = get_web_page(fake_url) + # WHEN: The get_web_page() method is called + returned_page = get_web_page(fake_url) - # THEN: The correct methods are called with the correct arguments and a web page is returned - MockRequest.assert_called_with(fake_url) - mocked_request_object.add_header.assert_called_with('User-Agent', 'user_agent') - self.assertEqual(1, mocked_request_object.add_header.call_count, - 'There should only be 1 call to add_header') - mock_get_user_agent.assert_called_with() - mock_urlopen.assert_called_with(mocked_request_object, timeout=30) - mocked_page_object.geturl.assert_called_with() - self.assertEqual(0, MockRegistry.call_count, 'The Registry() object should have never been called') - self.assertEqual(mocked_page_object, returned_page, 'The returned page should be the mock object') + # THEN: The correct methods are called with the correct arguments and a web page is returned + mocked_requests.get.assert_called_once_with(fake_url, headers={'User-Agent': 'user_agent'}, + proxies=None, timeout=30.0) + mocked_get_user_agent.assert_called_once_with() + assert MockRegistry.call_count == 0, 'The Registry() object should have never been called' + assert returned_page == 'text', 'The returned page should be the mock object' - def test_get_web_page_with_header(self): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.get_user_agent') + def test_get_web_page_with_header(self, mocked_get_user_agent, mocked_requests): """ Test that adding a header to the call to get_web_page() adds the header to the request """ - with patch('openlp.core.common.httputils.urllib.request.Request') as MockRequest, \ - patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent: - # GIVEN: Mocked out objects, a fake URL and a fake header - mocked_request_object = MagicMock() - MockRequest.return_value = mocked_request_object - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - mock_get_user_agent.return_value = 'user_agent' - fake_url = 'this://is.a.fake/url' - fake_header = ('Fake-Header', 'fake value') + # GIVEN: Mocked out objects, a fake URL and a fake header + mocked_requests.get.return_value = MagicMock(text='text') + mocked_get_user_agent.return_value = 'user_agent' + fake_url = 'this://is.a.fake/url' + fake_headers = {'Fake-Header': 'fake value'} - # WHEN: The get_web_page() method is called - returned_page = get_web_page(fake_url, header=fake_header) + # WHEN: The get_web_page() method is called + returned_page = get_web_page(fake_url, headers=fake_headers) - # THEN: The correct methods are called with the correct arguments and a web page is returned - MockRequest.assert_called_with(fake_url) - mocked_request_object.add_header.assert_called_with(fake_header[0], fake_header[1]) - self.assertEqual(2, mocked_request_object.add_header.call_count, - 'There should only be 2 calls to add_header') - mock_get_user_agent.assert_called_with() - mock_urlopen.assert_called_with(mocked_request_object, timeout=30) - mocked_page_object.geturl.assert_called_with() - self.assertEqual(mocked_page_object, returned_page, 'The returned page should be the mock object') + # THEN: The correct methods are called with the correct arguments and a web page is returned + expected_headers = dict(fake_headers) + expected_headers.update({'User-Agent': 'user_agent'}) + mocked_requests.get.assert_called_once_with(fake_url, headers=expected_headers, + proxies=None, timeout=30.0) + mocked_get_user_agent.assert_called_with() + assert returned_page == 'text', 'The returned page should be the mock object' - def test_get_web_page_with_user_agent_in_headers(self): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.get_user_agent') + def test_get_web_page_with_user_agent_in_headers(self, mocked_get_user_agent, mocked_requests): """ Test that adding a user agent in the header when calling get_web_page() adds that user agent to the request """ - with patch('openlp.core.common.httputils.urllib.request.Request') as MockRequest, \ - patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent: - # GIVEN: Mocked out objects, a fake URL and a fake header - mocked_request_object = MagicMock() - MockRequest.return_value = mocked_request_object - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - fake_url = 'this://is.a.fake/url' - user_agent_header = ('User-Agent', 'OpenLP/2.2.0') + # GIVEN: Mocked out objects, a fake URL and a fake header + mocked_requests.get.return_value = MagicMock(text='text') + fake_url = 'this://is.a.fake/url' + user_agent_headers = {'User-Agent': 'OpenLP/2.2.0'} - # WHEN: The get_web_page() method is called - returned_page = get_web_page(fake_url, header=user_agent_header) + # WHEN: The get_web_page() method is called + returned_page = get_web_page(fake_url, headers=user_agent_headers) - # THEN: The correct methods are called with the correct arguments and a web page is returned - MockRequest.assert_called_with(fake_url) - mocked_request_object.add_header.assert_called_with(user_agent_header[0], user_agent_header[1]) - self.assertEqual(1, mocked_request_object.add_header.call_count, - 'There should only be 1 call to add_header') - self.assertEqual(0, mock_get_user_agent.call_count, 'get_user_agent should not have been called') - mock_urlopen.assert_called_with(mocked_request_object, timeout=30) - mocked_page_object.geturl.assert_called_with() - self.assertEqual(mocked_page_object, returned_page, 'The returned page should be the mock object') + # THEN: The correct methods are called with the correct arguments and a web page is returned + mocked_requests.get.assert_called_once_with(fake_url, headers=user_agent_headers, + proxies=None, timeout=30.0) + assert mocked_get_user_agent.call_count == 0, 'get_user_agent() should not have been called' + assert returned_page == 'text', 'The returned page should be "test"' - def test_get_web_page_update_openlp(self): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.get_user_agent') + @patch('openlp.core.common.httputils.Registry') + def test_get_web_page_update_openlp(self, MockRegistry, mocked_get_user_agent, mocked_requests): """ Test that passing "update_openlp" as true to get_web_page calls Registry().get('app').process_events() """ - with patch('openlp.core.common.httputils.urllib.request.Request') as MockRequest, \ - patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent, \ - patch('openlp.core.common.httputils.Registry') as MockRegistry: - # GIVEN: Mocked out objects, a fake URL - mocked_request_object = MagicMock() - MockRequest.return_value = mocked_request_object - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - mock_get_user_agent.return_value = 'user_agent' - mocked_registry_object = MagicMock() - mocked_application_object = MagicMock() - mocked_registry_object.get.return_value = mocked_application_object - MockRegistry.return_value = mocked_registry_object - fake_url = 'this://is.a.fake/url' + # GIVEN: Mocked out objects, a fake URL + mocked_requests.get.return_value = MagicMock(text='text') + mocked_get_user_agent.return_value = 'user_agent' + mocked_registry_object = MagicMock() + mocked_application_object = MagicMock() + mocked_registry_object.get.return_value = mocked_application_object + MockRegistry.return_value = mocked_registry_object + fake_url = 'this://is.a.fake/url' - # WHEN: The get_web_page() method is called - returned_page = get_web_page(fake_url, update_openlp=True) + # WHEN: The get_web_page() method is called + returned_page = get_web_page(fake_url, update_openlp=True) - # THEN: The correct methods are called with the correct arguments and a web page is returned - MockRequest.assert_called_with(fake_url) - mocked_request_object.add_header.assert_called_with('User-Agent', 'user_agent') - self.assertEqual(1, mocked_request_object.add_header.call_count, - 'There should only be 1 call to add_header') - mock_urlopen.assert_called_with(mocked_request_object, timeout=30) - mocked_page_object.geturl.assert_called_with() - mocked_registry_object.get.assert_called_with('application') - mocked_application_object.process_events.assert_called_with() - self.assertEqual(mocked_page_object, returned_page, 'The returned page should be the mock object') + # THEN: The correct methods are called with the correct arguments and a web page is returned + mocked_requests.get.assert_called_once_with(fake_url, headers={'User-Agent': 'user_agent'}, + proxies=None, timeout=30.0) + mocked_get_user_agent.assert_called_once_with() + mocked_registry_object.get.assert_called_with('application') + mocked_application_object.process_events.assert_called_with() + assert returned_page == 'text', 'The returned page should be the mock object' - def test_get_url_file_size(self): + @patch('openlp.core.common.httputils.requests') + def test_get_url_file_size(self, mocked_requests): """ - Test that passing "update_openlp" as true to get_web_page calls Registry().get('app').process_events() + Test that calling "get_url_file_size" works correctly """ - with patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent: - # GIVEN: Mocked out objects, a fake URL - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - mock_get_user_agent.return_value = 'user_agent' - fake_url = 'this://is.a.fake/url' + # GIVEN: Mocked out objects, a fake URL + mocked_requests.head.return_value = MagicMock(headers={'Content-Length': 100}) + fake_url = 'this://is.a.fake/url' - # WHEN: The get_url_file_size() method is called - get_url_file_size(fake_url) + # WHEN: The get_url_file_size() method is called + file_size = get_url_file_size(fake_url) - # THEN: The correct methods are called with the correct arguments and a web page is returned - mock_urlopen.assert_called_with(fake_url, timeout=30) + # THEN: The correct methods are called with the correct arguments and a web page is returned + mocked_requests.head.assert_called_once_with(fake_url, allow_redirects=True, timeout=30.0) + assert file_size == 100 - @patch('openlp.core.ui.firsttimeform.urllib.request.urlopen') - def test_socket_timeout(self, mocked_urlopen): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.os.remove') + def test_socket_timeout(self, mocked_remove, mocked_requests): """ Test socket timeout gets caught """ # GIVEN: Mocked urlopen to fake a network disconnect in the middle of a download - mocked_urlopen.side_effect = socket.timeout() + mocked_requests.get.side_effect = IOError # WHEN: Attempt to retrieve a file - url_get_file(MagicMock(), url='http://localhost/test', f_path=self.tempfile) + url_get_file(MagicMock(), url='http://localhost/test', file_path=self.tempfile) # THEN: socket.timeout should have been caught # NOTE: Test is if $tmpdir/tempfile is still there, then test fails since ftw deletes bad downloaded files - self.assertFalse(os.path.exists(self.tempfile), 'FTW url_get_file should have caught socket.timeout') + mocked_remove.assert_called_with(self.tempfile) + assert mocked_remove.call_count == 3, 'os.remove() should have been called 3 times' diff --git a/tests/functional/openlp_core_ui/test_first_time.py b/tests/functional/openlp_core_ui/test_first_time.py index 35507e15e..eb9464375 100644 --- a/tests/functional/openlp_core_ui/test_first_time.py +++ b/tests/functional/openlp_core_ui/test_first_time.py @@ -22,9 +22,6 @@ """ Package to test the openlp.core.utils.__init__ package. """ -import urllib.request -import urllib.error -import urllib.parse from unittest import TestCase from unittest.mock import patch @@ -37,20 +34,21 @@ class TestFirstTimeWizard(TestMixin, TestCase): """ Test First Time Wizard import functions """ - def test_webpage_connection_retry(self): + @patch('openlp.core.common.httputils.requests') + def test_webpage_connection_retry(self, mocked_requests): """ Test get_web_page will attempt CONNECTION_RETRIES+1 connections - bug 1409031 """ # GIVEN: Initial settings and mocks - with patch.object(urllib.request, 'urlopen') as mocked_urlopen: - mocked_urlopen.side_effect = ConnectionError + mocked_requests.get.side_effect = IOError('Unable to connect') - # WHEN: A webpage is requested - try: - get_web_page(url='http://localhost') - except: - pass + # WHEN: A webpage is requested + try: + get_web_page('http://localhost') + except Exception as e: + assert isinstance(e, ConnectionError) - # THEN: urlopen should have been called CONNECTION_RETRIES + 1 count - self.assertEquals(mocked_urlopen.call_count, CONNECTION_RETRIES + 1, - 'get_web_page() should have tried {} times'.format(CONNECTION_RETRIES)) + # THEN: urlopen should have been called CONNECTION_RETRIES + 1 count + assert mocked_requests.get.call_count == CONNECTION_RETRIES, \ + 'get should have been called {} times, but was only called {} times'.format( + CONNECTION_RETRIES, mocked_requests.get.call_count) diff --git a/tests/functional/openlp_core_ui/test_firsttimeform.py b/tests/functional/openlp_core_ui/test_firsttimeform.py index 0f7c6be6a..c90cdd80b 100644 --- a/tests/functional/openlp_core_ui/test_firsttimeform.py +++ b/tests/functional/openlp_core_ui/test_firsttimeform.py @@ -34,7 +34,7 @@ from openlp.core.ui.firsttimeform import FirstTimeForm from tests.helpers.testmixin import TestMixin -FAKE_CONFIG = b""" +FAKE_CONFIG = """ [general] base url = http://example.com/frw/ [songs] @@ -45,7 +45,7 @@ directory = bibles directory = themes """ -FAKE_BROKEN_CONFIG = b""" +FAKE_BROKEN_CONFIG = """ [general] base url = http://example.com/frw/ [songs] @@ -54,7 +54,7 @@ directory = songs directory = bibles """ -FAKE_INVALID_CONFIG = b""" +FAKE_INVALID_CONFIG = """ This is not a config file Some text @@ -112,7 +112,7 @@ class TestFirstTimeForm(TestCase, TestMixin): patch('openlp.core.ui.firsttimeform.Settings') as MockedSettings, \ patch('openlp.core.ui.firsttimeform.gettempdir') as mocked_gettempdir, \ patch('openlp.core.ui.firsttimeform.check_directory_exists') as mocked_check_directory_exists, \ - patch.object(frw.application, 'set_normal_cursor') as mocked_set_normal_cursor: + patch.object(frw.application, 'set_normal_cursor'): mocked_settings = MagicMock() mocked_settings.value.return_value = True MockedSettings.return_value = mocked_settings @@ -192,7 +192,7 @@ class TestFirstTimeForm(TestCase, TestMixin): with patch('openlp.core.ui.firsttimeform.get_web_page') as mocked_get_web_page: first_time_form = FirstTimeForm(None) first_time_form.initialize(MagicMock()) - mocked_get_web_page.return_value.read.return_value = FAKE_BROKEN_CONFIG + mocked_get_web_page.return_value = FAKE_BROKEN_CONFIG # WHEN: The First Time Wizard is downloads the config file first_time_form._download_index() @@ -208,7 +208,7 @@ class TestFirstTimeForm(TestCase, TestMixin): with patch('openlp.core.ui.firsttimeform.get_web_page') as mocked_get_web_page: first_time_form = FirstTimeForm(None) first_time_form.initialize(MagicMock()) - mocked_get_web_page.return_value.read.return_value = FAKE_INVALID_CONFIG + mocked_get_web_page.return_value = FAKE_INVALID_CONFIG # WHEN: The First Time Wizard is downloads the config file first_time_form._download_index() @@ -225,14 +225,13 @@ class TestFirstTimeForm(TestCase, TestMixin): # GIVEN: Initial setup and mocks first_time_form = FirstTimeForm(None) first_time_form.initialize(MagicMock()) - mocked_get_web_page.side_effect = urllib.error.HTTPError(url='http//localhost', - code=407, - msg='Network proxy error', - hdrs=None, - fp=None) + mocked_get_web_page.side_effect = ConnectionError('') + mocked_message_box.Ok = 'OK' + # WHEN: the First Time Wizard calls to get the initial configuration first_time_form._download_index() # THEN: the critical_error_message_box should have been called - self.assertEquals(mocked_message_box.mock_calls[1][1][0], 'Network Error 407', - 'first_time_form should have caught Network Error') + mocked_message_box.critical.assert_called_once_with( + first_time_form, 'Network Error', 'There was a network error attempting to connect to retrieve ' + 'initial configuration information', 'OK') From e1ca15173d13b0f82d5e940134e316bde7b3d852 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Fri, 22 Sep 2017 20:50:45 -0700 Subject: [PATCH 08/13] Actually show the progress dialog when we're waiting for the thread to shut down. --- openlp.py | 3 ++- openlp/core/ui/mainwindow.py | 8 +++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/openlp.py b/openlp.py index 68001f2d1..73c0d9033 100755 --- a/openlp.py +++ b/openlp.py @@ -28,9 +28,10 @@ import multiprocessing import sys from openlp.core.common import is_win, is_macosx +from openlp.core.common.applocation import AppLocation from openlp.core import main -faulthandler.enable() +faulthandler.enable(open(str(AppLocation.get_directory(AppLocation.CacheDir) / 'error.log'), 'wb')) if __name__ == '__main__': """ diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index ae32c0347..14b34a715 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -1017,7 +1017,13 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): wait_dialog = QtWidgets.QProgressDialog('Waiting for some things to finish...', '', 0, 0, self) wait_dialog.setWindowModality(QtCore.Qt.WindowModal) wait_dialog.setAutoClose(False) - self.version_thread.wait() + wait_dialog.show() + retry = 0 + while self.version_thread.isRunning() and retry < 10: + self.application.processEvents() + self.version_thread.wait(500) + if self.version_thread.isRunning(): + self.version_thread.terminate() wait_dialog.close() except RuntimeError: # Ignore the RuntimeError that is thrown when Qt has already deleted the C++ thread object From a4bd979f19a9c9e8d82d6b9f978f67c2aed35417 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Fri, 22 Sep 2017 21:10:18 -0700 Subject: [PATCH 09/13] Hide the cancel button --- openlp/core/ui/mainwindow.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 4424944cf..508b6a34c 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -1017,11 +1017,13 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): wait_dialog = QtWidgets.QProgressDialog('Waiting for some things to finish...', '', 0, 0, self) wait_dialog.setWindowModality(QtCore.Qt.WindowModal) wait_dialog.setAutoClose(False) + wait_dialog.setCancelButton(None) wait_dialog.show() retry = 0 - while self.version_thread.isRunning() and retry < 10: + while self.version_thread.isRunning() and retry < 50: self.application.processEvents() - self.version_thread.wait(500) + self.version_thread.wait(100) + retry += 1 if self.version_thread.isRunning(): self.version_thread.terminate() wait_dialog.close() From 7239b1e400e947f180c912b528ec0c21804516cc Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Fri, 22 Sep 2017 22:23:56 -0700 Subject: [PATCH 10/13] Add a mock main_window --- tests/interfaces/openlp_plugins/bibles/test_lib_http.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/interfaces/openlp_plugins/bibles/test_lib_http.py b/tests/interfaces/openlp_plugins/bibles/test_lib_http.py index e4075ca19..859957f8d 100644 --- a/tests/interfaces/openlp_plugins/bibles/test_lib_http.py +++ b/tests/interfaces/openlp_plugins/bibles/test_lib_http.py @@ -38,6 +38,7 @@ class TestBibleHTTP(TestCase): Registry.create() Registry().register('service_list', MagicMock()) Registry().register('application', MagicMock()) + Registry().register('main_window', MagicMock()) def test_bible_gateway_extract_books(self): """ From cff194d320c3c89827d00ff5f3f7edf7dedc7faa Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Mon, 25 Sep 2017 11:50:24 -0700 Subject: [PATCH 11/13] Skip Bible HTTP tests on Jenkins to prevent the server from being blacklisted --- tests/interfaces/openlp_plugins/bibles/test_lib_http.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/interfaces/openlp_plugins/bibles/test_lib_http.py b/tests/interfaces/openlp_plugins/bibles/test_lib_http.py index 859957f8d..b896cd9e1 100644 --- a/tests/interfaces/openlp_plugins/bibles/test_lib_http.py +++ b/tests/interfaces/openlp_plugins/bibles/test_lib_http.py @@ -22,13 +22,15 @@ """ Package to test the openlp.plugin.bible.lib.https package. """ -from unittest import TestCase, skip +import os +from unittest import TestCase, skipIf from unittest.mock import MagicMock from openlp.core.common import Registry from openlp.plugins.bibles.lib.importers.http import BGExtract, CWExtract, BSExtract +@skipIf(os.environ.get('JENKINS_URL'), 'Skip Bible HTTP tests to prevent Jenkins from being blacklisted') class TestBibleHTTP(TestCase): def setUp(self): From 92551f4fa5a8d52fda4bf427c7e8ba569ab1b5e2 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Mon, 25 Sep 2017 12:01:54 -0700 Subject: [PATCH 12/13] Add requests to AppVeyor --- scripts/appveyor.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/appveyor.yml b/scripts/appveyor.yml index 0405d0e2c..316e5f73b 100644 --- a/scripts/appveyor.yml +++ b/scripts/appveyor.yml @@ -12,7 +12,7 @@ environment: install: # Install dependencies from pypi - - "%PYTHON%\\python.exe -m pip install sqlalchemy alembic chardet beautifulsoup4 Mako nose mock pyodbc==4.0.8 psycopg2 pypiwin32 pyenchant websockets asyncio waitress six webob" + - "%PYTHON%\\python.exe -m pip install sqlalchemy alembic chardet beautifulsoup4 Mako nose mock pyodbc==4.0.8 psycopg2 pypiwin32 pyenchant websockets asyncio waitress six webob requests" # Install mysql dependency - "%PYTHON%\\python.exe -m pip install http://cdn.mysql.com/Downloads/Connector-Python/mysql-connector-python-2.0.4.zip#md5=3df394d89300db95163f17c843ef49df" # Download and install lxml and pyicu (originally from http://www.lfd.uci.edu/~gohlke/pythonlibs/) From e90836e8179dd5ef5043d2a72cc87ccb90827b5b Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Mon, 25 Sep 2017 13:34:05 -0700 Subject: [PATCH 13/13] Fix up some issues with one of the tests --- openlp/core/common/httputils.py | 14 ++++++++------ .../openlp_core_common/test_httputils.py | 8 +++----- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/openlp/core/common/httputils.py b/openlp/core/common/httputils.py index 9a73659bd..a92cd92c9 100644 --- a/openlp/core/common/httputils.py +++ b/openlp/core/common/httputils.py @@ -142,7 +142,7 @@ def url_get_file(callback, url, file_path, sha256=None): :param callback: the class which needs to be updated :param url: URL to download - :param f_path: Destination file + :param file_path: Destination file :param sha256: The check sum value to be checked against the download value """ block_count = 0 @@ -151,7 +151,7 @@ def url_get_file(callback, url, file_path, sha256=None): log.debug('url_get_file: %s', url) while retries < CONNECTION_RETRIES: try: - with open(file_path, 'wb') as saved_file: + with file_path.open('wb') as saved_file: response = requests.get(url, timeout=float(CONNECTION_TIMEOUT), stream=True) if sha256: hasher = hashlib.sha256() @@ -168,20 +168,22 @@ def url_get_file(callback, url, file_path, sha256=None): if sha256 and hasher.hexdigest() != sha256: log.error('sha256 sums did not match for file %s, got %s, expected %s', file_path, hasher.hexdigest(), sha256) - os.remove(file_path) + if file_path.exists(): + file_path.unlink() return False break except IOError: trace_error_handler(log) - os.remove(file_path) if retries > CONNECTION_RETRIES: + if file_path.exists(): + file_path.unlink() return False else: retries += 1 time.sleep(0.1) continue - if callback.was_cancelled: - os.remove(file_path) + if callback.was_cancelled and file_path.exists(): + file_path.unlink() return True diff --git a/tests/functional/openlp_core_common/test_httputils.py b/tests/functional/openlp_core_common/test_httputils.py index 26edf4056..e620fa04e 100644 --- a/tests/functional/openlp_core_common/test_httputils.py +++ b/tests/functional/openlp_core_common/test_httputils.py @@ -228,8 +228,7 @@ class TestHttpUtils(TestCase, TestMixin): assert file_size == 100 @patch('openlp.core.common.httputils.requests') - @patch('openlp.core.common.httputils.os.remove') - def test_socket_timeout(self, mocked_remove, mocked_requests): + def test_socket_timeout(self, mocked_requests): """ Test socket timeout gets caught """ @@ -237,9 +236,8 @@ class TestHttpUtils(TestCase, TestMixin): mocked_requests.get.side_effect = IOError # WHEN: Attempt to retrieve a file - url_get_file(MagicMock(), url='http://localhost/test', file_path=self.tempfile) + url_get_file(MagicMock(), url='http://localhost/test', file_path=Path(self.tempfile)) # THEN: socket.timeout should have been caught # NOTE: Test is if $tmpdir/tempfile is still there, then test fails since ftw deletes bad downloaded files - mocked_remove.assert_called_with(self.tempfile) - assert mocked_remove.call_count == 3, 'os.remove() should have been called 3 times' + assert not os.path.exists(self.tempfile), 'tempfile should have been deleted'