Page MenuHomeSoftware Heritage
Differential D1733

swh.lister.core: Add tests for simple lister
ClosedPublic
Actions

Authored by nahimilega on Jul 14 2019, 8:28 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Maniphest Tasks
T1890: pypi lister: Add tests
Commits
rDLS08ade29e6de0: swh.lister.pypi: Add tests
rDLSf424f07c7e62: swh.lister.core: Add test for simple lister
Summary

There were previously no tests for the listers
which are using the class SimpleLister(like pypi)
Refactored test_lister.py of lister core to
accommodate tests for SimpleLister keeping the tests
undisturbed for other listers.

Also, Add tests for pypi lister
Closes T1890

Diff Detail

Repository
rDLS Listers
Branch
pypi
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6894
Build 9669: tox-on-jenkinsJenkins
Build 9668: arc lint + arc unit

Event Timeline

nahimilega created this revision.Jul 14 2019, 8:28 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJul 14 2019, 8:28 PM
swh-public-ci added a comment.Jul 14 2019, 8:30 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/297/ for more details.

Harbormaster completed remote builds in B6862: Diff 5822.Jul 14 2019, 8:30 PM
nahimilega added a subscriber: ardumont.Jul 14 2019, 8:32 PM
ardumont requested changes to this revision.Jul 15 2019, 10:25 AM

Same question as packagist lister, why don't we use the same mechanism as other listers?

We can keep this and add the other tests based on the same mechanism the other listers use.

This revision now requires changes to proceed.Jul 15 2019, 10:25 AM
nahimilega updated this revision to Diff 5852.Jul 16 2019, 6:02 PM
  • swh.lister.core: Add test for simple lister
  • swh.lister.pypi: Add tests
nahimilega retitled this revision from swh.lister.pypi: Add tests to swh.lister.core: Add tests for simple lister.Jul 16 2019, 6:02 PM
nahimilega edited the summary of this revision. (Show Details)
swh-public-ci added a comment.Jul 16 2019, 6:04 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/298/ for more details.

Harbormaster completed remote builds in B6888: Diff 5852.Jul 16 2019, 6:04 PM
nahimilega added inline comments.Jul 16 2019, 6:06 PM
swh/lister/core/tests/test_lister.py
250

Here we can't put this @requests_mock.Mocker() on top of the class because of Gitlab lister.

ardumont requested changes to this revision.Jul 17 2019, 10:43 AM

Thanks for working on this.

My main concern is about:

  • some tests are missing assertions, so actually testing nothing.
  • some docstrings are not clear enough (and contains typos). Please use the sphinx stanza as demonstrated.
  • Add docstrings to the new tests to clarify their intents.

That should be fixed prior to land.

Cheers,

swh/lister/core/tests/test_lister.py
27

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).

193

Same as before, please use the following stanza when specifying a class:

:class:`swh.lister.core.indexing_lister.IndexingHttpLister`
244

Same, please use :class: notation.

250

Ok.

259

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).

262–336

Same as test_fetch_none_nodb, there should be assertions.

280

Can you explicit the need to override the default get_fl implementation?

Probably initialization parameter divergence but still...

293

Same here, what does actually mock_response do?

Why the need for overriding it?

301

that actually tests nothing.

There should be assertion(s) after the actual method is called here (fl.run() is the method to be tested).

swh/lister/pypi/tests/test_lister.py
35

Same, add docstrings to explain the test's goal.

Something like:

List packages from simple api page should retrieve all packages within
This revision now requires changes to proceed.Jul 17 2019, 10:43 AM
nahimilega added inline comments.Jul 17 2019, 12:21 PM
swh/lister/core/tests/test_lister.py
83–91

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 updated this revision to Diff 5858.Jul 17 2019, 1:24 PM
nahimilega marked 6 inline comments as done.
  • Add comments on the test
  • Remove unnecessary test from HttpSimpleListerTester
swh/lister/core/tests/test_lister.py
262–336

I now realize these are not needed for simple lister, as the test the indexing properties of the lister

280

There is no parameter such as api_base_url in the simple lister.

293

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)

swh-public-ci added a comment.Jul 17 2019, 1:26 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/299/ for more details.

Harbormaster completed remote builds in B6894: Diff 5858.Jul 17 2019, 1:26 PM
nahimilega added inline comments.Jul 18 2019, 9:46 AM
swh/lister/core/tests/test_lister.py
193–212

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?

ardumont added inline comments.Jul 18 2019, 9:51 AM
swh/lister/pypi/tests/test_lister.py
56

Change the comment prior to the method into a docstring.

63

As lister.task_dict does not have side-effects (as in for example, api call, write to files, etc...), you can call directly it and check the output.

That'd simplify the test here.

