Fix error with --import-mode=importlib and modules containing dataclasses or pickle #7870

tadeu · 2020-10-07T10:51:21Z

src/_pytest/pathlib.py

nicoddemus · 2020-10-07T11:31:58Z

Thanks a lot @tadeu!

I'm not 100% sure the fix is correct (or even if it is fixable), but let see what the other maintainers think.

I might have the wrong idea about what benefits importlib should bring to the table: I always figured it was about not changing sys.path and sys.modules, but I might be wrong about the latter.

nicoddemus · 2020-10-08T13:06:38Z

(Marking this with the hacktoberfest-accepted label as this is clearly a quality contribution, but I'm afraid it is complex and might still require a bunch of discussions to move forward)

Summary: Pull Request resolved: #477 As titled. Currently pytest's `import-mode=importlib` option does not add the test file to `sys.module`, which can break some of the use cases. While [the community has came up with a fix](pytest-dev/pytest#7870), it's not yet clear when the particular PR will be merged and added to the latest pytest. So in the meanwhile, we could download and use the patched version by directly from the PR so we are not blocked. Reviewed By: ericlippert Differential Revision: D25480894 fbshipit-source-id: 5eddce784a247f68ebcaa5ae85065220f65d847a

nicoddemus · 2020-12-19T14:03:57Z

Picked this up again, I've rebased and changed the approach to use a dotted name for modules, anchored to config.rootpath to obtain a unique name.

bluetech

I haven't reviewed in depth but LGTM.

One thing I wonder is, can the empty modules added by insert_missing_modules cause any trouble? If a module name already exists, it doesn't override it, which is good, but I wonder about the opposite case, when insert_modules inserts an empty module, and after that a real module with that name wants to be imported. Is this case possible? And if so, what will happen?

nicoddemus · 2020-12-20T11:14:50Z

If a module name already exists, it doesn't override it, which is good, but I wonder about the opposite case, when insert_modules inserts an empty module, and after that a real module with that name wants to be imported. Is this case possible? And if so, what will happen?

Hmm good points, I think it will blow up, as someone might be expecting the real module (to say, call a function from it) and they will get a fake module instead.

However I think this is highly unlikely, because if one is using importlib mode, it is a premise that they won't be able to import things from other test modules, so I'm having a hard time coming up with a plausible example where this might be an issue.

bluetech · 2020-12-20T16:04:15Z

Got it. If it turns out to be a problem in practice, then worst case we can revert or fix it some other way.

…ickle Fixes pytest-dev#7856, fixes pytest-dev#7859

nicoddemus · 2021-04-02T20:46:26Z

Rebased on latest main, needs to be squashed on merge.

Sorry for dropping the ball on this one, this totally got out of my radar.

I will merge it tomorrow unless someone wants more time to give it another pass, in which case just let me know here.

…rt mode going forward isn't certain so avoiding relative imports with pytest seems more extensible pytest docs on this:https://docs.pytest.org/en/stable/goodpractices.html#tests-outside-application-code. Pytest dev branch discussion around this: pytest-dev/pytest#7245 AND pytest-dev/pytest#7870

pytest-dev/pytest#7870

The initial implementation (in pytest-dev#7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. That proved problematic, so we started adding the imported module to `sys.modules` in pytest-dev#7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then pytest-dev#10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix pytest-dev#10811, pytest-dev#10341

The initial implementation (in pytest-dev#7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. Not adding modules to `sys.modules` proved problematic, so we began to add the imported module to `sys.modules` in pytest-dev#7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then pytest-dev#10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix pytest-dev#10811, pytest-dev#10341

The initial implementation (in #7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. Not adding modules to `sys.modules` proved problematic, so we began to add the imported module to `sys.modules` in #7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then #10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix #10811 Fix #10341

tadeu force-pushed the fix-importlib-error branch from abcc1cf to c08f4d1 Compare October 7, 2020 11:06

tadeu changed the title ~~Fix error with --import-mode=importlib and modules containing dataclasses~~ Fix error with --import-mode=importlib and modules containing dataclasses or pickle Oct 7, 2020

tadeu force-pushed the fix-importlib-error branch 2 times, most recently from ac2cc44 to 6dad144 Compare October 7, 2020 11:18

tadeu requested a review from nicoddemus October 7, 2020 11:19

nicoddemus reviewed Oct 7, 2020

View reviewed changes

src/_pytest/pathlib.py Show resolved Hide resolved

nicoddemus added the hacktoberfest-accepted label Oct 8, 2020

maurosilber mentioned this pull request Oct 11, 2020

Feedback about --import-mode=importlib in pytest 6.0 #7245

Closed

davidhewitt mentioned this pull request Oct 25, 2020

[BLOCKED] --import-mode=importlib: use sys.path relative path if present (wait for #7870) #7936

Closed

5 tasks

nicoddemus self-assigned this Dec 9, 2020

horizon-blue mentioned this pull request Dec 10, 2020

Install a patched version of pytest on CircleCI facebookresearch/beanmachine#477

Closed

nicoddemus force-pushed the fix-importlib-error branch from 6dad144 to ebc2622 Compare December 19, 2020 14:02

bluetech reviewed Dec 19, 2020

View reviewed changes

bluetech approved these changes Dec 20, 2020

View reviewed changes

Base automatically changed from master to main March 9, 2021 20:40

nicoddemus added this to the 7.0 milestone Apr 1, 2021

tadeu and others added 8 commits April 2, 2021 17:16

Fix errors when using --import-mode=importlib with dataclasses or p…

1aebd9a

…ickle Fixes pytest-dev#7856, fixes pytest-dev#7859

Construct full module names when using importmode=importlib

f278286

Improve docs

4014529

Fix tests after rebase

fe95061

Fix linting

2c87a26

Improve tests

2a44829

Improve docs

eeb5889

Use getattr to avoid #type:ignore

72d9ef0

nicoddemus added 3 commits April 2, 2021 17:31

Inject sub-modules when importing with importlib

d67bb82

Fix tests

03e78ff

Fix linting

91661a0

nicoddemus force-pushed the fix-importlib-error branch from a1b3c0c to 91661a0 Compare April 2, 2021 20:40

nicoddemus changed the title ~~Fix error with --import-mode=importlib and modules containing dataclasses or pickle~~ Fix error with --import-mode=importlib and modules containing dataclasses or pickle [needs squash] Apr 2, 2021

nicoddemus changed the title ~~Fix error with --import-mode=importlib and modules containing dataclasses or pickle [needs squash]~~ [SQUASH] Fix error with --import-mode=importlib and modules containing dataclasses or pickle Apr 2, 2021

nicoddemus changed the title ~~[SQUASH] Fix error with --import-mode=importlib and modules containing dataclasses or pickle~~ Fix error with --import-mode=importlib and modules containing dataclasses or pickle Apr 5, 2021

nicoddemus merged commit b706a2c into pytest-dev:main Apr 5, 2021

saimn added a commit to saimn/pytest-doctestplus that referenced this pull request Jun 13, 2021

Update config._getconftest_pathlist for pytest-dev

9b9f271

pytest-dev/pytest#7870

saimn mentioned this pull request Jun 13, 2021

Update config._getconftest_pathlist for pytest-dev scientific-python/pytest-doctestplus#157

Merged

h-vetinari mentioned this pull request Jun 29, 2021

[pytest 7.0] Make --version write to stdout rather than stderr #8247

Merged

The-Compiler modified the milestones: 8.0, 7.0 Jun 29, 2021

horizon-blue mentioned this pull request Jun 29, 2021

Fixing CircleCI facebookresearch/beanmachine#886

Closed

eapolinario mentioned this pull request Sep 14, 2021

[flytekit] Test pytest with --import-mode=importlib flyteorg/flyte#1469

Closed

bluetech mentioned this pull request Feb 9, 2022

7.0.0 regression: Existence of conftest.py messes up package discovery with importlib + pythonpath mode (ModuleNotFoundError: No module named 'tests.<test-packages>') #9645

Closed

nicoddemus mentioned this pull request Jun 29, 2023

Fix duplicated imports with importlib mode and doctest-modules #11148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error with --import-mode=importlib and modules containing dataclasses or pickle #7870

Fix error with --import-mode=importlib and modules containing dataclasses or pickle #7870

tadeu commented Oct 7, 2020 •

edited

Loading

nicoddemus commented Oct 7, 2020 •

edited

Loading

nicoddemus commented Oct 8, 2020

nicoddemus commented Dec 19, 2020

bluetech left a comment

nicoddemus commented Dec 20, 2020

bluetech commented Dec 20, 2020

nicoddemus commented Apr 2, 2021 •

edited

Loading

Fix error with --import-mode=importlib and modules containing dataclasses or pickle #7870

Fix error with --import-mode=importlib and modules containing dataclasses or pickle #7870

Conversation

tadeu commented Oct 7, 2020 • edited Loading

nicoddemus commented Oct 7, 2020 • edited Loading

nicoddemus commented Oct 8, 2020

nicoddemus commented Dec 19, 2020

bluetech left a comment

Choose a reason for hiding this comment

nicoddemus commented Dec 20, 2020

bluetech commented Dec 20, 2020

nicoddemus commented Apr 2, 2021 • edited Loading

tadeu commented Oct 7, 2020 •

edited

Loading

nicoddemus commented Oct 7, 2020 •

edited

Loading

nicoddemus commented Apr 2, 2021 •

edited

Loading