-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
test(e2e): Run required E2E tests on PRs from forks #12791
Conversation
(github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) | ||
(needs.job_compile_bindings_profiling_node.result == 'success' || needs.job_compile_bindings_profiling_node.result == 'skipped') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the deleted check here is repeated in the optional e2e test step. So it should be fine to delete here as the "problematic" step is guarded already correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh that's right, good catch!
So it seems like dependabot PRs are failing: https://github.com/getsentry/sentry-javascript/actions/runs/9842683197/job/27172764177?pr=12800 I guess they need to be examined separately? |
Hmm then I think we need to adjust the guard for the optional e2e tests. Looking into it. Update: Fixed via #12818 |
This PR enables running E2E tests on PRs opened from forked repositories. Previously we excluded these because some E2E tests required secrets which are not available in the forks, causing e2e test runs to always fail. Now that we separated these tests from the e2e tests that don't require secrets (#12259) we should be good to re-enable the required e2e tests for external PRs.
For this to work though, I had to move the "Astro Cloudflare" test to optional tests since it requires a cloudflare token for deployment. IMO this is fine because the test doesn't have any other real tests anyway and we should create a dedicated Astro e2e test app with tests instead and add it to the required test apps.
Tested this with my forked repo and it seems like all required e2e tests are passing: #12794