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

[RNMobile] - Add waits to fix editor test flakiness #39668

Merged
merged 7 commits into from
Mar 29, 2022
Merged

[RNMobile] - Add waits to fix editor test flakiness #39668

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8885da0
add waits to fix editor test flakiness
jostnes Mar 23, 2022
dc4337d
update separator block expected html to latest
jostnes Mar 23, 2022
1e6fcbc
remove extra spaces
jostnes Mar 25, 2022
9541748
rename test data, move check to different method
jostnes Mar 28, 2022
8fb37dc
lint fix
jostnes Mar 28, 2022
c1c5296
missed one test data update
jostnes Mar 28, 2022
1b6d50c
lint fix
jostnes Mar 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 13 additions & 83 deletions packages/react-native-editor/__device-tests__/gutenberg-editor-block-insertion-2.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,21 @@
* Internal dependencies
*/
import { blockNames } from './pages/editor-page';
import {
headingExpectedHTML,
imageExpectedHTML,
listExpectedHTML,
moreExpectedHTML,
paragraphExpectedHTML,
separatorExpectedHTML,
} from './helpers/test-data';

describe( 'Gutenberg Editor tests for Block insertion 2', () => {
it( 'adds new block at the end of post', async () => {
await editorPage.addNewBlock( blockNames.heading );

await editorPage.addNewBlock( blockNames.list );

const expectedHtml = `<!-- wp:heading -->
<h2></h2>
<!-- /wp:heading -->

<!-- wp:list -->
<ul><li></li></ul>
<!-- /wp:list -->`;

const expectedHtml = `${ headingExpectedHTML }\n\n${ listExpectedHTML }`;
const html = await editorPage.getHtmlContent();
expect( html.toLowerCase() ).toBe( expectedHtml );
} );
Expand All @@ -27,21 +27,9 @@ describe( 'Gutenberg Editor tests for Block insertion 2', () => {
);

await headingBlockElement.click();

await editorPage.addNewBlock( blockNames.separator );

const expectedHtml = `<!-- wp:heading -->
<h2></h2>
<!-- /wp:heading -->

<!-- wp:separator -->
<hr class="wp-block-separator has-alpha-channel-opacity"/>
<!-- /wp:separator -->

<!-- wp:list -->
<ul><li></li></ul>
<!-- /wp:list -->`;

const expectedHtml = `${ headingExpectedHTML }\n\n${ separatorExpectedHTML }\n\n${ listExpectedHTML }`;
fluiddot marked this conversation as resolved.
Show resolved Hide resolved
const html = await editorPage.getHtmlContent();
expect( html.toLowerCase() ).toBe( expectedHtml );
} );
Expand All @@ -54,83 +42,25 @@ describe( 'Gutenberg Editor tests for Block insertion 2', () => {
await separatorBlockElement.click();

await editorPage.addNewBlock( blockNames.image, 'before' );
await editorPage.driver.sleep( 1000 );
await editorPage.closePicker();

const expectedHtml = `<!-- wp:heading -->
<h2></h2>
<!-- /wp:heading -->

<!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image -->

<!-- wp:separator -->
<hr class="wp-block-separator has-alpha-channel-opacity"/>
<!-- /wp:separator -->

<!-- wp:list -->
<ul><li></li></ul>
<!-- /wp:list -->`;

const expectedHtml = `${ headingExpectedHTML }\n\n${ imageExpectedHTML }\n\n${ separatorExpectedHTML }\n\n${ listExpectedHTML }`;
const html = await editorPage.getHtmlContent();
expect( html.toLowerCase() ).toBe( expectedHtml );
} );

it( 'inserts block at the end of post when no block is selected', async () => {
await editorPage.addNewBlock( blockNames.more );

const expectedHtml = `<!-- wp:heading -->
<h2></h2>
<!-- /wp:heading -->

<!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image -->

<!-- wp:separator -->
<hr class="wp-block-separator has-alpha-channel-opacity"/>
<!-- /wp:separator -->

<!-- wp:list -->
<ul><li></li></ul>
<!-- /wp:list -->

<!-- wp:more -->
<!--more-->
<!-- /wp:more -->`;

const expectedHtml = `${ headingExpectedHTML }\n\n${ imageExpectedHTML }\n\n${ separatorExpectedHTML }\n\n${ listExpectedHTML }\n\n${ moreExpectedHTML }`;
const html = await editorPage.getHtmlContent();
expect( html.toLowerCase() ).toBe( expectedHtml );
} );

it( 'creates a new Paragraph block tapping on the empty area below the last block', async () => {
await editorPage.addParagraphBlockByTappingEmptyAreaBelowLastBlock();

const expectedHtml = `<!-- wp:heading -->
<h2></h2>
<!-- /wp:heading -->

<!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image -->

<!-- wp:separator -->
<hr class="wp-block-separator has-alpha-channel-opacity"/>
<!-- /wp:separator -->

<!-- wp:list -->
<ul><li></li></ul>
<!-- /wp:list -->

<!-- wp:more -->
<!--more-->
<!-- /wp:more -->

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->`;

const expectedHtml = `${ headingExpectedHTML }\n\n${ imageExpectedHTML }\n\n${ separatorExpectedHTML }\n\n${ listExpectedHTML }\n\n${ moreExpectedHTML }\n\n${ paragraphExpectedHTML }`;
const html = await editorPage.getHtmlContent();
expect( html.toLowerCase() ).toBe( expectedHtml );
} );
Expand Down
24 changes: 24 additions & 0 deletions packages/react-native-editor/__device-tests__/helpers/test-data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,3 +141,27 @@ exports.fileBlockPlaceholder = `<!-- wp:file {"id":3,"href":"https://wordpress.o
exports.audioBlockPlaceholder = `<!-- wp:audio {"id":5} -->
<figure class="wp-block-audio"><audio controls src="https://cldup.com/59IrU0WJtq.mp3"></audio></figure>
<!-- /wp:audio -->`;

exports.headingExpectedHTML = `<!-- wp:heading -->
<h2></h2>
<!-- /wp:heading -->`;

exports.separatorExpectedHTML = `<!-- wp:separator -->
<hr class="wp-block-separator has-alpha-channel-opacity"/>
<!-- /wp:separator -->`;

exports.listExpectedHTML = `<!-- wp:list -->
<ul><li></li></ul>
<!-- /wp:list -->`;

exports.imageExpectedHTML = `<!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image -->`;

exports.moreExpectedHTML = `<!-- wp:more -->
<!--more-->
<!-- /wp:more -->`;

exports.paragraphExpectedHTML = `<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->`;
fluiddot marked this conversation as resolved.
Show resolved Hide resolved
43 changes: 31 additions & 12 deletions packages/react-native-editor/__device-tests__/pages/editor-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,31 +258,42 @@ class EditorPage {
// =========================

async addNewBlock( blockName, relativePosition ) {
// Click add button.
let identifier = 'Add block';
if ( isAndroid() ) {
identifier = 'Add block, Double tap to add a block';
}
const addButton = await this.driver.elementByAccessibilityId(
identifier
const addBlockButtonLocator = isAndroid()
? '//android.widget.Button[@content-desc="Add block, Double tap to add a block"]'
: '//XCUIElementTypeButton[@name="add-block-button"]';
Comment on lines +262 to +263
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to use XPath instead of accessibilityId because if an accessibilityId is not found, the Appium session is immediately terminated giving no chance to wait and retry. While XPaths are not recommended locators, it is the workaround for this case for now.

Let me know if anyone knows of a better alternative!

Copy link
Contributor

Choose a reason for hiding this comment

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

True, it's generally not recommended to use XPaths but I wouldn't oppose using it if there's no other option. I haven't investigated other approaches, but I recall that WebDriver automatically finishes the session upon some failures, even if the call is wrapped with try-catch 😞 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I tried wrapping it in a try-catch, hoping to bypass the error but it didn't work 😞 I also checked if it was configurable but looks like it isn't.


const addButton = await waitForVisible(
this.driver,
addBlockButtonLocator
);

if ( relativePosition === 'before' ) {
await longPressMiddleOfElement( this.driver, addButton );
const addBlockBeforeButtonLocator = isAndroid()
? '//android.widget.Button[@content-desc="Add Block Before"]'
: '//XCUIElementTypeButton[@name="Add Block Before"]';

const addBlockBeforeButton = await this.driver.elementByAccessibilityId(
'Add Block Before'
const addBlockBeforeButton = await waitForVisible(
this.driver,
addBlockBeforeButtonLocator
);

await addBlockBeforeButton.click();
} else {
await addButton.click();
}

// Click on block of choice.
const blockButton = await this.findBlockButton( blockName );

if ( isAndroid() ) {
await blockButton.click();

// For certain blocks, clicking on it will display additional options that the test should wait for before next steps
if ( blockName === 'Image' ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to only be affecting Android tests, there could be more but for this test file, looks like Image block is one of them. The locator is the part highlighted below:

image block

Copy link
Contributor

Choose a reason for hiding this comment

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

Media blocks like Image automatically open the media options picker. In this case, I think that waiting for the block picker should be part of the test that adds the block. Which by the way is already being done, as outlined in the following example:

gutenberg/packages/react-native-editor/__device-tests__/gutenberg-editor-image-@canary.test.js

Lines 10 to 12 in a4c8730

await editorPage.addNewBlock( blockNames.image );
await editorPage.driver.sleep( 1000 );
await editorPage.closePicker();

Although it would be great to use the waitForVisible helper for that element (i.e. media options picker) too 😅 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok, I was actually looking to find what that was called. I'll update this to move to closePicker() I think would make more sense there so that it covers all media blocks.

I think the implicit waits on the tests can then be removed with this change, but I'll do that in a future PR so that this PR can focus on this one test file first.

const locator =
'//android.view.ViewGroup[2]/android.view.ViewGroup/android.view.ViewGroup';
await waitForVisible( this.driver, locator );
}
} else {
await this.driver.execute( 'mobile: tap', {
element: blockButton,
Expand All @@ -299,12 +310,19 @@ class EditorPage {

// Attempts to find the given block button in the block inserter control.
async findBlockButton( blockName ) {
// Wait for the first block, Paragraph block, to load before looking for other blocks
const paragraphBlockLocator = isAndroid()
? '//android.widget.Button[@content-desc="Paragraph block"]/android.widget.TextView'
: '//XCUIElementTypeButton[@name="Paragraph block"]';
Comment on lines +306 to +309
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I've set that it waits for the Paragraph block to load because it's the first one on the blocks list, I'm not sure how often the list changes so this has the potential to fail in the future. Another option is to wait for the search box. What do you all think?

Copy link
Contributor

Choose a reason for hiding this comment

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

The block picker view has a testID attribute with value add-block-button, maybe we should rely on that, wdyt?
As an alternative, we might consider adding accessibility info, which apart from helping us query the element, would be a good improvement in terms of a11y.

gutenberg/packages/block-editor/src/components/inserter/index.native.js

Line 50 in 41325b9

testID: 'add-block-button',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok but looks like that is before the block picker view:
Screenshot 2022-03-28 at 11 01 44 AM

After some investigation, looks like we can actually create another wait helper that uses IDs! But it has to be an ID defined like this (i.e For Android, android:id/{value}):
Screenshot 2022-03-28 at 11 42 16 AM

There is an open issue for this here. Appium doc how to use this here.

I am not sure where this is defined (looks like not on the ./index.native.js file), if we can add an ID on a more appropriate view we could depend on that to load first before continuing the test.

I think, for now, I will leave it to wait for Paragraph block before continuing the test. Future improvements (i.e. add id and use it for the test) to be done in a separate PR. Is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, I'm afraid I referenced the wrong test ID. The one I'm wanted to share is InserterUI-Blocks:

gutenberg/packages/block-editor/src/components/block-types-list/index.native.js

Line 89 in 41325b9

testID={ `InserterUI-${ name }` }

In any case, I think it's quite reliable to use the Paragraph block as it's a block that will be always present.


await waitForVisible( this.driver, paragraphBlockLocator );
const blockAccessibilityLabel = `${ blockName } block`;
const blockAccessibilityLabelNewBlock = `${ blockAccessibilityLabel }, newly available`;

if ( isAndroid() ) {
const size = await this.driver.getWindowSize();
const x = size.width / 2;

// Checks if the Block Button is available, and if not will scroll to the second half of the available buttons.
while (
! ( await this.driver.hasElementByAccessibilityId(
Expand Down Expand Up @@ -613,8 +631,9 @@ class EditorPage {
if ( isAndroid() ) {
await swipeDown( this.driver );
} else {
const cancelButton = await this.driver.elementByAccessibilityId(
'Cancel'
const cancelButton = await waitForVisible(
this.driver,
'//XCUIElementTypeButton[@name="Cancel"]'
);
await cancelButton.click();
}
Expand Down