Page MenuHomeSoftware Heritage

swh.lister.core: Add tests for simple lister

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



There were previously no tests for the listers
which are using the class SimpleLister(like pypi)
Refactored 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

rDLS Listers
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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

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.




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


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


Same, please use :class: notation.




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


Same as test_fetch_none_nodb, there should be assertions.


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

Probably initialization parameter divergence but still...


Same here, what does actually mock_response do?

Why the need for overriding it?


that actually tests nothing.

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

35 ↗(On Diff #5852)

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

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 marked 6 inline comments as done.
  • Add comments on the test
  • Remove unnecessary test from HttpSimpleListerTester

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


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


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)


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?

56 ↗(On Diff #5858)

Change the comment prior to the method into a docstring.

63 ↗(On Diff #5858)

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)
56 ↗(On Diff #5858)

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.


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

56 ↗(On Diff #5858)

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.


Please add a fixme after 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.


well, no bad response then ;)

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.
63 ↗(On Diff #5858)

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 ?

  • Add FIXME in ambigious tests
  • Remove bad response from simple lister test



Let's land this.

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


63 ↗(On Diff #5858)

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
This revision was automatically updated to reflect the committed changes.
nahimilega marked an inline comment as done.