Skip to content

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

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

Merged
merged 20 commits into from
Jun 13, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ install:
- cmd: conda info -a

# install depenencies
- cmd: conda create -n test_env --yes --quiet python=%PYTHON_VERSION% pip numpy scipy pytables pandas nose pytest pytz ephem numba siphon -c conda-forge
- cmd: conda create -n test_env --yes --quiet python=%PYTHON_VERSION% pip numpy scipy pytables pandas nose pytest pytz ephem numba siphon pytest-mock -c conda-forge
- cmd: activate test_env
- cmd: python --version
- cmd: conda list
Expand Down
1 change: 1 addition & 0 deletions ci/requirements-py27-min.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ dependencies:
- pytz
- pytest
- pytest-cov
- pytest-mock
- nose
- pip:
- coveralls
1 change: 1 addition & 0 deletions ci/requirements-py27.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ dependencies:
- siphon
- pytest
- pytest-cov
- pytest-mock
- nose
- pip:
- coveralls
Expand Down
1 change: 1 addition & 0 deletions ci/requirements-py34.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ dependencies:
- siphon
- pytest
- pytest-cov
- pytest-mock
- nose
- pip:
- coveralls
Expand Down
1 change: 1 addition & 0 deletions ci/requirements-py35.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ dependencies:
- siphon
- pytest
- pytest-cov
- pytest-mock
- nose
- pip:
- coveralls
Expand Down
1 change: 1 addition & 0 deletions ci/requirements-py36.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ dependencies:
#- siphon
- pytest
- pytest-cov
- pytest-mock
- nose
- pip:
- coveralls
104 changes: 94 additions & 10 deletions docs/sphinx/source/contributing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ contribute.


Easy ways to contribute
-----------------------
~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Member Author

Choose a reason for hiding this comment

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

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


Here are a few ideas for you can contribute, even if you are new to
pvlib-python, git, or Python:
Expand All @@ -33,7 +33,7 @@ pvlib-python, git, or Python:


How to contribute new code
--------------------------
~~~~~~~~~~~~~~~~~~~~~~~~~~

Contributors to pvlib-python use GitHub's pull requests to add/modify
its source code. The GitHub pull request process can be intimidating for
Expand Down Expand Up @@ -81,29 +81,113 @@ changes, such as fixing documentation typos.


Testing
-------
~~~~~~~

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

``py.test pvlib``
``pytest pvlib``
Copy link
Member Author

Choose a reason for hiding this comment

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

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


or, for a single module:

``py.test pvlib/test/test_clearsky.py``
``pytest pvlib/test/test_clearsky.py``

While copy/paste coding should generally be avoided, it's a great way
to learn how to write unit tests!
or, for a single test:

Unit test code should be placed in the corresponding test module in the
pvlib/test directory.
``pytest pvlib/test/test_clearsky.py::test_ineichen_nans``

Use the ``--pdb`` flag to debug failures and avoid using ``print``.

New unit test code should be placed in the corresponding test module in
the pvlib/test directory.

Developers **must** include comprehensive tests for any additions or
modifications to pvlib.

pvlib-python contains 3 "layers" of code: functions, PVSystem/Location,
and ModelChain. Contributors will need to add tests that correspond to
the layer that they modify.

Functions
---------
Tests of core pvlib functions should ensure that the function returns
the desired output for a variety of function inputs. The tests should be
independent of other pvlib functions (see :issue:`394`). The tests
should ensure that all reasonable combinations of input types (floats,
nans, arrays, series, scalars, etc) work as expected. Remember that your
use case is likely not the only way that this function will be used, and
your input data may not be generic enough to fully test the function.
Write tests that cover the full range of validity of the algorithm.
It is also important to write tests that assert the return value of the
function or that the function throws an exception when input data is
beyond the range of algorithm validity.

PVSystem/Location
-----------------
The PVSystem and Location classes provide convenience wrappers around
the core pvlib functions. The tests in test_pvsystem.py and
test_location.py should ensure that the method calls correctly wrap the
function calls. Many PVSystem/Location methods pass one or more of their
object's attributes (e.g. PVSystem.module_parameters, Location.latitude)
to a function. Tests should ensure that attributes are passed correctly.
These tests should also ensure that the method returns some reasonable
data, though the precise values of the data should be covered by
function-specific tests discussed above.

We prefer to use the ``pytest-mock`` framework to write these tests. The
test below shows an example of testing the ``PVSystem.ashraeiam``
method. ``mocker`` is a ``pytest-mock`` object. ``mocker.spy`` adds
features to the ``pvsystem.ashraeiam`` *function* that keep track of how
it was called. Then a ``PVSystem`` object is created and the
``PVSystem.ashraeiam`` *method* is called in the usual way. The
``PVSystem.ashraeiam`` method is supposed to call the
``pvsystem.ashraeiam`` function with the angles supplied to the method
call and the value of ``b`` that we defined in ``module_parameters``.
The ``pvsystem.ashraeiam.assert_called_once_with`` call will test this
for us! Finally, we check that the output of the method call is
reasonable.

.. code-block:: python
def test_PVSystem_ashraeiam(mocker):
# mocker is a pytest-mock object.
# mocker.spy adds code to a function to keep track of how its called
mocker.spy(pvsystem, 'ashraeiam')

# set up inputs
module_parameters = pd.Series({'b': 0.05})
system = pvsystem.PVSystem(module_parameters=module_parameters)
thetas = 1

# call the method
iam = system.ashraeiam(thetas)

# 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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. That makes sense.


# check that the output is reasonable, but no need to duplicate
# the rigorous tests of the function
assert iam < 1.

Avoid writing PVSystem/Location tests that depend sensitively on the
return value of a statement as a substitute for using mock. These tests
are sensitive to changes in the functions, which is *not* what we want
to test here, and are difficult to maintain.

ModelChain
----------
The tests in test_modelchain.py should ensure that
``ModelChain.__init__`` correctly configures the ModelChain object to
eventually run the selected models. A test should ensure that the
appropriate method is actually called in the course of
``Model_chain.run_model``. A test should ensure that the model selection
does have a reasonable effect on the subsequent calculations, though the
precise values of the data should be covered by the function tests
discussed above. ``pytest-mock`` can also be used for testing ``ModelChain``.


This documentation
------------------
~~~~~~~~~~~~~~~~~~

If this documentation is unclear, help us improve it! Consider looking
at the `pandas
Expand Down
Loading