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

RemovedInPytest4Warning via _pytest\compat.py:321 #4131

Closed
blueyed opened this issue Oct 13, 2018 · 17 comments
Closed

RemovedInPytest4Warning via _pytest\compat.py:321 #4131

blueyed opened this issue Oct 13, 2018 · 17 comments
Labels
good first issue easy issue that is friendly to new contributor type: infrastructure improvement to development/releases/CI structure

Comments

@blueyed
Copy link
Contributor

blueyed commented Oct 13, 2018

There are many RemovedInPytest4Warnings with pytest's own tests, e.g.:

c:\projects\pytest\.tox\py36\lib\site-packages\_pytest\compat.py:321: RemovedInPytest4Warning: usage of Session.Class is deprecated, please use pytest.Class instead
360  return getattr(object, name, default)
361

(via https://ci.appveyor.com/project/pytestbot/pytest/builds/19474342/job/c83h5n8ne3snvf8o#L359)

Are those expected?

I would assume pytest should not causes warnings itself.

@nicoddemus
Copy link
Member

In most cases it is inevitable because we still need to test deprecated features.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 13, 2018

I see.. there are more than 130 of them though.
Maybe pytest's tests should be configured to ignore those warnings then maybe?

@nicoddemus
Copy link
Member

Maybe pytest's tests should be configured to ignore those warnings then maybe?

Hmmm yep, makes sense.

@nicoddemus nicoddemus added type: infrastructure improvement to development/releases/CI structure good first issue easy issue that is friendly to new contributor labels Oct 13, 2018
@RonnyPfannschmidt
Copy link
Member

@nicoddemus this is a bug in plugin factory parsing - we souldnt parse elements that are CompatProperties, or remove those things already

@nicoddemus
Copy link
Member

We shouldn't remove it now, those are scheduled to be removed in 4.1.

But I'm OK with just ignoring that for now, TBH. They will go away soon anyway.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 13, 2018

Cool, closing then.

@blueyed blueyed closed this as completed Oct 13, 2018
@blueyed
Copy link
Contributor Author

blueyed commented Oct 14, 2018

After all I think it is worth not having a bunch of warnings (that are expected).
I will look into it, but any pointers are appreciated of course - the one from Ronny seems good already.

@blueyed blueyed reopened this Oct 14, 2018
@nicoddemus
Copy link
Member

I think it is fine to ignore them by configuring filterwarnings in pytest.ini.

@Zac-HD
Copy link
Member

Zac-HD commented Oct 18, 2018

Ignoring internal warnings in our own tests seems very risky to me - at some point we'll deprecate something without removing an internal use, and then all downstream test suites using -Werror will fail 😭

In Hypothesis we get around this with an explicit decorator for all tests that should raise deprecation warnings - it even errors if no warning is emitted.

IMO we should also test Pytest with all warnings as errors, except for jobs deliberately testing older versions of dependencies, to make this practical for downstream users.

@edmorley
Copy link

I'm seeing these warnings in our project's own pytest runs. I presume this is not expected?
https://travis-ci.org/mozilla/treeherder/jobs/442405171#L777

Our pytest config is here:
https://github.com/mozilla/treeherder/blob/97480c66c9700b77dae581205207802848bdea6c/setup.cfg#L32-L61

@nicoddemus
Copy link
Member

nicoddemus commented Oct 18, 2018

Ignoring internal warnings in our own tests seems very risky to me

Oh I meant to ignore the warnings mentioned in this thread explicitly, not all internal warnings, which I agree would be very risky. 👍

IMO we should also test Pytest with all warnings as errors

We do that already 😉

pytest/tox.ini

Lines 206 to 220 in e4871f7

filterwarnings =
error
ignore:yield tests are deprecated, and scheduled to be removed in pytest 4.0:pytest.RemovedInPytest4Warning
ignore:Metafunc.addcall is deprecated and scheduled to be removed in pytest 4.0:pytest.RemovedInPytest4Warning
ignore:Module already imported so cannot be rewritten:pytest.PytestWarning
# produced by path.local
ignore:bad escape.*:DeprecationWarning:re
# produced by path.readlines
ignore:.*U.*mode is deprecated:DeprecationWarning
# produced by pytest-xdist
ignore:.*type argument to addoption.*:DeprecationWarning
# produced by python >=3.5 on execnet (pytest-xdist)
ignore:.*inspect.getargspec.*deprecated, use inspect.signature.*:DeprecationWarning
# pytest's own futurewarnings
ignore::pytest.PytestExperimentalApiWarning

@Zac-HD
Copy link
Member

Zac-HD commented Oct 18, 2018

That only applies after pytest has finished the configure stage though, right? In the configure stage, _issue_config_warning downgrades warnings from errors to capturing them, even if we run with python -Werror -m pytest. IMO we should

  • have a minimal dependency subset of the tests that we can run with Python-level warnings-as-errors
  • issue a warning directly from _issue_config_warning if warnings have already been configured
  • consider silencing warnings at the origin where we are using deprecated APIs for compatibility only

I'm poking at some of these now; I'll update with results when I have some.


diff --git a/src/_pytest/warnings.py b/src/_pytest/warnings.py
index 6c4b921..e98d56a 100644
--- a/src/_pytest/warnings.py
+++ b/src/_pytest/warnings.py
@@ -165,8 +165,12 @@ def _issue_config_warning(warning, config):
     :param config:
     """
     with warnings.catch_warnings(record=True) as records:
-        warnings.simplefilter("always", type(warning))
+        # Set this warning to 'always', unless warnings have been configured
+        # at the interpreter level with e.g. -Werror or PYTHONWARNINGS=ignore
+        if sys.warnoptions:
+            warnings.simplefilter("always", type(warning))
         warnings.warn(warning, stacklevel=2)
-    config.hook.pytest_warning_captured.call_historic(
-        kwargs=dict(warning_message=records[0], when="config", item=None)
-    )
+    if records:
+        config.hook.pytest_warning_captured.call_historic(
+            kwargs=dict(warning_message=records[0], when="config", item=None)
+        )

Pretty sure this works but the setup to test it is awfully convoluted. I'll leave it there for now before it turns into a serious un-fun slog.

@nicoddemus
Copy link
Member

I'm poking at some of these now; I'll update with results when I have some.

Awesome, thanks!

It is a little tricky to capture the warnings properly during pytest_configure because at that point the warnings plugin is not fully initialized, so the filter warnings filter is not materialized yet. But please share the results of your findings! 👍

@edmorley, you should not be seeing those warnings unless you or some plugins are accessing those properties. But we have realized that they might be triggered falsely in some test suites, and #4164 fixes this. 👍

@edmorley
Copy link

@edmorley, you should not be seeing those warnings unless you or some plugins are accessing those properties.

Thank you for the reply. The problem is that the current warning gives no indication as to what triggered the warning. For example:

$ python -3 -m pytest tests/ --runslow --ignore=tests/selenium/
/home/travis/venv/lib/python2.7/site-packages/_pytest/compat.py:329: RemovedInPytest4Warning: usage of Session.Class is deprecated, please use pytest.Class instead
  return getattr(object, name, default)

Perhaps this is another case of the warning's stacklevel needing to be adjusted?

@nicoddemus
Copy link
Member

Perhaps this is another case of the warning's stacklevel needing to be adjusted?

Definitely. #4221 will also help here.

@nicoddemus
Copy link
Member

nicoddemus commented Oct 25, 2018

Created #4236 to adjust the stack level.

@nicoddemus
Copy link
Member

Fixed by #2701

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue easy issue that is friendly to new contributor type: infrastructure improvement to development/releases/CI structure
Projects
None yet
Development

No branches or pull requests

5 participants