icesat2py / icepyx

Python tools for obtaining and working with ICESat-2 data
https://icepyx.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
218 stars 107 forks source link

Replace integration test review trigger with manual trigger #595

Closed mfisher87 closed 1 month ago

mfisher87 commented 2 months ago

@JessicaS11 I think we should remove the pull_request_review trigger and logic. I don't know what we need to get this right, so I replaced it with a manual trigger. This will help us get PRs moving again. What do you think?

TODO:

github-actions[bot] commented 2 months ago

Binder :point_left: Launch a binder notebook on this branch for commit 4484b6808b6f1e3af2289c4218563fcaa68ea6fa

I will automatically update this comment whenever this PR is modified

Binder :point_left: Launch a binder notebook on this branch for commit f551b3e6a8df59a17c17705dfbc8bec8ff31f4aa

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 65.20%. Comparing base (a98409f) to head (f551b3e). Report is 1 commits behind head on development.

:exclamation: There is a different number of reports uploaded between BASE (a98409f) and HEAD (f551b3e). Click for more details.

HEAD has 1 upload less than BASE | Flag | BASE (a98409f) | HEAD (f551b3e) | |------|------|------| ||3|2|
Additional details and impacted files ```diff @@ Coverage Diff @@ ## development #595 +/- ## =============================================== - Coverage 71.67% 65.20% -6.47% =============================================== Files 37 37 Lines 3064 3064 Branches 596 596 =============================================== - Hits 2196 1998 -198 - Misses 765 981 +216 + Partials 103 85 -18 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JessicaS11 commented 2 months ago

I don't know what we need to get this right, so I replaced it with a manual trigger.

I was looking at our setup, and I think the funny business might be coming from our if statement: if: "${{ !github.event.pull_request.head.repo.fork && (github.event.action != 'pull_request_review' || github.event.review.state == 'approved') }}"

My plain text of this logic is to run the test if (1) it's not a fork AND (2) it's (a) not a PR review OR (b) it's approved. I'm not sure why condition (a) is in there, since we're using the pull_request_review trigger but only want it to run if it's approved (and not on a fork). I know I don't have an intuition yet for saving computation by not evaluating past the first condition, but I can't make the logic of this one work out.

JessicaS11 commented 2 months ago

@mfisher87 Any further thoughts on this?

mfisher87 commented 2 months ago

Maybe we should close the PR? Things seem to be working well anyway right now. Can always come back to it :)

JessicaS11 commented 2 months ago

Things seem to be working well anyway right now. Can always come back to it :)

Strong disagree. The Integration Tests are being triggered far more often than they should be.

mfisher87 commented 2 months ago

When are they being triggered that you don't want them triggered? I've been struggling with them not being triggered when I do want them to be triggered. Do you think this manual triggering approach is the way forward for now?

mfisher87 commented 1 month ago

We decided today to avoid the rabbit hole of GitHub Actions debugging and disable the integration test merge requirement and stop running integration tests on approval for now due to finnickiness of the current config (https://github.com/icesat2py/icepyx/pull/603). I'm sure we can figure it out eventually :)