Add/application password e2e tests by JustinyAhin · Pull Request #1746 · WordPress/wordpress-develop

JustinyAhin · 2021-10-11T06:41:20Z

Related Trac ticket: https://core.trac.wordpress.org/ticket/54241.

Test scenarios

Create a new application password

Given that a test user exists
When I go to Users > Profile, and I create a new application password
I see the new application password success message
And the application name appears in the app password table
And the application password information are present in the response of GET /wp/v2/users/<user_id>/application-passwords

Create an application password with an existing name

Given that a test user exists
And has an existing application password
When I go to Users > Profile, and I create a new application password with the same name
I should receive an error message, and the new application password should not be created

Revoke a single application password

Given that a test user exists
And has an existing application password
When I go to Users > Profile
And I click on the Revoke button
The application password should be revoked with a displayed success message
And the application password information should not be present in the response of GET /wp/v2/users/<user_id>/application-passwords

Bulk revoke applications passwords

Given that a test user exists
And has two existing applications passwords
When I go to Users > Profile
And I click on the Revoke all application passwords button
Then all applications passwords should be removed with a displayed success message
And the applications passwords information should not be present in the response of GET /wp/v2/users/<user_id>/application-passwords

hellofromtonya · 2021-10-12T11:35:14Z

@kevin940726 and @tellthemachines this PR is ready for your review. Once approved, I'll get it committed.

kevin940726

Nice work! Mostly looks good to me. Just some nitpickings and some practices we can discuss for our first e2e test. :)

kevin940726 · 2021-10-15T03:36:08Z

