Differential D1733 Diff 5890 swh/lister/core/tests/test_lister.py

Changeset View

Standalone View

swh/lister/core/tests/test_lister.py

# Copyright (C) 2017-2018 the Software Heritage developers		# Copyright (C) 2019 the Software Heritage developers
# License: GNU General Public License version 3, or any later version		# License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information		# See top-level LICENSE file for more information

import abc		import abc
import time		import time
from unittest import TestCase		from unittest import TestCase
from unittest.mock import Mock, patch		from unittest.mock import Mock, patch

import requests_mock		import requests_mock
from sqlalchemy import create_engine		from sqlalchemy import create_engine
from testing.postgresql import Postgresql		from testing.postgresql import Postgresql

from swh.lister.core.abstractattribute import AbstractAttribute		from swh.lister.core.abstractattribute import AbstractAttribute


def noop(args, *kwargs):		def noop(args, *kwargs):
pass		pass


@requests_mock.Mocker()
class HttpListerTesterBase(abc.ABC):		class HttpListerTesterBase(abc.ABC):
"""Base testing class for subclasses of		"""Testing base class for listers.
		This contains methods for both :class:`HttpSimpleListerTester` and
swh.lister.core.indexing_lister.IndexingHttpLister.		:class:`HttpListerTester`.
swh.lister.core.page_by_page_lister.PageByPageHttpLister

See swh.lister.github.tests.test_gh_lister for an example of how		See :class:`swh.lister.gitlab.tests.test_lister` for an example of how
to customize for a specific listing service.		to customize for a specific listing service.
		ardumontUnsubmitted Done Inline Actions Right. Please, use the following format which is clearer to read. Testing base class for listers. This contains methods for both :class:`HttpSimpleListerTester` and :class:`HttpListerTester`. ... See :class:`swh.lister.gitlab.tests.test_lister` for ... (Note: It's unclear whether we want or need to add the fully qualified module name or not so, i'd say as a first approximation, it's fine) That has also the advantages to render correctly in the sphinx documentation (swh-docs module). Documentation we generate the documentation out of the docstrings (might as well format them correctly). ardumont: Right. Please, use the following format which is clearer to read. ``` Testing base class for…

"""		"""
Lister = AbstractAttribute('The lister class to test')		Lister = AbstractAttribute('The lister class to test')
test_re = AbstractAttribute('Compiled regex matching the server url. Must'
' capture the index value.')
lister_subdir = AbstractAttribute('bitbucket, github, etc.')		lister_subdir = AbstractAttribute('bitbucket, github, etc.')
good_api_response_file = AbstractAttribute('Example good response body')		good_api_response_file = AbstractAttribute('Example good response body')
bad_api_response_file = AbstractAttribute('Example bad response body')
first_index = AbstractAttribute('First index in good_api_response')
entries_per_page = AbstractAttribute('Number of results in good response')
LISTER_NAME = 'fake-lister'		LISTER_NAME = 'fake-lister'
convert_type = str
"""static method used to convert the "request_index" to its right type (for
indexing listers for example, this is in accordance with the model's
"indexable" column).

"""

# May need to override this if the headers are used for something		# May need to override this if the headers are used for something
def response_headers(self, request):		def response_headers(self, request):
return {}		return {}

# May need to override this if the server uses non-standard rate limiting		# May need to override this if the server uses non-standard rate limiting
# method.		# method.
# Please keep the requested retry delay reasonably low.		# Please keep the requested retry delay reasonably low.
Show All 9 Lines	def __init__(self, args, *kwargs):
self.response = None		self.response = None
self.fl = None		self.fl = None
self.helper = None		self.helper = None
if self.__class__ != HttpListerTesterBase:		if self.__class__ != HttpListerTesterBase:
self.run = TestCase.run.__get__(self, self.__class__)		self.run = TestCase.run.__get__(self, self.__class__)
else:		else:
self.run = noop		self.run = noop

def request_index(self, request):
m = self.test_re.search(request.path_url)
if m and (len(m.groups()) > 0):
return self.convert_type(m.group(1))

def mock_response(self, request, context):
self.fl.reset_backoff()
self.rate_limit = 1
context.status_code = 200
custom_headers = self.response_headers(request)
context.headers.update(custom_headers)
req_index = self.request_index(request)

if req_index == self.first_index:
response_file = self.good_api_response_file
else:
response_file = self.bad_api_response_file

with open('swh/lister/%s/tests/%s' % (self.lister_subdir,
response_file),
nahimilegaAuthorUnsubmitted Done Inline Actions The bad_api_response_file is used when the req_index is not equal to first index. But for simple lister, there is nothing such as index, so I am not able to understand on what condition shall I use `bad_api_response_file` nahimilega: The bad_api_response_file is used when the req_index is not equal to first index. But for…
ardumontUnsubmitted Done Inline Actions well, no bad response then ;) ardumont: well, no bad response then ;)
'r', encoding='utf-8') as r:
return r.read()

