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.

Sign up for GitHub

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

Jump to bottom

Test and CI upgrades and maintenance #308

Merged
merged 15 commits into from
Oct 19, 2018
Merged

Test and CI upgrades and maintenance #308

merged 15 commits into from
Oct 19, 2018

Conversation

alimanfoo
Copy link
Member

@alimanfoo alimanfoo commented Oct 18, 2018 •
edited
Loading

This PR includes several pieces of test and CI maintenance and upgrade work, including...

  • Drop py34 and add py37 to appveyor and travis
  • Move to py37 as primary test environment (runs doctests)
  • Move to npy115 as primary numpy version, with npy113 and npy114 also tested under py36
  • Upgrade all requirements pinned in requirements_dev.txt and requirements_dev_optional.txt (worth doing anyway, but hopefully also fixes a problem where coveralls was not working under py2)
  • Fix some pickle tests that were failing on my local system due to the fact the gnu dbm did not allow the same database file to be opened twice at the same time. I don't know why this didn't manifest previously, but have reworked pickle tests slightly to ensure that stores are closed if they can be prior to attempting to unpickle anything, based on work in fix pickle tests: avoid multiple simultaneous active writers #273.
  • Simplify the PR template.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Changes documented in docs/release.rst
  • Docs build locally (e.g., run tox -e docs)
  • AppVeyor and Travis CI passes
  • Test coverage to 100% (Coveralls passes)

@alimanfoo alimanfoo added this to the v2.3 milestone Oct 18, 2018
@alimanfoo alimanfoo self-assigned this Oct 18, 2018
@alimanfoo alimanfoo added the maintenance Work needed by a maintainer label Oct 18, 2018
@alimanfoo alimanfoo mentioned this pull request Oct 18, 2018
Merged
11 tasks
@alimanfoo alimanfoo changed the title Fix failing pickle tests Test and CI upgrades and maintenance Oct 18, 2018
@alimanfoo
Copy link
Member Author

OK, coverage is back up to 100% and all travis runs are passing.

Python 3.7 is still failing under appveyor, I think because numcodecs is not getting built properly, so Blosc is not available. I'm rerunning appveyor with more verbose logging for the numcodecs install to see if I can debug.

@jakirkham if you have any pearls of wisdom here, they'd be much appreciated.

alimanfoo
alimanfoo commented Oct 18, 2018
View reviewed changes
.github/PULL_REQUEST_TEMPLATE.md
* [ ] Add docstrings and API docs for any new/modified user-facing classes and functions
* [ ] New/modified features documented in docs/tutorial.rst
* [ ] Doctests in tutorial pass (e.g., run ``tox -e py36`` or ``python -m doctest -o NORMALIZE_WHITESPACE -o ELLIPSIS docs/tutorial.rst``)
Copy link
Member Author

@alimanfoo alimanfoo Oct 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Just to say I made the deletions above because they are all covered via the travis build, and so it seems a bit much to expect everyone to run all of these locally too.

@jakirkham
Copy link
Member

Python 3.7 is still failing under appveyor, I think because numcodecs is not getting built properly, so Blosc is not available. I'm rerunning appveyor with more verbose logging for the numcodecs install to see if I can debug.

We need to regenerate the C files with Cython 0.27.3+. ( cython/cython#1955 )

@alimanfoo
Copy link
Member Author

Many thanks @jakirkham, I can follow that up.

@alimanfoo
Copy link
Member Author

OK, all checks passing, I think this is good to go.

@alimanfoo alimanfoo requested a review from jakirkham October 18, 2018 20:56
jakirkham
jakirkham reviewed Oct 18, 2018
View reviewed changes
zarr/tests/test_storage.py
# round-trip through pickle
dump = pickle.dumps(store)
# some stores cannot be opened twice at the same time, need to close first
# store before can round-trip through pickle
Copy link
Member

@jakirkham jakirkham Oct 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Comment might need some tweaks. Took a go below.

close first store before can -> close store first before

Edit: Looks like this comment is repeated other places. So same suggestion applies for them as well.

jakirkham
jakirkham approved these changes Oct 18, 2018
View reviewed changes
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Looks fine to me. There was a comment that might need a little tweaking, but everything else looked fine. Feel free to adjust it and merge. Thanks for doing this. 😄

@alimanfoo
Copy link
Member Author

Thanks @jakirkham, comment adjusted.

@alimanfoo alimanfoo merged commit 2bc8e69 into zarr-developers:master Oct 19, 2018
@alimanfoo alimanfoo deleted the pickle-failures-20181018 branch October 19, 2018 00:03
@jakirkham jakirkham mentioned this pull request Oct 19, 2018
Closed
This was referenced Oct 19, 2018
Closed
Closed
@jakirkham jakirkham mentioned this pull request Oct 31, 2018
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakirkham jakirkham jakirkham approved these changes

Assignees

@alimanfoo alimanfoo

Labels
maintenance Work needed by a maintainer
Projects
None yet
Milestone
v2.3
Development

Successfully merging this pull request may close these issues.
2 participants
@alimanfoo @jakirkham