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 wp editor meta box test to Playwright #41519

Merged
merged 6 commits into from
Aug 11, 2022

Conversation

Rink9
Copy link
Contributor

@Rink9 Rink9 commented Jun 2, 2022

What?

Migrate wp-editor-meta-box.test.js to its Playwright version.

Why?

See #38570 for its background and rationale.

How?

See MIGRATION.md for migration steps.

Testing Instructions

npm run test-e2e:playwright test/e2e/specs/editor/plugins/wp-editor-meta-box.spec.js

Screencast

Screen.Recording.2022-06-03.at.12.48.47.AM.mov

@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, @Rink9! 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.

@Rink9 Rink9 changed the title add: playwright migration wp-editor-meta-box test Migrate wp editor meta box test to Playwright Jun 2, 2022
@Rink9
Copy link
Contributor Author

Rink9 commented Jun 3, 2022

@ntwb It seems CI are failing here can you please help me by reviewing this PR?

@Rink9
Copy link
Contributor Author

Rink9 commented Jun 27, 2022

Hi, @kevin940726 Can you please help me with reviewing this PR? It seems CI are failing here, what will be the nest step to move forward? Thanks :)

@kevin940726
Copy link
Member

Hi @Rink9!

You can click on the "Details" link next to the CI check to see exactly where it fails. For instance, clicking on the "Details" link of the "Static Analysis" check will show you that it fails on linting the JavaScript code. You can then go and fix that if it's related to your changes.

You can see the testing guideline for more detailed information. GLHF!

@Rink9
Copy link
Contributor Author

Rink9 commented Jun 28, 2022

@kevin940726
As I am currently noticing the static analysis CI that is failing is not related to the files I have changed. Feel free to give your feedback regarding this and let me know if I have anything more to do for this PR to get kindly reviewed by you.

image

@kevin940726
Copy link
Member

@Rink9 Those are only warnings and exist on trunk too so it's okay (recommended) to ignore them in this PR :)

The ones that fail the check are the errors in the report. You can scroll to the very bottom of the report and see that there's 1 error.

@Rink9 Rink9 force-pushed the add/playwright-wp-editor-meta-box-test branch from 6ff3df5 to 6c9d981 Compare July 1, 2022 18:31
@Rink9
Copy link
Contributor Author

Rink9 commented Jul 1, 2022

Hello @kevin940726, I have solved the problem with failing CI. Would you be able to take a look and review my PR? Please let me know if any further steps are necessary from my end.

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.

Hi! Have you read the migration guide and the best practices? It's recommended to follow the best practices while migrating the tests so that we can leverage the full power of Playwright. It means that in most cases you have to understand exactly what the tests do (or are trying to do) and modify them accordingly. The most common modifications involve changing to use role selectors and using the locator API. It could be quite difficult in some cases though, feel free to ping us if you have encountered any questions! ;)

@kevin940726 kevin940726 requested a review from talldan July 11, 2022 03:30
@Rink9 Rink9 force-pushed the add/playwright-wp-editor-meta-box-test branch from 97a7aab to 08dbdd8 Compare July 16, 2022 14:56
@Rink9
Copy link
Contributor Author

Rink9 commented Jul 16, 2022

Hi, @kevin940726 I addressed your feedbacks regarding role selectors. Can you please review that now are we good to go?

@Rink9
Copy link
Contributor Author

Rink9 commented Jul 21, 2022

@kevin940726 Sorry for pinging you again.... Any updates related to this PR?

@Rink9
Copy link
Contributor Author

Rink9 commented Jul 25, 2022

Hi @talldan Can you please take e look and review the PR?

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I left a couple of comments.

One other thing missing is that the old test file (https://github.com/WordPress/gutenberg/blob/trunk/packages/e2e-tests/specs/editor/plugins/wp-editor-meta-box.test.js) also needs to be deleted as part of this PR.

test/e2e/specs/editor/plugins/wp-editor-meta-box.spec.js Outdated Show resolved Hide resolved
test/e2e/specs/editor/plugins/wp-editor-meta-box.spec.js Outdated Show resolved Hide resolved
@Rink9
Copy link
Contributor Author

Rink9 commented Jul 26, 2022

Hi @talldan Thanks for your feedback! I have addressed your reviews. Can you please check once?

@Rink9
Copy link
Contributor Author

Rink9 commented Aug 8, 2022

Hi @talldan @kevin940726 If anyone can arrange some time, could you please kindly check this PR?

@talldan talldan added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Aug 8, 2022
@talldan
Copy link
Contributor

talldan commented Aug 8, 2022

I mentioned it in a DM to @Rink9, but mentioning it here as well for transparency.

I think there's still one thing to address on the PR - it looks like the old test file still needs to be deleted. Although I'm seeing it deleted in 3501674, I still can't see this change in the 'Files Changed' tab for the PR. Not sure why that would be, but hopefully it can be fixed.

@Rink9 Rink9 force-pushed the add/playwright-wp-editor-meta-box-test branch from 05916e9 to 3501674 Compare August 10, 2022 18:21
@Rink9
Copy link
Contributor Author

Rink9 commented Aug 10, 2022

Hi @talldan I just have fixed this file changed problem. Can you please take a look and share feedback?

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Thanks for all your work here! Congratulations on getting your first PR merged. 🎉

@talldan talldan merged commit 10af9ef into WordPress:trunk Aug 11, 2022
@github-actions github-actions bot added this to the Gutenberg 14.0 milestone Aug 11, 2022
@Rink9
Copy link
Contributor Author

Rink9 commented Aug 11, 2022

@talldan Thank you so much for your quick response. 😊

@Rink9 Rink9 deleted the add/playwright-wp-editor-meta-box-test branch August 11, 2022 03:23
@Rink9 Rink9 restored the add/playwright-wp-editor-meta-box-test branch August 11, 2022 07:47
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 [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.

3 participants