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 Image Size to Playwright #40467

Merged
merged 3 commits into from
Apr 22, 2022

Conversation

juhi123
Copy link
Contributor

@juhi123 juhi123 commented Apr 20, 2022

What?

Migrate image-size.test.js to its Playwright version.

Why?

See #38570 for its background and rationale.

How?

See MIGRATION.md for migration steps.

Testing Instructions
Run npm run test-e2e:playwright test/e2e/specs/editor/plugins/image-size.spec.js

@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @juhi123! 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.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Apr 20, 2022
* @this {import('./').PageUtils}
* @param {string} slug Plugin slug.
*/
export async function activatePlugin( slug ) {
Copy link
Contributor

@talldan talldan Apr 21, 2022

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.

There are activatePlugin and deactivatePlugin utils for playwright already in the codebase (as part of RequestUtils), so these PageUtils ones shouldn't be needed:
https://github.com/WordPress/gutenberg/blob/trunk/packages/e2e-test-utils-playwright/src/request/plugins.ts

edit: I think that also means the other new utils won't be needed.

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

talldan commented Apr 21, 2022

This looks good, I think the only thing (other than the comment above) is that other migration PRs have deleted the old puppeteer test file and any snapshots as part of the migration. Maybe the migration steps need to be updated to clarify that.

@juhi123
Copy link
Contributor Author

juhi123 commented Apr 21, 2022

Hey @talldan 👋

Thanks for reviewing the PR. Yeah, you are right, how did I miss that? I have removed those and used the existing ones.

I am not sure about the other files you are talking about, I haven't deleted any. Can you please let me know, what do I need to do to fix that.

Thanks!

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 addressing the feedback.

Just to clarify that earlier comment - you can go ahead and delete the old test as part of this PR. This file can be removed:
https://github.com/WordPress/gutenberg/blob/trunk/packages/e2e-tests/specs/editor/plugins/image-size.test.js

Comment on lines 55 to 57
getCurrentUser = getCurrentUser;
switchUserToAdmin = switchUserToAdmin;
switchUserToTest = switchUserToTest;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like these three utils are now unused since the activatePlugin and deactivatePlugin utils were removed.

What do you think about removing these too until they're needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you are right, removed these

if ( ( await this.getCurrentUser() ) === WP_ADMIN_USER.username ) {
return;
}
await this.loginUser( WP_ADMIN_USER.username, WP_ADMIN_USER.password );
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't see a loginUser utility for playwright.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need this anymore

@juhi123
Copy link
Contributor Author

juhi123 commented Apr 22, 2022

@talldan
I have removed the unnecessary utils and the old test also. Hope now it's all good.
Thanks!

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.

Great, everything looks spot on now 👍

@talldan talldan merged commit dc1529a into WordPress:trunk Apr 22, 2022
@talldan
Copy link
Contributor

talldan commented Apr 22, 2022

Congrats on your first contribution 🎉

@github-actions github-actions bot added this to the Gutenberg 13.2 milestone Apr 22, 2022
@juhi123
Copy link
Contributor Author

juhi123 commented Apr 22, 2022

Thank you @talldan for quick reviews

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.
2 participants