Migrate unit and integration tests from TravisCI to GitHub Actions #580

mfisher87 · 2024-08-21T23:47:45Z

Closes #547

This PR migrates the current TravisCI behavior into GitHub Actions.

New feature: Unit tests run on multiple versions of Python.

TODO:

@JessicaS11 add credentials currently being used for TravisCI tests to GitHub repository secrets
@JessicaS11 make new unit and integration test workflows "required" in GH repository settings
@JessicaS11 clean up required Travis check in repository settings
@JessicaS11 or @mfisher87 update CI badges on readme (currently point to Travis)

Note

This does not attempt to implement an approval process for PRs from forks. We don't have that now, so we're not losing anything by dropping Travis in this PR. This is challenging. I'll open another draft PR with that stuff, and we can hopefully figure it out.

See #588

github-actions · 2024-08-21T23:47:56Z

👈 Launch a binder notebook on this branch for commit 69dcec3

I will automatically update this comment whenever this PR is modified

👈 Launch a binder notebook on this branch for commit b98483c

👈 Launch a binder notebook on this branch for commit cd2f771

👈 Launch a binder notebook on this branch for commit 8ebabce

👈 Launch a binder notebook on this branch for commit e5b2610

👈 Launch a binder notebook on this branch for commit 7aab5bb

👈 Launch a binder notebook on this branch for commit 434b010

👈 Launch a binder notebook on this branch for commit 0038615

👈 Launch a binder notebook on this branch for commit a5b2609

👈 Launch a binder notebook on this branch for commit 2426af3

👈 Launch a binder notebook on this branch for commit 33592a5

👈 Launch a binder notebook on this branch for commit 0daf7ae

👈 Launch a binder notebook on this branch for commit 88bc5d9

👈 Launch a binder notebook on this branch for commit 39006bd

👈 Launch a binder notebook on this branch for commit 098bb98

👈 Launch a binder notebook on this branch for commit 417238f

👈 Launch a binder notebook on this branch for commit a950c87

👈 Launch a binder notebook on this branch for commit 7bb6cb5

👈 Launch a binder notebook on this branch for commit 3115443

👈 Launch a binder notebook on this branch for commit 0b82f05

👈 Launch a binder notebook on this branch for commit 4183ece

👈 Launch a binder notebook on this branch for commit 4e59c92

👈 Launch a binder notebook on this branch for commit 403643b

👈 Launch a binder notebook on this branch for commit 8ee626a

mfisher87 · 2024-08-23T16:32:48Z

I got a bit frustrated trying to get the integration test "secure approval" process working the way I want it to. The event isn't triggering, and I don't know how (of if it's possible) to debug that.

I'm going to spend some time on the Harmony -> ECS work today to give myself a break. I made a post in Openscapes Slack asking if anyone has ideas what's going wrong in the meantime :) Any tips greatly appreciated!

mfisher87 · 2024-08-26T19:39:18Z

I think I'm going to split this PR into two pieces: (1) migrate Travis -> GHA as-is; (2) the approval thing.

(1) alone provides value by reducing the number of unique automation systems we depend on.

.github/workflows/integration_test.yml

.github/actions/install-icepyx/action.yml

.github/workflows/integration_test.yml

JessicaS11 · 2024-08-27T19:49:45Z

.github/workflows/integration_test.yml

+  push:
+    branches:
+      - "main"
+      - "development"


Given these are slightly "expensive" tests since they download files (and running them for every commit could ultimately influence the icepyx metrics), what are our options for restricting how often they run for a given PR?

These triggers (branches:) would be for every merge, not for every commit on PRs. We can customize the pull_request: triggers. By default, they run on open and synchronize (effectively, when the PR is pushed to, not on every commit). The full list is here: https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request

I think the best way we can limit these runs is by adding paths: filters. That would enable e.g. docs changes to not trigger the expensive tests!

Should we consider this part of this PR, or create an issue to track the work?

(effectively, when the PR is pushed to, not on every commit).

Right - forgot this subtlety - but a PR can have a lot of pushes too!

Should we consider this part of this PR, or create an issue to track the work?

I would definitely consider it part of this PR. I don't want to have a setup running that will trigger so many runs.

I would definitely consider it part of this PR. I don't want to have a setup running that will trigger so many runs.

Don't want to push back too hard, because I am fine doing it however, but I do want to raise that Travis CI behaves this way already:

https://github.com/icesat2py/icepyx/blob/development/.travis.yml#L21-L24

I would lean towards breaking this work into two PRs: (1) migrate Travis -> GH; (2) more efficient. When those PRs are merged into development, they will produce two clearer commits and entries in the changelog. I'm OK with immediately following this PR up with step 2 if you're OK with dividing it!

Or "approved"? I think this is how I set up the uml diagram action so it only runs once per PR.

