Page MenuHomeSoftware Heritage
Differential D2120

cran.lister: Fix cran lister and add proper integration test
ClosedPublic
Actions

Authored by ardumont on Oct 10 2019, 9:03 PM.

Details

Reviewers
vlorentz
douardda
zack
Group Reviewers
Reviewers
Commits
rDLS8d50e0d9410f: cran.lister: Fix cran lister and add proper integration test
Summary

Which checks the cran lister tasks written in the scheduler.

Related d30d574dbe9acc8722cac02986485a9dbd3dd098
Related 5ea9d5ed392a12dc5558fe165b1cbf2c0dfbcbf0

Related T2032

Test Plan

tox

Diff Detail

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

Event Timeline

ardumont created this revision.Oct 10 2019, 9:03 PM
swh-public-ci added a comment.Oct 10 2019, 9:06 PM

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

Harbormaster completed remote builds in B8307: Diff 7105.Oct 10 2019, 9:06 PM
ardumont updated this revision to Diff 7106.Oct 10 2019, 9:10 PM
  • Update slightly docstrings
  • Revert code that did not needed to change
  • Improve the MANIFEST.in instruction
ardumont added inline comments.Oct 10 2019, 9:13 PM
swh/lister/cran/lister.py
84

This was a tryout at keeping metadata within the lister.
We now know it's the loader's concern so we can drop this.

94

We do not have any 'load-cran' task-type.
I don't know if we want some so for now...

swh/lister/cran/tests/data/list-r-packages.json
39

This is a sample extracted from a sample run of the lister through the docker-dev.
And then removing a lot of the sample.

swh/lister/cran/tests/test_lister.py
13

This tested nothing...
The cran lister broke and it did not trigger any issues.

swh-public-ci added a comment.Oct 10 2019, 9:13 PM

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

Harbormaster completed remote builds in B8308: Diff 7106.Oct 10 2019, 9:13 PM
ardumont edited the summary of this revision. (Show Details)Oct 10 2019, 9:14 PM
ardumont updated this revision to Diff 7107.Oct 10 2019, 9:14 PM

Update commit message to include the task reference

swh-public-ci added a comment.Oct 10 2019, 9:17 PM

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

Harbormaster completed remote builds in B8309: Diff 7107.Oct 10 2019, 9:17 PM
ardumont added a child revision: D2121: pypi.lister: Add integration test which checks the scheduled tasks.Oct 10 2019, 9:39 PM
ardumont removed a child revision: D2121: pypi.lister: Add integration test which checks the scheduled tasks.
ardumont added a child revision: D2122: npm.lister: Add integration test which checks the scheduled tasks.Oct 10 2019, 10:16 PM
ardumont removed a child revision: D2122: npm.lister: Add integration test which checks the scheduled tasks.Oct 10 2019, 10:20 PM
ardumont added a child revision: D2121: pypi.lister: Add integration test which checks the scheduled tasks.
ardumont mentioned this in D2125: CRAN lister: decode R lister output before JSON loading.Oct 11 2019, 9:59 AM
ardumont updated this revision to Diff 7115.Oct 11 2019, 10:12 AM

Integrate fix from D2125 about decoding R lister out prior to json loading

swh-public-ci added a comment.Oct 11 2019, 10:15 AM

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

Harbormaster completed remote builds in B8317: Diff 7115.Oct 11 2019, 10:15 AM
zack added a child revision: D2123: typing: minimal changes to make a no-op mypy run pass.Oct 11 2019, 10:25 AM
zack mentioned this in D2123: typing: minimal changes to make a no-op mypy run pass.
vlorentz requested changes to this revision.Oct 11 2019, 10:27 AM
vlorentz added inline comments.
swh/lister/cran/lister.py
54–59

It's not Python code.

"""Return task format dict. This creates tasks with args and kwargs set, for
example::

    args: ['package', 'https://cran.r-project.org/...', 'version']
    kwargs: {}
"""
swh/lister/cran/tests/test_lister.py
41

What about mocking the scheduler, like the indexer CLI tests do?

This revision now requires changes to proceed.Oct 11 2019, 10:27 AM
vlorentz mentioned this in D2121: pypi.lister: Add integration test which checks the scheduled tasks.Oct 11 2019, 10:28 AM
zack requested changes to this revision.Oct 11 2019, 10:35 AM
zack added a subscriber: zack.
zack added inline comments.
swh/lister/cran/lister.py
21

First, thanks of all for starting to add type annotations, it's great! :-)

Here, I think you want Mapping rather than Dict (duck typing and all).

Also, you should specify which key/value types you expect to have, or fallback to Mapping[Any, Any] if you don't want to do that yet.

115

as above: probably s/Dict/Mapping/, and having key/value types would be nice

zack resigned from this revision.Oct 11 2019, 10:35 AM
ardumont added inline comments.Oct 11 2019, 10:37 AM
swh/lister/cran/lister.py
54–59

thx.

swh/lister/cran/tests/test_lister.py
41

no, the point is to check the lister's output inside the scheduler.
That's an integration test, i don't want mocks involved.

ardumont marked 3 inline comments as done.Oct 11 2019, 1:19 PM
ardumont added inline comments.
swh/lister/cran/lister.py
21

right, i'll check what's the type here...
Also, it's wrong, it's a list... List[Mapping[str, str]], i think.

ardumont updated this revision to Diff 7120.Oct 11 2019, 1:20 PM
ardumont marked an inline comment as done.

Adapt according to reviews

swh-public-ci added a comment.Oct 11 2019, 1:23 PM

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

Harbormaster completed remote builds in B8322: Diff 7120.Oct 11 2019, 1:23 PM
vlorentz accepted this revision.Oct 11 2019, 1:28 PM
This revision is now accepted and ready to land.Oct 11 2019, 1:28 PM
Closed by commit rDLS8d50e0d9410f: cran.lister: Fix cran lister and add proper integration test (authored by ardumont). · Explain WhyOct 11 2019, 1:56 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLS8d50e0d9410f: cran.lister: Fix cran lister and add proper integration test.

Revision Contents
Changeset List

PathSize
2 lines
swh/
lister/
core/
3 lines
tests/
9 lines
cran/
145 lines
tests/
data/
39 lines
76 lines

Diff 7125

View Options

MANIFEST.in

Loading...
View Options

swh/lister/core/simple_lister.py

Loading...
View Options

swh/lister/core/tests/conftest.py

Loading...
View Options

swh/lister/cran/lister.py

Loading...
View Options

swh/lister/cran/tests/data/list-r-packages.json

Loading...
View Options

swh/lister/cran/tests/test_lister.py

Loading...
About · Developer information · Legal