Skip to content

"Multiple use" block validation logic improvement [with Playwright]#57576

Merged
getdave merged 10 commits into
WordPress:trunkfrom
fai-sal:fix/block-validation-on-multiple-use
Feb 9, 2024
Merged

"Multiple use" block validation logic improvement [with Playwright]#57576
getdave merged 10 commits into
WordPress:trunkfrom
fai-sal:fix/block-validation-on-multiple-use

Conversation

@fai-sal

@fai-sal fai-sal commented Jan 5, 2024 •
edited by getdave
Loading

Copy link
Copy Markdown
Contributor

Why

What

Co-authored-by: getdave get_dave@git.wordpress.org
Co-authored-by: Mamaduka mamaduka@git.wordpress.org
Co-authored-by: scruffian scruffian@git.wordpress.org

@Mamaduka Mamaduka added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Jan 5, 2024
Mamaduka
Mamaduka previously requested changes Jan 5, 2024
View reviewed changes

@Mamaduka Mamaduka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you, @fai-sal!

The correct directory for Playwright e2e tests is test/e2e/specs/editor/various.

I also would recommend checking the project e2e test best practices documentation - https://developer.wordpress.org/block-editor/contributors/code/testing-overview/e2e/#best-practices.

@fai-sal fai-sal requested a review from kevin940726 as a code owner January 5, 2024 08:22
getdave
getdave reviewed Jan 5, 2024
View reviewed changes

@getdave getdave left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Appreciate all your hard work here 🙇

The e2e tests will need to be rewritten to utilise Playwright APIs such as getByRole.

A lot of the await will be able to be removed as well. You should find it's a lot easier to author the tests as well.

Comment thread test/e2e/specs/editor/various/validate-multiple-use.spec.js Outdated
@fai-sal fai-sal requested review from Mamaduka and getdave January 10, 2024 17:36
getdave
getdave reviewed Jan 12, 2024
View reviewed changes

@getdave getdave left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for migrating to Playwright. A few comments about the test selectors 🙏

Comment thread test/e2e/specs/editor/various/validate-multiple-use.spec.js Outdated
Comment thread test/e2e/specs/editor/various/validate-multiple-use.spec.js Outdated
Comment thread test/e2e/specs/editor/various/validate-multiple-use.spec.js Outdated
Comment thread test/e2e/specs/editor/various/validate-multiple-use.spec.js Outdated
@fai-sal fai-sal requested a review from getdave January 13, 2024 07:53
@fai-sal

fai-sal commented Jan 25, 2024

Copy link
Copy Markdown
Contributor Author

Hi @getdave, If you can allocate some time, can you please review the PR again. Thanks 🙇

Mamaduka
Mamaduka reviewed Jan 25, 2024
View reviewed changes
Comment thread test/e2e/specs/editor/various/validate-multiple-use.spec.js Outdated
Mamaduka
Mamaduka reviewed Jan 25, 2024
View reviewed changes
Comment thread test/e2e/specs/editor/various/validate-multiple-use.spec.js Outdated
@fai-sal fai-sal requested a review from Mamaduka January 25, 2024 13:15
@getdave

getdave commented Jan 30, 2024

Copy link
Copy Markdown
Contributor

I think we should look to land this in time for WP 6.5. I've put it on the board for that project which should ensure a number of 👀 on it.

scruffian
scruffian reviewed Feb 9, 2024
View reviewed changes
Comment thread test/e2e/specs/editor/various/validate-multiple-use.spec.js Outdated
scruffian
scruffian reviewed Feb 9, 2024
View reviewed changes
Comment thread test/e2e/specs/editor/various/validate-multiple-use.spec.js Outdated
scruffian
scruffian reviewed Feb 9, 2024
View reviewed changes
Comment thread test/e2e/specs/editor/various/validate-multiple-use.spec.js Outdated
scruffian
scruffian approved these changes Feb 9, 2024
View reviewed changes

@scruffian scruffian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left a few nits but this looks good to me.

@getdave getdave mentioned this pull request Feb 9, 2024
Closed
@getdave getdave dismissed Mamaduka’s stale review February 9, 2024 13:38

Review points have been addressed

@getdave @scruffian
Apply comment wording improvements
8a3a1b6 
Co-authored-by: Ben Dwyer <ben@scruffian.com>
@github-actions

github-actions Bot commented Feb 9, 2024

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @ihdk.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: ihdk.

Co-authored-by: delowardev <delowardev@git.wordpress.org>
Co-authored-by: fai-sal <faisalahmed29@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: scruffian <scruffian@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

getdave
getdave approved these changes Feb 9, 2024
View reviewed changes

@getdave getdave left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's bring this in. Thank you for the persistence here 🙇

Comment thread test/e2e/specs/editor/various/validate-multiple-use.spec.js Outdated
@getdave getdave enabled auto-merge (squash) February 9, 2024 13:40
@getdave getdave merged commit 9ea082b into WordPress:trunk Feb 9, 2024
@github-actions github-actions Bot added this to the Gutenberg 17.7 milestone Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scruffian scruffian scruffian approved these changes
@getdave getdave getdave approved these changes
@ntwb ntwb Awaiting requested review from ntwb
@nerrad nerrad Awaiting requested review from nerrad
@ajitbohra ajitbohra Awaiting requested review from ajitbohra
@kevin940726 kevin940726 Awaiting requested review from kevin940726
@Mamaduka Mamaduka Awaiting requested review from Mamaduka

Assignees

@fai-sal fai-sal

Labels

[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.

Projects

None yet

Milestone

Gutenberg 17.7

Development

Successfully merging this pull request may close these issues.

Avoid multiple block adding while "multiple" is false in block.json

5 participants

@fai-sal @getdave @Mamaduka @scruffian @delowardev