Run Extra Tests with only a label

[sarthakaggarwal97]

Change the behaviour of the CI job triggered by the run-extra-tests label.

Run the tests immediately when applying the run-extra-tests label to a PR, without requiring an extra commit to be pushed to trigger the test run.

When the extra tests have run, the job removes the label.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.46%. Comparing base (8ab0152) to head (6b3985d).
⚠️ Report is 13 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2907      +/-   ##
============================================
+ Coverage     72.28%   72.46%   +0.17%     
============================================
  Files           129      129              
  Lines         70540    70540              
============================================
+ Hits          50993    51116     +123     
+ Misses        19547    19424     -123     

see 13 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[roshkhatri]

LGTM, just added a thought/suggestion if we would like to add.

[zuiderkwast]

Let's merge this and test it on real PRs.

[madolson]

Let's merge this and test it on real PRs.

I don't like this change. Now we have to keep re-adding the run-extra tests label.

[zuiderkwast]

Let's merge this and test it on real PRs.

I don't like this change. Now we have to keep re-adding the run-extra tests label.

@madolson Yeah, this is not perfect. The good thing is that we don't have to push a fake commit to make it run the first time.

If the workflow doesn't delete the label, it will run again whenever we do some small change to the PR such as adding another label (release-notes, needs-doc-pr, etc.).

If we can make it run only when there is a code change, and not when a label is added/removed (except the run-extra-tests label), that would be better. @sarthakaggarwal97 Do you want to experiment with this more?

[sarthakaggarwal97]

@zuiderkwast I can take a stab at it next week.

To understand it clearly, we want to run the extra tests when

1. Either run-extra-tests is newly applied
2. If there is a code change and run-extra-tests is already there

Is that correct?

[zuiderkwast]

@sarthakaggarwal97 yes that'd be ideal

[zuiderkwast]

If we do like this, we don't have to remove the label after running.

on:
  pull_request:
    types: [ labeled, ... ]

jobs:
  test:
    if: ${{ github.event.action == 'labeled' && github.event.label.name == 'run-extra-test' }}
    ...

Idea from https://stackoverflow.com/a/62331521

We'd also not need to use pull_request_target. We can use pull_request instead which is more practical because we can modify/test the actual job in the same PR.

[zuiderkwast]

Suggested change

	(github.event_name == 'pull_request' && github.event.pull_request.base.ref != 'unstable') ||
	(github.event_name == 'pull_request_target' && github.event.label.name == 'run-extra-tests')) &&
	(github.event_name == 'pull_request' &&
	(github.event.pull_request.base.ref != 'unstable') ||
	(github.event.action == 'labeled' && github.event.label.name == 'run-extra-tests') ||
	(github.event.action != 'labeled' && contains(github.event.pull_request.labels.*.name, 'run-extra-tests'))) &&

Like this?

• The event is 'labeled' and the label is 'run-extra-tests', or
• The event is something else (e.g. push a commit) and the PR already has the label

[sarthakaggarwal97]

I think it makes sense. This could solve our problem. I will try this out soon. It's a little harder to test (few steps to test in forked repo) but I can come up with a change next week.

[zuiderkwast]

It's easier to test when we don't use pull_request_target anymore. It doesn't need to be merged for testing it.

We can test it using another label name, temporarily, to prevent the pull_request_target job in unstable from picking it up.

[sarthakaggarwal97]

It's easier to test when we don't use pull_request_target anymore.

Ah that's true yes.