# given (<- usually assertion definitions here)
expected_task = {
...
}

# when (<- function or code to actually test, only 1 instruction usual to make stuff clearer)
actual_task = lister.task_dict(...)

# then (<- as many checks as you want)
self.assertEqual(actual_task, expected_task)
nahimilega added inline comments.Jul 18 2019, 9:54 AM
swh/lister/pypi/tests/test_lister.py
56

There is a funny thing happening when I am putting doccsting to the method.
As soon as I add triple backquotes, flake8 gives an error that as mock_create_tasks:(line 58) is over intended.

ardumont added inline comments.Jul 18 2019, 9:57 AM
swh/lister/core/tests/test_lister.py
193–212

Not sure either, the thing is, they do not check anything at the end...
That's wrong.

swh/lister/pypi/tests/test_lister.py
56

Yes, that's another issue though.

If you remove the with patch as stated just below, you should not have the problem anymore.

Also, once in a while, it's ok to use # noqa at the end of the statement to make flake8 silent so, that could become:

with patch .... as mock_create_tasks:  # noqa
...

But i'd prefer you simplify the test to the suggestion below.

ardumont added inline comments.Jul 18 2019, 10:02 AM
swh/lister/core/tests/test_lister.py
193–212

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 added inline comments.Jul 18 2019, 12:14 PM
swh/lister/core/tests/test_lister.py
83–91

well, no bad response then ;)

ardumont requested changes to this revision.Jul 18 2019, 1:20 PM

I forgot to request changes.

Just a couple of fixes and that should be good to go.

This revision now requires changes to proceed.Jul 18 2019, 1:20 PM
nahimilega marked 7 inline comments as done and an inline comment as not done.Jul 18 2019, 1:38 PM
nahimilega added inline comments.
swh/lister/pypi/tests/test_lister.py
63

I don't think we can do it because the output of task_dict has a datetime object of current date.

{'policy': 'recurring', 'type': 'load-pypi', 'next_run': datetime.datetime(2019, 7, 18, 11, 33, 46, 211689, tzinfo=datetime.timezone.utc), 'arguments': {'args': ('test_pack', 'https://abc'), 'kwargs': {'project_metadata_url': 'https://def'}}}

Now two things can be done

  1. Either we remove the datetime object from the actual_task output and then compare
  2. Leave this test as it is (ie with mock_create_task and all that)

Which option shall I choose ?

nahimilega updated this revision to Diff 5890.Jul 18 2019, 1:48 PM
  • Add FIXME in ambigious tests
  • Remove bad response from simple lister test
swh-public-ci added a comment.Jul 18 2019, 1:50 PM

Build is green
See https://jenkins.softwareheritage.org/job/DLS/job/tox/300/ for more details.

Harbormaster completed remote builds in B6925: Diff 5890.Jul 18 2019, 1:51 PM
ardumont accepted this revision.Jul 18 2019, 5:58 PM

Cool.

Thanks.

Let's land this.

(And now on for the packagist lister to try and test it ;)

Cheers,

swh/lister/pypi/tests/test_lister.py
63

The python datetime thing is really annoying.
I consider it a side-effect and that's usually something that i mock ;).

But fine, let's keep the test as is for now.

This revision is now accepted and ready to land.Jul 18 2019, 5:58 PM
ardumont mentioned this in D1584: swh.lister.packagist.Jul 18 2019, 5:59 PM
Closed by commit rDLSf424f07c7e62: swh.lister.core: Add test for simple lister (authored by nahimilega). · Explain WhyJul 18 2019, 6:50 PM
This revision was automatically updated to reflect the committed changes.
nahimilega marked an inline comment as done.

Revision Contents
Changeset List

PathSize
swh/
lister/
core/
tests/
297 lines
pypi/
tests/
12 lines
63 lines

Diff 5858

View Options

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') 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')
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.disable_deleted_repo_tasks = Mock(return_value=None)
fl.run(min_bound=self.first_index)
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):
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.
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=1, max_bound=1) # stores no results fl.run(min_bound=1, max_bound=1) # stores no results
@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)
@requests_mock.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…
def test_fetch_multiple_pages_nodb(self, http_mocker): def test_fetch_multiple_pages_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) fl.run(min_bound=self.first_index)
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…
View Options

swh/lister/pypi/tests/api_response.html

  • This file was added.
<!DOCTYPE html>
<html>
<head>
<title>Simple index</title>
</head>
<body>
<a href="/simple/0lever-so/">0lever-so</a>
<a href="/simple/0lever-utils/">0lever-utils</a>
<a href="/simple/0-orchestrator/">0-orchestrator</a>
<a href="/simple/0wned/">0wned</a>
</body>
</html>
No newline at end of file
View Options

swh/lister/pypi/tests/test_lister.py

  • This file was added.
