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

Add end 2 end tests InnerBlocks renderAppender #14996

Merged
merged 3 commits into from
Dec 16, 2019
Merged

Add end 2 end tests InnerBlocks renderAppender #14996

merged 3 commits into from
Dec 16, 2019

Conversation

jorgefilipecosta
Copy link
Member

Description

This PR adds end 2 end tests to renderAppender property of InnerBlocks added in #14241.

How has this been tested?

I checked end 2 end tests execute with success.

@jorgefilipecosta jorgefilipecosta added [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Apr 16, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the add/end-2-end-test-innerBlocks-renderAppender branch from 63ac58e to 6078276 Compare April 16, 2019 12:58
aduth
aduth previously requested changes Apr 24, 2019
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

There's a failed test in the build in this newly added suite. It should be made to pass consistently.

packages/e2e-tests/plugins/inner-blocks-renderappender/index.js Outdated Show resolved Hide resolved
packages/e2e-tests/plugins/inner-blocks-renderappender/index.js Outdated Show resolved Hide resolved
@jorgefilipecosta jorgefilipecosta force-pushed the add/end-2-end-test-innerBlocks-renderAppender branch 2 times, most recently from f9c411d to da0c6a7 Compare May 1, 2019 10:47
@jorgefilipecosta jorgefilipecosta force-pushed the add/end-2-end-test-innerBlocks-renderAppender branch 2 times, most recently from e4967b5 to 9ce9b74 Compare October 4, 2019 16:16
@jorgefilipecosta jorgefilipecosta force-pushed the add/end-2-end-test-innerBlocks-renderAppender branch 2 times, most recently from a6225e9 to 271817e Compare October 14, 2019 11:03
@jorgefilipecosta
Copy link
Member Author

The PR is rebased and is updated the tests are passing consistently. I think it is ready for a review.
@gziolo would you be able to have a look?

@jorgefilipecosta jorgefilipecosta dismissed aduth’s stale review October 14, 2019 12:55

Review was addressed.

@jorgefilipecosta jorgefilipecosta force-pushed the add/end-2-end-test-innerBlocks-renderAppender branch from 271817e to 6fe1c75 Compare November 27, 2019 11:38
@jorgefilipecosta
Copy link
Member Author

Hi @aduth, this PR was rebased could you a new look?

aduth
aduth reviewed Dec 4, 2019
View reviewed changes
packages/e2e-tests/plugins/inner-blocks-renderappender.php Outdated
/**
* Registers a custom script for the plugin.
*/
function enqueue_inner_blocks_renderappender_script() {
Copy link
Member

Choose a reason for hiding this comment

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

By its camelCase renderAppender, we should treat "render appender" as two words, reflecting this as well in its kebab-case and snake_case forms as render-appender.php and render_appender respectively.

packages/e2e-tests/plugins/inner-blocks-renderappender/index.js Outdated Show resolved Hide resolved
packages/e2e-tests/plugins/inner-blocks-renderappender/index.js Outdated
var InnerBlocks = wp.blockEditor.InnerBlocks;
var withSelect = wp.data.withSelect;

var dataSelector = withSelect( function( select, ownProps ) {
Copy link
Member

Choose a reason for hiding this comment

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

Optional: Could use hooks form now.

packages/e2e-tests/specs/plugins/inner-blocks-renderappender.test.js Outdated
openAllBlockInserterCategories,
} from '@wordpress/e2e-test-utils';

const QUOTE_INSERT_BUTTON_SELECTOR = '//button[contains(concat(" ", @class, " "), " block-editor-block-types-list__item ")][./span[contains(text(),"Quote")]]';
Copy link
Member

Choose a reason for hiding this comment

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

If it was really the case that the inner span selection ended up being an issue resolved in #18771, then I think we should be a bit more wary about it. I think there might be some options with XPath to traverse back up to the parent of a matched span, if we need to match it? https://devhints.io/xpath#more-examples

Generally, I kinda wish these elements were just easier to target. I'd almost be okay with adapting the output in the source code for the sake of the tests, if it makes them more reliable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the selection mechanism to use logic similar to what we used on #18771.

packages/e2e-tests/specs/plugins/inner-blocks-renderappender.test.js Outdated
'Quote',
'Video',
] );
const quoteButton = ( await page.$x( QUOTE_INSERT_BUTTON_SELECTOR ) )[ 0 ];
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some inline comments in this test case to explain the steps? As it stands, it's pretty difficult to follow.

@jorgefilipecosta jorgefilipecosta force-pushed the add/end-2-end-test-innerBlocks-renderAppender branch from 6fe1c75 to d6d57d3 Compare December 6, 2019 13:03
@jorgefilipecosta jorgefilipecosta force-pushed the add/end-2-end-test-innerBlocks-renderAppender branch from d6d57d3 to ddd783c Compare December 6, 2019 13:13
@jorgefilipecosta
Copy link
Member Author

Hi @aduth, thank you for the review I applied changes that I hope to address all the points raised 👍

@jorgefilipecosta jorgefilipecosta merged commit d0ae224 into master Dec 16, 2019
@jorgefilipecosta jorgefilipecosta deleted the add/end-2-end-test-innerBlocks-renderAppender branch December 16, 2019 14:36
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@gziolo gziolo Awaiting requested review from gziolo

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees
No one assigned
Labels
[Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Milestone
Gutenberg 7.2
Development

Successfully merging this pull request may close these issues.
3 participants
@jorgefilipecosta @aduth @youknowriad