-
-
Notifications
You must be signed in to change notification settings - Fork 363
Maintenance on tests, CI and contributor 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
Maintenance on tests, CI and contributor docs #468
Conversation
I'm also finding that it's not possible currently to run the tox tests locally because the network stores (redis, mongo, ABS) are not available. It would be good to find some way to avoid running those unless they are on CI. |
The Azure test failure seems to be an API break. ( #467 ) As a short term fix, we might consider constraining the dependency to a version that we know works. |
I've pinned |
Appveyor has installed Azure Storage Emulator version 5.9:
|
Travis has passed. Appveyor is passing. @jakirkham, @shikharsg, I'd be grateful if you could take a look at this PR. |
Hm, I'm looking at the contributing guide (docs/contributing.rst), that could use updating. Also I'd like to find a way to avoid contributors unintentionally running the pymongo, redis or ABS tests when they don't have the relevant services installed locally and getting test failures. Doesn't make for a nice contributor experience. Needs some thought. |
Seems reasonable to me. Thanks for tackling this @alimanfoo! 🙂 |
We should be able to do things like this to avoid running tests without dependencies. |
Yes that works as a way of avoiding tests if the client libraries are not installed. But the current contributing docs can lead a developer to install the client libraries, but these tests also require a service (or service emulator) to be running locally, and so the redis, mongo and azure tests fail. This is annoying for a potential contributor, it would be better to guide them to a situation where they don't have the service client libraries installed, which requires some reworking of requirements files and contributor docs. |
This has ended up being a bit more of a change than originally planned, but think it needed doing. I've updated the issue description with info on the changes included here. |
@jakirkham if you wouldn't mind re-reviewing I'd be grateful. |
All looks good to me. Thanks for the fix @alimanfoo |
Thanks @shikharsg. Just to add, to make life even simpler for contributors I've added some logic to check for an environment variable |
Upside of the new environment variables is that |
Coveralls is currently down for maintenance, so think it's OK to merge this without coveralls passing. |
Agree. Seems like a sure short method of ensuring that the developer actually wanted to run the tests. |
This seems like a useful thing to get merged soon to help with other PRs, so I'll merge later today if no objections. |
This PR does some maintenance on the testing and CI config, and on the contributor documentation on how to install dependencies and run tests. Changes include:
ZARR_TEST_ABS
,ZARR_TEST_MONGO
andZARR_TEST_REDIS
which must be set to "1" in order to run tests that require the corresponding services to be running locally, otherwise those tests are skipped - this simplifies the experience for most contributors who won't want to set up those services or run those tests.azure-storage-blob
for compatibility with the Azure Storage Emulator (resolves Azure tests failing on AppVeyor CI #467).TODO:
tox -e docs
)