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

Run browser tests on forked PRs by a dedicated label #4616

Merged
merged 4 commits into from
Apr 4, 2021

Conversation

outsideris
Copy link
Contributor

Description of the Change

It is the only way I found to run browser tests on forked PR.

After reviewing code of a forked PR, maintainers can attach a run-browser-test label.
Maintainers should check there is no malicious code to steal our secrets or do something bad on our repository.

This Browser Tests workflow will run based on forked PRs with writing repo and access secrets permissions. So, it can be dangerous if the forked PR has malicious code.

I've tested pull_request_target event.

스크린샷 2021-03-31 오후 5 48 07

When users open a pull request, push, pull_reqeust and pull_request_target will be run automatically. But pull_request_target: types: [labeled](in this case Browser Tests) will not be run because labeled type.

스크린샷 2021-03-31 오후 5 48 54

If maintainers attach a label except run-browser-test label, Browser Tests workflow will be triggered, but it skipped because the label is not run-browser-test. In the above screenshot, pull request safe run workflow is pull_request_target workflow with labeled type.

스크린샷 2021-03-31 오후 5 49 32

When maintainers attach the run-browser-test label, Browser Tests workflow will be triggered. In above screenshot, I tested it with safe-to-run label.

스크린샷 2021-03-31 오후 5 50 51

After that, when author of the forked PR pushed new commits, only default workflows will. be run.

That means maintainers should remove and re-attache run-browser-test label to run Browser Tests again.

And when the forked PR already has run-browser-test label, Browser Tests workflow will be triggered if maintainers attach a new label except run-browser-test label. We have to be careful with this.

Alternate Designs

We can use workflow_dispatch event. workflow_dispatch is a workflow that we should trigger it manually with PR number.

I think there are some cons.

  • Process is not convenient. maintainers enter PR number and trigger the workflow manually.
  • It couldn't be integrated with PR. So, we cannot the result of the workflow in GitHub's checks tab. It is hard to know whether browser tests are successful or fail.

Therefore, I think pull_request_target event with labeled type is better than workflow_dispatch event.

Why should this be in core?

All forked PRs failed because of browser tests.

Applicable issues

close #4553

Signed-off-by: Outsider <outsideris@gmail.com>
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris outsideris added the qa label Mar 31, 2021
@coveralls
Copy link

coveralls commented Mar 31, 2021

Coverage Status

Coverage remained the same at 94.201% when pulling af8c9c6 on outsideris:forked-pr-browser-test into d7ed5c2 on mochajs:master.

@juergba
Copy link
Contributor

juergba commented Mar 31, 2021

When users open a PR, push, pull_request and pull_request_target will be run automatically.

There shouldn't be any push event for users = forked PRs, right?

Your solution is awkward, but I don't see any alternative way either...

You could remove the run-browser-test label automatically after browser test's completion.
There is a GH action re-run-workflow which:
once-label: When this label is added to a pull request, re-run the workflow once and remove the label again.
What do you think about above GH re-run-workflow?
Or any other app which removes labels (search the marketplace with "remove labels").

@outsideris
Copy link
Contributor Author

There shouldn't be any push event for users = forked PRs, right?

No, it shouldn't be triggered by forked PRs. I just mentioned push, pull_request and pull_request_target because I want to explain normal workflows like our smoke, lint and tests.

You could remove the run-browser-test label automatically after browser test's completion.

That's a good idea. I will try it.

Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris
Copy link
Contributor Author

I used Add Remove Label actions.

It will remove run-browser-test label after browser test workflow whether it succeeds or fails.

And I will more test with real PRs after merging this because there are some limitations outside of this repository.

@outsideris outsideris requested a review from a team April 3, 2021 12:16
Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some small comments only, lgtm.

.github/workflows/browser-test.yml Outdated Show resolved Hide resolved
.github/workflows/browser-test.yml Outdated Show resolved Hide resolved
Signed-off-by: Outsider <outsideris@gmail.com>
@outsideris
Copy link
Contributor Author

fixed.

I will merge this to test with real forked PRs.

@outsideris outsideris merged commit 34643e4 into mochajs:master Apr 4, 2021
@juergba juergba added the semver-patch implementation requires increase of "patch" version number; "bug fixes" label Apr 5, 2021
@juergba juergba added this to the next milestone Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Github actions: browser tests fail for PR's raised from forked repos
3 participants