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

Re-enable tests for PRs #415

Merged
merged 3 commits into from Jan 20, 2022
Merged

Re-enable tests for PRs #415

merged 3 commits into from Jan 20, 2022

Conversation

nshalman
Copy link
Contributor

@nshalman nshalman commented Jan 14, 2022

I imagine that disabling testing for PRs may have been needed to unbreak things while cleaning up the older releases in the test matrix.
It would be good to re-enable tests on PRs.

I imagine that disabling for PRs may have been needed to unbreak things while cleaning up the older releases in the test matrix.
It would be good to re-enable tests on PRs.
@nshalman
Copy link
Contributor Author

nshalman commented Jan 14, 2022

Ah, lint is unhappy. I think there are other PRs that relate.

@nshalman
Copy link
Contributor Author

nshalman commented Jan 14, 2022

@shogo82148 Do you think it will be easy to safely clean up the lint errors? Should we re-enable tests and for now just ensure that no NEW lint failures creep in?

@nshalman nshalman requested a review from shogo82148 Jan 14, 2022
@nathany
Copy link
Contributor

nathany commented Jan 15, 2022

I don't think this is necessary. Opening a pull request is also a push. If you look through the checks, you can see twice as many tests runs as needed. Trying to save some cycles.

If you want to take a closer look at GitHub's docs for Actions, I only looked at the very first example I saw, which was on push only.

As far as linting, #382 attempts to do that, but there's a lot there.

Maybe it would make sense to disable the linting until it's fixed, or even selectively disable certain errors that are "TODOs" to fix. At least then the build would pass?

@nathany nathany closed this Jan 15, 2022
@shogo82148
Copy link
Member

shogo82148 commented Jan 15, 2022

I opened #416 for fixing the lint.

If you look through the checks, you can see twice as many tests runs as needed. Trying to save some cycles.

That's right.
But note that the commit hashes to be checked out are different for push and pull_request events.
The commit hash is refs/pull/:prNumber/merge on pull_request events, however it is "updated ref" (typically same as refs/pull/:prNumber/head) on push events.

Generally speaking, refs/pull/:prNumber/merge and refs/pull/:prNumber/head have different contents.
Because someone might commit to the base branch before opening the pull request.

@nshalman
Copy link
Contributor Author

nshalman commented Jan 16, 2022

The pull_request event also annotates the PR more clearly with the success or failure which I find to be helpful when reviewing PRs, as well as being usable to block merging when tests are failing. I'll take a look at #416.

@nshalman nshalman deleted the tests-on-prs branch Jan 17, 2022
@nathany nathany restored the tests-on-prs branch Jan 19, 2022
@nathany
Copy link
Contributor

nathany commented Jan 19, 2022

Do you need both push and pull_request though?

I'd prefer just one or the other - either one.

Btw, the main branch has the GitHub setting enabled "Require branches to be up to date before merging"

@nathany nathany reopened this Jan 19, 2022
@nathany
Copy link
Contributor

nathany commented Jan 19, 2022

We can also add test (macos-latest, 1.17), etc. to the list of status checks that are required in the branch settings. Does that annotates the PR more clearly?

Copy link
Contributor

@nathany nathany left a comment

I don't like wasting cycles, but if you feel it's helpful, then 👌🏼

@nshalman
Copy link
Contributor Author

nshalman commented Jan 20, 2022

I don't like wasting cycles, but if you feel it's helpful, then 👌🏼

Worse than wasting computer cycles is wasting maintainer cycles looking for things that computers could have caught.

@nshalman nshalman merged commit ebaf806 into main Jan 20, 2022
33 checks passed
@nshalman nshalman deleted the tests-on-prs branch Jan 20, 2022
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.

None yet

3 participants