Control: tags -1 patch Here are two patches to solve the bug. They might need some rework, as I'm not quite sure I did my best here. Essentially, the issue was the Python3 transition: the file wnpp_rm is treated as bytes, but the function receiving it and analyzing was expecting content of type str. I used a function I wrote for a previous feature request to ensure that the content is decoded. Cheers, -- PEB
From 144960dfc453fc5d97913854bdf50ecf7051f86c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pierre-Elliott=20B=C3=A9cue?= <becue@crans.org>
Date: Thu, 30 Nov 2017 22:29:21 +0100
Subject: [PATCH 1/2] Adds an only_if_updated argument to
get_resource{content,text}
If set to True, then both functions return None instead of the content
---
distro_tracker/core/tests/tests_utils.py | 84 ++++++++++++++++++++++++--------
distro_tracker/core/utils/http.py | 28 +++++++++--
2 files changed, 88 insertions(+), 24 deletions(-)
diff --git a/distro_tracker/core/tests/tests_utils.py b/distro_tracker/core/tests/tests_utils.py
index 572d31f..d5e6d9b 100644
--- a/distro_tracker/core/tests/tests_utils.py
+++ b/distro_tracker/core/tests/tests_utils.py
@@ -771,6 +771,12 @@ class HttpCacheTest(SimpleTestCase):
headers=headers,
status_code=status_code)
+ @staticmethod
+ def cache_is_expired_false(self):
+ """Function to override cache.is_expired when needed"""
+
+ return False
+
def setUp(self):
# Set up a cache directory to use in the tests
self.cache_directory = tempfile.mkdtemp(suffix='test-cache')
@@ -1091,42 +1097,82 @@ class HttpCacheTest(SimpleTestCase):
content = cache.get_content(url, compression=None)
self.assertEqual(content, b"Hello world!")
- def test_get_resource_content_utility_function_cached(self):
+ @mock.patch('distro_tracker.core.utils.http.requests')
+ def test_get_resource_content_utility_function_cached(self, mock_requests):
"""
Tests the :func:`distro_tracker.core.utils.http.get_resource_content`
utility function when the resource is cached in the given cache
instance.
"""
- mock_cache = mock.create_autospec(HttpCache)
- mock_cache.is_expired.return_value = False
- expected_content = b"Some content"
- mock_cache.get_content.return_value = expected_content
+ self.response_content = b"Hello world!"
+ self.set_mock_response(mock_requests)
+ cache = HttpCache(self.cache_directory)
url = 'http://some.url.com'
- content = get_resource_content(url, cache=mock_cache)
-
# The expected content is retrieved
- self.assertEqual(content, expected_content)
- # The function did not update the cache
- self.assertFalse(mock_cache.update.called)
+ content = get_resource_content(url, cache=cache)
+ self.assertEqual(content, self.response_content)
- def test_get_resource_content_utility_function_not_cached(self):
+ # Forces the cache to return False when asked if something is expired.
+ cache.is_expired = self.cache_is_expired_false
+
+ # To make sure that .update is not called, we shoot the function once
+ # with only_if_updated set to True. The return has to be None if
+ # .update is not called.
+ content = get_resource_content(url, cache=cache, only_if_updated=True)
+ self.assertIsNone(content)
+
+ # Then, call again without only_if_updated.
+ content = get_resource_content(url, cache=cache)
+ self.assertEqual(content, self.response_content)
+
+ @mock.patch('distro_tracker.core.utils.http.requests')
+ def test_get_resource_content_utility_function_not_cached(self,
+ mock_requests):
"""
Tests the :func:`distro_tracker.core.utils.http.get_resource_content`
utility function when the resource is not cached in the given cache
instance.
"""
- mock_cache = mock.create_autospec(HttpCache)
- mock_cache.is_expired.return_value = True
- expected_content = b"Some content"
- mock_cache.get_content.return_value = expected_content
+ self.response_content = b"Hello world!"
+ self.set_mock_response(mock_requests)
+ cache = HttpCache(self.cache_directory)
url = 'http://some.url.com'
- content = get_resource_content(url, mock_cache)
+ content = get_resource_content(url, cache=cache)
+ self.assertEqual(content, self.response_content)
+
+ # Forces the cache to return False when asked if something is expired.
+ cache.is_expired = self.cache_is_expired_false
+
+ # If the cache is filled, then a call with only_if_updated will return
+ # None.
+ content = get_resource_content(url, cache=cache, only_if_updated=True)
+ self.assertIsNone(content)
+
+ @mock.patch('distro_tracker.core.utils.http.requests')
+ def test_get_resource_content_utility_function_cached_only(self,
+ mock_requests):
+ """
+ Tests the :func:`distro_tracker.core.utils.http.get_resource_content`
+ utility function when the resource is cached in the given cache
+ instance, and with the only_if_updated=True bit.
+ """
+ self.response_content = b"Hello world!"
+ self.set_mock_response(mock_requests)
+ cache = HttpCache(self.cache_directory)
+ url = 'http://some.url.com'
+
+ # First call has to work with only_if_updated set to True.
+ content = get_resource_content(url, cache=cache, only_if_updated=True)
+ self.assertEqual(content, self.response_content)
+
+ # Forces the cache to return False when asked if something is expired.
+ cache.is_expired = self.cache_is_expired_false
- self.assertEqual(content, expected_content)
- # The function updated the cache
- mock_cache.update.assert_called_once_with(url)
+ # Second call has to return None.
+ content = get_resource_content(url, cache=cache, only_if_updated=True)
+ self.assertIsNone(content)
@mock.patch('distro_tracker.core.utils.http.get_resource_content')
def test_get_resource_text(self, mock_get_resource_content):
diff --git a/distro_tracker/core/utils/http.py b/distro_tracker/core/utils/http.py
index 6756fee..2241cd2 100644
--- a/distro_tracker/core/utils/http.py
+++ b/distro_tracker/core/utils/http.py
@@ -184,7 +184,10 @@ class HttpCache(object):
return md5(url.encode('utf-8')).hexdigest()
-def get_resource_content(url, cache=None, compression="auto"):
+def get_resource_content(url,
+ cache=None,
+ only_if_updated=False,
+ compression="auto"):
"""
A helper function which returns the content of the resource found at the
given URL.
@@ -202,6 +205,9 @@ def get_resource_content(url, cache=None, compression="auto"):
``DISTRO_TRACKER_CACHE_DIRECTORY`` cache directory
is used.
:type cache: :class:`HttpCache` or an object with an equivalent interface
+ :param only_if_updated: if set to `True` returns None when no update is
+ done. Otherwise, returns the content in any case.
+ :type only_if_updated: bool
:param compression: Specifies the compression method used to generate the
resource, and thus the compression method one should use to decompress
it. If auto, then guess it from the url file extension.
@@ -215,18 +221,27 @@ def get_resource_content(url, cache=None, compression="auto"):
cache = HttpCache(cache_directory_path)
try:
+ updated = False
if cache.is_expired(url):
- cache.update(url)
+ _, updated = cache.update(url)
+
+ if not updated and only_if_updated:
+ return
+
return cache.get_content(url, compression=compression)
except:
pass
-def get_resource_text(url, cache=None, compression="auto", encoding="utf-8"):
+def get_resource_text(url,
+ cache=None,
+ only_if_updated=False,
+ compression="auto",
+ encoding="utf-8"):
"""
Clone of :py:func:`get_resource_content` which transparently decodes
the downloaded content into text. It supports the same parameters
- and add the encoding parameter.
+ and adds the encoding parameter.
:param encoding: Specifies an encoding to decode the resource content.
:type encoding: str
@@ -235,7 +250,10 @@ def get_resource_text(url, cache=None, compression="auto", encoding="utf-8"):
:rtype: str
"""
- content = get_resource_content(url, cache=cache, compression=compression)
+ content = get_resource_content(url,
+ cache=cache,
+ only_if_updated=only_if_updated,
+ compression=compression)
if content is not None:
return content.decode(encoding)
--
2.11.0
From 74327664c045cbd19236a8a9d7f6fcf9d0354e0f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pierre-Elliott=20B=C3=A9cue?= <becue@crans.org>
Date: Thu, 30 Nov 2017 22:31:19 +0100
Subject: [PATCH 2/2] Uses get_resource_text with the new only_if_updated for
UpdateWnppStats
* Closes: 883101
* The bug was due to Python3 transition. _get_wnpp_content returns
bytes, the split('|') call was trigerring an error, as '|' is str and
not bytes. Use get_resource_text instead, and remove the oboslete
function.
* Altered tests to use mock on requests in order to have a coherend
behaviour with the changes.
---
distro_tracker/vendor/debian/tests.py | 36 ++++++++++++++++-----------
distro_tracker/vendor/debian/tracker_tasks.py | 14 +++--------
2 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/distro_tracker/vendor/debian/tests.py b/distro_tracker/vendor/debian/tests.py
index a060234..8c9858d 100644
--- a/distro_tracker/vendor/debian/tests.py
+++ b/distro_tracker/vendor/debian/tests.py
@@ -3477,6 +3477,7 @@ class UbuntuPanelTests(TestCase):
self.assertIn(ubuntu_version, response_content)
+@mock.patch('distro_tracker.core.utils.http.requests')
class UpdateWnppStatsTaskTests(TestCase):
"""
@@ -3502,7 +3503,7 @@ class UpdateWnppStatsTaskTests(TestCase):
:param content: A list of (package_name, issues) pairs. ``issues`` is
a list of dicts describing the WNPP bugs the package has.
"""
- self.task._get_wnpp_content.return_value = '\n'.join(
+ return '\n'.join(
'{pkg}: {issues}'.format(
pkg=pkg,
issues='|'.join(
@@ -3515,19 +3516,20 @@ class UpdateWnppStatsTaskTests(TestCase):
def run_task(self):
self.task.execute()
- def test_action_item_created(self):
+ def test_action_item_created(self, mock_requests):
"""
Tests that an :class:`ActionItem
<distro_tracker.core.models.ActionItem>` instance is created when the
package has a WNPP bug.
"""
wnpp_type, bug_id = 'O', 12345
- self.set_wnpp_content([(
+ content = self.set_wnpp_content([(
self.package.name, [{
'wnpp_type': wnpp_type,
'bug_id': bug_id,
}]
)])
+ set_mock_response(mock_requests, text=content)
self.run_task()
@@ -3552,19 +3554,20 @@ class UpdateWnppStatsTaskTests(TestCase):
' been orphaned and needs a maintainer.</a>')
self.assertEqual(dsc, item.short_description)
- def test_action_item_created_unknown_type(self):
+ def test_action_item_created_unknown_type(self, mock_requests):
"""
Tests that an :class:`ActionItem
<distro_tracker.core.models.ActionItem>` instance is created when the
package has a WNPP bug of an unknown type.
"""
wnpp_type, bug_id = 'RFC', 12345
- self.set_wnpp_content([(
+ content = self.set_wnpp_content([(
self.package.name, [{
'wnpp_type': wnpp_type,
'bug_id': bug_id,
}]
)])
+ set_mock_response(mock_requests, text=content)
self.run_task()
@@ -3589,7 +3592,7 @@ class UpdateWnppStatsTaskTests(TestCase):
' contains an entry for this package.</a>')
self.assertEqual(dsc, item.short_description)
- def test_action_item_updated(self):
+ def test_action_item_updated(self, mock_requests):
"""
Tests that an existing :class:`ActionItem
<distro_tracker.core.models.ActionItem>` instance is updated when there
@@ -3609,12 +3612,13 @@ class UpdateWnppStatsTaskTests(TestCase):
old_timestamp = old_item.last_updated_timestamp
# Set new WNPP info
wnpp_type, bug_id = 'O', 12345
- self.set_wnpp_content([(
+ content = self.set_wnpp_content([(
self.package.name, [{
'wnpp_type': wnpp_type,
'bug_id': bug_id,
}]
)])
+ set_mock_response(mock_requests, text=content)
self.run_task()
@@ -3630,7 +3634,7 @@ class UpdateWnppStatsTaskTests(TestCase):
}
self.assertEqual(expected_data, item.extra_data['wnpp_info'])
- def test_action_item_not_updated(self):
+ def test_action_item_not_updated(self, mock_requests):
"""
Tests that an existing :class:`ActionItem
<distro_tracker.core.models.ActionItem>` instance is not updated when
@@ -3649,12 +3653,13 @@ class UpdateWnppStatsTaskTests(TestCase):
})
old_timestamp = old_item.last_updated_timestamp
# Set "new" WNPP info
- self.set_wnpp_content([(
+ content = self.set_wnpp_content([(
self.package.name, [{
'wnpp_type': wnpp_type,
'bug_id': bug_id,
}]
)])
+ set_mock_response(mock_requests, text=content)
self.run_task()
@@ -3664,7 +3669,7 @@ class UpdateWnppStatsTaskTests(TestCase):
item = ActionItem.objects.all()[0]
self.assertEqual(old_timestamp, item.last_updated_timestamp)
- def test_action_item_removed(self):
+ def test_action_item_removed(self, mock_requests):
"""
Tests that an existing :class:`ActionItem
<distro_tracker.core.models.ActionItem>` instance is removed when there
@@ -3682,6 +3687,7 @@ class UpdateWnppStatsTaskTests(TestCase):
},
})
# Set "new" WNPP info
+ set_mock_response(mock_requests, text="")
self.run_task()
@@ -3689,26 +3695,27 @@ class UpdateWnppStatsTaskTests(TestCase):
# No more actino items
self.assertEqual(0, ActionItem.objects.count())
- def test_action_item_not_created(self):
+ def test_action_item_not_created(self, mock_requests):
"""
Tests that an :class:`ActionItem
<distro_tracker.core.models.ActionItem>` instance is not created for non
existing packages.
"""
wnpp_type, bug_id = 'O', 12345
- self.set_wnpp_content([(
+ content = self.set_wnpp_content([(
'no-exist', [{
'wnpp_type': wnpp_type,
'bug_id': bug_id,
}]
)])
+ set_mock_response(mock_requests, text=content)
self.run_task()
# No action items
self.assertEqual(0, ActionItem.objects.count())
- def test_action_item_multiple_packages(self):
+ def test_action_item_multiple_packages(self, mock_requests):
"""
Tests that an :class:`ActionItem
<distro_tracker.core.models.ActionItem>` instance is created for
@@ -3728,10 +3735,11 @@ class UpdateWnppStatsTaskTests(TestCase):
name='other-package',
source=True)
packages = [other_package, self.package]
- self.set_wnpp_content([
+ content = self.set_wnpp_content([
(package.name, [wnpp_item])
for package, wnpp_item in zip(packages, wnpp)
])
+ set_mock_response(mock_requests, text=content)
self.run_task()
diff --git a/distro_tracker/vendor/debian/tracker_tasks.py b/distro_tracker/vendor/debian/tracker_tasks.py
index 541db22..d5976cd 100644
--- a/distro_tracker/vendor/debian/tracker_tasks.py
+++ b/distro_tracker/vendor/debian/tracker_tasks.py
@@ -34,6 +34,7 @@ from distro_tracker.vendor.debian.models import PackageExcuses
from distro_tracker.vendor.debian.models import UbuntuPackage
from distro_tracker.core.utils.http import HttpCache
from distro_tracker.core.utils.http import get_resource_content
+from distro_tracker.core.utils.http import get_resource_text
from distro_tracker.core.utils.misc import get_data_checksum
from distro_tracker.core.utils.packages import package_hashdir
from .models import DebianContributor
@@ -1758,16 +1759,6 @@ class UpdateWnppStatsTask(BaseTask):
if 'force_update' in parameters:
self.force_update = parameters['force_update']
- def _get_wnpp_content(self):
- url = 'https://qa.debian.org/data/bts/wnpp_rm'
- cache = HttpCache(settings.DISTRO_TRACKER_CACHE_DIRECTORY)
- if not cache.is_expired(url):
- return
- response, updated = cache.update(url, force=self.force_update)
- if not updated:
- return
- return response.content
-
def get_wnpp_stats(self):
"""
Retrieves and parses the wnpp stats for all packages. WNPP stats
@@ -1775,7 +1766,8 @@ class UpdateWnppStatsTask(BaseTask):
:returns: A dict mapping package names to wnpp stats.
"""
- content = self._get_wnpp_content()
+ url = 'https://qa.debian.org/data/bts/wnpp_rm'
+ content = get_resource_text(url, only_if_updated=True)
if content is None:
return
--
2.11.0
Attachment:
signature.asc
Description: PGP signature