search

karma-runner / karma

Spectacular Test Runner for JavaScript
http://karma-runner.github.io
MIT License
11.96k stars 1.71k forks source link

Allow BS and SL Testing from Fork PRs #3786

Closed jginsburgn closed 2 years ago

jginsburgn commented 2 years ago

Did you allow access to GitHub secrets from forks somehow? Because otherwise, any PRs from forks will fail because they don't have access to the secrets.

You are absolutely right! I think we should, however, should have a mechanism for PRs from forks to test in SauceLabs and BrowserStack. I am going to move this discussion to a bug!

Originally posted by @jginsburgn in https://github.com/karma-runner/karma/issues/3779#issuecomment-1095965695

jginsburgn commented 2 years ago

I think that it important to fully test a PR before allowing it to be merged to master. For this reason, we should come up with a mechanism of allowing conducting all tests, including the ones that need credentials for 3P services (like SauceLabs and BrowserStack) from fork PRs, without openly revealing such credentials. An idea I have is to have a pre-master, which would be the base branch for fork PRs. It should be protected as the master branch, and once a merge is conducted, it should conduct all integration tests. If any fails, we should hard reset that pre-master branch and reopen the original PR displaying the failure. If they all pass, we should fast forward master to pre-master as a workflow.

@devoto13 thoughts?

@karma-runner/google-web-test-team

jginsburgn commented 2 years ago

BTW, this is also applicable in the following repos:

devoto13 commented 2 years ago

I'm not sure, it feels like it would complicate the workflow and create more friction for the external contributors. We're going to have two branches, shall we also have pre-beta branch? How can we enforce/communicate that external PRs should be opened against those branches?

IMO the current system is fine. If the PR looks risky from the cross-browser compatibility perspective, then the maintainer can always push the branch to the main repo and run BS/SauceLabs tests before merging. I think the effort would be pretty much the same.

I also think that the post-merge BS failures will become very rare if we manage to drop the IE support in the next major as all supported browsers will be evergreen.

Having said that I agree that it's nice to be able to run all tests (including 3P services) in all PRs, I just don't see how we can achieve that without introducing a lot of extra complexity.

jginsburgn commented 2 years ago

@devoto13

What you say is true. I agree.

If the PR looks risky from the cross-browser compatibility perspective, then the maintainer can always push the branch to the main repo and run BS/SauceLabs tests before merging. I think the effort would be pretty much the same.

Let's stick with this.