Page MenuHomeSoftware Heritage

Add pytest fixture to allow mocking requests with data file
ClosedPublic

Authored by ardumont on Tue, Oct 8, 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

ardumont created this revision.Tue, Oct 8, 10:44 AM

Build has FAILED

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

ardumont updated this revision to Diff 6992.Tue, Oct 8, 11:06 AM

Try and fix for python3.5

ardumont edited the summary of this revision. (Show Details)Tue, Oct 8, 11:51 AM
douardda requested changes to this revision.Tue, Oct 8, 1:43 PM
douardda added a subscriber: douardda.
douardda added inline comments.
tox.ini
8–9

you can get rid of this I guess

This revision now requires changes to proceed.Tue, Oct 8, 1:43 PM
olasd requested changes to this revision.Tue, Oct 8, 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.

olasd added a comment.Tue, Oct 8, 2:04 PM
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)

ardumont added inline comments.Tue, Oct 8, 2:27 PM
tox.ini
8–9

yup

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.

ardumont updated this revision to Diff 7024.Tue, Oct 8, 4:53 PM

Rename local_get into requests_mock_datadir

And dependent factory or other fixtures with the same pattern name

ardumont updated this revision to Diff 7030.Tue, Oct 8, 5:21 PM

Try to improve the fixture factory's docstring

ardumont updated this revision to Diff 7031.Tue, Oct 8, 5:51 PM

Add missing test artifacts in MANIFEST.in

olasd accepted this revision.Tue, Oct 8, 7:54 PM
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.

douardda added a comment.EditedWed, Oct 9, 10:13 AM
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
ardumont updated this revision to Diff 7032.Wed, Oct 9, 10:33 AM
  • tox.ini: Use tests installed files instead of working directory
  • Move pytest plugin within swh.core
ardumont edited the summary of this revision. (Show Details)Wed, Oct 9, 10:33 AM
ardumont updated this revision to Diff 7033.Wed, Oct 9, 10:39 AM

Rephrase docstring

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Oct 9, 10:44 AM
This revision was automatically updated to reflect the committed changes.