Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--snapshot-default-extension doesn't support pytest 7 pythonpath #719

Closed
john-kurkowski opened this issue Mar 14, 2023 · 4 comments · Fixed by #734
Closed

--snapshot-default-extension doesn't support pytest 7 pythonpath #719

john-kurkowski opened this issue Mar 14, 2023 · 4 comments · Fixed by #734
Labels
bug Something isn't working good first issue Good for newcomers released

Comments

@john-kurkowski
Copy link
Contributor

john-kurkowski commented Mar 14, 2023

Describe the bug

--snapshot-default-extension doesn't support pytest 7's pythonpath configuration option, for pytest-only additions to the Python path.

For my project, I'm using --snapshot-default-extension so the right extension and serializer are in place, before Syrupy begins its reporting. My Syrupy extensions are for tests only, so they live outside of my src/ folder. Only the src/ folder of my project seems to be on the default Python path. So when running tests, I need to tell Syrupy about my extensions, somehow. I'd love to use the vanilla pytest command directly, configured in pyproject.toml, without having to pass a custom PYTHONPATH to pytest every time.

To reproduce

See my branch, john-kurkowski/syrupy#default-extension-pythonpath. In the final commit, https://github.com/john-kurkowski/syrupy/commit/ea9779371583253c03b0bdf47c09ca6f5526d909, switching from modifying sys.path to setting pytest's --pythonpath breaks 2/3 of the branch's test cases. EDIT: pytest's pythonpath an INI configuration option, not CLI.

diff --git a/tests/integration/test_snapshot_option_extension.py b/tests/integration/test_snapshot_option_extension.py
index de8e807..42b2eec 100644
--- a/tests/integration/test_snapshot_option_extension.py
+++ b/tests/integration/test_snapshot_option_extension.py
@@ -26,11 +26,11 @@ def testfile(testdir):
     return testdir
 
 
-def test_snapshot_default_extension_option_success(monkeypatch, testfile):
-    monkeypatch.syspath_prepend(testfile.tmpdir)
-
+def test_snapshot_default_extension_option_success(testfile):
     result = testfile.runpytest(
         "-v",
+        "--pythonpath",
+        testfile.tmpdir,
         "--snapshot-update",
         "--snapshot-default-extension",
         "extension_file.MySingleFileExtension",
@@ -63,11 +63,11 @@ def test_snapshot_default_extension_option_module_not_found(testfile):
     assert result.ret
 
 
-def test_snapshot_default_extension_option_member_not_found(monkeypatch, testfile):
-    monkeypatch.syspath_prepend(testfile.tmpdir)
-
+def test_snapshot_default_extension_option_member_not_found(testfile):
     result = testfile.runpytest(
         "-v",
+        "--pythonpath",
+        testfile.tmpdir,
         "--snapshot-update",
         "--snapshot-default-extension",
         "extension_file.DoesNotExistExtension",

Expected behavior

Tests in my branch should pass.

Environment:

  • OS: macOS
  • Syrupy Version: 4.0.1
  • Python Version: 3.11.1

Workaround

Set PYTHONPATH prior to invoking the pytest CLI.

PYTHONPATH=path/to/my/extensions/folder pytest --snapshot-default-extension some_module.SomeExtension
@noahnu noahnu added bug Something isn't working good first issue Good for newcomers labels Mar 27, 2023
noahnu pushed a commit that referenced this issue Apr 4, 2023
@noahnu
Copy link
Collaborator

noahnu commented Apr 4, 2023

Are you interested in contributing a fix? I copied your test from your branch to #731

@john-kurkowski
Copy link
Contributor Author

I am interested! My instinct was that the issue might be more architectural, and outside the scope of a first time contributor. That Syrupy's choice of hooks into pytest occur early for a different feature, but too early for pytest core's --pythonpath to take hold. Let me think about it.

@john-kurkowski
Copy link
Contributor Author

Ok, there is hope. The unexpected "does not exist" error is raised during pytest's plugin discovery's command line parsing. This suggests to me two solutions.

  1. Rearchitecting pytest to honor --pythonpath before its plugin discovery (sounds difficult to change the upstream project)
  2. Defer this project's module import until after pytest honors --pythonpath (sounds easier)

I'm looking into the latter. I'm trying out removing the code from this comment from #667. Do you remember why the eager import was necessary? Was it for DX, to fail fast with one failure, before test subprocesses try to import the module and spam the user with multiple failures?

@john-kurkowski john-kurkowski changed the title --snapshot-default-extension doesn't support pytest 7 --pythonpath --snapshot-default-extension doesn't support pytest 7 pythonpath Apr 7, 2023
noahnu pushed a commit that referenced this issue Apr 25, 2023
* test: add coverage for bug #719

* fix: defer snapshot default extension import

Fixes unable to use pytest's `pythonpath` option with this project's
`--snapshot-default-extension` option. Does cause extension import
errors to raise later than CLI argument parsing, and therefore emit on
stdout, instead of stderr.

---------

Co-authored-by: Noah Negin-Ulster <noah.negin-ulster@tophatmonocle.com>
tophat-opensource-bot pushed a commit that referenced this issue Apr 25, 2023
## [4.0.2](v4.0.1...v4.0.2) (2023-04-25)

### Bug Fixes

* defer snapshot default extension import ([#734](#734)) ([dfd5910](dfd5910)), closes [#719](#719)
@tophat-opensource-bot
Copy link
Contributor

🎉 This issue has been resolved in version 4.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants