Page MenuHomeSoftware Heritage

Added LaunchpadLister
Needs RevisionPublic

Authored by legau on Tue, Mar 10, 3:11 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Summary

First draft for (T1734)

It uses only launchpadlib and no http requests. This is because we've been discussing with the people at launchpad and they're
planning on adding a new endpoint git_repositories.getAllRepositories just for this. Using only the library it will be easier to implement
once they're done.

It is functionnal but tests are not ready yet. Looking for improvements and remarks on its design as it overrides almost everything.

Diff Detail

Repository
rDLS Listers
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11332
Build 17144: tox-on-jenkinsJenkins
Build 17143: arc lint + arc unit

Event Timeline

legau created this revision.Tue, Mar 10, 3:11 PM
legau updated this revision to Diff 10135.Wed, Mar 18, 4:28 PM

Updated with git_repositories.getRepositories

Tasks tests added, Still no lister tests

ardumont added a subscriber: ardumont.EditedTue, Mar 24, 5:58 PM

Awesome.

Thanks for your interest and work on this.

Still no lister tests

Please add at least one integration test like the gnu one [1].
You could check other lister, they all have at least one integration test like this one.

That does not delve into the intricacies of the lister contrary to some other tests.
It allows to directly check the scheduled tasks (what's been listed and outputted in the scheduler's db).

You can then check the task's shape is somewhat in the form you expect.

[1] https://forge.softwareheritage.org/source/swh-lister/browse/master/swh/lister/gnu/tests/test_lister.py$12-52

Cheers,

legau updated this revision to Diff 10254.Wed, Mar 25, 4:29 PM
  • Added Tests and annotations
ardumont added inline comments.Wed, Mar 25, 6:21 PM
requirements-test.txt
6

you should not need it for tests as it's runtime required already.

swh/lister/launchpad/__init__.py
1

2020

swh/lister/launchpad/lister.py
28

What are those parameters?

You might want to use some defined at [1]

[1] https://forge.softwareheritage.org/source/swh-lister/browse/master/swh/lister/__init__.py$13-19

swh/lister/launchpad/tasks.py
40

Please remove it.

swh/lister/launchpad/tests/test_lister.py
9

I did not fully realize earlier.

Does that test currently load a real launchpad instance?
(i believe so and the diff description seems to agree ;)

I don't see the data test file.

requests_mock_datadir is a fixture to transform http requests into lookup within the ./data/<https_server-name>/remaining-file-named-after-remaining-http-query-parameters...

legau added inline comments.Wed, Mar 25, 8:27 PM
swh/lister/launchpad/lister.py
28

These arguments are what the launchpad instance need to be set up as described here https://help.launchpad.net/API/launchpadlib

swh/lister/launchpad/tests/test_lister.py
9

Yes it does, what would you recommend to use to mock launchpadlib data ?

ardumont added inline comments.Thu, Mar 26, 9:25 AM
swh/lister/launchpad/tests/test_lister.py
9

We are using pytest and this allows to define fixtures like requests_mock_datadir.

So as a first step, i'd look if there is any existing fixture around launchpadlib [1]
Note that checking how they are testing themselves should be interesting.
As that would help in understanding how to define the fixture if we need to do it ourselves.

If there is no fixture yet, you can try to define one which would mock the launchpadlib calls to return a fixed dataset of repositories to list. Just define a json file in the "tests/data/launchpadlib/dataset.json" and make the fixture returns it. Then in the tests, you can further ensure the data listed and the test dataset are consistent together.

We use pytest-mocker for that, for example [2]

[1] https://pytest.readthedocs.io/en/2.7.3/plugins_index/index.html

[2] https://forge.softwareheritage.org/source/swh-storage/browse/master/swh/storage/tests/test_retry.py$58-90

ardumont accepted this revision.Fri, Mar 27, 10:52 AM

Requiring changes for the sake of the change on the testing part.
We don't want to depend on third party dataset for the testing.
That could fail for no good reasons.

I think i'm fine with the implementation otherwise.

This revision is now accepted and ready to land.Fri, Mar 27, 10:52 AM
ardumont requested changes to this revision.Fri, Mar 27, 10:53 AM

wrong selection sorry

This revision now requires changes to proceed.Fri, Mar 27, 10:53 AM