def mock_limit_n_response(self, n, request, context):		def mock_limit_n_response(self, n, request, context):
self.fl.reset_backoff()		self.fl.reset_backoff()
if self.rate_limit <= n:		if self.rate_limit <= n:
return self.mock_rate_quota(n, request, context)		return self.mock_rate_quota(n, request, context)
else:		else:
return self.mock_response(request, context)		return self.mock_response(request, context)

def mock_limit_once_response(self, request, context):
return self.mock_limit_n_response(1, request, context)

def mock_limit_twice_response(self, request, context):		def mock_limit_twice_response(self, request, context):
return self.mock_limit_n_response(2, request, context)		return self.mock_limit_n_response(2, request, context)

		def get_api_response(self, identifier):
		fl = self.get_fl()
		if self.response is None:
		self.response = fl.safely_issue_request(identifier)
		return self.response

def get_fl(self, override_config=None):		def get_fl(self, override_config=None):
"""Retrieve an instance of fake lister (fl).		"""Retrieve an instance of fake lister (fl).

"""		"""
if override_config or self.fl is None:		if override_config or self.fl is None:
self.fl = self.Lister(api_baseurl='https://fakeurl',		self.fl = self.Lister(api_baseurl='https://fakeurl',
override_config=override_config)		override_config=override_config)
self.fl.INITIAL_BACKOFF = 1		self.fl.INITIAL_BACKOFF = 1

self.fl.reset_backoff()		self.fl.reset_backoff()
return self.fl		return self.fl

def get_api_response(self):		def disable_scheduler(self, fl):
fl = self.get_fl()		fl.schedule_missing_tasks = Mock(return_value=None)
if self.response is None:
self.response = fl.safely_issue_request(self.first_index)
return self.response

		def disable_db(self, fl):
		fl.winnow_models = Mock(return_value=[])
		fl.db_inject_repo = Mock(return_value=fl.MODEL())
		fl.disable_deleted_repo_tasks = Mock(return_value=None)

		def init_db(self, db, model):
		engine = create_engine(db.url())
		model.metadata.create_all(engine)

		@requests_mock.Mocker()
