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

Separate unit tests from integration tests #130

Closed
r24mille opened this issue Aug 19, 2017 · 2 comments · Fixed by #131 or #153
Closed

Separate unit tests from integration tests #130

r24mille opened this issue Aug 19, 2017 · 2 comments · Fixed by #131 or #153

Comments

@r24mille
Copy link
Contributor

Currently, there are many tests in the test directory that call external resources, rather than mocking requests/responses. Some also require environment variables (e.g. setting EIA_KEY in test_eia.py). This is causing issues in Travis and we're losing the value from existing continuous integration work that has been done.

I propose the following changes:

  1. The codebase seems to follow a tests outside application code directory structure, which is fine. In line with this Stack Overflow thread I'd further organize directories into:
pyiso
  + --- tests
  |       + --- unit
  |       |         | test_base.py
  |       |         | test_client_one.py
  |       |         | test_client_two.py
  |       |
  |       + --- integration 
  |                 | integration_test_client_one.py
  |                 | integration_test_client_two.py
  |       
  + --- pyiso
          | base.py
          | client_one.py
          | client_two.py
  1. Fix travis.yml so that the build server only runs unit tests when pull requests are opened. I believe this is just changing the [nosetests](https://nose.readthedocs.io/en/latest/usage.html) script on line 26 to something like:
script: nosetests -w ./tests/unit

This only leaves one question: What automated process should run the integration tests? Should the maintainers just be responsible for running integration tests before formalizing a release? Should the integration tests just be there for developers to run out-of-band so they can be confident in their code before opening pull requests?

r24mille added a commit to r24mille/pyiso that referenced this issue Aug 20, 2017
This is the minimal change necessary to fix the BPA unit tests.
r24mille added a commit to r24mille/pyiso that referenced this issue Aug 20, 2017
…ory.

This keeps the tests cleaner and more readable.
r24mille added a commit to r24mille/pyiso that referenced this issue Aug 20, 2017
It was not used anywhere in the unit tests and it is not Excel content
as the file extension suggests.
r24mille added a commit to r24mille/pyiso that referenced this issue Aug 20, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Aug 22, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Aug 22, 2017
It makes a network request for the latest content, so it should go in the /pyiso/tests/integration directory.
r24mille added a commit to r24mille/pyiso that referenced this issue Aug 23, 2017
Moved and renamed files to follow the proposed structure of issue WattTime#130.
r24mille added a commit to r24mille/pyiso that referenced this issue Aug 23, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Aug 24, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Aug 30, 2017
…n tests.

Unit and integration tests are now segregated into separate locations.
With the new location for integration tests, I've added the NBPower
integration tests which I'd previously held back from PR WattTime#125.
r24mille added a commit to r24mille/pyiso that referenced this issue Aug 30, 2017
…ests.

Unit and integration tests are now segregated into separate locations.
With the new location for integration tests, I've added the SaskClient
integration tests which I'd previously held back from PR WattTime#126.
@r24mille
Copy link
Contributor Author

Hmm... it seems #131 auto-closed this issue when it got merged in (?) Anyway, it may be useful to split the work up into multiple issues or re-open this issue. I don't have permissions, since I'm not a collaborator.

@ajdonnison
Copy link
Contributor

Reopening as it needs more work.

@ajdonnison ajdonnison reopened this Aug 30, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 2, 2017
Now that there's a place for intergration tests, I've added several
for the `IESOClient`. Several tests are annotated with @skiptest.
I'm trying to find a better way to address the gap in the data
(which is valid) but still check something meaningful with the test.
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 3, 2017
 I would like to upgrade further, but don't think it's prudent until we get
 unit tests and integration tests sorted in WattTime#130. Then we could probably
 bump the version up further.

 For now, I think upgrading the dependency from 3.6.1 to 3.6.4 is fairly
 safe (unit tests pass) and the version change bring in stabilization fixes
 only. See http://lxml.de/3.6/changes-3.6.4.html for details.
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 4, 2017
This should create a coverage report which can be sent via the
coveralls build steps in travis.yml
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 4, 2017
This commit splits test_ercot.py into unit tests, which have no external
dependencies, and integration tests, which test pyiso's integration
with external systems (e.g. ERCOT's web servers).
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 4, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 5, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 5, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 6, 2017
This makes the unit tests easier to read and work with.
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 6, 2017
ajdonnison added a commit that referenced this issue Sep 7, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 20, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 21, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 22, 2017
This allows the class to be initialized and unit tests which don't make web
requests to pass.
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 22, 2017
Having the fixtures_base_path outside the function wasn't necessary
and was less clear.
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 22, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 22, 2017
Also I renamed some of the fixtures that were being used for expected
responses to be clearer.
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 22, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 22, 2017
Historical data from 2015 no longer exists. I changed the date to 2017
to kick the problem down the road for two years ;)
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 23, 2017
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 23, 2017
I also tweaked test_auth(...) so that it mocks the response from the ENSOe
server similar to the other mocked auth calls.
r24mille added a commit to r24mille/pyiso that referenced this issue Sep 23, 2017
ajdonnison pushed a commit that referenced this issue Sep 25, 2017
* #130 Fixed test_dt_index(...) in test_caiso.py

The DatetimeIndex class was moved to from pandas.tseries.index to
pandas.core.indexes.datetimes with the release of 0.20.1:

http://pandas.pydata.org/pandas-docs/stable/whatsnew.html#reorganization-of-the-library-privacy-changes

This means that our requirement of pandas >= 0.18 could cause
test_dt_index(...) to fail in test_caiso.py sporadically if users fulfill
the requirement with pandas >= 0.20.1. It felt foolish to increment one of
 pyiso's core requirements for the sake of a unit test. So I changed the test
 to check for the type(...) class name rather than the type(...) exactly.

* #130 Increased default request timeout to 30 seconds.

I've increased the default request timeout to 30 seconds because
test_get_lmp_dataframe_fifteen(...) from test_caiso.py was
sporadically failing for me, not because the request was bad but
because processing time was right at the threshold of the request
processing timeout.

Increasing the default timeout from 20 seconds to 30 seconds seemed
reasonable, since 20 seconds is rather short. However, any further
increases to the timeout due to large .zip requests from CAISO should
possibly be isolated to the CAISOClient.

* Removed TIMEOUT_SECONDS constant from BaseClient.

The field was not being referenced anywhere. It only caused confusion
given that __init__ has a timeout_seconds parameter with a default value.

* #130 Split up integration tests and unit tests out of test_caiso.py

They now follow the directory conventions being established with #130.

* #130 Increased LMP request timeout for CAISO to 45 seconds.

* #130 Pulled static content out of test_caiso.py into fixtures directory.

* #130 Rely on setUp(...) to call client_factory('CAISO') before each test.

Unit and integration tests were each creating their own CAISOClient at that
start of each test. That type of setup can be left to the setUp(...) method.

* #130 Fixed BaseClient test timeout.

All we should be checking is that the request timeout is assignable. Checking
its default value with a unit test is just annoying.

* #130 Fixed byte encoding issues in mocked response.

This was causing errors in Python 3.
ajdonnison pushed a commit that referenced this issue Sep 25, 2017
)

* #130 Split up unit tests and integration tests up from EIAClient.
* #130 Moved EIA docstring from unit test to integration test.
* #130 Added static 'EIA_KEY' environment variable for mocked unit tests.
* Edited EIAClient's set_url(...) method for clarity.
ajdonnison pushed a commit that referenced this issue Sep 25, 2017
)

* #130 Added fake ISONE environment variables for unit tests.

This allows the class to be initialized and unit tests which don't make web
requests to pass.

* #130 Minor cleanup for read_fixtures(...) function.

Having the fixtures_base_path outside the function wasn't necessary
and was less clear.

* #130 Moved test fixture files into isone subdirectory.

* #130 Fixed mocked requests in ISONE unit tests.

Also I renamed some of the fixtures that were being used for expected
responses to be clearer.

* #130 Split ISONE integration tests out of unit tests.

* #130 Fixed up the LMP integration tests for ISONE.

Historical data from 2015 no longer exists. I changed the date to 2017
to kick the problem down the road for two years ;)

* #130 Fixed up mocked response encoding issue for Python 3.
ajdonnison pushed a commit that referenced this issue Sep 25, 2017
* #130 Moved test_eu.py into unit test directory.

* #130 Added test ENTSOe username/password environment variables.

I also tweaked test_auth(...) so that it mocks the response from the ENSOe
server similar to the other mocked auth calls.
r24mille added a commit to r24mille/pyiso that referenced this issue Dec 2, 2017
The TestNYISOTrade class in test_trade.py was mocking static responses in an integration test. With improvements made to mocked responses in WattTime#183, testing static responses are handled by unit tests.

This commit changes the integration tests in TestNYISOTrade to use current times, so that the tests dynamically test our ability to integration with the external NYISO server (in line with WattTime#130).
r24mille added a commit to r24mille/pyiso that referenced this issue Dec 2, 2017
…NYISOGenMix

The TestNYISOGenMix class in test_genmix.py had comment-out references to static responses. They're now properly dynamic (furthered by WattTime#130) so the old comments are no longer relevant.

This commit removes the comments, deletes the now-unused files the comments were about, and corrected some minor errors in the TestNYISOGenMix integration tests.
r24mille added a commit to r24mille/pyiso that referenced this issue Dec 2, 2017
…CAISOLoad

The TestCAISOLoad class in test_load.py had comment-out references to static responses. The tests are now now dynamic (furthered by WattTime#130) so the old comments are no longer relevant.

This commit removes the comments and deletes the now  unused files the comments were about.
r24mille added a commit to r24mille/pyiso that referenced this issue Dec 6, 2017
…tNEVPLoad

The TestNEVPLoad class in test_load.py had commented-out references to static responses. The tests are now now dynamic (furthered by WattTime#130) so the old comments are no longer relevant.

This commit removes the comments and deletes the now  unused file the comments were about.
r24mille added a commit to r24mille/pyiso that referenced this issue Dec 6, 2017
…tSPPCLoad

The TestSPPCLoad class in test_load.py had commented-out references to static responses. The tests are now now dynamic (furthered by WattTime#130) so the old comments are no longer relevant.

This commit removes the comments and deletes the now  unused file the comments were about.
r24mille added a commit to r24mille/pyiso that referenced this issue Dec 6, 2017
…tGenerationTask

The TestGenerationTask class in test_tasks.py had commented-out references to static responses. The tests are now now dynamic (furthered by WattTime#130) so the old comments are no longer relevant.

This commit removes the comments and deletes the now  unused file the comments were about.
r24mille added a commit to r24mille/pyiso that referenced this issue Dec 6, 2017
…tSVERIGenMix

The TestSVERIGenMix class in test_genmix.py had commented-out references to static responses. The tests are now now dynamic (furthered by WattTime#130) so the old comments are no longer relevant.

This commit removes the comments and deletes the now  unused files the comments were about.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants