Changeset View
Standalone View
swh/lister/maven/tests/test_lister.py
- This file was added.
# Copyright (C) 2021 The Software Heritage developers | ||||||||||||||||||||||||||||
# See the AUTHORS file at the top-level directory of this distribution | ||||||||||||||||||||||||||||
# License: GNU General Public License version 3, or any later version | ||||||||||||||||||||||||||||
# See top-level LICENSE file for more information | ||||||||||||||||||||||||||||
from pathlib import Path | ||||||||||||||||||||||||||||
from typing import List | ||||||||||||||||||||||||||||
import pytest | ||||||||||||||||||||||||||||
import requests | ||||||||||||||||||||||||||||
from swh.lister.maven.lister import MavenLister | ||||||||||||||||||||||||||||
from swh.scheduler.model import ListedOrigin | ||||||||||||||||||||||||||||
MVN_URL = "https://repo1.maven.org/maven2/" # main maven repo url | ||||||||||||||||||||||||||||
INDEX_URL = "https://indexes/export.fld" # index directory url | ||||||||||||||||||||||||||||
URL_POM_1 = MVN_URL + "al/aldi/sprova4j/0.1.0/sprova4j-0.1.0.pom" | ||||||||||||||||||||||||||||
URL_POM_2 = MVN_URL + "al/aldi/sprova4j/0.1.1/sprova4j-0.1.1.pom" | ||||||||||||||||||||||||||||
LIST_SRC = ( | ||||||||||||||||||||||||||||
MVN_URL + "al/aldi/sprova4j/0.1.0/sprova4j-0.1.0-sources.jar", | ||||||||||||||||||||||||||||
MVN_URL + "al/aldi/sprova4j/0.1.1/sprova4j-0.1.1-sources.jar", | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
LIST_SRC_DATA = ( | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
"type": "jar", | ||||||||||||||||||||||||||||
"url": "https://repo1.maven.org/maven2/al/aldi/sprova4j" | ||||||||||||||||||||||||||||
+ "/0.1.0/sprova4j-0.1.0-sources.jar", | ||||||||||||||||||||||||||||
"time": 1626109619335, | ||||||||||||||||||||||||||||
"gid": "al.aldi", | ||||||||||||||||||||||||||||
"aid": "sprova4j", | ||||||||||||||||||||||||||||
"version": "0.1.0", | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
"type": "jar", | ||||||||||||||||||||||||||||
"url": "https://repo1.maven.org/maven2/al/aldi/sprova4j" | ||||||||||||||||||||||||||||
+ "/0.1.1/sprova4j-0.1.1-sources.jar", | ||||||||||||||||||||||||||||
"time": 1626111425534, | ||||||||||||||||||||||||||||
"gid": "al.aldi", | ||||||||||||||||||||||||||||
"aid": "sprova4j", | ||||||||||||||||||||||||||||
"version": "0.1.1", | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
LIST_GIT = ( | ||||||||||||||||||||||||||||
"git://github.com/aldialimucaj/sprova4j.git", | ||||||||||||||||||||||||||||
"https://github.com/aldialimucaj/sprova4j.git", | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
@pytest.fixture | ||||||||||||||||||||||||||||
def maven_index(datadir) -> str: | ||||||||||||||||||||||||||||
text = Path(datadir, "https_indexes", "export.fld").read_text() | ||||||||||||||||||||||||||||
return text | ||||||||||||||||||||||||||||
@pytest.fixture | ||||||||||||||||||||||||||||
def maven_pom_1(datadir) -> str: | ||||||||||||||||||||||||||||
text = Path(datadir, "https_maven.org", "sprova4j-0.1.0.pom").read_text() | ||||||||||||||||||||||||||||
return text | ||||||||||||||||||||||||||||
@pytest.fixture | ||||||||||||||||||||||||||||
def maven_pom_2(datadir) -> str: | ||||||||||||||||||||||||||||
text = Path(datadir, "https_maven.org", "sprova4j-0.1.1.pom").read_text() | ||||||||||||||||||||||||||||
return text | ||||||||||||||||||||||||||||
douardda: Is it really necessary for these functions to be fixtures? | ||||||||||||||||||||||||||||
borisbaldassariAuthorUnsubmitted Done Inline ActionsI'm quite new to python fixtures, but as I understand it I'd say yes. The script is expected to http-get these files, so fixtures are the right way to mock it, right? That's what I've seen on other listers too. Is there a better way to do it? borisbaldassari: I'm quite new to python fixtures, but as I understand it I'd say yes. The script is expected to… | ||||||||||||||||||||||||||||
def check_listed_origins(lister_urls: List[str], scheduler_origins: List[ListedOrigin]): | ||||||||||||||||||||||||||||
"""Asserts that the two collections have the same origin URLs. | ||||||||||||||||||||||||||||
douarddaUnsubmitted Done Inline ActionsThe "two collections" is not clear to me. douardda: The "two collections" is not clear to me. | ||||||||||||||||||||||||||||
Does not test last_update.""" | ||||||||||||||||||||||||||||
sorted_lister_urls = list(sorted(lister_urls)) | ||||||||||||||||||||||||||||
sorted_scheduler_origins = list(sorted(scheduler_origins, key=lambda x: x.url)) | ||||||||||||||||||||||||||||
assert len(sorted_lister_urls) == len(sorted_scheduler_origins) | ||||||||||||||||||||||||||||
for l_url, s_origin in zip(sorted_lister_urls, sorted_scheduler_origins): | ||||||||||||||||||||||||||||
assert l_url == s_origin.url | ||||||||||||||||||||||||||||
douarddaUnsubmitted Done Inline Actionswhy not just assert sorted_lister_urls == sorted_scheduler_origins ? Also the list in list(sorted(x)) is not necessary I believe. douardda: why not just `assert sorted_lister_urls == sorted_scheduler_origins` ?
Assuming you build… | ||||||||||||||||||||||||||||
borisbaldassariAuthorUnsubmitted Done Inline ActionsWell spotted, thanks. I removed the function and did an assert == on sorted lists. And renamed the variable, you're once again definitely right. Thanks! borisbaldassari: Well spotted, thanks. I removed the function and did an assert == on sorted lists. And renamed… | ||||||||||||||||||||||||||||
def test_maven_full_listing( | ||||||||||||||||||||||||||||
swh_scheduler, requests_mock, mocker, maven_index, maven_pom_1, maven_pom_2, | ||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||
"""Covers full listing of multiple pages, checking page results and listed | ||||||||||||||||||||||||||||
origins, statelessness.""" | ||||||||||||||||||||||||||||
Done Inline Actionsthis comment doesn't seem accurate vlorentz: this comment doesn't seem accurate | ||||||||||||||||||||||||||||
Done Inline ActionsGood point. Removed rate-limit and page size. rate-limit is tested below, and page size doesn't make sense (only one yield per page). borisbaldassari: Good point. Removed rate-limit and page size. rate-limit is tested below, and page size doesn't… | ||||||||||||||||||||||||||||
lister = MavenLister( | ||||||||||||||||||||||||||||
scheduler=swh_scheduler, url=MVN_URL, instance="maven.org", index_url=INDEX_URL, | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
index_text = maven_index | ||||||||||||||||||||||||||||
p1_text = maven_pom_1 | ||||||||||||||||||||||||||||
p2_text = maven_pom_2 | ||||||||||||||||||||||||||||
douarddaUnsubmitted Done Inline ActionsWhy these local variables? They look unnecessary to me. douardda: Why these local variables? They look unnecessary to me. | ||||||||||||||||||||||||||||
borisbaldassariAuthorUnsubmitted Done Inline ActionsAgreed. Were there once for readability, not relevant anymore. Thanks! borisbaldassari: Agreed. Were there once for readability, not relevant anymore. Thanks! | ||||||||||||||||||||||||||||
requests_mock.get(INDEX_URL, text=index_text) | ||||||||||||||||||||||||||||
requests_mock.get(URL_POM_1, text=p1_text) | ||||||||||||||||||||||||||||
requests_mock.get(URL_POM_2, text=p2_text) | ||||||||||||||||||||||||||||
# end test setup | ||||||||||||||||||||||||||||
stats = lister.run() | ||||||||||||||||||||||||||||
# start test checks | ||||||||||||||||||||||||||||
assert stats.pages == 4 | ||||||||||||||||||||||||||||
assert stats.origins == 4 | ||||||||||||||||||||||||||||
scheduler_origins = swh_scheduler.get_listed_origins(lister.lister_obj.id).results | ||||||||||||||||||||||||||||
check_listed_origins(LIST_GIT + LIST_SRC, scheduler_origins) | ||||||||||||||||||||||||||||
for s in scheduler_origins: | ||||||||||||||||||||||||||||
if s.visit_type == "jar": | ||||||||||||||||||||||||||||
for list in LIST_SRC_DATA: | ||||||||||||||||||||||||||||
douarddaUnsubmitted Done Inline ActionsIt's generally not a good idea to shadow a builtin function/type (list here). Better choose another variable name. Especially since it looks to be a dictionary... douardda: It's generally not a good idea to shadow a builtin function/type (`list` here). Better choose… | ||||||||||||||||||||||||||||
borisbaldassariAuthorUnsubmitted Done Inline ActionsVery good point. Changed to src. Thanks! borisbaldassari: Very good point. Changed to src. Thanks! | ||||||||||||||||||||||||||||
if list.get("url") == s.url: | ||||||||||||||||||||||||||||
assert ( | ||||||||||||||||||||||||||||
list.get("time") | ||||||||||||||||||||||||||||
== s.extra_loader_arguments["artifacts"][0]["time"] | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
assert ( | ||||||||||||||||||||||||||||
list.get("gid") | ||||||||||||||||||||||||||||
== s.extra_loader_arguments["artifacts"][0]["gid"] | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
assert ( | ||||||||||||||||||||||||||||
list.get("aid") | ||||||||||||||||||||||||||||
== s.extra_loader_arguments["artifacts"][0]["aid"] | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
assert ( | ||||||||||||||||||||||||||||
list.get("version") | ||||||||||||||||||||||||||||
== s.extra_loader_arguments["artifacts"][0]["version"] | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
douarddaUnsubmitted Done Inline Actions
douardda: | ||||||||||||||||||||||||||||
break | ||||||||||||||||||||||||||||
Done Inline ActionsWouldn't it be easier to test this in check_listed_origins, as it zips the two lists? vlorentz: Wouldn't it be easier to test this in check_listed_origins, as it zips the two lists? | ||||||||||||||||||||||||||||
Done Inline ActionsWell, that depends on the previous comment. As it is right now, only origins of type jar should have the metadata. Postponing the answer until the discussion on metadata is settled. borisbaldassari: Well, that depends on the previous comment. As it is right now, only origins of type jar should… | ||||||||||||||||||||||||||||
Done Inline ActionsRefactored. borisbaldassari: Refactored. | ||||||||||||||||||||||||||||
else: | ||||||||||||||||||||||||||||
raise AssertionError | ||||||||||||||||||||||||||||
assert lister.get_state_from_scheduler() is None | ||||||||||||||||||||||||||||
Done Inline Actions
btw, for loops in Python can take an else clause like this vlorentz: btw, `for` loops in Python can take an `else` clause like this | ||||||||||||||||||||||||||||
Done Inline ActionsWouah! I'm new to python, but start really liking it.. Thanks! ;-) borisbaldassari: Wouah! I'm new to python, but start really liking it.. Thanks! ;-) | ||||||||||||||||||||||||||||
@pytest.mark.parametrize("http_code", [400, 500, 502]) | ||||||||||||||||||||||||||||
def test_maven_list_http_error( | ||||||||||||||||||||||||||||
swh_scheduler, requests_mock, mocker, maven_index, http_code | ||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||
"""Test handling of some HTTP errors commonly encountered""" | ||||||||||||||||||||||||||||
lister = MavenLister(scheduler=swh_scheduler, url=MVN_URL, index_url=INDEX_URL) | ||||||||||||||||||||||||||||
requests_mock.get(INDEX_URL, text=maven_index) | ||||||||||||||||||||||||||||
requests_mock.get(URL_POM_1, status_code=http_code) | ||||||||||||||||||||||||||||
with pytest.raises(requests.HTTPError): | ||||||||||||||||||||||||||||
douarddaUnsubmitted Done Inline Actionsplease explain which HTTP error this is actually testing. douardda: please explain which HTTP error this is actually testing. | ||||||||||||||||||||||||||||
borisbaldassariAuthorUnsubmitted Done Inline ActionsAdded documentation and more tests:
borisbaldassari: Added documentation and more tests:
* http error on maven_index not found,
* added error 404… | ||||||||||||||||||||||||||||
lister.run() | ||||||||||||||||||||||||||||
scheduler_origins = swh_scheduler.get_listed_origins(lister.lister_obj.id).results | ||||||||||||||||||||||||||||
assert len(scheduler_origins) == 2 | ||||||||||||||||||||||||||||
douarddaUnsubmitted Done Inline ActionsI don't get why we have 2 listed origins if there have been HTTP error in the process. If it's expected, it needs a bit of explanation. douardda: I don't get why we have 2 listed origins if there have been HTTP error in the process. If it's… | ||||||||||||||||||||||||||||
borisbaldassariAuthorUnsubmitted Done Inline ActionsRight. Yes, it's expected. Added this comment:
borisbaldassari: Right. Yes, it's expected. Added this comment:
# If the maven_index step succeeded but not the… |
Is it really necessary for these functions to be fixtures?