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

test(e2e): Run required E2E tests on PRs from forks #12791

Merged
merged 2 commits into from
Jul 8, 2024

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jul 8, 2024

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

@Lms24 Lms24 self-assigned this Jul 8, 2024
@Lms24 Lms24 requested a review from mydea July 8, 2024 08:07
@Lms24 Lms24 mentioned this pull request Jul 8, 2024
@Lms24 Lms24 marked this pull request as ready for review July 8, 2024 12:19
@Lms24 Lms24 requested a review from lforst July 8, 2024 12:19
Comment on lines -917 to +916
(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')
Copy link
Member Author

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.

Copy link
Member

@mydea mydea left a 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!

@Lms24 Lms24 merged commit aded253 into develop Jul 8, 2024
119 of 120 checks passed
@Lms24 Lms24 deleted the lms/test-e2e-run-on-prs-from-forks branch July 8, 2024 13:56
@AbhiPrasad
Copy link
Member

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?

@Lms24
Copy link
Member Author

Lms24 commented Jul 9, 2024

Hmm then I think we need to adjust the guard for the optional e2e tests. Looking into it.

Update: Fixed via #12818

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants