Differential D6133 Diff 22590 swh/lister/maven/tests/test_lister.py

Changeset View

Standalone View

View Options

swh/lister/maven/tests/test_lister.py

This file was added.

# 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

douarddaUnsubmitted

Not Done

Is it really necessary for these functions to be fixtures?

douardda: Is it really necessary for these functions to be fixtures?

borisbaldassariAuthorUnsubmitted

Done

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…

def check_listed_origins(lister_urls: List[str], scheduler_origins: List[ListedOrigin]):

"""Asserts that the two collections have the same origin URLs.

douarddaUnsubmitted

Done

The "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

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

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…

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."""

vlorentzUnsubmitted

Done

this comment doesn't seem accurate

vlorentz: this comment doesn't seem accurate

borisbaldassariAuthorUnsubmitted

Done

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…

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

Why these local variables? They look unnecessary to me.

douardda: Why these local variables? They look unnecessary to me.

borisbaldassariAuthorUnsubmitted

Done

Agreed. 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

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

Very 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

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:

break

vlorentzUnsubmitted

Done

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

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

Refactored.

borisbaldassari: Refactored.

else:

raise AssertionError

assert lister.get_state_from_scheduler() is None

vlorentzUnsubmitted

Done

== 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

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! ;-)

@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

please explain which HTTP error this is actually testing.

douardda: please explain which HTTP error this is actually testing.

borisbaldassariAuthorUnsubmitted

Done

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…

lister.run()

scheduler_origins = swh_scheduler.get_listed_origins(lister.lister_obj.id).results

assert len(scheduler_origins) == 2

douarddaUnsubmitted

Done

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

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

If the maven_index step succeeded but not the get_pom step,
then we get only the 2 jar origins (and not the 2 additional
src origins).

borisbaldassari: Right. Yes, it's expected. Added this comment: # If the maven_index step succeeded but not the…