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

jostnes · 2022-03-23T03:18:53Z

What?

This PR adds waitForVisible() to check that locators are available before continuing the test for all tests on gutenberg-editor-block-insertion-2.test.js

This PR also moved all the test data on that test file to test-data.js so that it is consistent with other tests and also if there is a change in the HTML, we would now only need to change it in one place instead of multiple.

Why?

The waits are added to ensure that the screen is at the correct stage before the test continues, this will help reduce flakiness.

How?

By adding waitForVisible() to the tests, the test will fail if the locator is not present.

Testing Instructions

Ensure that the gutenberg-editor-block-insertion-2.test.js test file passes on both local runs and on CI. I've run it 10 times here, while there are failures those are other flaky tests/other reasons. That will be tackled separately in future PRs.

Screenshots or screencast

N/A

jostnes · 2022-03-25T03:23:33Z

packages/react-native-editor/__device-tests__/pages/editor-page.js

+			? '//android.widget.Button[@content-desc="Add block, Double tap to add a block"]'
+			: '//XCUIElementTypeButton[@name="add-block-button"]';


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!

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 😞 .

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.

jostnes · 2022-03-25T03:28:49Z

packages/react-native-editor/__device-tests__/pages/editor-page.js

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


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:

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 😅 .

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.

jostnes · 2022-03-25T03:30:50Z

packages/react-native-editor/__device-tests__/pages/editor-page.js

+		// 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"]';


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?

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',

Ah ok but looks like that is before the block picker view:

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}):

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?

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.

fluiddot

Thanks @jostnes for introducing these amazing fixes which will help on reducing the test flakiness 🏅.

I added some comments, most of them reply to yours. Let me know if you'd like me to expand any of them.

packages/react-native-editor/__device-tests__/helpers/test-data.js

packages/react-native-editor/__device-tests__/gutenberg-editor-block-insertion-2.test.js

fluiddot · 2022-03-25T17:25:28Z

packages/react-native-editor/__device-tests__/pages/editor-page.js

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


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 😅 .

fluiddot · 2022-03-25T17:28:52Z

packages/react-native-editor/__device-tests__/pages/editor-page.js

+			? '//android.widget.Button[@content-desc="Add block, Double tap to add a block"]'
+			: '//XCUIElementTypeButton[@name="add-block-button"]';


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 😞 .

fluiddot · 2022-03-25T17:34:23Z

packages/react-native-editor/__device-tests__/pages/editor-page.js

+		// 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"]';


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',

jostnes · 2022-03-28T05:09:39Z

Thanks for the review @fluiddot! I've made some changes based on the comments and this is now ready for a re-review.

fluiddot

Thanks for the review @fluiddot! I've made some changes based on the comments and this is now ready for a re-review.

Thanks @jostnes for addressing the feedback, the PR looks great to me 🎊 . Let me know if you want me to merge it.

add waits to fix editor test flakiness

8885da0

geriux added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) Mobile App - Automation Label used to initiate Mobile App PR Automation labels Mar 23, 2022

wpmobilebot mentioned this pull request Mar 23, 2022

[RNMobile] - Add waits to fix editor test flakiness wordpress-mobile/gutenberg-mobile#4687

Merged

update separator block expected html to latest

dc4337d

jostnes marked this pull request as ready for review March 25, 2022 03:19

jostnes commented Mar 25, 2022

View reviewed changes

remove extra spaces

1e6fcbc

jostnes requested a review from fluiddot March 25, 2022 07:54

fluiddot reviewed Mar 25, 2022

View reviewed changes

jostnes added 4 commits March 28, 2022 12:36

rename test data, move check to different method

9541748

lint fix

8fb37dc

missed one test data update

c1c5296

lint fix

1b6d50c

jostnes requested a review from fluiddot March 28, 2022 05:09

fluiddot approved these changes Mar 28, 2022

View reviewed changes

jostnes merged commit c653bae into WordPress:trunk Mar 29, 2022

jostnes deleted the rnmobile/editor-test-fixes branch March 29, 2022 01:22

github-actions bot added this to the Gutenberg 13.0 milestone Mar 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jostnes commented Mar 23, 2022 •

edited

Loading

jostnes Mar 25, 2022

fluiddot Mar 25, 2022

jostnes Mar 28, 2022

jostnes Mar 25, 2022

fluiddot Mar 25, 2022

jostnes Mar 28, 2022

jostnes Mar 25, 2022

fluiddot Mar 25, 2022

jostnes Mar 28, 2022

fluiddot Mar 28, 2022

fluiddot left a comment

fluiddot Mar 25, 2022

fluiddot Mar 25, 2022

fluiddot Mar 25, 2022

jostnes commented Mar 28, 2022

fluiddot left a comment

		? '//android.widget.Button[@content-desc="Add block, Double tap to add a block"]'
		: '//XCUIElementTypeButton[@name="add-block-button"]';

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

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

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

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

Conversation

jostnes commented Mar 23, 2022 • edited Loading

What?

Why?

How?

Testing Instructions

Screenshots or screencast

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluiddot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jostnes commented Mar 28, 2022

fluiddot left a comment

Choose a reason for hiding this comment

jostnes commented Mar 23, 2022 •

edited

Loading