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.

Sign up for GitHub

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

Jump to bottom

Migrate mentions tests to playwright #43064

Merged
merged 13 commits into from
Nov 4, 2022
Merged

Migrate mentions tests to playwright #43064

merged 13 commits into from
Nov 4, 2022

Conversation

juhi123
Copy link
Contributor

@juhi123 juhi123 commented Aug 8, 2022 •
edited
Loading

What?

Part of #38851. Migrate mentions.test.js to its Playwright version.

Why?

See this post for an overview of the migration.

How?

By following the migration guide.

Testing Instructions
Run npm run test-e2e:playwright test/e2e/specs/editor/various/mentions.spec.js

@juhi123 juhi123 changed the title Migrate mention tests to playwright Migrate mentions tests to playwright Aug 8, 2022
@juhi123
Copy link
Contributor Author

juhi123 commented Aug 8, 2022

FYI: First two test cases are passing, but the last one "Insert two subsequent mentions" is not passing.
Can anybody please help me fix that, I couldn't figure out why it wasn't working

talldan
talldan reviewed Aug 10, 2022
View reviewed changes
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.

test/e2e/specs/editor/various/mentions.spec.js Outdated Show resolved Hide resolved
test/e2e/specs/editor/various/mentions.spec.js Outdated Show resolved Hide resolved
test/e2e/specs/editor/various/mentions.spec.js Outdated Show resolved Hide resolved
@talldan talldan mentioned this pull request Aug 10, 2022
Merged
@juhi123
Copy link
Contributor Author

juhi123 commented Aug 30, 2022

Hey @talldan
Thanks for all your feedback, I have fixed all.
Please have a look

@juhi123
Copy link
Contributor Author

juhi123 commented Oct 28, 2022

@kevin940726 @JustinyAhin Can you please check if it looks good to you ?

@kevin940726 kevin940726 mentioned this pull request Oct 28, 2022
Closed
kevin940726
kevin940726 reviewed Oct 28, 2022
View reviewed changes
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.

Sorry for the late reviews! Thanks for your patience!

package-lock.json Outdated
@@ -18668,7 +18668,7 @@
"app-root-dir": {
"version": "1.0.2",
"resolved": "https://registry.npmjs.org/app-root-dir/-/app-root-dir-1.0.2.tgz",
"integrity": "sha512-jlpIfsOoNoafl92Sz//64uQHGSyMrD2vYG5d8o2a4qGvyNCvXur7bzIsWtAC/6flI2RYAp3kv8rsfBtaLm7w0g==",
"integrity": "sha1-OBh+wt6nV3//Az/8sSFyaS/24Rg=",
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this file is changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't made any changes to these files 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it could be like that sometimes after running npm install. You can just revert the changes here by running git checkout trunk -- package-lock.json or something similar.

packages/data/tsconfig.json Outdated Show resolved Hide resolved
packages/e2e-test-utils-playwright/src/editor/click-block-appender.ts Outdated Show resolved Hide resolved
test/e2e/specs/editor/various/mentions.spec.js Outdated Show resolved Hide resolved
test/e2e/specs/editor/various/mentions.spec.js Outdated Show resolved Hide resolved
test/e2e/specs/editor/various/mentions.spec.js Outdated Show resolved Hide resolved
test/e2e/specs/editor/various/mentions.spec.js Outdated Show resolved Hide resolved
test/e2e/specs/editor/various/mentions.spec.js Outdated Show resolved Hide resolved
test/e2e/specs/editor/various/mentions.spec.js Outdated Show resolved Hide resolved
test/e2e/specs/editor/various/mentions.spec.js Outdated Show resolved Hide resolved
@juhi123
Copy link
Contributor Author

juhi123 commented Nov 2, 2022

Hey @kevin940726
I have addressed all the feedbacks, please have a look when time permits :)

@kevin940726
Copy link
Member

Hi there! I don't think all the feedback has been addressed yet 😅 . Would you mind checking it again?

@juhi123
Copy link
Contributor Author

juhi123 commented Nov 3, 2022

@kevin940726 Yeah you are right :D it got missed somehow. Addressed now, the only one thing I am not really sure, what do I need to do to fix that, left comment there.

kevin940726
kevin940726 reviewed Nov 3, 2022
View reviewed changes
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.

Looks good to me! Just some accidentally changed files left 💯

packages/e2e-test-utils-playwright/src/request-utils/index.ts Outdated Show resolved Hide resolved
@juhi123
Copy link
Contributor Author

juhi123 commented Nov 3, 2022

@kevin940726 It should be fine now, I hope.

@kevin940726
Copy link
Member

There are still changes in the package-lock.json that are unrelated to this PR. Once they're reverted, then I think this would be ready to ship!

@juhi123 juhi123 force-pushed the add/playwright-mention-test branch 3 times, most recently from dedeb79 to 712a223 Compare November 4, 2022 04:17
Juhi Saxena and others added 5 commits November 4, 2022 09:56
Fix changes
345155f
Fix remaininging changes
39e094e
@juhi123
Fix tsconfig
89f19d4 
Somehow code editor was applying formatting everytime after commit
@juhi123
Fix formatting
5dbf4e2
@juhi123
Fix formatting
46553c8
@juhi123
Copy link
Contributor Author

juhi123 commented Nov 4, 2022

@kevin940726 Thanks for your patience. Now I can see only 2 files changed, so it should be good now 🤞

kevin940726
kevin940726 approved these changes Nov 4, 2022
View reviewed changes
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 a lot! 🙇

@kevin940726 kevin940726 merged commit 20e96b2 into WordPress:trunk Nov 4, 2022
@github-actions github-actions bot added this to the Gutenberg 14.6 milestone Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@talldan talldan talldan left review comments

@kevin940726 kevin940726 kevin940726 approved these changes

@ntwb ntwb Awaiting requested review from ntwb ntwb is a code owner

@nerrad nerrad Awaiting requested review from nerrad nerrad is a code owner

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Gutenberg 14.6
Development

Successfully merging this pull request may close these issues.
3 participants
@juhi123 @kevin940726 @talldan