Page MenuHomeSoftware Heritage
Differential D6133 Diff 22249 swh/lister/maven/tests/test_lister.py

Changeset View

Standalone View

View Options

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 = "http://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_GIT = (
"git://github.com/aldialimucaj/sprova4j.git",
"https://github.com/aldialimucaj/sprova4j.git",
)
@pytest.fixture
def maven_index(datadir) -> str:
text = Path(datadir, "http_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
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, 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
def test_maven_full_listing(
swh_scheduler, requests_mock, mocker, maven_index, maven_pom_1, maven_pom_2,
):
"""Covers full listing of multiple pages, rate-limit, page size (required for test),
checking page results and listed origins, statelessness."""
douarddaUnsubmitted
Not Done
Inline Actions

Is it really necessary for these functions to be fixtures?

douardda: Is it really necessary for these functions to be fixtures?
borisbaldassariAuthorUnsubmitted
Done
Inline Actions

I'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…
lister = MavenLister(
scheduler=swh_scheduler,
url=MVN_URL,
douarddaUnsubmitted
Done
Inline Actions

The "two collections" is not clear to me.

douardda: The "two collections" is not clear to me.
instance="maven.org",
index_url="http://indexes/export.fld",
)
index_text = maven_index
p1_text = maven_pom_1
p2_text = maven_pom_2
print(f"MOCK {INDEX_URL} with {index_text}")
requests_mock.get(INDEX_URL, text=index_text)
requests_mock.get(URL_POM_1, text=p1_text)
douarddaUnsubmitted
Done
Inline Actions

why not just assert sorted_lister_urls == sorted_scheduler_origins ?
Assuming you build sorted_scheduler_origins as sorted(x.url for x in scheduler_origins) I mean.

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 Actions

Well 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…
requests_mock.get(URL_POM_2, text=p2_text)
# end test setup
stats = lister.run()
# start test checks
assert stats.pages == 4
vlorentzUnsubmitted
Done
Inline Actions

this comment doesn't seem accurate

vlorentz: this comment doesn't seem accurate
borisbaldassariAuthorUnsubmitted
Done
Inline Actions

Good 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…
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)
assert lister.get_state_from_scheduler() is None
douarddaUnsubmitted
Done
Inline Actions

Why these local variables? They look unnecessary to me.

douardda: Why these local variables? They look unnecessary to me.
borisbaldassariAuthorUnsubmitted
Done
Inline Actions

Agreed. Were there once for readability, not relevant anymore. Thanks!

borisbaldassari: Agreed. Were there once for readability, not relevant anymore. 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, index_url="http://indexes/export.fld"
)
requests_mock.get(INDEX_URL, text=maven_index)
requests_mock.get(URL_POM_1, 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) == 2
vlorentzUnsubmitted
Done
Inline Actions
== s.extra_loader_arguments["artifacts"][0]["version"]
)
- found = True
- break
- assert found
+ else:
+ assert False, "not found"
assert lister.get_state_from_scheduler() is None

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
borisbaldassariAuthorUnsubmitted
Done
Inline Actions

Wouah! I'm new to python, but start really liking it.. Thanks! ;-)

borisbaldassari: Wouah! I'm new to python, but start really liking it.. Thanks! ;-)
vlorentzUnsubmitted
Done
Inline Actions

Wouldn'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?
borisbaldassariAuthorUnsubmitted
Done
Inline Actions

Well, 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…
borisbaldassariAuthorUnsubmitted
Done
Inline Actions

Refactored.

borisbaldassari: Refactored.
douarddaUnsubmitted
Done
Inline Actions
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"]
- )
+ artifact = s.extra_loader_arguments["artifacts"][0]
+ assert list.get("time") == artifact["time"]
+ assert list.get("gid") == artifact["gid"]
+ assert list.get("aid") == artifact["aid"]
+ assert list.get("version") == artifact["version"]
break
douardda:
douarddaUnsubmitted
Done
Inline Actions

It'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 Actions

Very good point. Changed to src. Thanks!

borisbaldassari: Very good point. Changed to src. Thanks!
douarddaUnsubmitted
Done
Inline Actions

please explain which HTTP error this is actually testing.

douardda: please explain which HTTP error this is actually testing.
borisbaldassariAuthorUnsubmitted
Done
Inline Actions

Added documentation and more tests:

  • http error on maven_index not found,
  • added error 404 error code.
borisbaldassari: Added documentation and more tests: * http error on maven_index not found, * added error 404…
douarddaUnsubmitted
Done
Inline Actions

I 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 Actions

Right. Yes, it's expected. Added this comment:

  1. If the maven_index step succeeded but not the get_pom step,
  2. then we get only the 2 jar origins (and not the 2 additional
  3. src origins).
borisbaldassari: Right. Yes, it's expected. Added this comment: # If the maven_index step succeeded but not the…
About · Developer information · Legal