Page MenuHomeSoftware Heritage

Add pytest fixture to allow mocking requests with data file
ClosedPublic

Authored by ardumont on Oct 8 2019, 10:44 AM.

Diff Detail

Repository
rDCORE Foundations and core functionalities
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Build has FAILED

mmmph, possibly some python3.5 and pathlib issue or some such... investigating

douardda requested changes to this revision.Oct 8 2019, 1:43 PM
douardda added a subscriber: douardda.
douardda added inline comments.
tox.ini
7–8

you can get rid of this I guess

This revision now requires changes to proceed.Oct 8 2019, 1:43 PM
olasd requested changes to this revision.Oct 8 2019, 1:57 PM
olasd added a subscriber: olasd.

While I understand what the fixture does, its documentation is lacking (you're only really documenting the inner callback, that users won't call into, not the fixture itself). I guess most of the documentation of the callback should move to the fixture itself?

For the fixture name, maybe something like requests_mock_from_datadir(_factory) / requests_mock_from_datadir_with_visits would be clearer (even though it's a mouthful)?

Finally, you should make sure that the rationale for adding a new toplevel module for the pytest plugin is really clear (the paste or the reference to pytest/2042 really aren't).

From my tests and reading documentation further (pytest, tox), the issue really is a limitation of the way we're using the tox + pytest combination: we're running the tests on the checked out version of the module, instead of the version that's installed by tox (which is what pytest expects). If we adapt tox.ini to run tests on the installed version of the module (by running pytest [...] {envsitepackagesdir}/swh/core/..., then we can ship the pytest plugin wherever we want without triggering the dreaded ImportMismatchError.

This is also how I have caught that the data files you're adding are not shipped in the sdist, and need to be added to MANIFEST.in.

In D2082#48297, @olasd wrote:

Finally, you should make sure that the rationale for adding a new toplevel module for the pytest plugin is really clear (the paste or the reference to pytest/2042 really aren't).

From my tests and reading documentation further (pytest, tox), the issue really is a limitation of the way we're using the tox + pytest combination: we're running the tests on the checked out version of the module, instead of the version that's installed by tox (which is what pytest expects). If we adapt tox.ini to run tests on the installed version of the module (by running pytest [...] {envsitepackagesdir}/swh/core/..., then we can ship the pytest plugin wherever we want without triggering the dreaded ImportMismatchError.

(btw, I'm not saying that this diff should change that; just that we should think about cleaning up our tox config at some point)

For the fixture name, maybe something like requests_mock_from_datadir(_factory) / requests_mock_from_datadir_with_visits would be clearer (even though it's a mouthful)?

I'm kinda sold on requests_mock_datadir and requests_mock_datadir_visits, thx.

Rename local_get into requests_mock_datadir

And dependent factory or other fixtures with the same pattern name

Try to improve the fixture factory's docstring

Add missing test artifacts in MANIFEST.in

In D2082#48297, @olasd wrote:

From my tests and reading documentation further (pytest, tox), the issue really is a limitation of the way we're using the tox + pytest combination: we're running the tests on the checked out version of the module, instead of the version that's installed by tox (which is what pytest expects). If we adapt tox.ini to run tests on the installed version of the module (by running pytest [...] {envsitepackagesdir}/swh/core/..., then we can ship the pytest plugin wherever we want without triggering the dreaded ImportMismatchError.

well this is the default behavior of tox: execute tests from local working directory against installed python package. However if this default makes sense for project which tests are not part of the package itself, we may indeed consider executing tests that are installed with the package. It would probably be a sensible choice.

I am inclined to give this a try before landing this diff. Thanks for pointing this out.

In D2082#48297, @olasd wrote:

From my tests and reading documentation further (pytest, tox), the issue really is a limitation of the way we're using the tox + pytest combination: we're running the tests on the checked out version of the module, instead of the version that's installed by tox (which is what pytest expects). If we adapt tox.ini to run tests on the installed version of the module (by running pytest [...] {envsitepackagesdir}/swh/core/..., then we can ship the pytest plugin wherever we want without triggering the dreaded ImportMismatchError.

well this is the default behavior of tox: execute tests from local working directory against installed python package. However if this default makes sense for project which tests are not part of the package itself, we may indeed consider executing tests that are installed with the package. It would probably be a sensible choice.

I am inclined to give this a try before landing this diff. Thanks for pointing this out.

Gave a quick test and it works fine (with the pytest plugin moved back within swh.core package).
The only drawback I've encountered is this prevent from specifying the test file to run via tox from cmd line (not easily at least), eg.

tox -e py3-core -- swh/core/tests/test_cli.py

won't work any more.

We may live with this limitation, since the test selection mechanism of pytest (using the -k option for example) still works:

tox -e py3-core  -- -k test_cli
  • tox.ini: Use tests installed files instead of working directory
  • Move pytest plugin within swh.core
This revision was not accepted when it landed; it landed in state Needs Review.Oct 9 2019, 10:44 AM
This revision was automatically updated to reflect the committed changes.