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

Use nox for our GitHub Actions workflows #872

Merged
merged 11 commits into from
Nov 7, 2024
Merged

Use nox for our GitHub Actions workflows #872

merged 11 commits into from
Nov 7, 2024

Conversation

jhkennedy
Copy link
Collaborator

@jhkennedy jhkennedy commented Nov 6, 2024

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 of nox.

This PR fully leans us into nox:

  • drops scripts/integration-test.sh in favor can use the integration-tests nox session
    • the integration-tests session understands the special 99 exit status and will consider it a pass
    • Instead of using the script to print a warning, pytest will now raise a warning when the number of failures is below our allowable threshold, which looks like:
      ...
      tests/integration/conftest.py:40
        .../earthaccess/tests/integration/conftest.py:40: UserWarning: 
        WARNING: The integration test suite has returned 99 because the failure rate was less than a hardcoded threshold. For more details see:
        `pytest_sessionfinish` in tests/integration/conftest.py.
          warn(
      
      -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
      ================================================================= short test summary info =================================================================
      FAILED tests/integration/test_cloud_download.py::test_multi_file_granule - Exception
      ================================================== 1 failed, 57 passed, 21 warnings in 577.11s (0:09:37) ==================================================
      nox > Session integration-tests was successful.
      
  • uses nox for our GitHub Actions workflows
    • drops our install-pkg action as nox handles that for us now
    • add a test-min-deps session to nox for the test-mindeps workflow

In 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 the EARTHACCESS_ALLOWABLE_FAILURE_STATUS_CODE environment variable. This primarily is so you can set it to return 0 instead of 99 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
  • Please review our
    contributing documentation
    before getting started.
  • Populate a descriptive title. For example, instead of "Updated README.md", use a
    title such as "Add testing details to the contributor section of the README".
    Example PRs: #763
  • Populate the body of the pull request with:
  • Update CHANGELOG.md with details about your change in a section titled
    ## Unreleased. If such a section does not exist, please create one. Follow
    Common Changelog for your additions.
    Example PRs: #763
  • Update the documentation and/or the README.md with details of changes to the
    earthaccess 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 we
will help you out!

  • Add unit tests for any new features.
  • Apply formatting and linting autofixes. You can add a GitHub comment in this Pull
    Request containing "pre-commit.ci autofix" to automate this.
  • Ensure all automated PR checks (seen at the bottom of the "conversation" tab) pass.
  • Get at least one approving review.

📚 Documentation preview 📚: https://earthaccess--872.org.readthedocs.build/en/872/

Copy link

github-actions bot commented Nov 6, 2024

Binder 👈 Launch a binder notebook on this branch for commit 883e800

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit a7c238e

Binder 👈 Launch a binder notebook on this branch for commit e4f101f

Binder 👈 Launch a binder notebook on this branch for commit b1f1e52

Binder 👈 Launch a binder notebook on this branch for commit be7a416

Binder 👈 Launch a binder notebook on this branch for commit f654a3f

Binder 👈 Launch a binder notebook on this branch for commit a1d7b01

@jhkennedy jhkennedy requested review from betolink, mfisher87 and chuckwondo and removed request for betolink and mfisher87 November 6, 2024 23:42
@jhkennedy jhkennedy marked this pull request as ready for review November 6, 2024 23:44
Copy link
Collaborator

@chuckwondo chuckwondo left a 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],
Copy link
Collaborator

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)?

Copy link
Collaborator Author

@jhkennedy jhkennedy Nov 7, 2024

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

Copy link
Collaborator

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).

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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...)

docs/contributing/development.md Show resolved Hide resolved
docs/contributing/development.md Outdated Show resolved Hide resolved
.github/workflows/integration-test.yml Show resolved Hide resolved
Copy link
Collaborator

@chuckwondo chuckwondo left a 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)

@jhkennedy jhkennedy merged commit d4f24df into main Nov 7, 2024
14 checks passed
@jhkennedy jhkennedy deleted the nox-it-all branch November 7, 2024 20:07
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.

2 participants