Add path filter to workflows?

[garrettw]

Is your feature request related to a problem? Please describe.
It would be nice if the workflow that runs unit tests would only run if PHP or Composer files have changed in a given PR.

Describe the solution you'd like
I’d like to add path filters to the workflow to keep it from running if relevant files haven’t changed.

Describe alternatives you've considered
None

Additional context
Getting unit tests to run in parallel would help it to be less of a blocker, but it would still take time to run that might could be avoided.

[melroy89]

Unit tests are actually running within 5 seconds :).

It's the integration tests that take longer. However BentiGorlich recently combined the two into a single workflow: #1300 (comment).

But yes the integration test is running for like 22 mins. On Matrix chat we were already discussion to get it paratest working in the future. But again the issue is NOT unit tests, the issue is integration tests which takes 99% of the time.

There is often no easy way to determine whether or not you will need to run integration (or unit) test for a specific file changed. However, we could maybe come-up with a plan to skip the integration on PRs if the src directory is not changed. Something like that. And always run integration tests on the main branch.

[garrettw]

My mistake, I meant integration tests.
My thought was to only run it if src/ or composer.* changes.

[melroy89]

Yes makes sense to me. And just be sure always run it on the main branch as well?

[garrettw]

As opposed to … running it on PRs?
I like it running on PRs.

[melroy89]

Always run it on the main branch. And only run it on PRs in case of changes, is what I mean to say.

[garrettw]

Ah ok, I think I understand now. Apply path filters to PR runs only.

[garrettw]

I was just looking through the docs about path filters, and I found this:
https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#example-including-paths

If a workflow is skipped due to path filtering ... then checks associated with that workflow will remain in a "Pending" state. A pull request that requires those checks to be successful will be blocked from merging.

So for this to work, the automated-tests job would have to be set as not required. Hmm... thoughts?
(It would still run automatically, but it wouldn't block merging if it failed.)

The only way around this limitation would be to always run the job, but somehow make a path/branch filter inside the job that would just immediately pass the test if no relevant changes are found. I feel like this should be possible, but I'd have to do more research to find out how to do it.

[melroy89]

Split the workflow again like it was. Create a separate workflow for integration test. Making this workflow optional. While the unit test is always required.

[BentiGorlich]

I don't think that we should consider not marking the integration tests as required... If there is no way to do it, then I'd just not do it...

[melroy89]

I don't think that we should consider not marking the integration tests as required...

Double negative.. Which makes it very hard to understand it for me 😖 . You mean, you do want to keep integration tests required?

[garrettw]

Yes, I think that’s what he meant, and I can understand that position and I anticipated it. That’s why I mentioned a workaround.

[melroy89]

You can skip steps in a workflow yes. Using a if: https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/using-conditions-to-control-job-execution

[garrettw]

Oh, this is good info. Thanks, I’ll look over it.

[melroy89]

Great! Also.. you can use a previous step to determine whether or not to run the integration test and use for example "an output parameter": https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/workflow-commands-for-github-actions#setting-an-output-parameter.

When you then use in the follow-up if step to continue or skip the step.

[garrettw]

This is proving to be challenging 😅

I do have a question about how we want to handle this. Let me lay out a scenario:

• PR is created with commits that change PHP files
• Integration tests run, as they’re supposed to
• A new commit is pushed to the source branch that only contains changes to non-PHP files

Should we:

1. Run the integration tests again because PHP files are changed in another commit in the PR?
2. Skip the integration tests this time because the most recent push did not change PHP files?

[melroy89]

This is proving to be challenging 😅

I do have a question about how we want to handle this. Let me lay out a scenario:

* PR is created with commits that change PHP files

* Integration tests run, as they’re supposed to

* A new commit is pushed to the source branch that only contains changes to non-PHP files

Should we:

1. Run the integration tests again because PHP files are changed in another commit in the PR?

2. Skip the integration tests this time because the most recent push did not change PHP files?

My logical answer would be option 2.

Also because we will run integration tests again on the main branch as well, so we will detect it no matter what. But yea.. in theory any file change apart from PHP files could lead to failing tests of course. A simple package.json file update or any other config file.

[garrettw]

Of course this partly depends on what info is easiest to access about the event triggering the test run. I am debugging that on a different PR right now.
So far, I have found the following:

• When a PR is opened, the pull_request event is fired, github.head_ref contains the source branch name, and github.base_ref contains the target branch name. The SHA for each of those commits can be found in github.pull_request.base.sha and github.pull_request.head.sha.
  • Crucially, github.event.action == 'opened'
• When new commit(s) are pushed to an open PR, github.event_name is still pull_request but github.event.action == 'synchronize'. Also, the most recent commit SHA before the push can be found in github.event.before and the last commit SHA contained in the push is in github.event.after
• On a force-push, everything is identical to a normal push as above, BUT before and after are no longer useful because they reflect the head commits before and after the force push, which is not what we want to compare. So in this case, it should be treated like a new PR. The question is how to identify or differentiate a force-push from a regular push. (Here's the workflow run.) I don't see any way to do that.

[melroy89]

I see.. Hm, maybe we are making it unnecessary complex for us. What about: https://github.com/marketplace/actions/changed-files?

Example from above link:

on:
  pull_request:
    branches:
      - main

jobs:
  automated-tests:
    runs-on: ubuntu-latest
    name: Automated tests
    permissions:
      pull-requests: read

    steps:
      - name: Get changed files
        id: changed-files
        uses: tj-actions/changed-files@v45

That will show all changes.. So we can also check for only files (these are all PHP files anyway) that are changed in the src directory, using:

      - name: Get all src files that have changed
        id: changed-src-files
        uses: tj-actions/changed-files@v45
        with:
          files: |
            src/**

Then, the outputs we could leverage, could be: all_changed_files or .. modified_keys or.. all_changed_files_count.

A simple if check can be enough for the integration test (didn't test it):

      - name: Integration test
        if: steps.changed-src-files.outputs.all_changed_files_count != '0'
        [...]