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

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

Merged
merged 20 commits into from
May 16, 2024
Merged

Conversation

Sahejkm
Copy link
Contributor

@Sahejkm Sahejkm commented May 16, 2024

Copy link

vercel bot commented May 16, 2024

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 16, 2024 4:41am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 16, 2024 4:41am

@facebook-github-bot 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 16, 2024
Copy link

github-actions bot commented May 16, 2024

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%) 483 ms (+20.44% 🔺) 962 ms
packages/lexical-rich-text/dist/LexicalRichText.js 34.56 KB (0%) 692 ms (0%) 1.3 s (+9.28% 🔺) 2 s
packages/lexical-plain-text/dist/LexicalPlainText.js 34.51 KB (0%) 691 ms (0%) 2.1 s (+90.02% 🔺) 2.8 s

@Sahejkm Sahejkm changed the title Add label e2e on approve [Lexical] Add label to run e2e tests on approve May 16, 2024
@github-actions github-actions bot added the extended-tests Run extended e2e tests on a PR label May 16, 2024
Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only concern is that we might want another tag for which this would not happen (e.g. an existing no-extended-tests label). We don't have anything like that yet, so I don't think it should be a blocker.

@Sahejkm Sahejkm added this pull request to the merge queue May 16, 2024
Merged via the queue into main with commit 5b0bb73 May 16, 2024
50 checks passed
Comment on lines +22 to +24
e2e-tests:
needs: label_pull_requests
uses: ./.github/workflows/call-e2e-all-tests.yml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right. Once the label is added, it should kick off the other tests-extended workflow automatically, as that already gets triggered when labels are added.

Copy link
Contributor Author

@Sahejkm Sahejkm May 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this and found out that 1 GitHub workflow action can't trigger another workflow action and hence this was needed, with a "needs" check , the second job won't run unless 1 job is not skipped. Without this tests don't trigger on approve unless there is a manual action, other way to fix this is use PAT tokens to trigger the 1st job to add labels,but I think we should still use the GitHub action bot.


concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cancel in progress?

cancel-in-progress: true

jobs:
label_pull_requests:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming format used for all other workflows is foo-bar. So this one should probably be called something like label-after-approved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, will update the naming conventions

@@ -0,0 +1,24 @@
name: Add Label to run extended tests on approve
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file name of this workflow should be 'add-label-after-approved'. This workflow shouldn't be running e2e tests, just adding a label

@necolas
Copy link
Contributor

necolas commented May 18, 2024

I think the workflow file merged in this PR needs revisiting

@etrepum
Copy link
Collaborator

etrepum commented May 18, 2024

It's too bad that GitHub doesn't really offer any way to simulate these things, even to see what a matrix would expand to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Add extended-tests label on approval
5 participants