alisterscott commented 8 years ago

Find each xpath selector.
Add data-attribute to wp-calypso React code, adding a wp-calypso unit test for these so they don't get accidentally get removed in the future.
Replace the xpath with the CSS selector for the added data-attribute.

hoverduck commented 7 years ago

Xpath selectors are inefficient, fragile, and difficult to maintain. We should scrub the list of the ones we're using and link them to open issues in wp-calypso to have data attributes added that we can use instead.

performance

zhongruige-zz commented 7 years ago

I've started looking into this (removing xpaths is always a happy project), and I created a branch to work on, which you can see here.

Please let me know if you have any comments/suggestions. Thanks!

alisterscott commented 7 years ago

@zhongruige do you mind creating a pull request for your changes on your branch? It makes it easier to review/comment

zhongruige-zz commented 7 years ago

@alisterscott Sure thing! I'll go ahead and do that now.

zhongruige-zz commented 7 years ago

Hi there! I just wanted to give a quick update on where I am at with this.

There's still two pages I am not sure on where they point to, wp-admin-snippets-page, and the admin-jetpack-settings-page. The latter looks like it should be the settings page in wp-admin, but it doesn't seem to match up to the current UI I have in my account.

Overall: I am currently stuck on xpath selectors in the following files:

customer-page.js: one here and one here
themes-page.js: one here and one here
site-selector-component.js

There's also few that are external sites facebook-page as well as twitter-feed-page under that use xpath selectors. Being external sites there isn't much we can do about those, I think.

alisterscott commented 7 years ago

Thanks for the update @zhongruige

There's still two pages I am not sure on where they point to, wp-admin-snippets-page, and the admin-jetpack-settings-page. The latter looks like it should be the settings page in wp-admin, but it doesn't seem to match up to the current UI I have in my account.

These are for Jetpack e2e tests that run directly on a WordPress site (not Calypso), and we're moving away from that so don't worry about those two (or any wp-admin pages)

For clickPopoverItem( name ) { could you try re-writing the test slightly to avoid using that method?

For the other ones, is it possible to add data attributes to wp-calypso that may help (like you already did)? Otherwise re-write the tests, otherwise we can just leave them if it not possible to use css selectors.

alisterscott commented 7 years ago

Hi @zhongruige

The wp-admin pages have been removed in #519 and merged so you don't need to worry about these two

zhongruige-zz commented 7 years ago

Hi @alisterscott thank you for letting me know!

I also just deleted the branch and closed the pull request. I'll make smaller branches to address the xpath selectors, and in one of them I will look at rewriting the clickPopoverItem( name ) { method mentioned above. As part of it, I'll look into adding data attributes if possible to Calypso.

Thanks!

zhongruige-zz commented 7 years ago

For clickPopoverItem( name ) { could you try re-writing the test slightly to avoid using that method?

I was thinking about adding a data-e2e-label={ option.label } attribute to more-button.jsx in Calypso (around line 97), then create two new methods clickTryAndCustomize() and clickActivate() which would use the following CSS selectors, respectively: button[data-e2e-label="Try & Customize"] and button[data-e2e-label="Activate"]. Do you think that would work, or would you still like it rewritten?

I'd also like to add selectors for menuDisplayedAsPrimary( menuName ) in customizer-page.js but so far haven't been able to find where those are in Calypso.

Aside from those there are only a few left, and it seems the xpath selector will be better since it's looking specifically for certain text:

pluginTitledShown( pluginTitle, searchTerm ) within plugins-browser-page.js, since it's looking specifically for the plugin title.
view-post-page.js with the commentEventuallyShown( comment ) selector as it is looking for a specific comment
previewShowsWidget( widgetTitle, widgetContent ) in customizer-page.js as it looks for a specific widget title and content

Do you feel these should be rewritten or should we leave them as is for now?

Otherwise, the rest of the xpath selectors left are just from external sites :)

hoverduck commented 7 years ago

@zhongruige I like the idea of adding data attributes to the elements in Calypso wherever possible, but is there anything to key it on other than the label (there may not be)? I'd just much rather have something discrete that could work in multiple languages in case we ever expand this suite of tests to handle other locales. Specifically in the remaining cases you listed that may not be an option.

As for menuDisplayedAsPrimary, I'd say don't worry about that one for now. The customizer itself doesn't live entirely inside Calypso (yet), so that's probably why you can't find it.

zhongruige-zz commented 7 years ago

@hoverduck Best I could find was to use the label, and I did have my concerns with doing it that way since it is a string that can be localized.

Thank you for the explanation on menuDisplayedAsPrimary as well! Guess we can look at updating that selector once it is migrated into Calypso.

Automattic / wp-e2e-tests

Remove all xpath selectors #30

performance