openlp-core team mailing list archive

[Simon Hanna <simon@xxxxxxxxxxx>]

Simon Hanna has proposed merging lp:~thelinuxguy/openlp/fix-version-check into lp:openlp.

Requested reviews:
  OpenLP Core (openlp-core)

For more details, see:
https://code.launchpad.net/~thelinuxguy/openlp/fix-version-check/+merge/335602

Fixed the version checking to be more robust

* Strip the response so empty responses that contain whitespace are in fact empty
* Change http to https to result in one less query
* Add test for responses containing white space
* Add .cache to bzrignore (generated by pytest when tests fail)
-- 
Your team OpenLP Core is requested to review the proposed merge of lp:~thelinuxguy/openlp/fix-version-check into lp:openlp.

=== modified file '.bzrignore'
--- .bzrignore	2017-10-10 07:08:44 +0000
+++ .bzrignore	2017-12-29 10:42:37 +0000
@@ -45,3 +45,4 @@
 output
 htmlcov
 openlp-test-projectordb.sqlite
+.cache

=== modified file 'openlp/core/version.py'
--- openlp/core/version.py	2017-11-18 11:23:15 +0000
+++ openlp/core/version.py	2017-12-29 10:42:37 +0000
@@ -76,12 +76,12 @@
         """
         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)
-        download_url = 'http://www.openlp.org/files/version.txt'
+        # time.sleep(1)
+        download_url = 'https://www.openlp.org/files/version.txt'
         if self.current_version['build']:
-            download_url = 'http://www.openlp.org/files/nightly_version.txt'
+            download_url = 'https://www.openlp.org/files/nightly_version.txt'
         elif int(self.current_version['version'].split('.')[1]) % 2 != 0:
-            download_url = 'http://www.openlp.org/files/dev_version.txt'
+            download_url = 'https://www.openlp.org/files/dev_version.txt'
         headers = {
             'User-Agent': 'OpenLP/{version} {system}/{release}; '.format(version=self.current_version['full'],
                                                                          system=platform.system(),
@@ -92,7 +92,7 @@
         while retries < 3:
             try:
                 response = requests.get(download_url, headers=headers)
-                remote_version = response.text
+                remote_version = response.text.strip()
                 log.debug('New version found: %s', remote_version)
                 break
             except OSError:

=== modified file 'tests/functional/openlp_core/test_version.py'
--- tests/functional/openlp_core/test_version.py	2017-09-13 06:08:38 +0000
+++ tests/functional/openlp_core/test_version.py	2017-12-29 10:42:37 +0000
@@ -63,7 +63,7 @@
         worker.start()
 
     # THEN: The check completes and the signal is emitted
-    expected_download_url = 'http://www.openlp.org/files/version.txt'
+    expected_download_url = 'https://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')
@@ -88,7 +88,7 @@
         worker.start()
 
     # THEN: The check completes and the signal is emitted
-    expected_download_url = 'http://www.openlp.org/files/dev_version.txt'
+    expected_download_url = 'https://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')
@@ -113,7 +113,7 @@
         worker.start()
 
     # THEN: The check completes and the signal is emitted
-    expected_download_url = 'http://www.openlp.org/files/nightly_version.txt'
+    expected_download_url = 'https://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')
@@ -122,6 +122,31 @@
 
 @patch('openlp.core.version.platform')
 @patch('openlp.core.version.requests')
+def test_worker_empty_response(mock_requests, mock_platform):
+    """Test the VersionWorkder.start() method for empty responses"""
+    # 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='\n')
+    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 = 'https://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)
+    assert mock_new_version.emit.call_count == 0
+    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
@@ -138,7 +163,7 @@
         worker.start()
 
     # THEN: The check completes and the signal is emitted
-    expected_download_url = 'http://www.openlp.org/files/version.txt'
+    expected_download_url = 'https://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