astropy / pytest-remotedata

Pytest plugin to control whether tests are run that have remote data
BSD 3-Clause "New" or "Revised" License
23 stars 15 forks source link

Resolving warnings due to markers and configuration #34

Closed kevindugan closed 6 years ago

kevindugan commented 6 years ago

This pull request resolves the warnings due to deprecated MarkInfo objects like:

C:\Miniconda3\envs\remoteData\lib\site-packages\pytest_remotedata-0.3.1.dev0-py3.7.egg\pytest_remotedata\plugin.py:73: RemovedInPytest4Warning: MarkInfo objects are deprecated as they contain merged marks which are hard to deal with correctly.
Please use node.get_closest_marker(name) or node.iter_markers(name).
Docs: https://docs.pytest.org/en/latest/mark.html#updating-code
  if len(remote_data.args) > 0:
C:\Miniconda3\envs\remoteData\lib\site-packages\pytest_remotedata-0.3.1.dev0-py3.7.egg\pytest_remotedata\plugin.py:76: RemovedInPytest4Warning: MarkInfo objects are deprecated as they contain merged marks which are hard to deal with correctly.
Please use node.get_closest_marker(name) or node.iter_markers(name).
Docs: https://docs.pytest.org/en/latest/mark.html#updating-code
  source = remote_data.kwargs.get('source', 'any')

and also from deprecated configuration

C:\Miniconda3\envs\remoteData\lib\site-packages\_pytest\config\findpaths.py:42: RemovedInPytest4Warning: [pytest] section in setup.cfg files is deprecated, use [tool:pytest] instead.
  config=config,

Should close #31

drdavella commented 6 years ago

@kevindugan thanks for the contribution! As you can see in the CI failures this change is not backwards-compatible with earlier versions of pytest, so we'll either need to add a workaround for earlier versions, or we should consider dropping support for versions of pytest earlier than 3.6.

@bsipocz, @astrofrog thoughts on supporting only pytest>=3.6 going forward?

kevindugan commented 6 years ago

@drdavella Sure thing! Those warnings were bugging me 😄.

As an outside opinion, I think that more people are concerned with the Python version compatibility rather than pytest. If we support Python 2.7 and later versions, I don't think we would lose users over a pytest version restriction. However, I'll defer to you guys and make a work-around if that's what you prefer.

drdavella commented 6 years ago

@kevindugan I'm inclined to agree, but I just want to make sure I don't make this decision unilaterally.

kevindugan commented 6 years ago

If we decide to differentiate between pytest versions, here is my suggestion for how to implement this MR.

diff --git a/pytest_remotedata/plugin.py b/pytest_remotedata/plugin.py
index 510ebdd..9ce40f2 100644
--- a/pytest_remotedata/plugin.py
+++ b/pytest_remotedata/plugin.py
@@ -5,6 +5,7 @@ make use of online data.
 """
 import pytest
 from .disable_internet import turn_off_internet, turn_on_internet
+from distutils.version import StrictVersion

 def pytest_addoption(parser):
@@ -61,8 +62,12 @@ def pytest_unconfigure():

 def pytest_runtest_setup(item):

-    remote_data = item.get_closest_marker('remote_data')
-    internet_off = item.get_closest_marker('internet_off')
+    if StrictVersion(pytest.__version__) < StrictVersion("3.6"):
+        remote_data = item.get_marker('remote_data')
+        internet_off = item.get_marker('internet_off')
+    else:
+        remote_data = item.get_closest_marker('remote_data')
+        internet_off = item.get_closest_marker('internet_off')

     remote_data_config = item.config.getvalue("remote_data")
bsipocz commented 6 years ago

@bsipocz, @astrofrog thoughts on supporting only pytest>=3.6 going forward?

Naively I would say it's OK, but I suspect the linux packaging people might not be super happy about it? @olebole, you have raised issues about pytest versions quite recently, how do you feel about this?

olebole commented 6 years ago

@bsipocz Generally, keeping support older versions makes backports easier for us; especially when the support is as easy as the suggestion by @kevindugan. Having his solution would make backports more robust; we can apply this however locally on the backported package as well. But if this does not make the code less readable, and has no other side effects: why not keep supporting both here? If things get ugly, we still can re-evaluate.

kevindugan commented 6 years ago

@drdavella What was the issue with the CI? I wasn't able to figure out why there were failing tests.

drdavella commented 6 years ago

@kevindugan unfortunately sometimes the HTTP connections on travis time out when we're running tests with remote data. Usually restarting them is enough to get the tests to pass.

drdavella commented 6 years ago

Thanks @kevindugan!

kevindugan commented 6 years ago

@drdavella Sure thing! Do you have an estimate on how long it would take these changes to appear in the Anaconda release of this package?

drdavella commented 6 years ago

Hopefully I will be able to release this early next week.

drdavella commented 6 years ago

This has been released on pypi and has been submitted to conda-forge: https://github.com/conda-forge/pytest-remotedata-feedstock/pull/4.

After that this update still needs to be reflected in pytest-astropy.