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://maven.org/" | ||||||||||||||||||||||||||||
MVN_INDEX_URL = MVN_URL + ".index/" # index directory url | ||||||||||||||||||||||||||||
MVN_PROPERTIES_URL = ( | ||||||||||||||||||||||||||||
MVN_INDEX_URL + "nexus-maven-repository-index.properties" | ||||||||||||||||||||||||||||
) # index properties file | ||||||||||||||||||||||||||||
MVN_INDEX_MAIN_URL = ( | ||||||||||||||||||||||||||||
MVN_INDEX_URL + "nexus-maven-repository-index.gz" | ||||||||||||||||||||||||||||
) # main index file | ||||||||||||||||||||||||||||
MVN_INDEX_INCR_URL = ( | ||||||||||||||||||||||||||||
MVN_INDEX_URL + "nexus-maven-repository-index.1.gz" | ||||||||||||||||||||||||||||
) # incremental index file | ||||||||||||||||||||||||||||
LIST_SRC = ( | ||||||||||||||||||||||||||||
"https://repo1.maven.org/maven2/al/aldi/sprova4j/0.1.0/sprova4j-0.1.0-sources.jar", | ||||||||||||||||||||||||||||
"https://repo1.maven.org/maven2/al/aldi/sprova4j/0.1.1/sprova4j-0.1.1-sources.jar", | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
@pytest.fixture | ||||||||||||||||||||||||||||
def maven_properties(datadir) -> str: | ||||||||||||||||||||||||||||
text = Path( | ||||||||||||||||||||||||||||
datadir, "https_maven.org", ".index", "nexus-maven-repository-index.properties" | ||||||||||||||||||||||||||||
).read_text() | ||||||||||||||||||||||||||||
return text | ||||||||||||||||||||||||||||
@pytest.fixture | ||||||||||||||||||||||||||||
def maven_index_main(datadir): | ||||||||||||||||||||||||||||
content = Path( | ||||||||||||||||||||||||||||
datadir, "https_maven.org", ".index", "nexus-maven-repository-index.gz" | ||||||||||||||||||||||||||||
).read_bytes() | ||||||||||||||||||||||||||||
return content | ||||||||||||||||||||||||||||
@pytest.fixture | ||||||||||||||||||||||||||||
def maven_index_incr(datadir): | ||||||||||||||||||||||||||||
content = Path( | ||||||||||||||||||||||||||||
datadir, "https_maven.org", ".index", "nexus-maven-repository-index.1.gz" | ||||||||||||||||||||||||||||
).read_bytes() | ||||||||||||||||||||||||||||
return content | ||||||||||||||||||||||||||||
def check_listed_origins(lister_urls: List[str], scheduler_origins: List[ListedOrigin]): | ||||||||||||||||||||||||||||
"""Asserts that the two collections have the same origin URLs. | ||||||||||||||||||||||||||||
Does not test last_update.""" | ||||||||||||||||||||||||||||
sorted_lister_urls = list(sorted(lister_urls)) | ||||||||||||||||||||||||||||
sorted_scheduler_origins = list(sorted(scheduler_origins)) | ||||||||||||||||||||||||||||
assert len(sorted_lister_urls) == len(sorted_scheduler_origins) | ||||||||||||||||||||||||||||
douardda: Is it really necessary for these functions to be fixtures? | ||||||||||||||||||||||||||||
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… | ||||||||||||||||||||||||||||
for l_url, s_origin in zip(sorted_lister_urls, sorted_scheduler_origins): | ||||||||||||||||||||||||||||
assert l_url == s_origin.url | ||||||||||||||||||||||||||||
Done Inline ActionsThe "two collections" is not clear to me. douardda: The "two collections" is not clear to me. | ||||||||||||||||||||||||||||
def test_maven_full_listing( | ||||||||||||||||||||||||||||
swh_scheduler, | ||||||||||||||||||||||||||||
requests_mock, | ||||||||||||||||||||||||||||
mocker, | ||||||||||||||||||||||||||||
maven_properties, | ||||||||||||||||||||||||||||
maven_index_main, | ||||||||||||||||||||||||||||
maven_index_incr, | ||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||
"""Covers full listing of multiple pages, rate-limit, page size (required for test), | ||||||||||||||||||||||||||||
checking page results and listed origins, statelessness.""" | ||||||||||||||||||||||||||||
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… | ||||||||||||||||||||||||||||
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… | ||||||||||||||||||||||||||||
lister = MavenLister(scheduler=swh_scheduler, url=MVN_URL, instance="maven.org") | ||||||||||||||||||||||||||||
prop = maven_properties | ||||||||||||||||||||||||||||
ind_main = maven_index_main | ||||||||||||||||||||||||||||
ind_incr = maven_index_incr | ||||||||||||||||||||||||||||
# r1_text, r1_headers, r1_result, r1_origin_urls = tuleap_repo_1 | ||||||||||||||||||||||||||||
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… | ||||||||||||||||||||||||||||
# r2_text, r2_headers, r2_result, r2_origin_urls = tuleap_repo_2 | ||||||||||||||||||||||||||||
# r3_text, r3_headers, r3_result, r3_origin_urls = tuleap_repo_3 | ||||||||||||||||||||||||||||
requests_mock.get(MVN_PROPERTIES_URL, text=prop) | ||||||||||||||||||||||||||||
requests_mock.get(MVN_INDEX_MAIN_URL, content=ind_main) | ||||||||||||||||||||||||||||
requests_mock.get(MVN_INDEX_INCR_URL, content=ind_incr) | ||||||||||||||||||||||||||||
# end test setup | ||||||||||||||||||||||||||||
Done Inline ActionsWhy these local variables? They look unnecessary to me. douardda: Why these local variables? They look unnecessary to me. | ||||||||||||||||||||||||||||
Done Inline ActionsAgreed. Were there once for readability, not relevant anymore. Thanks! borisbaldassari: Agreed. Were there once for readability, not relevant anymore. Thanks! | ||||||||||||||||||||||||||||
stats = lister.run() | ||||||||||||||||||||||||||||
# start test checks | ||||||||||||||||||||||||||||
assert stats.pages == 2 | ||||||||||||||||||||||||||||
assert stats.origins == 2 | ||||||||||||||||||||||||||||
# scheduler_origins = swh_scheduler.get_listed_origins(lister.lister_obj.id).results | ||||||||||||||||||||||||||||
# check_listed_origins( | ||||||||||||||||||||||||||||
# r1_origin_urls + r2_origin_urls + r3_origin_urls, scheduler_origins | ||||||||||||||||||||||||||||
# ) | ||||||||||||||||||||||||||||
# check_listed_origins(GIT_REPOS, scheduler_origins) | ||||||||||||||||||||||||||||
assert lister.get_state_from_scheduler() is None | ||||||||||||||||||||||||||||
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… | ||||||||||||||||||||||||||||
Done Inline ActionsVery good point. Changed to src. Thanks! borisbaldassari: Very good point. Changed to src. Thanks! | ||||||||||||||||||||||||||||
@pytest.mark.parametrize("http_code", [400, 500, 502]) | ||||||||||||||||||||||||||||
def test_maven_list_http_error(swh_scheduler, requests_mock, http_code): | ||||||||||||||||||||||||||||
"""Test handling of some HTTP errors commonly encountered""" | ||||||||||||||||||||||||||||
lister = MavenLister(scheduler=swh_scheduler, url=MVN_URL) | ||||||||||||||||||||||||||||
requests_mock.get(MVN_PROPERTIES_URL, status_code=http_code) | ||||||||||||||||||||||||||||
with pytest.raises(requests.HTTPError): | ||||||||||||||||||||||||||||
lister.run() | ||||||||||||||||||||||||||||
scheduler_origins = swh_scheduler.get_listed_origins(lister.lister_obj.id).results | ||||||||||||||||||||||||||||
assert len(scheduler_origins) == 0 | ||||||||||||||||||||||||||||
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! ;-) | ||||||||||||||||||||||||||||
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. | ||||||||||||||||||||||||||||
Done Inline Actions
douardda: | ||||||||||||||||||||||||||||
Done Inline Actionsplease explain which HTTP error this is actually testing. douardda: please explain which HTTP error this is actually testing. | ||||||||||||||||||||||||||||
Done Inline ActionsAdded documentation and more tests:
borisbaldassari: Added documentation and more tests:
* http error on maven_index not found,
* added error 404… | ||||||||||||||||||||||||||||
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… | ||||||||||||||||||||||||||||
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?