Le jeudi 30 novembre 2017 à 22:39:52+0100, Pierre-Elliott Bécue a écrit : > 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.
Based on comments I received on IRC, here are new patches that are a little better and more atomic. -- PEB
From 4e8798ae4d3d79227a09607e58dff1339210493a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pierre-Elliott=20B=C3=A9cue?= <be...@crans.org>
Date: Fri, 1 Dec 2017 14:43:42 +0100
Subject: [PATCH 1/4] Adds an only_if_updated argument to get_resource
functions
---
distro_tracker/core/tests/tests_utils.py | 97 ++++++++++++++++++++++++++++++--
distro_tracker/core/utils/http.py | 28 +++++++--
2 files changed, 114 insertions(+), 11 deletions(-)
diff --git a/distro_tracker/core/tests/tests_utils.py b/distro_tracker/core/tests/tests_utils.py
index 572d31f..3f5d75d 100644
--- a/distro_tracker/core/tests/tests_utils.py
+++ b/distro_tracker/core/tests/tests_utils.py
@@ -764,13 +764,11 @@ Section: libs
class HttpCacheTest(SimpleTestCase):
- def set_mock_response(self, mock_requests, headers=None, status_code=200):
- set_mock_response(
- mock_requests,
- text=self.response_content.decode('utf-8'),
- headers=headers,
- status_code=status_code)
+ """Tests for the HttpCache utility"""
+ #
+ # Tests constructors
+ #
def setUp(self):
# Set up a cache directory to use in the tests
self.cache_directory = tempfile.mkdtemp(suffix='test-cache')
@@ -783,6 +781,30 @@ class HttpCacheTest(SimpleTestCase):
import shutil
shutil.rmtree(self.cache_directory)
+ #
+ # Tests helper functions, to avoid code redundancy
+ #
+ def set_mock_response(self, mock_requests, headers=None, status_code=200):
+ """Defines a mock response for the cases where http.requests is
+ patched via mock."""
+ set_mock_response(
+ mock_requests,
+ text=self.response_content.decode('utf-8'),
+ headers=headers,
+ status_code=status_code)
+
+ def get_resource_function_common_setup(self):
+ """
+ Common setup function for the get_resource function tests
+ to avoid code redundancy.
+ """
+ mock_cache = mock.create_autospec(HttpCache)
+ self.response_content = b"Some content"
+ mock_cache.get_content.return_value = self.response_content
+ url = 'http://some.url.com'
+
+ return url, mock_cache
+
def test_parse_cache_control_header(self):
"""
Tests the utility function for parsing a Cache-Control header into a
@@ -1128,6 +1150,69 @@ class HttpCacheTest(SimpleTestCase):
# The function updated the cache
mock_cache.update.assert_called_once_with(url)
+ def test_get_resource_content_with_only_arg_and_cache_expired(self):
+ """
+ Tests the :func:`distro_tracker.core.utils.http.get_resource_content`
+ utility function with the only_if_updated argument and an expired
+ cache.
+ """
+
+ url, mock_cache = self.get_resource_function_common_setup()
+
+ # Cache expired.
+ mock_cache.is_expired.return_value = True
+ mock_cache.update.return_value = (None, True)
+
+ # First call.
+ content = get_resource_content(url, mock_cache, only_if_updated=True)
+
+ # Has to work
+ self.assertEqual(content, self.response_content)
+
+ # The function updated the cache
+ mock_cache.update.assert_called_once_with(url)
+
+ def test_get_resource_content_with_only_arg_and_cache_not_expired(self):
+ """
+ Tests the :func:`distro_tracker.core.utils.http.get_resource_content`
+ utility function with the only_if_updated argument and a not expired
+ cache.
+ """
+
+ url, mock_cache = self.get_resource_function_common_setup()
+
+ # This time, the cache is not expired.
+ mock_cache.is_expired.return_value = False
+
+ content = get_resource_content(url, mock_cache, only_if_updated=True)
+
+ # No content, because not updated.
+ self.assertIsNone(content)
+
+ # The function did not update the cache
+ self.assertFalse(mock_cache.update.called)
+
+ def test_get_resource_content_with_only_arg_cache_expired_no_update(self):
+ """
+ Tests the :func:`distro_tracker.core.utils.http.get_resource_content`
+ utility function with the only_if_updated argument, an expired cache
+ but no real update done.
+ """
+
+ url, mock_cache = self.get_resource_function_common_setup()
+
+ # Cache expired.
+ mock_cache.is_expired.return_value = True
+ mock_cache.update.return_value = (None, False)
+
+ content = get_resource_content(url, mock_cache, only_if_updated=True)
+
+ # Nothing returned
+ self.assertIsNone(content)
+
+ # The function updated the cache
+ mock_cache.update.assert_called_once_with(url)
+
@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..37dd437 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,
+ compression="auto",
+ only_if_updated=False):
"""
A helper function which returns the content of the resource found at the
given URL.
@@ -206,6 +209,9 @@ def get_resource_content(url, cache=None, compression="auto"):
resource, and thus the compression method one should use to decompress
it. If auto, then guess it from the url file extension.
:type compression: str
+ :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
:returns: The bytes representation of the resource found at the given url
:rtype: bytes
@@ -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,
+ compression="auto",
+ only_if_updated=False,
+ 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 100d8bb8d3030e8f7a0ef8c330a3942a96fc31e8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pierre-Elliott=20B=C3=A9cue?= <be...@crans.org>
Date: Fri, 1 Dec 2017 14:44:22 +0100
Subject: [PATCH 2/4] Refactors the HttpCache tests using
get_resource_function_common_setup
---
distro_tracker/core/tests/tests_utils.py | 36 +++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/distro_tracker/core/tests/tests_utils.py b/distro_tracker/core/tests/tests_utils.py
index 3f5d75d..480bf73 100644
--- a/distro_tracker/core/tests/tests_utils.py
+++ b/distro_tracker/core/tests/tests_utils.py
@@ -805,6 +805,9 @@ class HttpCacheTest(SimpleTestCase):
return url, mock_cache
+ #
+ # Proper tests - Headers functionalities
+ #
def test_parse_cache_control_header(self):
"""
Tests the utility function for parsing a Cache-Control header into a
@@ -982,6 +985,9 @@ class HttpCacheTest(SimpleTestCase):
self.assertTrue(cache.is_expired(url))
+ #
+ # Proper tests - Caching behaviour
+ #
@mock.patch('distro_tracker.core.utils.http.requests')
def test_cache_remove_url(self, mock_requests):
"""
@@ -998,6 +1004,9 @@ class HttpCacheTest(SimpleTestCase):
self.assertFalse(url in cache)
+ #
+ # Proper tests - ETags
+ #
@mock.patch('distro_tracker.core.utils.http.requests')
def test_conditional_get_etag(self, mock_requests):
"""
@@ -1081,6 +1090,9 @@ class HttpCacheTest(SimpleTestCase):
headers={'Cache-Control': 'no-cache'})
self.assertTrue(updated)
+ #
+ # Proper tests - Compression utilities
+ #
@mock.patch('distro_tracker.core.utils.http.requests')
def test_get_content_detects_compression(self, mock_requests):
"""
@@ -1119,16 +1131,17 @@ class HttpCacheTest(SimpleTestCase):
utility function when the resource is cached in the given cache
instance.
"""
- mock_cache = mock.create_autospec(HttpCache)
+
+ url, mock_cache = self.get_resource_function_common_setup()
+
+ # In this test, the is_expired call has to return False
mock_cache.is_expired.return_value = False
- expected_content = b"Some content"
- mock_cache.get_content.return_value = expected_content
- url = 'http://some.url.com'
content = get_resource_content(url, cache=mock_cache)
# The expected content is retrieved
- self.assertEqual(content, expected_content)
+ self.assertEqual(content, self.response_content)
+
# The function did not update the cache
self.assertFalse(mock_cache.update.called)
@@ -1138,15 +1151,18 @@ class HttpCacheTest(SimpleTestCase):
utility function when the resource is not cached in the given cache
instance.
"""
- mock_cache = mock.create_autospec(HttpCache)
+
+ url, mock_cache = self.get_resource_function_common_setup()
+
+ # In this test, the cache is expired, and hence update has
+ # to be called.
mock_cache.is_expired.return_value = True
- expected_content = b"Some content"
- mock_cache.get_content.return_value = expected_content
- url = 'http://some.url.com'
+ mock_cache.update.return_value = (None, True)
content = get_resource_content(url, mock_cache)
- self.assertEqual(content, expected_content)
+ self.assertEqual(content, self.response_content)
+
# The function updated the cache
mock_cache.update.assert_called_once_with(url)
--
2.11.0
From adb0a19dbedc95c16a743699090224e13565e4b7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pierre-Elliott=20B=C3=A9cue?= <be...@crans.org>
Date: Fri, 1 Dec 2017 15:45:30 +0100
Subject: [PATCH 3/4] Fixes bug #883101 by using get_resource_text in
UpdateWnppStatsTask
---
distro_tracker/vendor/debian/tracker_tasks.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/distro_tracker/vendor/debian/tracker_tasks.py b/distro_tracker/vendor/debian/tracker_tasks.py
index 541db22..18a67fd 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
@@ -1775,7 +1776,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
From fccd9c27bfbd2783fd98bf90b3d40d040fd61e31 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pierre-Elliott=20B=C3=A9cue?= <be...@crans.org>
Date: Fri, 1 Dec 2017 15:46:43 +0100
Subject: [PATCH 4/4] Gets rid of _get_wnpp_content function and update tests
---
distro_tracker/vendor/debian/tests.py | 36 ++++++++++++++++-----------
distro_tracker/vendor/debian/tracker_tasks.py | 10 --------
2 files changed, 22 insertions(+), 24 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 18a67fd..d5976cd 100644
--- a/distro_tracker/vendor/debian/tracker_tasks.py
+++ b/distro_tracker/vendor/debian/tracker_tasks.py
@@ -1759,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
--
2.11.0
signature.asc
Description: PGP signature
