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

Pinterest Block: Add an e2e test. #14096

Merged
merged 15 commits into from
Dec 2, 2019
Merged

Pinterest Block: Add an e2e test. #14096

merged 15 commits into from
Dec 2, 2019

Conversation

pento
Copy link
Contributor

@pento pento commented Nov 22, 2019

Follow up to #13905.

This PR adds an e2e test, ensuring that the Pinterest block remains usable in the future.

It also tidies up some bugs in the existing block e2e tests, which were getting in the way of a clean test run.

Changes proposed in this Pull Request:

Adds an e2e test.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing improvement.

Testing instructions:

N/A, since e2e tests only run on Travis.

Proposed changelog entry for your changes:

N/A

@pento pento added this to the 8.0 milestone Nov 22, 2019
@pento pento requested a review from a team November 22, 2019 00:18
@pento pento self-assigned this Nov 22, 2019
@jetpackbot
Copy link

jetpackbot commented Nov 22, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: December 3, 2019.
Scheduled code freeze: November 26, 2019

Generated by 🚫 dangerJS against 4476621

@pento pento added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Nov 22, 2019
@pento
Copy link
Contributor Author

pento commented Nov 22, 2019

I couldn't see why the WordAds block was failing, and had enough of debugging on Travis.

It'd be nice if #12842 landed, to make these things a bit easier.

@jeherve jeherve requested a review from brbrr November 22, 2019 14:14
@@ -54,8 +54,8 @@ export async function waitForSelector( page, selector, options = {} ) {
* @param {Object} options Custom options to modify function behavior.
*/
export async function waitAndClick( page, selector, options = { visible: true } ) {
await waitForSelector( page, selector, options );
return await page.click( selector, options );
const element = await waitForSelector( page, selector, options );
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 changed this to use the element returned by waitForSelector(), as I'd originally modified waitForSelector() to accept an XPath selector, too (9f62cb1). That's since been reverted, I have no strong feelings either way about this change.

@@ -6,7 +6,7 @@ import { waitAndClick, waitForSelector } from '../page-helper';

export default class PostFrontendPage extends Page {
constructor( page ) {
const expectedSelector = '#main article.post';
const expectedSelector = '.post';
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 reviewed all of the Core default themes, there was no consistent selector with the level of specificity of #main article.post: sometimes #main existed, sometimes it didn't. Sometimes it was a <div>, sometimes a <main>, sometimes it had role="main" defined. Sometimes it used <article> sometimes a <div>. However, they all use the .post class, given how long this class has been defined, I would expect it to continue into the future.

I don't think the lower specificity is a problem, since it's always loading a single post page: if there's no post, the .post class won't be used anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

const blockIconSelector = `.editor-inserter__menu button[aria-label*='${ blockName }']`;
async insertBlock( blockName, blockTitle ) {
await searchForBlock( blockTitle );
const blockIconSelector = `.editor-block-list-item-jetpack-${ blockName }`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gutenberg removed the aria-label property, and changed their equivalent selector in tests to an XPath selector. I found this to be unreliable, and felt fragile when applied to beta blocks, which have the "Beta" term in their displayed title. It's much more reliable to use the unique name that block defines.

@pento
Copy link
Contributor Author

pento commented Nov 24, 2019

For some reason the WordAds block looks like it isn't properly focussing after it's inserted, going by the e2e test failure report.

WordAds e2e test failure

I wasn't able to reproduce this behaviour locally, I tried forcing it to focus (2805b85), but that didn't help.

@jeherve jeherve modified the milestones: 8.0, 8.1 Nov 25, 2019
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Dec 2, 2019
@pento pento merged commit 9432874 into master Dec 2, 2019
@pento pento deleted the add/pinterest-e2e-tests branch December 2, 2019 20:58
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants