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

Small cleanups on _pytest.compat #5451

Merged
merged 3 commits into from
Jun 16, 2019

Conversation

nicoddemus
Copy link
Member

Small improvements and cleanups

Small improvements and cleanups
@codecov
Copy link

codecov bot commented Jun 15, 2019

Codecov Report

Merging #5451 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5451      +/-   ##
==========================================
+ Coverage   95.92%   96.01%   +0.09%     
==========================================
  Files         114      114              
  Lines       25498    25483      -15     
  Branches     2479     2478       -1     
==========================================
+ Hits        24458    24468      +10     
+ Misses        728      709      -19     
+ Partials      312      306       -6
Impacted Files Coverage Δ
testing/python/metafunc.py 94.88% <ø> (ø) ⬆️
src/_pytest/python.py 93% <100%> (-0.03%) ⬇️
src/_pytest/compat.py 96.99% <100%> (-0.03%) ⬇️
src/_pytest/fixtures.py 97.52% <100%> (-0.02%) ⬇️
src/_pytest/assertion/util.py 94.86% <100%> (+4.16%) ⬆️
src/_pytest/logging.py 95.85% <100%> (ø) ⬆️
src/_pytest/python_api.py 97.33% <100%> (+0.84%) ⬆️
src/_pytest/assertion/rewrite.py 93.49% <0%> (+0.54%) ⬆️
src/_pytest/doctest.py 95.43% <0%> (+0.76%) ⬆️
testing/test_pdb.py 99% <0%> (+0.99%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 240828d...d8fa434. Read the comment docs.

@@ -304,8 +305,8 @@ def _setup_collect_fakemodule():

pytest.collect = ModuleType("pytest.collect")
pytest.collect.__all__ = [] # used for setns
for attr in COLLECT_FAKEMODULE_ATTRIBUTES:
setattr(pytest.collect, attr, getattr(pytest, attr))
for attr_name in COLLECT_FAKEMODULE_ATTRIBUTES:
Copy link
Member

Choose a reason for hiding this comment

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

can we remove that in pytest 5?

Copy link
Member Author

Choose a reason for hiding this comment

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

#5180 lists deprecations to remove in 5.1 (the deprecations will be errors in 5.0, removed in 5.1 as we did in 4.0/4.1).

Unfortunately it doesn't seem the pytest.collect has been deprecated at all, so unless I'm mistaken our only option is to deprecate it in 5.X and remove in 6.X I'm afraid. 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how to implement this deprecation thought without using a __getattr__ at module level, but that's 3.7+ only.

Copy link
Member

Choose a reason for hiding this comment

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

class MyModule(ModuleType):
    def __getattr__(...):
        ...

sys.modules[...] = ...

would work (it's essentially what py37 does)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I had the exact same thought right after I posted my comment, but got side-tracked by #5452. 😛

@@ -970,7 +964,7 @@ class FixtureFunctionMarker:
name = attr.ib(default=None)

def __call__(self, function):
if isclass(function):
if inspect.isclass(function):
Copy link
Member

Choose a reason for hiding this comment

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

wouldnt isinstance(..., type) do as we are python3 only now

Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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


@contextmanager
def nullcontext():
yield
Copy link
Member

Choose a reason for hiding this comment

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

coudl even do from contextlib import ExitStack as nullcontext

@@ -304,8 +305,8 @@ def _setup_collect_fakemodule():

pytest.collect = ModuleType("pytest.collect")
pytest.collect.__all__ = [] # used for setns
for attr in COLLECT_FAKEMODULE_ATTRIBUTES:
setattr(pytest.collect, attr, getattr(pytest, attr))
for attr_name in COLLECT_FAKEMODULE_ATTRIBUTES:
Copy link
Member

Choose a reason for hiding this comment

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

class MyModule(ModuleType):
    def __getattr__(...):
        ...

sys.modules[...] = ...

would work (it's essentially what py37 does)

@nicoddemus nicoddemus merged commit 689ce11 into pytest-dev:master Jun 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants