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

Cross platform test suite compatibility #127

Merged
merged 47 commits into from
Feb 15, 2018

Conversation

JFPerkins
Copy link
Contributor

This PR gets the minimal set of AllenSDK tests to pass on any platform (Windows, OSX, linux) under python 2.7 and 3.6. The minimal set of tests is defined as the following environment variables set:
TEST_NWB_FILES=skip
TEST_OBSERVATORY_EXPERIMENT_PLOTS_DATA=skip
TEST_API_ENDPOINT=http://api.brain-map.org
TEST_COMPLETE=false

In the process of doing this the following SDK bugs were discovered and fixed:

  • Manifest.safe_mkdir("/") resulted in uncaught OSError on both OSX and Windows due to the error number being different than EEXIST.
  • fn_temp_dir and md_temp_dir defined where they were were not operating as pytest fixtures at the proper scope.

In order to run on Windows, the --boxed flag cannot be used because it is just an alias for --forked which explicitly uses os.fork() to run tests in submodules (as an aside, --boxed is deprecated and unmaintained). This lead to the discovery of a bunch of monkeypatching and mock assignments that were performed improperly and leaked scope across test modules. The majority of the changes in this PR are fixing those mocks. Some patterns that were used and should be avoided because they break downstream tests when not run in isolated:

The bad patch:

import allensdk.core.json_utilities as ju
from mock import MagicMock

# This shows a specific function but the problem is general to how the patch is applied
def some_function_or_fixture():
    ju.read_url_get = MagicMock() # read_url_get is now a mock when json_utilities is used in later tests even if we don't want it to be

The import/reload patch:

import pytest
from mock import patch, Mock

@pytest.fixture
def nrrd_read():
    with patch('nrrd.read',
                     Mock(name='nrrd_read_file_mcm',
                               return_value=('mock_annotation_data',
                               'mock_annotation_image'))) as nrrd_read:
        import allensdk.core.mouse_connectivity_cache as MCC
        import allensdk.api.queries.mouse_connectivity_api as MCA
        import allensdk.api.queries.reference_space_api as RSA
        reload(MCC)
        reload(RSA)
        reload(MCA)

    return nrrd_read

@pytest.mark.run('last')
def test_cleanup(): # even after this cleanup runs, later MouseConnectivityCache tests break
    import allensdk.api.queries.mouse_connectivity_api
    reload(allensdk.api.queries.mouse_connectivity_api)
    import allensdk.core.mouse_connectivity_cache
   reload(allensdk.core.mouse_connectivity_cache)

All instances of the above patterns that were causing tests to fail have been removed and replaced with properly scoped patches. This doesn't mean that all instances of the patterns have been found (since it's possible a bad patch was made on a function that isn't used again in a downstream test and I didn't systematically search through every test).

Other side effects:

  • Windows builds on appveyor configured with appveyor.yml
  • Linux and OSX builds on travis configured with .travis.yml
  • Accumulated cross-platform coverage reported on codecov (looks very low since only the minimal set of tests run)
  • Tests that used to leave random files sitting around now put those files in temporary directories so they get cleaned up
  • test_cell_filters.py downloads a zipped version of the cell specimen table from the brain observatory website and places it on the cache path for testing the filter functions. This avoids a 10+ minute RMA query.
  • Running tests not --boxed is actually faster because module-scoped fixtures are actually only called once per module (in --boxed they are called every test since every test things it is in a unique session and module)

@codecov-io
Copy link

codecov-io commented Feb 6, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@76919a2). Click here to learn what that means.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #127   +/-   ##
=========================================
  Coverage          ?   42.83%           
=========================================
  Files             ?       88           
  Lines             ?    11484           
  Branches          ?        0           
=========================================
  Hits              ?     4919           
  Misses            ?     6565           
  Partials          ?        0
Impacted Files Coverage Δ
allensdk/config/manifest.py 67.56% <77.77%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76919a2...a11ed2d. Read the comment docs.

Copy link
Contributor

@dyf dyf left a comment

Choose a reason for hiding this comment

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

This is amazing.

@NileGraddis NileGraddis merged commit d23a9fb into AllenInstitute:master Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants