Only run CI e2e tests on approved PRs #6080

necolas · 2024-05-10T21:28:33Z

Avoid running the entire suite of (expensive) e2e tests on every PR that is opened or updated. Only run if PR is approved.

Reference: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-a-workflow-when-a-pull-request-is-approved

vercel · 2024-05-10T21:28:36Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 12, 2024 10:18pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 12, 2024 10:18pm

etrepum · 2024-05-10T21:34:18Z

It might be nicer to move the e2e tests to a separate workflow file, and maybe also do a single run of e2e tests in the test.yml workflow and a more comprehensive matrix on approval

necolas · 2024-05-10T21:45:37Z

Yes, I floated some similar ideas as well. At the moment it's assumed that all the e2e tests are being run every time a PR is updated, and every time a merge to main happens. Lexical is way over its CI budget. As a first step, I'd like to validate that a relatively small and quick change can demonstrate reduction in CI costs. If we can do that, I think further organization and optimization sounds great.

etrepum · 2024-05-10T21:55:40Z

I suggested the separate workflow file because it would be a smaller logical change since the approval stuff could be floated out to one place instead of N different places

etrepum · 2024-05-10T21:57:52Z

but maybe my understanding of the schema is a bit off, I haven't tried to do it. looks good either way!

necolas · 2024-05-10T22:00:32Z

Haven't seen this before; cool feature.

It looks like some open PRs haven't run the e2e tests. Hard to tell why some open PR have or haven't run all tests.

I suggested the separate workflow file because it would be a smaller logical change since the approval stuff could be floated out to one place instead of N different places

For sure. Same goes for the github.repository_owner == 'facebook' condition

acywatson · 2024-05-10T22:00:36Z

I agree with this in spirit, but in practice it's gonna slow us down significantly to have to get a stamp before we can see the CI run outcome.

Is there a way to make an exception for codeowners or specific users? (i.e., something like: if it's a maintainer, run on submit, otherwise, run on approve.)

acywatson · 2024-05-10T22:01:07Z

It looks like some open PRs haven't run the e2e tests. Hard to tell why some open PR have or haven't run all tests.

I believe this is because of the budget issue

etrepum · 2024-05-10T22:11:28Z

I think what would be budget and velocity friendly would be to run a very limited matrix of e2e before approval (but not in draft), e.g. mac chromium or whatever is cheapest, and then do the full suite later on approval or some other condition that doesn't happen on every commit for every contributor. If there was some parameters around how much these things cost and what the budget are the scope could be widened a bit to cover more combinations (e.g. mac+chromium and mac+chromium+collab). We could even consider ditching some things like legacy events altogether unless that's still important enough to worry about.

Breaking the build merely by contributing too much isn't something I anticipated 😆

necolas · 2024-05-10T22:34:16Z

Is there a way to make an exception for codeowners or specific users?

There is a way to do that. I looked through recent commits and PRs, and it looks like most of them are created by Meta employees or Lexical collaborators - it would limit the budget impact to exclude maintainers.

I think what @etrepum suggested could be a good balance, e.g., you decide one or two e2e suites to run always, and the rest are deferred until PR approval. Later we could try to exclude more PRs (like this one) from running e2e tests at all; that would help allocate more budget to the PRs that benefit most.

acywatson · 2024-05-10T22:35:48Z

I think what @etrepum suggested could be a good balance

Yea, this is a good idea.

necolas · 2024-05-10T22:38:50Z

Once the existing CI workflow is back up and running, I'm happy to keep iterating on feedback until you're happy it doesn't negatively impact development.

.github/workflows/test.yml

github-actions · 2024-05-10T22:42:36Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
packages/lexical/dist/Lexical.js	23.93 KB (0%)	479 ms (0%)	344 ms (+27.46% 🔺)	823 ms
packages/lexical-rich-text/dist/LexicalRichText.js	34.56 KB (0%)	692 ms (0%)	1.2 s (+18.6% 🔺)	1.9 s
packages/lexical-plain-text/dist/LexicalPlainText.js	34.51 KB (0%)	691 ms (0%)	1.1 s (+5.2% 🔺)	1.8 s

necolas · 2024-05-12T02:37:59Z

I mocked the e2e test workflow to make iterating / reviewing faster and cheaper.

PR skips extended tests by default:

Added ~~approved~~ "extended-tests" label (name TBD) and extended tests run:

Removed label and updated diff, extended tests don't run:

etrepum

This looks like a great approach to me, I think it makes sense (as a follow-up) to add a workflow that automatically adds the label so reviewers don't have to remember to do a second action after approving a PR

etrepum · 2024-05-12T15:30:36Z

.github/workflows/tests-extended.yml

+
+jobs:
+  e2e-tests:
+    if: github.repository_owner == 'facebook' && contains(github.event.pull_request.labels.*.name, 'approved')


To bike-shed slightly on the name I would recommend changing the allow label to something like 'extended-tests' or 'e2e-tests' instead of 'approved'. My reasoning behind this is:

We may also want to have a deny label like 'no-extended-tests' or 'no-e2e-tests' for PRs that a reviewer knows won't benefit from the full suite

A contributor with sufficient privileges working on something that they know would benefit from the full suite could self-label without looking like they are rubber-stamping their own PR

Thanks. Renamed to 'extended-tests'.

An alternative / addition to a "deny label" would be to expand on the paths-ignore to help automatically exclude PRs that only change files like READMEs, etc., which don't need functional testing.

I was also looking at the patch Dominic introduced to run e2e tests with beforeinput disabled - those of you more familiar with the status of "legacy events" will be better placed to determine if they still need as much testing.

etrepum · 2024-05-12T15:31:26Z

.github/workflows/call-e2e-test.yml

+        with:
+          node-version: ${{ matrix.node-version }}
+          cache: npm
+      - name: Mock e2e test


We of course need to un-mock before merge but great idea to make this quick, cheap and easy to verify!

Avoid running the entire suite of (expensive) e2e tests on every PR that is opened or updated. Only run if PR is approved. Reference: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-a-workflow-when-a-pull-request-is-approved

acywatson

I think it makes sense (as a follow-up) to add a workflow that automatically adds the label so reviewers don't have to remember to do a second action after approving a PR

Makes sense. This lgtm now.

Thanks for working on this, @necolas

acywatson · 2024-05-12T21:45:46Z

We're waiting on a commit to unmock before we merge, right?

necolas · 2024-05-12T22:22:45Z

We're waiting on a commit to unmock before we merge, right?

Mock removed.

Thanks for working on this, @necolas

No worries. The Lexical OSS project has some cool features I learnt about too. Very impressed with what you've all done here

Sahejkm

LGTM, thanks @necolas for optimising the workflows for budget control.

necolas requested review from zurfyx, fantactuka, acywatson, Fetz and ivailop7 as code owners May 10, 2024 21:28

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 10, 2024

vercel bot deployed to Preview – lexical May 10, 2024 21:28 View deployment

vercel bot deployed to Preview – lexical-playground May 10, 2024 21:29 View deployment

ivailop7 previously approved these changes May 10, 2024

View reviewed changes

necolas dismissed ivailop7’s stale review via acb1f54 May 10, 2024 21:58

necolas force-pushed the ci-workflow-e2e-on-approval branch from 5ff4054 to acb1f54 Compare May 10, 2024 21:58

vercel bot deployed to Preview – lexical May 10, 2024 21:59 View deployment

vercel bot deployed to Preview – lexical-playground May 10, 2024 21:59 View deployment

necolas force-pushed the ci-workflow-e2e-on-approval branch from acb1f54 to 3faf0bf Compare May 10, 2024 22:40

vercel bot deployed to Preview – lexical May 10, 2024 22:41 View deployment

vercel bot deployed to Preview – lexical-playground May 10, 2024 22:41 View deployment

necolas commented May 10, 2024

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

necolas removed the extended-tests Run extended e2e tests on a PR label May 12, 2024

necolas force-pushed the ci-workflow-e2e-on-approval branch from 4c16520 to 0caf03e Compare May 12, 2024 02:36

vercel bot deployed to Preview – lexical May 12, 2024 02:37 View deployment

vercel bot deployed to Preview – lexical-playground May 12, 2024 02:37 View deployment

etrepum reviewed May 12, 2024

View reviewed changes

necolas force-pushed the ci-workflow-e2e-on-approval branch from 0caf03e to 58920a3 Compare May 12, 2024 20:06

vercel bot deployed to Preview – lexical May 12, 2024 20:06 View deployment

vercel bot deployed to Preview – lexical-playground May 12, 2024 20:07 View deployment

acywatson previously approved these changes May 12, 2024

View reviewed changes

necolas dismissed acywatson’s stale review via 93a58c3 May 12, 2024 22:17

necolas force-pushed the ci-workflow-e2e-on-approval branch from 58920a3 to 93a58c3 Compare May 12, 2024 22:17

vercel bot deployed to Preview – lexical May 12, 2024 22:18 View deployment

vercel bot deployed to Preview – lexical-playground May 12, 2024 22:18 View deployment

etrepum mentioned this pull request May 12, 2024

[lexical-react] Feature: React 19 unit tests #6048

Merged

Sahejkm approved these changes May 13, 2024

View reviewed changes

acywatson added the extended-tests Run extended e2e tests on a PR label May 13, 2024

StyleT approved these changes May 13, 2024

View reviewed changes

acywatson merged commit 32279f4 into main May 13, 2024
47 checks passed

necolas deleted the ci-workflow-e2e-on-approval branch May 13, 2024 16:11

etrepum mentioned this pull request May 13, 2024

Feature: Add extended-tests label on approval #6097

Closed

This was referenced May 16, 2024

[Lexical] Add label and run extended test on approve #6117

Closed

[Lexical ]Add label and run extended test on approve #6118

Closed

[Lexical] Add label and run extended test on approve #6119

Closed

[Lexical] Add label to run e2e tests on approve #6120

Merged

This was referenced Jun 26, 2024

[Snyk] Upgrade @lexical/react from 0.15.0 to 0.16.0 phazingazrael/Terra-Logger#30

Open

[Snyk] Upgrade lexical from 0.15.0 to 0.16.0 phazingazrael/Terra-Logger#31

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only run CI e2e tests on approved PRs #6080

Only run CI e2e tests on approved PRs #6080

necolas commented May 10, 2024

vercel bot commented May 10, 2024 •

edited

Loading

etrepum commented May 10, 2024

necolas commented May 10, 2024 •

edited

Loading

etrepum commented May 10, 2024

etrepum commented May 10, 2024

necolas commented May 10, 2024 •

edited

Loading

acywatson commented May 10, 2024

acywatson commented May 10, 2024

etrepum commented May 10, 2024

necolas commented May 10, 2024

acywatson commented May 10, 2024

necolas commented May 10, 2024

github-actions bot commented May 10, 2024 •

edited

Loading

necolas commented May 12, 2024 •

edited

Loading

etrepum left a comment

etrepum May 12, 2024

necolas May 12, 2024

etrepum May 12, 2024

acywatson left a comment

acywatson commented May 12, 2024

necolas commented May 12, 2024

Sahejkm left a comment

Only run CI e2e tests on approved PRs #6080

Only run CI e2e tests on approved PRs #6080

Conversation

necolas commented May 10, 2024

vercel bot commented May 10, 2024 • edited Loading

etrepum commented May 10, 2024

necolas commented May 10, 2024 • edited Loading

etrepum commented May 10, 2024

etrepum commented May 10, 2024

necolas commented May 10, 2024 • edited Loading

acywatson commented May 10, 2024

acywatson commented May 10, 2024

etrepum commented May 10, 2024

necolas commented May 10, 2024

acywatson commented May 10, 2024

necolas commented May 10, 2024

github-actions bot commented May 10, 2024 • edited Loading

size-limit report 📦

necolas commented May 12, 2024 • edited Loading

etrepum left a comment

Choose a reason for hiding this comment

etrepum May 12, 2024

Choose a reason for hiding this comment

necolas May 12, 2024

Choose a reason for hiding this comment

etrepum May 12, 2024

Choose a reason for hiding this comment

acywatson left a comment

Choose a reason for hiding this comment

acywatson commented May 12, 2024

necolas commented May 12, 2024

Sahejkm left a comment

Choose a reason for hiding this comment

vercel bot commented May 10, 2024 •

edited

Loading

necolas commented May 10, 2024 •

edited

Loading

necolas commented May 10, 2024 •

edited

Loading

github-actions bot commented May 10, 2024 •

edited

Loading

necolas commented May 12, 2024 •

edited

Loading