def test_is_within_bounds(self, http_mocker):		def test_is_within_bounds(self, http_mocker):
fl = self.get_fl()		fl = self.get_fl()
self.assertFalse(fl.is_within_bounds(1, 2, 3))		self.assertFalse(fl.is_within_bounds(1, 2, 3))
self.assertTrue(fl.is_within_bounds(2, 1, 3))		self.assertTrue(fl.is_within_bounds(2, 1, 3))
self.assertTrue(fl.is_within_bounds(1, 1, 1))		self.assertTrue(fl.is_within_bounds(1, 1, 1))
self.assertTrue(fl.is_within_bounds(1, None, None))		self.assertTrue(fl.is_within_bounds(1, None, None))
self.assertTrue(fl.is_within_bounds(1, None, 2))		self.assertTrue(fl.is_within_bounds(1, None, 2))
self.assertTrue(fl.is_within_bounds(1, 0, None))		self.assertTrue(fl.is_within_bounds(1, 0, None))
self.assertTrue(fl.is_within_bounds("b", "a", "c"))		self.assertTrue(fl.is_within_bounds("b", "a", "c"))
self.assertFalse(fl.is_within_bounds("a", "b", "c"))		self.assertFalse(fl.is_within_bounds("a", "b", "c"))
self.assertTrue(fl.is_within_bounds("a", None, "c"))		self.assertTrue(fl.is_within_bounds("a", None, "c"))
self.assertTrue(fl.is_within_bounds("a", None, None))		self.assertTrue(fl.is_within_bounds("a", None, None))
self.assertTrue(fl.is_within_bounds("b", "a", None))		self.assertTrue(fl.is_within_bounds("b", "a", None))
self.assertFalse(fl.is_within_bounds("a", "b", None))		self.assertFalse(fl.is_within_bounds("a", "b", None))
self.assertTrue(fl.is_within_bounds("aa:02", "aa:01", "aa:03"))		self.assertTrue(fl.is_within_bounds("aa:02", "aa:01", "aa:03"))
self.assertFalse(fl.is_within_bounds("aa:12", None, "aa:03"))		self.assertFalse(fl.is_within_bounds("aa:12", None, "aa:03"))
with self.assertRaises(TypeError):		with self.assertRaises(TypeError):
fl.is_within_bounds(1.0, "b", None)		fl.is_within_bounds(1.0, "b", None)
with self.assertRaises(TypeError):		with self.assertRaises(TypeError):
fl.is_within_bounds("A:B", "A::B", None)		fl.is_within_bounds("A:B", "A::B", None)

def test_api_request(self, http_mocker):
http_mocker.get(self.test_re, text=self.mock_limit_twice_response)
with patch.object(time, 'sleep', wraps=time.sleep) as sleepmock:
self.get_api_response()
self.assertEqual(sleepmock.call_count, 2)

def test_repos_list(self, http_mocker):		class HttpListerTester(HttpListerTesterBase, abc.ABC):
http_mocker.get(self.test_re, text=self.mock_response)		"""Base testing class for subclass of
li = self.get_fl().transport_response_simplified(
self.get_api_response()
)
self.assertIsInstance(li, list)
self.assertEqual(len(li), self.entries_per_page)

def test_model_map(self, http_mocker):		:class:`swh.lister.core.indexing_lister.IndexingHttpLister`

		See :class:`swh.lister.github.tests.test_gh_lister` for an example of how
		to customize for a specific listing service.

		"""
		last_index = AbstractAttribute('Last index in good_api_response')
		first_index = AbstractAttribute('First index in good_api_response')
		bad_api_response_file = AbstractAttribute('Example bad response body')
		entries_per_page = AbstractAttribute('Number of results in good response')
		test_re = AbstractAttribute('Compiled regex matching the server url. Must'
		' capture the index value.')
		convert_type = str
		"""static method used to convert the "request_index" to its right type (for
		indexing listers for example, this is in accordance with the model's
		"indexable" column).

		"""
		def mock_response(self, request, context):
		self.fl.reset_backoff()
		self.rate_limit = 1
		context.status_code = 200
		custom_headers = self.response_headers(request)
		context.headers.update(custom_headers)
		req_index = self.request_index(request)

		if req_index == self.first_index:
		response_file = self.good_api_response_file
		else:
		response_file = self.bad_api_response_file

		with open('swh/lister/%s/tests/%s' % (self.lister_subdir,
		response_file),
		'r', encoding='utf-8') as r:
		return r.read()

		def request_index(self, request):
		m = self.test_re.search(request.path_url)
		if m and (len(m.groups()) > 0):
		return self.convert_type(m.group(1))

		@requests_mock.Mocker()
		def test_fetch_multiple_pages_yesdb(self, http_mocker):