There is no approved event type. That workflow checks the current state of the PR after triggering the workflow, and if it's not "approved", it sets the workflow to "skipped":

icepyx/.github/workflows/uml_action.yml

Line 10 in a52bc37

if: github.event.review.state == 'approved'

If there was an approved event type, then we could avoid the triggering in the first place :(

Since we have other possible triggers we would need something like ${{ github.event.action != "pull_request_review" || github.event.review.state == "approved" }}. Otherwise, integration tests would be skipped on our release PRs until they were approved, and if we want that behavior we can remove the pull_request trigger. Thoughts?

There is no approved event type.

Guess that was wishful thinking on my part then.

Since we have other possible triggers we would need something like ${{ github.event.action != "pull_request_review" || github.event.review.state == "approved" }}. Otherwise, integration tests would be skipped on our release PRs until they were approved, and if we want that behavior we can remove the pull_request trigger. Thoughts?

I'm happy with the latest edits versus this. I don't think we tend to have a lot of official "reviews" until we're getting pretty close to merging, and it would be great to have the tests run on release PRs right away.

I'm happy with the latest edits versus this. I don't think we tend to have a lot of official "reviews" until we're getting pretty close to merging, and it would be great to have the tests run on release PRs right away.

I might need to rescind this. I'm noticing the integration test is running, which makes me think that GH considers a comment (regardless of approval) to be a "review", thus triggering the action for any comment made on the PR.

I'm thinking the least unpalatable option at this point is your suggestion to remove the PR trigger and add something like ${{ github.event.action != "pull_request_review" || github.event.review.state == "approved" }}.

Let's try it on for size!

.github/workflows/unit_test.yml

Co-authored-by: Jessica Scheick <[email protected]>

codecov · 2024-08-27T20:03:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.45%. Comparing base (a52bc37) to head (8ee626a).
Report is 1 commits behind head on development.

Additional details and impacted files

@@               Coverage Diff               @@
##           development     #580      +/-   ##
===============================================
+ Coverage        65.36%   65.45%   +0.09%     
===============================================
  Files               36       36              
  Lines             3037     3037              
  Branches           531      594      +63     
===============================================
+ Hits              1985     1988       +3     
+ Misses             970      964       -6     
- Partials            82       85       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JessicaS11 · 2024-08-27T20:11:38Z

I got a bit frustrated trying to get the integration test "secure approval" process working the way I want it to. The event isn't triggering, and I don't know how (of if it's possible) to debug that.

Is it possible that it's because the action isn't yet in development?

Co-authored-by: Jessica Scheick <[email protected]>

mfisher87 · 2024-08-27T20:17:48Z

Is it possible that it's because the action isn't yet in development?

I did run in to that initially. I've been testing on my fork, and I did merge into development over there, and still not seeing the events I expected!

mfisher87 · 2024-08-27T20:24:01Z

@JessicaS11 I stealth-edited some TODOs in the description of this PR. Just realized Travis will block merge if it doesn't run :)

.github/workflows/integration_test.yml

It's provided as a fixture

.github/workflows/integration_test.yml

.github/workflows/unit_test.yml

mfisher87 · 2024-08-27T21:14:31Z

I was confused why the tests weren't passing, and it turns out they also depend on an envvar named $NSIDC_LOGIN. When they are both populated with the password, things go!

Changed based on feedback: #580 (comment)

README.rst

Based on feedback: #580 (comment)

README.rst

Co-authored-by: Jessica Scheick <[email protected]>

for more information, see https://pre-commit.ci

JessicaS11

Thanks for all the problem solving and changes on this one! I'm excited to get CI moved onto GH, so thanks for making it happen, Matt.

mfisher87 · 2024-08-29T01:26:06Z

Any time! 🚀

mfisher87 added 3 commits August 26, 2024 16:58

Migrate unit and integration tests to GitHub Actions

85fade9

Extract checkout from re-usable action

3015f76

Apply shell param to composite action

0038615

mfisher87 force-pushed the test-with-gha branch from 434b010 to 0038615 Compare August 26, 2024 23:00

Run integration tests on PRs

a5b2609

mfisher87 changed the title ~~Migrate unit and integration tests to GitHub Actions~~ Migrate unit and integration tests from TravisCI to GitHub Actions Aug 26, 2024

mfisher87 commented Aug 26, 2024

View reviewed changes

.github/workflows/integration_test.yml Show resolved Hide resolved

mfisher87 marked this pull request as ready for review August 26, 2024 23:06

mfisher87 requested review from JessicaS11 and weiji14 August 26, 2024 23:06

mfisher87 mentioned this pull request Aug 26, 2024

Workflow to approve integration tests from forks #588

Closed

mfisher87 added the review::priority This change is preferred for review label Aug 27, 2024

JessicaS11 reviewed Aug 27, 2024

View reviewed changes

.github/actions/install-icepyx/action.yml Outdated Show resolved Hide resolved

JessicaS11 reviewed Aug 27, 2024

View reviewed changes

.github/workflows/integration_test.yml Outdated Show resolved Hide resolved

JessicaS11 reviewed Aug 27, 2024

View reviewed changes

.github/workflows/unit_test.yml Outdated Show resolved Hide resolved

mfisher87 and others added 2 commits August 27, 2024 13:54

Remove erroneous comma

2426af3

Co-authored-by: Jessica Scheick <[email protected]>

Remove redundant word

33592a5

Co-authored-by: Jessica Scheick <[email protected]>

Clarify reason for selected Python versions in CI

0daf7ae

Co-authored-by: Jessica Scheick <[email protected]>

mfisher87 commented Aug 27, 2024

View reviewed changes

.github/workflows/integration_test.yml Outdated Show resolved Hide resolved

mfisher87 added 2 commits August 27, 2024 14:32

Remove earthdata username envvar

88bc5d9

It's provided as a fixture

Fixup PR-from-fork test

39006bd

JessicaS11 reviewed Aug 27, 2024

View reviewed changes

.github/workflows/integration_test.yml Outdated Show resolved Hide resolved

JessicaS11 reviewed Aug 27, 2024

View reviewed changes

.github/workflows/unit_test.yml Outdated Show resolved Hide resolved

Filter integration test runs by path

417238f

mfisher87 force-pushed the test-with-gha branch from 098bb98 to 417238f Compare August 27, 2024 20:58

Attempt to provide correct envvars to integration test

a950c87

mfisher87 added 3 commits August 27, 2024 15:40

Give jobs a clearer name

7bb6cb5

Changed based on feedback: #580 (comment)

Add README badge for GitHub Actions

3115443

Merge branch 'development' into test-with-gha

0b82f05

JessicaS11 reviewed Aug 27, 2024

View reviewed changes

README.rst Outdated Show resolved Hide resolved

Adjust integration test triggers

4183ece

Based on feedback: #580 (comment)

JessicaS11 reviewed Aug 28, 2024

View reviewed changes

README.rst Outdated Show resolved Hide resolved

JessicaS11 reviewed Aug 28, 2024

View reviewed changes

README.rst Outdated Show resolved Hide resolved

mfisher87 and others added 4 commits August 28, 2024 18:28

Add separate badges for unit and integration tests

5a606bc

Co-authored-by: Jessica Scheick <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

4e59c92

for more information, see https://pre-commit.ci

Apply missing badge references

403643b

Only run for reviews that are approving

8ee626a

mfisher87 requested a review from JessicaS11 August 29, 2024 00:43

JessicaS11 approved these changes Aug 29, 2024

View reviewed changes

mfisher87 merged commit bd09d73 into development Aug 29, 2024
10 checks passed

mfisher87 deleted the test-with-gha branch August 29, 2024 01:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate unit and integration tests from TravisCI to GitHub Actions #580

Migrate unit and integration tests from TravisCI to GitHub Actions #580

mfisher87 commented Aug 21, 2024 •

edited by JessicaS11

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

mfisher87 commented Aug 23, 2024 •

edited

Loading

mfisher87 commented Aug 26, 2024

JessicaS11 Aug 27, 2024

mfisher87 Aug 27, 2024

mfisher87 Aug 27, 2024

JessicaS11 Aug 27, 2024

mfisher87 Aug 27, 2024

mfisher87 Aug 27, 2024 •

edited

Loading

JessicaS11 Aug 28, 2024

JessicaS11 Aug 28, 2024

JessicaS11 Aug 28, 2024

mfisher87 Aug 29, 2024

codecov bot commented Aug 27, 2024 •

edited

Loading

JessicaS11 commented Aug 27, 2024

mfisher87 commented Aug 27, 2024

mfisher87 commented Aug 27, 2024

mfisher87 commented Aug 27, 2024

JessicaS11 left a comment

mfisher87 commented Aug 29, 2024

Migrate unit and integration tests from TravisCI to GitHub Actions #580

Migrate unit and integration tests from TravisCI to GitHub Actions #580

Conversation

mfisher87 commented Aug 21, 2024 • edited by JessicaS11 Loading

github-actions bot commented Aug 21, 2024 • edited Loading

mfisher87 commented Aug 23, 2024 • edited Loading

mfisher87 commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfisher87 Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 27, 2024 • edited Loading

Codecov Report

JessicaS11 commented Aug 27, 2024

mfisher87 commented Aug 27, 2024

mfisher87 commented Aug 27, 2024

mfisher87 commented Aug 27, 2024

JessicaS11 left a comment

Choose a reason for hiding this comment

mfisher87 commented Aug 29, 2024

mfisher87 commented Aug 21, 2024 •

edited by JessicaS11

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

mfisher87 commented Aug 23, 2024 •

edited

Loading

mfisher87 Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 27, 2024 •

edited

Loading