+    const testApplicationName = 'Test Application';
+
+    beforeEach(async() => {
+        await revokeAllApplicationPasswords();


While we do need a test to manually revoke all passwords, I don't think we have to do it for every test just to clear the states. Is it possible to call APIs to clear the states here instead of repeating this for every test? It could potentially speed up our tests too.

Yeah, interesting. It saves 1 to 1.5 seconds for the whole test.

How far we want to go with optimizations? I guess we could try the same thing with creating a password? As the creation feature is already tested.

Yeah, I'd say the rule of thumb is to avoid testing the same thing manually repeatedly when there are alternatives. E2E tests are already super slow by its nature, don't waste time on making it unnecessarily slower.

kevin940726 · 2021-10-15T03:41:09Z

+    await page.type('#new_application_password_name', `${applicationName}`);
+    await page.click('#do_new_application_password');
+
+    await page.waitForSelector('.notice');


Nit: It isn't really clear what notice we're waiting for here. What if there're some other notices happening at the same time? I don't really have the knowledge for this feature so I'm not sure if this is a legitimate concern though 😅.

Yeah, this makes sense. I've now added a new selector to all the waiting for notice message like this #application-passwords-section .notice.

There's something weird, though. When creating a new app password, the notice message has these class names notice notice-success is-dismissible new-application-password-notice.

But when I do await page.waitForSelector('.new-application-password-notice');, it results in a timeout error and the selector is not found 🤔. Do you have any ideas of where this could come from?

I haven't tested it myself, but you can debug it with the --puppeteer-interactive launch flag when running wp-scripts test-e2e. Not sure if it's implemented in core though?

You're using createApplicationPassword twice in the 'should not allows to create two applications passwords with the same name' test, but the second time around you're passing the same name, so it'll error. The '.new-application-password-notice' class is only present on the success notice, not the error one, so I'm guessing that's where the timeout is happening.

There are at least two ways we can approach this:

If createApplicationPassword is meant to successfully complete the password creation flow, then checking for the presence of '.new-application-password-notice' is the only meaningful option, because '#application-passwords-section .notice' will display in case of error too. But if this is just a helper to fill in the password creation form, then we needn't do any check in the function itself - we can instead do the most relevant check (for success or error) in the test itself, after running the function.

If we go with the first, then in the 'should not allows to create two applications passwords with the same name' test we should just repeat the password creation steps manually for the second time, instead of calling the function.

In terms of test readability, I'm a little more inclined towards the second option, but fine with either.

I just tested it and it works for successful creations of application passwords, but not for failures. That's because the failure notices don't have the new-application-password-notice class. #application-passwords-section .notice seems like a good enough solution for this 👍 .

@kevin940726 did you see my comment above? I'm not sure we should just let createApplicationPassword fail silently when the password isn't created for some reason.

Oops, nope! GitHub failed to update it when I was commenting 😅 .

Though I think it's not a problem, this function is just a utility helper used in this test, so I think it makes sense to customize it for the needs of the tests. This helper is just an intermediate step towards where the assertions happen, and the actual assertions are done in each test for both successful and failed results.

kevin940726 · 2021-10-15T06:08:42Z

Just found out there's a console error warning in the test output:

https://github.com/WordPress/wordpress-develop/pull/1746/checks?check_run_id=3902891514#step:13:17

I don't think it's a big deal, but would be great if we could get rid of it somehow.

JustinyAhin · 2021-10-15T06:37:54Z

Thanks for all the reviews @kevin940726. This is exactly the kind of feedback I was hoping :).

kevin940726 · 2021-10-19T03:57:53Z

One more thing I noticed, I think we should follow the coding style guide in tests as well? Not sure why eslint isn't picking those up. Might worth to fix that too in another PR.

tellthemachines

Thanks for working on this! I left a few comments below.

JustinyAhin · 2021-10-20T07:42:57Z

Just found out there's a console error warning in the test output:

@kevin940726 I haven't figured out yet where the console error came from. I remember though it started when making API calls.

kevin940726

I think this looks good! Great work! ♥️ 👍

Still some places not following the style guide though but we can tackle them in another PR (which should fix the eslint issue).

JustinyAhin · 2021-10-21T06:52:41Z

Still some places not following the style guide though but we can tackle them in another PR (which should fix the eslint issue).

What is needed for the ESLint PR @kevin940726?
I can have a look maybe

hellofromtonya · 2021-11-01T21:42:05Z

Committed via changeset https://core.trac.wordpress.org/changeset/51966.

Thank you everyone for your contribution 🎉

kevin940726 · 2021-11-02T01:58:15Z

What is needed for the ESLint PR @kevin940726? I can have a look maybe

Oops, totally missed this. We should configure ESLint so that it'll correctly use the @wordpress/eslint-plugin rulesets and lint the test files. I'm not sure if it's been done before in core though, but it's standard in gutenberg.

JustinyAhin added 4 commits October 8, 2021 15:12

Create applications-passwords.test.js

ad3cdd1

Merge branch 'master' into add/application-password-e2e-tests

2b9e8d0

Update applications-passwords.test.js

cca4f13

Merge branch 'master' into add/application-password-e2e-tests

7668f60

swissspidy reviewed Oct 11, 2021

View reviewed changes

Comment thread tests/e2e/specs/settings/applications-passwords.test.js Outdated

CS fix

b6bd5bc

hellofromtonya requested a review from tellthemachines October 12, 2021 11:26

juhi123 reviewed Oct 14, 2021

View reviewed changes

Comment thread tests/e2e/specs/settings/applications-passwords.test.js Outdated

JustinyAhin added 2 commits October 14, 2021 15:01

Update applications-passwords.test.js

c03bfe4

Merge branch 'master' into add/application-password-e2e-tests

f56135e

kevin940726 reviewed Oct 15, 2021

View reviewed changes

Update applications-passwords.test.js

e7827c8

JustinyAhin added 2 commits October 15, 2021 08:12

Be more specific for notices messages selection

4a10e1f

Update applications-passwords.test.js

41ffe84

Use regex to match string when creating an application password

850d472

kevin940726 reviewed Oct 19, 2021

View reviewed changes

Comment thread tests/e2e/specs/settings/applications-passwords.test.js Outdated

Comment thread tests/e2e/specs/settings/applications-passwords.test.js Outdated

Comment thread tests/e2e/specs/settings/applications-passwords.test.js Outdated

tellthemachines reviewed Oct 19, 2021

View reviewed changes

Comment thread tests/e2e/specs/settings/applications-passwords.test.js Outdated

Comment thread tests/e2e/specs/settings/applications-passwords.test.js Outdated

JustinyAhin added 6 commits October 19, 2021 09:06

Merge branch 'master' into add/application-password-e2e-tests

8e7e6e3

Rename the test folder from settings to profile

f0401cb

Typos fixes

2c5f26b

Use ALL CAPS formatting for constant names

991abd4

Update revokeAllApplicationPasswordsWithApi()

bb7eed6

Update applications-passwords.test.js

059d6d3

kevin940726 reviewed Oct 19, 2021

View reviewed changes

Comment thread tests/e2e/specs/profile/applications-passwords.test.js Outdated

Comment thread tests/e2e/specs/profile/applications-passwords.test.js Outdated

JustinyAhin added 2 commits October 19, 2021 11:42

Update applications-passwords.test.js

a14ebfc

Update applications-passwords.test.js

8bad9a3

JustinyAhin added 3 commits October 20, 2021 09:05

Update applications-passwords.test.js

0b957b9

CS fixes

442365f

Update applications-passwords.test.js

f6b9707

kevin940726 approved these changes Oct 21, 2021

View reviewed changes

kevin940726 mentioned this pull request Oct 21, 2021

Add a method to disable auto-accepting dialogs WordPress/gutenberg#35828

Merged

7 tasks

hellofromtonya closed this Nov 1, 2021

Conversation

JustinyAhin commented Oct 11, 2021

Test scenarios

Create a new application password

Create an application password with an existing name

Revoke a single application password

Bulk revoke applications passwords

Uh oh!

Uh oh!

hellofromtonya commented Oct 12, 2021

Uh oh!

Uh oh!

kevin940726 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevin940726 commented Oct 15, 2021

Uh oh!

JustinyAhin commented Oct 15, 2021

Uh oh!

kevin940726 commented Oct 19, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tellthemachines left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JustinyAhin commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevin940726 left a comment

Choose a reason for hiding this comment

Uh oh!

JustinyAhin commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hellofromtonya commented Nov 1, 2021

Uh oh!

kevin940726 commented Nov 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JustinyAhin commented Oct 20, 2021 •

edited

Loading

JustinyAhin commented Oct 21, 2021 •

edited

Loading