http_mocker.get(self.test_re, text=self.mock_response)		http_mocker.get(self.test_re, text=self.mock_response)
fl = self.get_fl()		initdb_args = Postgresql.DEFAULT_SETTINGS['initdb_args']
li = fl.transport_response_simplified(self.get_api_response())		initdb_args = ' '.join([initdb_args, '-E UTF-8'])
di = li[0]		db = Postgresql(initdb_args=initdb_args)
self.assertIsInstance(di, dict)
pubs = [k for k in vars(fl.MODEL).keys() if not k.startswith('_')]
for k in pubs:
if k not in ['last_seen', 'task_id', 'id']:
self.assertIn(k, di)

def disable_scheduler(self, fl):		fl = self.get_fl(override_config={
fl.schedule_missing_tasks = Mock(return_value=None)		'lister': {
		'cls': 'local',
		'args': {'db': db.url()}
		}
		})
		self.init_db(db, fl.MODEL)

def disable_db(self, fl):		self.disable_scheduler(fl)
fl.winnow_models = Mock(return_value=[])
fl.db_inject_repo = Mock(return_value=fl.MODEL())		fl.run(min_bound=self.first_index)
fl.disable_deleted_repo_tasks = Mock(return_value=None)

		self.assertEqual(fl.db_last_index(), self.last_index)
		partitions = fl.db_partition_indices(5)
		self.assertGreater(len(partitions), 0)
		for k in partitions:
		self.assertLessEqual(len(k), 5)
		self.assertGreater(len(k), 0)

		@requests_mock.Mocker()
def test_fetch_none_nodb(self, http_mocker):		def test_fetch_none_nodb(self, http_mocker):
http_mocker.get(self.test_re, text=self.mock_response)		http_mocker.get(self.test_re, text=self.mock_response)
fl = self.get_fl()		fl = self.get_fl()
		ardumontUnsubmitted Done Inline Actions Same as before, please use the following stanza when specifying a class: :class:`swh.lister.core.indexing_lister.IndexingHttpLister` ardumont: Same as before, please use the following stanza when specifying a class: ``` :class:`swh.

self.disable_scheduler(fl)		self.disable_scheduler(fl)
self.disable_db(fl)		self.disable_db(fl)

fl.run(min_bound=1, max_bound=1) # stores no results		fl.run(min_bound=1, max_bound=1) # stores no results
		# FIXME: Determine what this method tries to test and add checks to
		# actually test

		@requests_mock.Mocker()
def test_fetch_one_nodb(self, http_mocker):		def test_fetch_one_nodb(self, http_mocker):
http_mocker.get(self.test_re, text=self.mock_response)		http_mocker.get(self.test_re, text=self.mock_response)
fl = self.get_fl()		fl = self.get_fl()

self.disable_scheduler(fl)		self.disable_scheduler(fl)
self.disable_db(fl)		self.disable_db(fl)

fl.run(min_bound=self.first_index, max_bound=self.first_index)		fl.run(min_bound=self.first_index, max_bound=self.first_index)
		# FIXME: Determine what this method tries to test and add checks to
		# actually test

		@requests_mock.Mocker()
def test_fetch_multiple_pages_nodb(self, http_mocker):		def test_fetch_multiple_pages_nodb(self, http_mocker):
		nahimilegaAuthorUnsubmitted Done Inline Actions I am not sure what these tests do, so I have not added docstring here. I think they test the indexing property of the lister. Am I correct? nahimilega: I am not sure what these tests do, so I have not added docstring here. I think they test the…
		ardumontUnsubmitted Done Inline Actions Not sure either, the thing is, they do not check anything at the end... That's wrong. ardumont: Not sure either, the thing is, they do not check anything at the end... That's wrong.
		ardumontUnsubmitted Done Inline Actions Please add a fixme after `fl.run` and we'll be done with it. Something like: # FIXME: Determine what this method tries to test and add checks to actually test Same goes for the `test_fetch_one_nodb`. ardumont: Please add a fixme after `fl.run` and we'll be done with it. Something like: ``` # FIXME…
http_mocker.get(self.test_re, text=self.mock_response)		http_mocker.get(self.test_re, text=self.mock_response)
fl = self.get_fl()		fl = self.get_fl()

self.disable_scheduler(fl)		self.disable_scheduler(fl)
self.disable_db(fl)		self.disable_db(fl)

fl.run(min_bound=self.first_index)		fl.run(min_bound=self.first_index)
		# FIXME: Determine what this method tries to test and add checks to
		# actually test

def init_db(self, db, model):		@requests_mock.Mocker()
engine = create_engine(db.url())		def test_repos_list(self, http_mocker):
model.metadata.create_all(engine)		"""Test the number of repos listed by the lister


class HttpListerTester(HttpListerTesterBase, abc.ABC):		"""
last_index = AbstractAttribute('Last index in good_api_response')		http_mocker.get(self.test_re, text=self.mock_response)
		li = self.get_fl().transport_response_simplified(
		self.get_api_response(self.first_index)
		)
		self.assertIsInstance(li, list)
		self.assertEqual(len(li), self.entries_per_page)

@requests_mock.Mocker()		@requests_mock.Mocker()
def test_fetch_multiple_pages_yesdb(self, http_mocker):		def test_model_map(self, http_mocker):
		"""Check if all the keys of model are present in the model created by
		the `transport_response_simplified`

		"""
http_mocker.get(self.test_re, text=self.mock_response)		http_mocker.get(self.test_re, text=self.mock_response)
initdb_args = Postgresql.DEFAULT_SETTINGS['initdb_args']		fl = self.get_fl()
initdb_args = ' '.join([initdb_args, '-E UTF-8'])		li = fl.transport_response_simplified(
db = Postgresql(initdb_args=initdb_args)		self.get_api_response(self.first_index))
		di = li[0]
		ardumontUnsubmitted Done Inline Actions Same, please use :class: notation. ardumont: Same, please use :class: notation.
		self.assertIsInstance(di, dict)
		pubs = [k for k in vars(fl.MODEL).keys() if not k.startswith('_')]
		for k in pubs:
		if k not in ['last_seen', 'task_id', 'id']:
		self.assertIn(k, di)

		nahimilegaAuthorUnsubmitted Done Inline Actions Here we can't put this `@requests_mock.Mocker()` on top of the class because of Gitlab lister. nahimilega: Here we can't put this `@requests_mock.Mocker()` on top of the class because of Gitlab lister.
		ardumontUnsubmitted Done Inline Actions Ok. ardumont: Ok.
fl = self.get_fl(override_config={		@requests_mock.Mocker()
'lister': {		def test_api_request(self, http_mocker):
'cls': 'local',		"""Test API request for rate limit handling
'args': {'db': db.url()}
}
})
self.init_db(db, fl.MODEL)

self.disable_scheduler(fl)		"""
		http_mocker.get(self.test_re, text=self.mock_limit_twice_response)
		with patch.object(time, 'sleep', wraps=time.sleep) as sleepmock:
		self.get_api_response(self.first_index)
		self.assertEqual(sleepmock.call_count, 2)
		ardumontUnsubmitted Done Inline Actions Add a docstring explaining what you are testing. There are (as existing tests unfortunately) too many instructions to read to understand what that method actually tests. As you are adding it, you know what you are testing. Might as well documenting now the test (same goes for other methods of that class). ardumont: Add a docstring explaining what you are testing. There are (as existing tests unfortunately)…

fl.run(min_bound=self.first_index)

self.assertEqual(fl.db_last_index(), self.last_index)		class HttpSimpleListerTester(HttpListerTesterBase, abc.ABC):
partitions = fl.db_partition_indices(5)		"""Base testing class for subclass of
self.assertGreater(len(partitions), 0)		:class:`swh.lister.core.simple)_lister.SimpleLister`
for k in partitions:
self.assertLessEqual(len(k), 5)		See :class:`swh.lister.pypi.tests.test_lister` for an example of how
self.assertGreater(len(k), 0)		to customize for a specific listing service.

		"""
		entries = AbstractAttribute('Number of results in good response')
		PAGE = AbstractAttribute("The server api's unique page to retrieve and "
		"parse for information")

		def get_fl(self, override_config=None):
		"""Retrieve an instance of fake lister (fl).

		"""
		if override_config or self.fl is None:
		self.fl = self.Lister(
		override_config=override_config)
		ardumontUnsubmitted Done Inline Actions Can you explicit the need to override the default `get_fl` implementation? Probably initialization parameter divergence but still... ardumont: Can you explicit the need to override the default `get_fl` implementation? Probably…
		nahimilegaAuthorUnsubmitted Done Inline Actions There is no parameter such as `api_base_url` in the simple lister. nahimilega: There is no parameter such as `api_base_url` in the simple lister.
		self.fl.INITIAL_BACKOFF = 1

		self.fl.reset_backoff()
		return self.fl

		def mock_response(self, request, context):
		self.fl.reset_backoff()
		self.rate_limit = 1
		context.status_code = 200
		custom_headers = self.response_headers(request)
		context.headers.update(custom_headers)
		response_file = self.good_api_response_file

		ardumontUnsubmitted Done Inline Actions Same here, what does actually `mock_response` do? Why the need for overriding it? ardumont: Same here, what does actually `mock_response` do? Why the need for overriding it?
		nahimilegaAuthorUnsubmitted Done Inline Actions Because there is the use of `index` in `mock_response` of `HttpListerTester`. Whereas there is no index in the simple lister. There is a problem in this `mock_response` method. I stated the problem in the other comment(don't know how to utilize bad_api_response) nahimilega: Because there is the use of `index` in `mock_response` of `HttpListerTester`. Whereas there is…
		with open('swh/lister/%s/tests/%s' % (self.lister_subdir,
		response_file),
		'r', encoding='utf-8') as r:
		return r.read()

		@requests_mock.Mocker()
		def test_api_request(self, http_mocker):
		"""Test API request for rate limit handling
		ardumontUnsubmitted Done Inline Actions that actually tests nothing. There should be assertion(s) after the actual method is called here (`fl.run()` is the method to be tested). ardumont: that actually tests nothing. There should be assertion(s) after the actual method is called…

		"""
		http_mocker.get(self.PAGE, text=self.mock_limit_twice_response)
		with patch.object(time, 'sleep', wraps=time.sleep) as sleepmock:
		self.get_api_response(0)
		self.assertEqual(sleepmock.call_count, 2)

		@requests_mock.Mocker()
		def test_model_map(self, http_mocker):
		"""Check if all the keys of model are present in the model created by
		the `transport_response_simplified`

		"""
		http_mocker.get(self.PAGE, text=self.mock_response)
		fl = self.get_fl()
		li = fl.list_packages(self.get_api_response(0))
		li = fl.transport_response_simplified(li)
		di = li[0]
		self.assertIsInstance(di, dict)
		pubs = [k for k in vars(fl.MODEL).keys() if not k.startswith('_')]
		for k in pubs:
		if k not in ['last_seen', 'task_id', 'id']:
		self.assertIn(k, di)

		@requests_mock.Mocker()
		def test_repos_list(self, http_mocker):
		"""Test the number of packages listed by the lister

		"""
		http_mocker.get(self.PAGE, text=self.mock_response)
		li = self.get_fl().list_packages(
		self.get_api_response(0)
		)
		self.assertIsInstance(li, list)
		self.assertEqual(len(li), self.entries)
		ardumontUnsubmitted Done Inline Actions Same as `test_fetch_none_nodb`, there should be assertions. ardumont: Same as `test_fetch_none_nodb`, there should be assertions.
		nahimilegaAuthorUnsubmitted Done Inline Actions I now realize these are not needed for simple lister, as the test the indexing properties of the lister nahimilega: I now realize these are not needed for simple lister, as the test the indexing properties of…