# Copyright (C) 2019 the Software Heritage developers
# License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information
import requests_mock
import unittest
from unittest.mock import patch
from swh.lister.pypi.lister import PyPILister
from swh.lister.core.tests.test_lister import HttpSimpleListerTester
lister = PyPILister()
expected_packages = ['0lever-so', '0lever-utils', '0-orchestrator', '0wned']
expected_model = {
'uid': 'arrow',
'name': 'arrow',
'full_name': 'arrow',
'html_url': 'https://pypi.org/pypi/arrow/json',
'origin_url': 'https://pypi.org/project/arrow/',
'origin_type': 'pypi',
}
class PyPIListerTester(HttpSimpleListerTester, unittest.TestCase):
Lister = PyPILister
PAGE = 'https://pypi.org/simple/'
lister_subdir = 'pypi'
good_api_response_file = 'api_response.html'
bad_api_response_file = 'api_empty_response.json'
entries = 4
@requests_mock.Mocker()
def test_list_packages(self, http_mocker):
"""List packages from simple api page should retrieve all packages within
ardumontUnsubmitted
Done
Inline Actions

Same, add docstrings to explain the test's goal.

Something like:

List packages from simple api page should retrieve all packages within
ardumont: Same, add docstrings to explain the test's goal. Something like: ``` List packages from…
"""
http_mocker.get(self.PAGE, text=self.mock_response)
fl = self.get_fl()
packages = fl.list_packages(self.get_api_response(0))
for package in expected_packages:
assert package in packages
def test_transport_response_simplified(self):
"""Test model created by the lister
"""
fl = self.get_fl()
model = fl.transport_response_simplified(['arrow'])
assert len(model) == 1
for key, values in model[0].items():
assert values == expected_model[key]
# Test the task creation of lister
def test_task_dict(self):
ardumontUnsubmitted
Not Done
Inline Actions

Change the comment prior to the method into a docstring.

ardumont: Change the comment prior to the method into a docstring.
nahimilegaAuthorUnsubmitted
Not Done
Inline Actions

There is a funny thing happening when I am putting doccsting to the method.
As soon as I add triple backquotes, flake8 gives an error that as mock_create_tasks:(line 58) is over intended.

nahimilega: There is a funny thing happening when I am putting doccsting to the method. As soon as I add…
ardumontUnsubmitted
Not Done
Inline Actions

Yes, that's another issue though.

If you remove the with patch as stated just below, you should not have the problem anymore.

Also, once in a while, it's ok to use # noqa at the end of the statement to make flake8 silent so, that could become:

with patch .... as mock_create_tasks:  # noqa
...

But i'd prefer you simplify the test to the suggestion below.

ardumont: Yes, that's another issue though. If you remove the `with patch` as stated just below, you…
with patch('swh.lister.pypi.lister.utils.create_task_dict') \
as mock_create_tasks:
lister.task_dict(origin_type='pypi', origin_url='https://abc',
name='test_pack', html_url='https://def')
mock_create_tasks.assert_called_once_with(
'load-pypi', 'recurring', 'test_pack', 'https://abc',
project_metadata_url='https://def')
ardumontUnsubmitted
Not Done
Inline Actions

As lister.task_dict does not have side-effects (as in for example, api call, write to files, etc...), you can call directly it and check the output.

That'd simplify the test here.

# given (<- usually assertion definitions here)
expected_task = {
...
}

# when (<- function or code to actually test, only 1 instruction usual to make stuff clearer)
actual_task = lister.task_dict(...)

# then (<- as many checks as you want)
self.assertEqual(actual_task, expected_task)
ardumont: As `lister.task_dict` does not have side-effects (as in for example, api call, write to files…
nahimilegaAuthorUnsubmitted
Not Done
Inline Actions

I don't think we can do it because the output of task_dict has a datetime object of current date.

{'policy': 'recurring', 'type': 'load-pypi', 'next_run': datetime.datetime(2019, 7, 18, 11, 33, 46, 211689, tzinfo=datetime.timezone.utc), 'arguments': {'args': ('test_pack', 'https://abc'), 'kwargs': {'project_metadata_url': 'https://def'}}}

Now two things can be done

  1. Either we remove the datetime object from the actual_task output and then compare
  2. Leave this test as it is (ie with mock_create_task and all that)

Which option shall I choose ?

nahimilega: I don't think we can do it because the output of `task_dict` has a datetime object of current…
ardumontUnsubmitted
Not Done
Inline Actions

The python datetime thing is really annoying.
I consider it a side-effect and that's usually something that i mock ;).

But fine, let's keep the test as is for now.

ardumont: The python datetime thing is really annoying. I consider it a side-effect and that's usually…
About · Developer information · Legal