you should not need it for tests as it's runtime required already.

2020

What are those parameters?

You might want to use some defined at [1]

[1] https://forge.softwareheritage.org/source/swh-lister/browse/master/swh/lister/__init__.py$13-19

Please remove it.

I did not fully realize earlier.

Does that test currently load a real launchpad instance?
(i believe so and the diff description seems to agree ;)

I don't see the data test file.

requests_mock_datadir is a fixture to transform http requests into lookup within the ./data/<https_server-name>/remaining-file-named-after-remaining-http-query-parameters...

These arguments are what the launchpad instance need to be set up as described here https://help.launchpad.net/API/launchpadlib

Yes it does, what would you recommend to use to mock launchpadlib data ?

We are using pytest and this allows to define fixtures like requests_mock_datadir.

So as a first step, i'd look if there is any existing fixture around launchpadlib [1]
Note that checking how they are testing themselves should be interesting.
As that would help in understanding how to define the fixture if we need to do it ourselves.

If there is no fixture yet, you can try to define one which would mock the launchpadlib calls to return a fixed dataset of repositories to list. Just define a json file in the "tests/data/launchpadlib/dataset.json" and make the fixture returns it. Then in the tests, you can further ensure the data listed and the test dataset are consistent together.

We use pytest-mocker for that, for example [2]

[1] https://pytest.readthedocs.io/en/2.7.3/plugins_index/index.html

[2] https://forge.softwareheritage.org/source/swh-storage/browse/master/swh/storage/tests/test_retry.py$58-90

When you'll have your fixture working as you want.
Define said lister fixture in swh/lister/launchpad/tests/conftest.py (or even just before the test that uses it, i would not mind).

@pytest.fixture
def lister_launchpad(swh_listers):
    with patch('launchpadlib.launchpad.Launchpad', MagicMock()):
        lister = swh_listers['launchpad']
    ... 
    # define how the mock should behave when calling given methods...
    # what you actually defined below in the beginning of the tests.
    ...
    lister.launchpad.git_repositories.getRepositories = MagicMock(
        side_effect=[mock_lp_response(i) for i in range(3)])
    ...

    return lister

And then instead of using swh_listers in your test definition, use the fixture define here:

def test_launchpad_lister(lister_launchpad):
    ...

That way, when reading the test, we can concentrate on checking the expected behavior of the lister.
And not the setup of the fixture (for that we will read the fixture definition ;)

Please, make it readable (pretty print).

Personally, i use within emacs, M-x json-pretty-print ;)

we have a datadir fixture [1] for that, "inject" datadir in the test signature and:

response_filepath  = os.path.join(datadir, f'response{page}.json')
with open(response_filepath, encoding='utf-8') as f:
...
`

[1] https://forge.softwareheritage.org/source/swh-core/browse/master/swh/core/pytest_plugin.py$0-121

That was my first thought but swh_listers instantiates all the listers at once so in LaunchpadLister.init login to launchpad is already called when you want to retrieve swh_listers['launchpad']

That should not be an issue. Have you tried?

And If that becomes an issue nonetheless, do not hesitate to split the existing fixtures to be able to reuse some part within your own (as long as other tests are still working, it's fine ;):

Something like, in the main conftest:

@pytest.fixture
def lister_db_url(postgresql_proc):
    db_url = 'postgresql://{user}@{host}:{port}/{dbname}'.format(
        host=postgresql_proc.host,
        port=postgresql_proc.port,
        user='postgres',
        dbname='tests')
    logger.debug('lister db_url: %s', db_url)
    return db_url


@pytest.fixture
def swh_listers(request, lister_db_url, swh_scheduler):
    listers = {}

    # Prepare schema for all listers
    for lister_name in SUPPORTED_LISTERS:
        lister = get_lister(lister_name, db_url=lister_db_url)
        lister.scheduler = swh_scheduler  # inject scheduler fixture
        listers[lister_name] = lister
    initialize(create_engine(lister_db_url), drop_tables=True)

    # Add the load-archive-files expected by some listers (gnu, cran, ...)
    swh_scheduler.create_task_type({
        'type': 'load-archive-files',
        'description': 'Load archive files.',
        'backend_name': 'swh.loader.package.tasks.LoadArchive',
        'default_interval': '1 day',
    })

    return listers

In launchpadlister one:

@pytest.fixture
def launchpad_lister(request, lister_db_url, swh_scheduler):
    # all the necessary cogs for mocking launchpad...
    # ...
    with patch('launchpadlib.launchpad.Launchpad', MagicMock()):
        lister = get_lister('launchpad', db_url=lister_db_url)
        lister.scheduler = swh_scheduler  # inject scheduler fixture
        initialize(create_engine(lister_db_url), drop_tables=True)
    return lister

Indeed it was an issue because when you use swh_listers fixture it is called before any patch so swh_listers['launchpad'].launchpad is already instanciated with a real Launchpad instance.
I'm using what you wrote it works great

And actually, you are making the mock being injected to swh_listers without using it later.

That feels strange.

drop swh_listers, you don't need it any longer.

you are monkey-patching here and no longer mocking.

Why not the initial with patch stanza you used earlier, that felt more standard, well at least on how we define it here in the team ;)

Yes that's right

lister.launchpad.git_repositories.getRepositories is already a mocked object, would it be ok just to redefine its side_effect like

lister.launchpad.git_repositories.getRepositories.side_effect =[mock_lp_response(i) for i in range(3)]

I don't know yet, i'd like to see the current build's status first ;)
So i'm focusing on that first, see my main comment ;)