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

build(cypress): Use pull_request_target event to run cypress #11750

Merged

Conversation

robdiciuccio
Copy link
Member

@robdiciuccio robdiciuccio commented Nov 19, 2020

SUMMARY

Updates Cypress e2e test Github Actions workflow to use the pull_request_target event in order to work around the secrets sharing limitation for forked repos. Also adds concurrency via multiple containers.

NOTE: the pull_request event trigger is required to get e2e tests passing in this PR, but will be removed in a subsequent PR in favor of pull_request_target.

Replaces #11730

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

e2e test workflow should run with a populated CYPRESS_RECORD_KEY value from the upstream repo's secrets.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov-io
Copy link

codecov-io commented Nov 19, 2020

Codecov Report

Merging #11750 (1bd3b80) into master (7dac150) will decrease coverage by 10.57%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #11750       +/-   ##
===========================================
- Coverage   63.81%   53.24%   -10.58%     
===========================================
  Files         956      438      -518     
  Lines       46845    15752    -31093     
  Branches     4590     4079      -511     
===========================================
- Hits        29895     8387    -21508     
+ Misses      16766     7365     -9401     
+ Partials      184        0      -184     
Flag Coverage Δ
cypress 53.24% <ø> (?)
javascript ?
python ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...uperset-frontend/src/dashboard/util/dnd-reorder.js 0.00% <0.00%> (-100.00%) ⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js 0.00% <0.00%> (-100.00%) ⬇️
...dashboard/components/resizable/ResizableHandle.jsx 0.00% <0.00%> (-100.00%) ⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js 0.00% <0.00%> (-93.48%) ⬇️
...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx 11.76% <0.00%> (-88.24%) ⬇️
...src/dashboard/components/gridComponents/Header.jsx 10.52% <0.00%> (-86.85%) ⬇️
...rc/dashboard/components/gridComponents/Divider.jsx 13.33% <0.00%> (-86.67%) ⬇️
...ontend/src/dashboard/util/getDashboardFilterKey.ts 14.28% <0.00%> (-85.72%) ⬇️
...end/src/SqlLab/components/ExploreResultsButton.jsx 8.00% <0.00%> (-84.00%) ⬇️
...nd/src/views/CRUD/data/query/QueryPreviewModal.tsx 14.70% <0.00%> (-82.97%) ⬇️
... and 867 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dac150...1bd3b80. Read the comment docs.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Dec 16, 2020
@robdiciuccio robdiciuccio force-pushed the rd/cypress-pull-request-target-v2 branch 2 times, most recently from 0b3a55a to f48400c Compare December 16, 2020 01:06
@robdiciuccio robdiciuccio force-pushed the rd/cypress-pull-request-target-v2 branch from f48400c to cc13b58 Compare December 16, 2020 01:08
@robdiciuccio robdiciuccio requested review from ktmud and nytai December 16, 2020 02:59
@robdiciuccio robdiciuccio marked this pull request as ready for review December 16, 2020 02:59
@robdiciuccio
Copy link
Member Author

robdiciuccio commented Dec 16, 2020

Cypress runs are not showing up in the Cypress dashboard for some reason. The push trigger was removed for PRs due to the duplicate runs occurring on the dashboard (for push and pull_request events), but I'm not seeing any pull_request triggered runs in the dashboard for the past several runs.

@ktmud
Copy link
Member

ktmud commented Dec 16, 2020

push run and pull_request run are different as push runs on the code in the branch, pull_request tests the code after merging with master. Normally this doesn't matter as most branches are up to date when the PR is created, but there is this difference so I just want to make sure we made a conscious decision here.

@robdiciuccio
Copy link
Member Author

Thanks for the clarification @ktmud. The pull_request triggered workflow runs are reported on PRs, making it a bit confusing (and likely unknown) that push workflow runs are also running in the background. Though, the Cypress dashboard reports mainly push triggered runs, with an occasional pull_request event. Unclear why this is.

Another strange thing, push-based workflows seem to be running concurrently, even though they should not(?) have access to secrets in the upstream repo.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Dec 16, 2020
@robdiciuccio
Copy link
Member Author

Restoring push on all branches to test if concurrency works and if it's reported in the Cypress dashboard.

@robdiciuccio
Copy link
Member Author

Confirmed: the push triggered tests are run in parallel.

@ktmud
Copy link
Member

ktmud commented Dec 16, 2020

I think the pull_request run (the one that is used in CI status reporting in PRs) still don't run in parallel, which means we probably have spun up two jobs unnecessarily.

image

It seems pull_request_target can only run in the background (like push) and can not be used to update CI status.

I'm wondering if we should just revert back to the push and pull_request, but 1). skip Container 2 and Container 3 if CYPRESS_RECORD_KEY is not accessible; 2) let committers know that if they create branches and make PRs from the apache repo, the CI will run faster.

@robdiciuccio
Copy link
Member Author

@ktmud correct, the pull_request runs are not running in parallel because they don't have access to the record key. The goal here is to merge this PR, then open a subsequent PR to remove the pull_request event in favor of pull_request_target, which should have access to the record key. If this plan doesn't work, we can revert this and try a different approach.

@ktmud
Copy link
Member

ktmud commented Dec 16, 2020

@ktmud correct, the pull_request runs are not running in parallel because they don't have access to the record key. The goal here is to merge this PR, then open a subsequent PR to remove the pull_request event in favor of pull_request_target, which should have access to the record key. If this plan doesn't work, we can revert this and try a different approach.

Makes sense!

@robdiciuccio
Copy link
Member Author

Here's how this worked in superset-ui: apache-superset/superset-ui#853

@robdiciuccio robdiciuccio merged commit 8fe9ee0 into apache:master Dec 16, 2020
@robdiciuccio robdiciuccio deleted the rd/cypress-pull-request-target-v2 branch December 16, 2020 18:45
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants