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

Pull request tests run twice? #145

Closed
d-maurer opened this issue May 16, 2022 · 8 comments · Fixed by #146
Closed

Pull request tests run twice? #145

d-maurer opened this issue May 16, 2022 · 8 comments · Fixed by #146
Labels
question Further information is requested

Comments

@d-maurer
Copy link

I have the impression that the tests after a PR push are run twice, once with label (push) and once with label (pull_request). If this impression is correct, what is the motivation?

If there is no strong reason, we should avoid test duplication, as this would reduce testing time.

@d-maurer d-maurer added the question Further information is requested label May 16, 2022
@dataflake
Copy link
Member

dataflake commented May 16, 2022

The impression is correct, the GitHub Actions for those tests are triggered on both pushes and pull request pushes.

I think it's set up that way to avoid situations where no tests are run at all, like someone creates a PR from a fork. In that case the forked branch (and tests that may or may not run on it, depending on the forking side's setup) is not readily visible to a reviewer.

A quick glance at the GH Actions docs didn't show an obvious way to say "if two triggers want to execute the tests just skip one of them".

@dataflake
Copy link
Member

I found an article that describes how to write a condition that basically says "run on push and run on PR if the PR is from a forked repository", see https://wildwolf.name/github-actions-how-to-avoid-running-the-same-workflow-multiple-times/:

on:
  push:
    branches:
    - '**'
  pull_request:
    branches:
    - '**'
jobs:
  build:
    runs-on: ubuntu-latest
    if: github.event_name != "pull_request" || github.event.pull_request.head.repo.full_name == github.event.pull_request.base.repo.full_name

@mauritsvanrees
Copy link
Member

I have lately done it like this:

on:
  push:
    branches: [ master ]
  pull_request:
  workflow_dispatch:

In the branches you would list master or main plus any maintenance branches.

@dataflake
Copy link
Member

Think about pushes against maintenance branches other than main/master. You would probably have to maintain a list of active branch names there that differ from repository to repository.

@mauritsvanrees
Copy link
Member

That is true. For an individual package this would be fine, but here in meta not.

eventualbuddha added a commit to decaffeinate/decaffeinate that referenced this issue Sep 11, 2022
eventualbuddha added a commit to decaffeinate/decaffeinate that referenced this issue Sep 11, 2022
mauritsvanrees added a commit to buildout/buildout that referenced this issue Sep 13, 2022
@mauritsvanrees
Copy link
Member

I am trying this in the zc.buildout repository. It failed at first because I copied an older version of the condition from above, which made the workflow invalid. So let me paste the current version from meta so others can copy the correct line:

if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name != github.event.pull_request.base.repo.full_name

Note that you need single quotes. Thanks @mgedmin.

@marcelm
Copy link

marcelm commented Oct 28, 2022

And a drive-by comment to those struggling with this: The corrected version by @mauritsvanrees fixes two typos compared to the original: There need to be single quotes and it should be a test for inequality (!=).

@ingydotnet
Copy link

Easier to read formatting for YAML posted by @mauritsvanrees:

if: github.event_name != 'pull_request' ||
  github.event.pull_request.head.repo.full_name !=
  github.event.pull_request.base.repo.full_name

aszlig added a commit to nixcloud/ip2unix that referenced this issue Aug 6, 2023
This is seems to be a common[1] issue[2] and there really is not a very
satisfactory solution other than adding an "if" on the job rather than
on the top-level workflow. Of course, there were other solutions listed,
but they'd involve managing a hardcoded list of branch names where the
"on push" action is triggered.

So I went for the workaround listed in [1], which adds an if to the job
specification.

[1]: zopefoundation/meta#145
[2]: open-telemetry/opentelemetry-python#1370

Signed-off-by: aszlig <aszlig@nix.build>
Constrat added a commit to MaaAssistantArknights/MaaAssistantArknights that referenced this issue Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants