conftest.fail_on_pvlib_version applied to functions that require args or kwargs

[cwhanse]

• I am familiar with the contributing guidelines
• Tests added
• Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
• New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
• Pull request is nearly complete and ready for detailed review.
• Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

Adding capability to pass arguments to a function decorated with fail_on_pvlib_version. Allows use of fixtures when testing for deprecation message.

[cwhanse]

OK so that didn't work, I'll fool with it some more.

[kandersolar]

I can't say whether passing values through the decorator is the best approach -- maybe @wholmgren's suggestion about mocking is better. I am much more familiar with decorators than mocking though, so this is how to get your test to pass:

$ git diff
diff --git a/pvlib/tests/conftest.py b/pvlib/tests/conftest.py
index f347f86..115ba1c 100644
--- a/pvlib/tests/conftest.py
+++ b/pvlib/tests/conftest.py
@@ -19,11 +19,11 @@ pvlib_base_version = \
 # test function may not take args, kwargs, or fixtures.
 def fail_on_pvlib_version(version, *args):
     # second level of decorator takes the function under consideration
-    def wrapper(func, *args):
+    def wrapper(func):
         # third level defers computation until the test is called
         # this allows the specific test to fail at test runtime,
         # rather than at decoration time (when the module is imported)
-        def inner(*args):
+        def inner():
             # fail if the version is too high
             if pvlib_base_version >= parse_version(version):
                 pytest.fail('the tested function is scheduled to be '
diff --git a/pvlib/tests/test_conftest.py b/pvlib/tests/test_conftest.py
index eb186f5..5ec634f 100644
--- a/pvlib/tests/test_conftest.py
+++ b/pvlib/tests/test_conftest.py
@@ -37,6 +37,6 @@ deprec_func = deprecated('0.8', alternative='alt_func',
 
 
 @fail_on_pvlib_version('0.9', some_data)
-def test_deprecated_09():
+def test_deprecated_09(some_data):
     with pytest.warns(pvlibDeprecationWarning):
-        deprec_func(some_data())
+        deprec_func(some_data)

[cwhanse]

Thanks @kanderso-nrel but that doesn't run locally when I test it. I don't have an opinion whether modifying fail_on_pvlib_version or using mock is better, but it's a chance for me to learn about more about decorators and mock. I don't recall when but this has come up before, where instead of using a fixture to test a decorated function, the fixture's data had to be copied into the test.

_____________________________ test_deprecated_09 ______________________________

    def inner():
        # fail if the version is too high
        if pvlib_base_version >= parse_version(version):
            pytest.fail('the tested function is scheduled to be '
                        'removed in %s' % version)
        # otherwise return the function to be executed
        else:
>           return func(*args)
E           TypeError: test_deprecated_09() missing 1 required positional argument: 'some_data'

[kandersolar]

Hmm, strange. I just installed it into a fresh environment and it seems to work for me. Here's the branch I was using: https://github.com/kanderso-nrel/pvlib-python/tree/failtest

The logic being that you want to give *args to fail_on_pvlib_version to eventually give to func, but including *args parameters in wrapper and inner causes the real *args to be shadowed with an empty tuple when func is called.

[cwhanse]

@kanderso-nrel @wholmgren I think this works to pass arguments from the decorator to the decorated function. I checked that the value passed through the decorator is the same as the value inside the decorated function. I'm not sure how this all works so your review is needed.

The advantage here (from my perspective) is to test a deprecated function using the same pattern of fixtures as is used before deprecation.

[kandersolar]

@cwhanse I'm sorry, I think I had partially misunderstood the problem this PR is trying to solve. With the way this PR currently works, I don't think pytest is automatically replacing the some_data parameter with what the some_data function returns, it's just passing in the function itself. It passes the test because the test doesn't really check the parameter value, but you'd have to manually call it inside the test functions you want to decorate to actually get the value. Specifically in this case, adding the line assert some_data == "some data" to test_deprecated_09 causes a failure with AssertionError: assert <function some_data at 0x7f701747c710> == 'some data'.

Is the goal to be able to just tack @fail_on_pvlib_version('0.9') onto an existing test (that uses fixtures) with no other modifications? If so, I don't think you need to explicitly pass the arguments through the fail_on_pvlib_version decorator. Using functools.wraps will make the inner wrapper function look the same as the original function, letting pytest detect the use of the fixture and making everything work as usual. Here's another diff (from the most recent commit on this PR) that shows what I'm talking about. Sorry again for the confusion, I should have paid more attention to the context before chiming in.

Click to expand

(base) kevin@carrots-deluxe:~/projects/pvlib-python[conftest|!?P]$ git diff
diff --git a/pvlib/tests/conftest.py b/pvlib/tests/conftest.py
index 981d115..6e2d3fd 100644
--- a/pvlib/tests/conftest.py
+++ b/pvlib/tests/conftest.py
@@ -6,6 +6,7 @@ import numpy as np
 import pandas as pd
 from pkg_resources import parse_version
 import pytest
+import functools
 
 import pvlib
 
@@ -17,13 +18,14 @@ pvlib_base_version = \
 # for example @fail_on_pvlib_version('0.7') will cause a test to fail
 # on pvlib versions 0.7a, 0.7b, 0.7rc1, etc.
 # args and kwargs will be passed to the function being decorated.
-def fail_on_pvlib_version(version, *args, **kwargs):
+def fail_on_pvlib_version(version):
     # second level of decorator takes the function under consideration
     def wrapper(func):
         # third level defers computation until the test is called
         # this allows the specific test to fail at test runtime,
         # rather than at decoration time (when the module is imported)
-        def inner():
+        @functools.wraps(func)
+        def inner(*args, **kwargs):
             # fail if the version is too high
             if pvlib_base_version >= parse_version(version):
                 pytest.fail('the tested function is scheduled to be '
diff --git a/pvlib/tests/test_conftest.py b/pvlib/tests/test_conftest.py
index 6ed5974..ec22477 100644
--- a/pvlib/tests/test_conftest.py
+++ b/pvlib/tests/test_conftest.py
@@ -36,9 +36,11 @@ deprec_func = deprecated('0.8', alternative='alt_func',
                          name='deprec_func', removal='0.9')(alt_func)
 
 
-@fail_on_pvlib_version('0.9', some_data, test_string="test")
-def test_deprecated_09(some_data, test_string=None):
+@fail_on_pvlib_version('0.9')
+def test_deprecated_09(some_data, test_string="test"):
     with pytest.warns(pvlibDeprecationWarning):  # test for deprecation warning
+        # check that pytest is passing in the fixture value, not the function
+        assert some_data == "some data"
         # use assert to test that alternate function was called
         assert some_data == deprec_func(some_data)[0]
         # check that the kwarg was accepted

[cwhanse]

Is the goal to be able to just tack @fail_on_pvlib_version('0.9') onto an existing test (that uses fixtures) with no other modifications?

Yes exactly

[cwhanse]

thanks @kanderso-nrel @wholmgren please take another look.

[cwhanse]

@wholmgren @kanderso-nrel this one good to merge?

[wholmgren]

nit below, but ok to merge without further review.

[wholmgren]

nit: shouldn't the function name be something like test_fail_on_pvlib_version_args_kwargs?

[kandersolar]

another minor nit: this comment could be improved, arguments are passed to the test, not the decorator

[kandersolar]

Looks like the whatsnew entry could still use an update. Other than that LGTM

[wholmgren]

Thanks @cwhanse and @kanderso-nrel !