use pytest-mock to simplify and improve tests, improve test docs #468

wholmgren · 2018-05-30T23:34:32Z

Rewrites PVSystem tests using pytest-mock and adds a new ModelChain test. Please see the whatsnew diff and the new documentation for more details on how and why.

The changes to test_pvsystem.py are complete (I think). I don't currently have time to go through the entire test_modelchain.py module and improve the tests to use the new methods. I suggest calling it good enough for this pull request. It at least provides an example for people working with the modelchain code in the future.

See discussion in #466 in this comment and below for more context.

Closes issue #xxxx N/A
Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests must pass on the TravisCI and Appveyor testing services.
Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff and/or landscape.io linting service.
New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
Updates entries to docs/sphinx/source/api.rst for API changes.
Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.

cwhanse · 2018-05-31T02:24:40Z

docs/sphinx/source/contributing.rst

+
+        # did the method call the function as we expected?
+        # mocker.spy added assert_called_once_with to the function
+        pvsystem.ashraeiam.assert_called_once_with(thetas, b=0.05)


Here, why do we pass b=-0.05 instead of module_parameters[b]? If b=0.05 is intended then why is module_parameters needed? Just looking to clarify.

In the example above, either b=0.05 or b=module_parameters['b'] would work. What is important is that something dict-like is passed to PVSystem. For example, this would also work:
system = pvsystem.PVSystem(module_parameters={'b': 0.05}) followed by the assert statement as shown. Or this would work:

b = 0.05 system = pvsystem.PVSystem(module_parameters=dict(b=b)) # ... pvsystem.ashraeiam.assert_called_once_with(thetas, b=b)

Would one of the above be a better example? Or more comments?

I think it would appear more clear to me if we change line 157 to module_parameters = {'b' : 0.05} which avoids recasting the dict as a pandas object. Also, to me it is more consistent to use the dict in the assertion, assert_called_once_with(thetas, module_parameters['b']) instead of assert_called_once_with(thetas, b=0.05)

Thanks. That makes sense.

wholmgren

@cwhanse do you want to review this and merge if acceptable? I know I need to make one change to replace a test I accidentally removed. I'll do that next.

Use the "squash and merge" feature so that we can maintain a simple commit history. If you don't already see it, it's under the arrow button on the right side of green merge button.

wholmgren · 2018-06-11T16:53:02Z

pvlib/test/test_pvsystem.py

@@ -21,38 +21,11 @@

 from conftest import needs_numpy_1_10, requires_scipy

-latitude = 32.2


I removed this block of code to make all of tests self-contained. A small amount of it is moved into a couple of functions. None of these changes are completely necessary for pytest-mock improvements, but I was already in "improve the tests" mode.

wholmgren · 2018-06-11T16:54:28Z

docs/sphinx/source/contributing.rst

@@ -9,7 +9,7 @@ contribute.


 Easy ways to contribute
-----------------------
+~~~~~~~~~~~~~~~~~~~~~~~


I think these needed to change to make the new level of organization work below (Functions, PVsystem/Location, ModelChain).

wholmgren · 2018-06-11T16:55:12Z

docs/sphinx/source/contributing.rst


 pvlib's unit tests can easily be run by executing ``py.test`` on the
 pvlib directory:

-``py.test pvlib``
+``pytest pvlib``


Older versions of pytest were called with py.test. Now it's just pytest.

wholmgren · 2018-06-11T16:56:25Z

pvlib/test/test_pvsystem.py

@@ -113,15 +99,14 @@ def test_ashraeiam_scalar():
    assert_allclose(iam, expected, equal_nan=True)


-@needs_numpy_1_10


numpy 1.10 is only needed for tests that use assert_allclose(equal_nan=True)

wholmgren · 2018-06-11T17:04:07Z

pvlib/test/test_pvsystem.py

    out = system.sapm_spectral_loss(airmass)
-
-
-@pytest.mark.parametrize("expected", [1.03173953])


I accidentally deleted this test in a merge. I'll fix next.

Added below.

wholmgren · 2018-06-11T17:09:44Z

pvlib/test/test_pvsystem.py

+    pvsystem.sapm_effective_irradiance.assert_called_once_with(
+        poa_direct, poa_diffuse, airmass_absolute, aoi, sapm_module_params,
+        reference_irradiance=reference_irradiance)
+    assert_allclose(out, 1, atol=0.1)


This low-precision test will fail if we change the units of effective irradiance. That could be an argument for or against having any low-precision tests at all, depending on your perspective.

