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.

Sign up for GitHub

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

Jump to bottom

fix: defer snapshot default extension import #734

Merged
merged 11 commits into from
Apr 25, 2023
Merged

fix: defer snapshot default extension import #734

merged 11 commits into from
Apr 25, 2023

Conversation

john-kurkowski
Copy link
Contributor

@john-kurkowski john-kurkowski commented Apr 6, 2023 •
edited

Description

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.

Also fix shadowing AttributeError during import.

Related Issues

  • Closes --snapshot-default-extension doesn't support pytest 7 pythonpath #719
    • Relevant quote:

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

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

      I'm looking into the latter.

Checklist

  • This PR has sufficient documentation.
  • This PR has sufficient test coverage.
  • This PR title satisfies semantic convention.

Additional Comments

No additional comments.

@john-kurkowski john-kurkowski marked this pull request as draft April 7, 2023 00:20
john-kurkowski
john-kurkowski commented Apr 7, 2023
View reviewed changes
tests/integration/test_snapshot_option_extension_pythonpath.py Outdated
"-v",
"--snapshot-update",
"--snapshot-default-extension",
"extension_file.MySingleFileExtensions",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is a typo. The snapshot is successfully generated if I replace this with extension_file.MySingleFileExtension, even though I haven't customized pythonpath in this test case. This could mean project has been honoring pytest's import paths all along. I think this PR still fixes a bug, but I'm not sure the description is right. I converted this PR to draft. Let me investigate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured it out. A few lines above this comment from #667, there's an in-memory cache. I didn't quite grok why the cache is there, but assuming it is necessary, the test module in this PR now clears the cache between test cases. I fixed the typo on this line. Tests pass as expected, locally.

@john-kurkowski john-kurkowski marked this pull request as ready for review April 7, 2023 22:48
@noahnu
Copy link
Collaborator

noahnu commented Apr 14, 2023

Tests are failing on windows

@john-kurkowski
Copy link
Contributor Author

I see. This'll be fun to track down without Windows. 😄 I'll noodle on it.

FAILED tests/integration/test_snapshot_option_extension_pythonpath.py::test_snapshot_default_extension_option_success - Failed:  nomatch: '1 snapshot generated\\.'
…
FAILED tests/integration/test_snapshot_option_extension_pythonpath.py::test_snapshot_default_extension_option_failure - Failed:  nomatch: ".*: Member 'DoesNotExistExtension' not found.*"
…
================== 2 failed, 243 passed, 4 xpassed in 30.35s ==================

Noah Negin-Ulster and others added 11 commits April 25, 2023 08:56
@noahnu noahnu merged commit dfd5910 into tophat:main Apr 25, 2023
11 of 12 checks passed
@noahnu
Copy link
Collaborator

noahnu commented Apr 25, 2023

@all-contributors add @john-kurkowski for bug

@allcontributors allcontributors bot mentioned this pull request Apr 25, 2023
Merged
@allcontributors
Copy link
Contributor

@noahnu

I've put up a pull request to add @john-kurkowski! 🎉

tophat-opensource-bot pushed a commit that referenced this pull request Apr 25, 2023
@semantic-release-bot
chore(release): 4.0.2 [skip ci]
928dcad 
## [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
Collaborator

🎉 This PR is included in version 4.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@john-kurkowski john-kurkowski deleted the bugfix/719 branch April 26, 2023 22:39
@john-kurkowski
Copy link
Contributor Author

Thanks for accepting the change and for the assist!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noahnu noahnu noahnu left review comments

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

--snapshot-default-extension doesn't support pytest 7 pythonpath
3 participants
@john-kurkowski @noahnu @tophat-opensource-bot