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

Migrate Child Block Test to Playwright #55199

Merged

Conversation

pooja-muchandikar
Copy link
Contributor

What?

Part of #38851.
Migrate child-blocks.test.js to its Playwright version.

Why?

Part of #38851.

How?

See MIGRATION.md for migration steps.

Testing Instructions

Run npm run test:e2e:playwright test/e2e/specs/editor/plugins/child-blocks.spec.js

@pooja-muchandikar
Copy link
Contributor Author

Hi @kevin940726 @Mamaduka,

I hope you are doing great!

Could you help review this PR?

Thanks!

@Mamaduka Mamaduka added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Package] E2E Tests /packages/e2e-tests labels Oct 12, 2023
Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

Thank you, @pooja-muchandikar!

I think some of the locators can be improved. Especially ones that use classes instead of role-based selectors.

Recently, I did a similar migration (#51677); you can check the inner-blocks-allowed-blocks.spec.js file and use it as a blueprint.

@pooja-muchandikar
Copy link
Contributor Author

Hi @Mamaduka,

Thanks for sharing the feedbacks! All the feedbacks are addressed please review.

Thanks!

@Mamaduka
Copy link
Member

Thanks, @pooja-muchandikar!

While changes look good, the two tests now check a different thing. They should test a block appender based on the original tests. Let's ensure that migrate tests are still checking for the same thing.

Affected tests

  • shows up in a parent block
  • display in a parent block with allowedItems.

Block appender

CleanShot 2023-10-13 at 13 25 30

@Mamaduka
Copy link
Member

Mamaduka commented Nov 7, 2023

@pooja-muchandikar, let me know if you have any additional questions.

@pooja-muchandikar
Copy link
Contributor Author

Hi @Mamaduka,

I have addressed the feedback and pushed the changes. Please review and let me know if there are any further suggestions.

Thanks

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

I just ran the original test in headed mode and adjusted my suggestions. The tests should be passing now.

test/e2e/specs/editor/plugins/child-blocks.spec.js Outdated Show resolved Hide resolved
test/e2e/specs/editor/plugins/child-blocks.spec.js Outdated Show resolved Hide resolved
@pooja-muchandikar
Copy link
Contributor Author

Hi @Mamaduka

Feedbacks addressed.

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

Thank you, @pooja-muchandikar! One last nitpick and I think we're good to merge once CI checks are green.

test/e2e/specs/editor/plugins/child-blocks.spec.js Outdated Show resolved Hide resolved
@Mamaduka Mamaduka merged commit 38726ab into WordPress:trunk Nov 7, 2023
48 checks passed
@github-actions github-actions bot added this to the Gutenberg 17.1 milestone Nov 7, 2023
@pooja-muchandikar pooja-muchandikar deleted the migrate/child-block-test branch November 7, 2023 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] E2E Tests /packages/e2e-tests [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants