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 rtl test case to it's playwright version #41495

Merged
merged 12 commits into from
Jun 27, 2022

Conversation

pavanpatil1
Copy link
Contributor

What?

Part of #38851.
Migrate rtl-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/various/rtl.spec.js

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jun 2, 2022
@github-actions
Copy link

github-actions bot commented Jun 2, 2022

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @pavanpatil1! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@pavanpatil1
Copy link
Contributor Author

Hi @kevin940726, Hope you're doing well. Could you please help me with reviewing this PR?.

All test cases are passed. But seems like one End-to-End Tests / Admin - 3 CI check is failing. The error message which is shown is not related to test case which I worked upon. the error is on the test case which I haven't modified. Can you take a look at it?

image

Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

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

Nice job! Mostly looks good to me 👍

Yeah, I think the e2e failure is unrelated to this PR.

test/e2e/specs/editor/various/rtl.spec.js Outdated Show resolved Hide resolved
test/e2e/specs/editor/various/rtl.spec.js Outdated Show resolved Hide resolved
editor,
page,
pageUtils,
} ) => {
await page.keyboard.press( 'Enter' );

// Wait for rich text editor to load.
await page.waitForSelector( '.block-editor-rich-text__editable' );
Copy link
Member

Choose a reason for hiding this comment

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

Could we change this selector to something accessible? For instance, selecting it using a role selector, or replacing it along with the press( 'Enter' ) above with page.click( 'role=button[name="Add default block"i]' ).

Copy link
Member

Choose a reason for hiding this comment

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

We still need to refactor this line to use role selectors instead though. Or maybe just remove this line if it's not doing anything meaningful now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @kevin940726, We have refactored that line and Added the role selector

page.click( 'role=button[name="Add default block"i]' );
. We can't remove that line I guess.

Copy link
Member

Choose a reason for hiding this comment

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

I mean to remove this line.

await page.waitForSelector( '.block-editor-rich-text__editable' );

I don't think we need it anymore?

@pavanpatil1
Copy link
Contributor Author

Hi @kevin940726, I have addressed all the above feedback. Could you please review it again ?.

Thanks!.

@kevin940726
Copy link
Member

Have you seen this comment and my latest review? :)

This is very close to being merged! Thanks for the hard work!

@pavanpatil1
Copy link
Contributor Author

Hi @kevin940726,

#41495 (comment) comment is solved here

page.click( 'role=button[name="Add default block"i]' );
. It is not showing updated due to the selected string was different.

Could you please review it again?..

page,
pageUtils,
} ) => {
page.click( 'role=button[name="Add default block"i]' );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
page.click( 'role=button[name="Add default block"i]' );
await page.click( 'role=button[name="Add default block"i]' );

@pavanpatil1
Copy link
Contributor Author

Hi @kevin940726, Above feedbacks are addressed. Could you check once ?.

@kevin940726
Copy link
Member

Have you seen #41495 (comment) ? GitHub makes it less discoverable 😅

@pavanpatil1
Copy link
Contributor Author

Hi @kevin940726, All feedbacks are addressed could you review it again ?.

Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@kevin940726 kevin940726 merged commit 1a6d79c into WordPress:trunk Jun 27, 2022
@github-actions github-actions bot added this to the Gutenberg 13.6 milestone Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants