-
Notifications
You must be signed in to change notification settings - Fork 84
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
Use nox for our GitHub Actions workflows #872
Conversation
👈 Launch a binder notebook on this branch for commit 883e800 I will automatically update this comment whenever this PR is modified 👈 Launch a binder notebook on this branch for commit a7c238e 👈 Launch a binder notebook on this branch for commit e4f101f 👈 Launch a binder notebook on this branch for commit b1f1e52 👈 Launch a binder notebook on this branch for commit be7a416 👈 Launch a binder notebook on this branch for commit f654a3f 👈 Launch a binder notebook on this branch for commit a1d7b01 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I've been wanting to see the workflows use the nox commands for consistency, so this is nice.
# NOTE: integration test are permitted to pass if the failure rate was less than a hardcoded threshold. | ||
# PyTest will return 99 if there were some failures, but less than the threshold. For more details, see: | ||
# `pytest_sessionfinish` in tests/integration/conftest.py | ||
success_codes=[0, 99], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we also need to replace the hard-coded 99
with the value of EARTHACCESS_ALLOWABLE_FAILURE_STATUS_CODE
here too (defaulting to 99, if not set)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so here because we're not passing that environment variable down to the test environment, so it should always use the default 99
.:
https://github.com/nsidc/earthaccess/pull/872/files#diff-f7a16a65f061822bcc73b8296f4dc837353d379d8d9cc5307982cb6941442835R56-R59
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's why I'm a bit confused. Where/when/how do you expect to set the var and have it used? You mentioned something about running tests from your IDE, so I guess I'm wondering how that would work (I just run them from the terminal, and haven't attempted running from my IDE).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's only useful for directly invoking pytest tests/integration ...
either in the terminal or via your IDE so that you get a successful status code. If you invoke via nox, it handles that bit for you, but making VS Code and PyCharm aware of nox is significantly harder than making them aware of pytest.
Preview of the upcoming PR where I go into it a bit in the context of an IDE:
https://github.com/nsidc/earthaccess/blob/dev-guide/docs/contributing/development.md#ide-setup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to merge this one and open the next one so we can continue the conversation there.
(ideally, we wouldn't have flakey tests...)
Co-authored-by: Chuck Daniels <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks again! (approved, but just asked for more info about the env var and how it relates, or doesn't, to running tests from and IDE)
In #735 it was proposed we adopt
nox
as a task runner and starting in #733 we've now dropped most of our helper scripts in favor ofnox
.This PR fully leans us into
nox
:scripts/integration-test.sh
in favor can use theintegration-tests
nox sessionintegration-tests
session understands the special99
exit status and will consider it a passnox
for our GitHub Actions workflowsinstall-pkg
action asnox
handles that for us nowtest-min-deps
session tonox
for the test-mindeps workflowIn order to better support IDEs and using pytest directly, I've also allowed the status code returned by pytest when there are some failures, but less than the
ACCEPTABLE_FAILURE_RATE
, to be customized with theEARTHACCESS_ALLOWABLE_FAILURE_STATUS_CODE
environment variable. This primarily is so you can set it to return0
instead of99
such that calling pytest directly or using an integrated test tool will see the test as successful.Pull Request (PR) draft checklist - click to expand
contributing documentation
before getting started.
title such as "Add testing details to the contributor section of the README".
Example PRs: #763
example
closes #1
. SeeGitHub docs - Linking a pull request to an issue.
CHANGELOG.md
with details about your change in a section titled## Unreleased
. If such a section does not exist, please create one. FollowCommon Changelog for your additions.
Example PRs: #763
README.md
with details of changes to theearthaccess interface, if any. Consider new environment variables, function names,
decorators, etc.
Click the "Ready for review" button at the bottom of the "Conversation" tab in GitHub
once these requirements are fulfilled. Don't worry if you see any test failures in
GitHub at this point!
Pull Request (PR) merge checklist - click to expand
Please do your best to complete these requirements! If you need help with any of these
requirements, you can ping the
@nsidc/earthaccess-support
team in a comment and wewill help you out!
Request containing "pre-commit.ci autofix" to automate this.
📚 Documentation preview 📚: https://earthaccess--872.org.readthedocs.build/en/872/