Here, we are testing that pvsystem.sapm_effective_irradiance is called and returns a reasonable value, correct? If so, then I would change airmass_absolute = 1.5, aoi = 0 and poa_direct = 900. We should keep reference_irradiance = 1000. With these inputs the method call should return 1.0 exactly. At least until we act on #472

I updated the input parameters.

Here, we are testing that pvsystem.sapm_effective_irradiance is called and returns a reasonable value, correct?

Yes, so I kept atol=0.1.

wholmgren · 2018-06-11T17:14:57Z

pvlib/test/test_pvsystem.py

    system = pvsystem.PVSystem(modules_per_string=2, strings_per_inverter=3)
+    m = mocker.patch(
+        'pvlib.pvsystem.scale_voltage_current_power', autospec=True)


This test function uses a different pattern than the others. Unlike mocker.spy, mocker.patch can only be used to test that something is called because it completely replaces the function/method under consideration. I chose this pattern here because I think the input data is a little more complicated than is worth maintaining in the method test. Could be changed, though.

cwhanse · 2018-06-12T14:57:53Z

pvlib/test/test_pvsystem.py

+    pvsystem.sapm_effective_irradiance.assert_called_once_with(
+        poa_direct, poa_diffuse, airmass_absolute, aoi, sapm_module_params,
+        reference_irradiance=reference_irradiance)
+    assert_allclose(out, 1, atol=0.1)


Here, we are testing that pvsystem.sapm_effective_irradiance is called and returns a reasonable value, correct? If so, then I would change airmass_absolute = 1.5, aoi = 0 and poa_direct = 900. We should keep reference_irradiance = 1000. With these inputs the method call should return 1.0 exactly. At least until we act on #472

wholmgren added 14 commits May 28, 2018 20:31

replace PVSystem iam tests with mocked versions

783ddf3

add autospec

df7289d

add pytest-mock to ci

716bb71

use mocker.spy for most tests

422e0b3

mock PVSystem.pvwatts tests

4e2fafc

PVSystem.sapm mocked

f947617

remove unnecessary code in test_pvsystem

546f134

update contibuting testing recommendations

1354188

clean up

2a51f74

more pvsystem mocks

9c57d0b

add mock dc test to test_modelchain.py

ac37a3a

add contributing example for modelchain

cccb42a

update whatsnew

f8a90b0

improve compatibility with older python

0e1a267

cwhanse reviewed May 31, 2018

View reviewed changes

wholmgren added 2 commits May 31, 2018 09:25

improve example test

4d0668e

merge upstream changes

b80a1e4

wholmgren commented Jun 11, 2018

View reviewed changes

wholmgren added 3 commits June 11, 2018 10:40

add mocked PVSystem fs spectral loss test

d0aecb3

fix flake8 warnings

5c80fd3

update for calcparams_desoto signature change

c5b8f67

cwhanse approved these changes Jun 12, 2018

View reviewed changes

cwhanse requested changes Jun 12, 2018

View reviewed changes

update PVSystem sapm eff irr params

4bf3579

cwhanse approved these changes Jun 12, 2018

View reviewed changes

cwhanse merged commit a3e05f2 into pvlib:master Jun 13, 2018

wholmgren deleted the mock branch June 13, 2018 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use pytest-mock to simplify and improve tests, improve test docs #468

use pytest-mock to simplify and improve tests, improve test docs #468

wholmgren commented May 30, 2018 •

edited

Loading

cwhanse May 31, 2018

wholmgren May 31, 2018

cwhanse May 31, 2018

wholmgren May 31, 2018

wholmgren left a comment

wholmgren Jun 11, 2018

wholmgren Jun 11, 2018

wholmgren Jun 11, 2018

wholmgren Jun 11, 2018

wholmgren Jun 11, 2018

wholmgren Jun 11, 2018

wholmgren Jun 11, 2018

cwhanse Jun 12, 2018

wholmgren Jun 12, 2018

wholmgren Jun 11, 2018

cwhanse Jun 12, 2018

		@@ -21,38 +21,11 @@

		from conftest import needs_numpy_1_10, requires_scipy

		latitude = 32.2

		@@ -113,15 +99,14 @@ def test_ashraeiam_scalar():
		assert_allclose(iam, expected, equal_nan=True)


		@needs_numpy_1_10

		out = system.sapm_spectral_loss(airmass)


		@pytest.mark.parametrize("expected", [1.03173953])

use pytest-mock to simplify and improve tests, improve test docs #468

use pytest-mock to simplify and improve tests, improve test docs #468

Conversation

wholmgren commented May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren commented May 30, 2018 •

edited

Loading