Include UI tests for blocks with Appium by JavonDavis · Pull Request #676 · wordpress-mobile/gutenberg-mobile

JavonDavis · 2019-02-26T20:01:02Z

Part of #745

This is an initial PR to kick off on device UI tests.

It introduces:

Jest as the test runner
Appium as the UI Test Framework and the WebDriver Agent as the client
Setup for Appium to run as a separate process before all tests with the Appium log writing to a appium-out.log file
Setup for writing code to interact with blocks
Simple test interacting with the paragraph block

How it looks now

Before all

An appium process is spawned -- This reduces an extra manual step of having to fire up appium manually
The initial-html.js file is replaced with `initial-device-tests.html.js, this file clears out all the blocks and give the tests a blank slate to start with
WebDriver instance for interacting with UI elements created

After all

revert intial renaming
Quit the webdriver
Kill the appium process running in the background

Interacting with blocks

The challenge here is that blocks are created dynamically, with no reliable way for uniquely identifying a newly created block on fly(At a UI level at least). The change made here to facilitate this was to fit newly created blocks with a testID. The testID prop translates to accessibilityIdentifier on iOS and the content description on Android and is the recommended identifiers for locating elements on screen for mobile UI tests.

The testID for a block is a combination of the blockName and the the client Id. Specifically,
const testID = this.props.getBlockName( clientId ) + '-' + clientId;

So a testID will look something like, core/paragraph-667c78f0-2dcc-4cba-ab3c-aef6e9e96c44

When new block is created it's searched for by it's prefixing blockName, then based on the set of blocks that existed before(based on the clientIDs) the new block is identified. With that reference to the new block you can go on to write new interactions with that block.

This PR starts by writing a test interacting with the Paragraph block and also adds useful testIDs in other areas of the sample app.

As an addition, from the spec for the ToolbarButton https://github.com/WordPress/gutenberg/blob/master/packages/components/src/toolbar-button/index.js, there is no label prop, I changed this to title which I think was the intention here.

To test

Run yarn start

Android(First time or after a change in the app code)

Ensure there's a connected Android Emulator or device

Run yarn android

iOS(First time or after a change in the app code)

Run yarn iOS

Android

Run TEST_RN_PLATFORM=android yarn test:ui

iOS

Run TEST_RN_PLATFORM=ios yarn test:ui

Update

With Appium testID doesn't populate the contentDescription for Android. Instead for consistency only accessibilityLabel will be used for interacting with the elements. This results in the need for the labels to be localized in most cases.

The testID value for a specific block will now look like

block-${ value.index }-${ this.props.getBlockName( clientId ) }

For best practice and for easier constructing of identifiers without the need for client id which is an internal value which the user wouldn't have any knowledge of.

the new commands for running the tests are

yarn test:ui:ios and yarn test:ui:android

With the addition of these commands, the code used change out the initial-html file is removed.

testId doesn\'t translate to android, so need to use accessibilityLabel

… config

…appium

hypest · 2019-03-06T17:56:26Z

Tried out the PR @JavonDavis and works! I see the test running on an Android device 🎉

I will do another pass to have a look at the code side of things and report here again. Thanks!

hypest · 2019-03-08T11:04:30Z

Hey @JavonDavis, this PR is a good start! I had a super quick look at the code changes too and I think I'll need some extra information on the individual changes to gain insight.

So, can you please expand the PR description to include some detailed explanation of the individual changes introduced in this PR? I'll wait for that before adding comments as part of a PR review. Thanks!

Also, it'd be good if you can have a look at the code correctness tests that currently fail on Travis. It'd probably be useful if you run ./.travis/travis-checks-js.sh locally on your machine and fix the lint errors.

A more crucial aspect of the new tests though is to manage to have them running on CI. Can you collaborate with our friends in Platform 9¾ for a plan? It'd be nice if we could add support for CI in this PR but, no problem if we opt for working on that on a separate PR.

JavonDavis · 2019-03-11T16:20:06Z

So, can you please expand the PR description to include some detailed explanation of the individual changes introduced in this PR? I'll wait for that before adding comments as part of a PR review. Thanks!

Also, it'd be good if you can have a look at the code correctness tests that currently fail on Travis. It'd probably be useful if you run ./.travis/travis-checks-js.sh locally on your machine and fix the lint errors.

@hypest Definitely! This was still in draft mode so that's the reason I haven't included those details as yet, I'll update and ping you again

A more crucial aspect of the new tests though is to manage to have them running on CI. Can you collaborate with our friends in Platform 9¾ for a plan? It'd be nice if we could add support for CI in this PR but, no problem if we opt for working on that on a separate PR.

For sure! This is currently in progress, the plan is to use Firebase test labs with CircleCi to get this working.

…rg-mobile into try/e2e-tests-appium

* Using data file for device tests

JavonDavis · 2019-03-27T16:24:17Z

@hypest @pinarol This is ready for another look now... Sorry for the delay on this it took me some time to work through some issues building the app on the CI... Also my bad for the number of commits and changes in this one PR 🙈The addition of the CI really consumed me... Can you guys please take another look when you get a chance?

Tug · 2019-03-28T14:52:33Z

README.md

+This repository uses appium to run UI tests. The tests live in `__device-tests__` and are written using Appium to run tests against simulators and real devices. To run these you'll need to check off a few things: 
+
+* For now you'll need run `yarn start`, and then either `yarn ios` or `yarn android` at least once before trying to run the tests on the respective platform
+* [Appium cli](https://github.com/appium/appium/blob/master/docs/en/about-appium/getting-started.md) installed and available globally, I'd also recommend using [appium doctor](https://github.com/appium/appium-doctor) to ensure all of appium's dependencies are good to go. You don't have to worry about starting the server yourself, the tests handle starting the server on port 4728, just be sure that the port is free or feel free to change the port number in the test file. 


It doesn't look like installing appium globally is actually necessary. It seems to work fine if we add it to our dev dependencies and just boot it using yarn appium.

Right! I'll make that change, thanks!

Tug · 2019-03-28T14:55:29Z

package.json

    "preios:xcode10": "cd node_modules/react-native && ./scripts/ios-install-third-party.sh && cd third-party/glog-0.3.5 && [ -f libglog.pc ] || ../../scripts/ios-configure-glog.sh",
    "ios": "react-native run-ios",
    "test": "cross-env NODE_ENV=test node node_modules/jest/bin/jest.js --verbose --config jest.config.js",
+    "device-tests":"cross-env NODE_ENV=test node node_modules/jest/bin/jest.js --detectOpenHandles --verbose --config jest_ui.config.js",


We can use jest here instead of node node_modules/jest/bin/jest.js, yarn can look into node_modules/.bin

__device-tests__/helpers/caps.js

Tug · 2019-03-28T15:18:25Z

src/app/initial-html.js

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


Minor but I'm curious we're removing empty lines here?

I saw this in the diff and I meant to change out this back to the original and forgot to get back to it actually. My IDE keeps removing the empty lines

Tug · 2019-03-28T15:24:35Z

.circleci/config.yml

+    - run:
+        name: Install command line tools
+        command: |
+          sudo npm install -g react-native-cli


Same here, we can just use yarn react-native instead when we're inside the repository directory and dev dependencies are installed

I think I attempted this and it resulted in an EACESS error on the CI. Let me try and dig up an old build in circle and see.

Tug · 2019-03-28T15:26:13Z

.circleci/config.yml

+          name: Bundle Debug android
+          command: |
+            mkdir -p android/app/src/main/assets
+            react-native bundle --platform android --dev false --entry-file index.js --bundle-output android/app/src/main/assets/index.android.bundle --assets-dest android/app/src/main/res


Probably worth creating an npm script for that, we could call it bundle:android:test for instance

Hmm yeah it's probably worth making that update. I'll make the change

Tug · 2019-03-28T17:22:12Z

Tested it with both platforms emulators and it worked great! Awesome work on that 👍

I'll review the code more thoroughly but so far it looks good, it could probably use a bit of cleanup though. Like, I'm not sure if bin/tmp should be included? Shouldn't we add it to .gitignore maybe?

pinarol

I thinks this looks good overall, I just added some minor comments and questions. Thanks for all your effort @JavonDavis ! Looking fwd to see our UI tests in action 🎉

pinarol · 2019-03-28T14:40:20Z

src/block-management/block-toolbar.js


 		return (
-			<View style={ styles.container }>
+			<View style={ styles.container } accessible={ false } accessibilityLabel="View Add block" >


Is the word "View Add block" used from somewhere? I can't find it when I search it. We can just remove it if it is unused?
Same for the below:
accessibilityLabel="Scrollview Add block"
accessibilityLabel={ 'Toolbar Add block' }

It'd be good to choose a formatting strategy for accessibilityLabels just for sake of consistency(for those we don't have an option of using a localized title). I see in some places that they are separated by dash: "xxx-xxx", but in other places like "Xxx Xxxx Xx..."(I don't have a special choice BTW).

Agreed, planning to sync with @etoledom on this. For now the ones you mentioned they were actually included for me to have a better understanding of how the hierarchy looked when translated to the XML view in the emulator to help me with debugging and identifying the right element. In that case they're not necessary or being used right now but I'd like to instead choose an appropriate and consistent name and leave them in to possibly help in the future. This can be more directly related to #476

pinarol · 2019-03-28T14:43:39Z

src/block-management/block-holder.js

-					<View style={ [ ! isSelected && styles.blockContainer, isSelected && styles.blockContainerFocused ] }>{ this.getBlockForType() }</View>
+					<View
+						accessibile={ true }
+						accessibilityLabel={ this.props.testID } style={ [ ! isSelected && styles.blockContainer, isSelected && styles.blockContainerFocused ] }>{ this.getBlockForType() }</View>


we can beautify indentation here a bit

True, I can make this update!

pinarol · 2019-03-28T16:40:21Z

src/block-management/inline-toolbar/index.js

-			<View style={ styles.toolbar }>
+			<View style={ styles.toolbar } >
 				<ToolbarButton
+					accessibilityLabel="toolbar"


how about we make this same as label where available: __( 'Move up' ) ?
That way in the future when we implement real accessibility it'd require us less refactor

Yeah, toolbar here makes no sense and I think was there as placeholder when I was testing something else, I can change this name

JavonDavis · 2019-03-28T18:02:10Z

Tested it with both platforms emulators and it worked great! Awesome work on that 👍

I'll review the code more thoroughly but so far it looks good, it could probably use a bit of cleanup though. Like, I'm not sure if bin/tmp should be included? Shouldn't we add it to .gitignore maybe?

Thanks for giving it a run @Tug. I'll look out for your feedback on the code!

As it relates to bin/tmp. At the moment it's necessary, it relates to an earlier conversation on the PR with @hypest(the code has been removed but weird I can't find to the comment) The app expects an empty post when it opens and does that by changing out the contents of the initial html file as a pre and post step using the files in the bin/tmp folder. I looked at other ways of doing this(maybe passing environment variables to metro when bundling) but none were working out like we needed it to here and I didn't want to spend too much time on it right now. Before it's current state, it was changed out in the before step in test itself 879c5e3 might be a commit to checkout to see what it was doing before.

Tug · 2019-03-29T14:41:52Z

ios/gutenberg.xcodeproj/xcshareddata/xcschemes/gutenberg.xcscheme

         <BuildActionEntry
            buildForTesting = "YES"
-            buildForRunning = "YES"
+            buildForRunning = "NO"


No sure what those mean, are we disabling the some types of build?

Umm, this should not be necessary... No we're not disabling anything additional for this, this can be removed

Actually my bad, this is necessary when building the app for the simulator, without this it leads to an architecture conflict when building the app

JavonDavis added 5 commits February 16, 2019 01:43

Changed accessibility of block elements, included .nvmrc

702077a

combining blockID and client ID to make testID for locating blocks

aa306fa

included testID for simple editor check

2bfd973

renamed test files to better match with intent

03858c7

Update jest to ignoer page objects and locators

9e9fa98

testId doesn\'t translate to android, so need to use accessibilityLabel

JavonDavis changed the title ~~Try/e2e tests appium~~ Appium Tests Feb 26, 2019

JavonDavis changed the title ~~Appium Tests~~ Appium UI Tests Feb 26, 2019

JavonDavis changed the title ~~Appium UI Tests~~ Include UI tests for blocks with Appium Feb 26, 2019

JavonDavis added 7 commits February 26, 2019 15:04

simple interaction with blocks

88b7391

merge in changes for master and use environment variable to determine…

2a76cd6

… config

instability in type function, had to resort to a typing letter by letter

8e949ac

simplified to type, this works well on android but not so much on ios

5fea42b

update before and after step in test to handle starting and stopping …

06535c2

…appium

use relative path for app paths

f4f7e22

updated README

879c5e3

JavonDavis added 10 commits March 11, 2019 15:44

started refactoring into page object model

fd163ae

removed page and block functions from spec file

89ab16f

reverted type function for consistency on iOS

16ef7ab

remove console logs

409e1ac

fix majority of travis checks

815ba1d

Merge branch 'develop' of https://github.com/wordpress-mobile/gutenbe…

5fd1d39

…rg-mobile into try/e2e-tests-appium

resolved flow errors

55c994e

resolved all eslint problems

98a576f

excluding device tests from jest config

5a203c8

* resolved build error caused by initial data set

2a8b1c8

* Using data file for device tests

JavonDavis marked this pull request as ready for review March 13, 2019 15:55

submodule update

1e8a20e

fixed linter error and merged in develop

628bb9e

JavonDavis requested review from hypest and pinarol and removed request for hypest March 27, 2019 16:25

JavonDavis self-assigned this Mar 27, 2019

JavonDavis added the Testing Anything related to automated tests label Mar 27, 2019

Tug reviewed Mar 28, 2019

View reviewed changes

__device-tests__/helpers/caps.js Show resolved Hide resolved

Tug reviewed Mar 28, 2019

View reviewed changes

pinarol approved these changes Mar 28, 2019

View reviewed changes

JavonDavis added 2 commits March 28, 2019 18:00

changes to labels and indentation in certain areas

a0fb6a8

fixed linter error with extra space

f045042

Tug reviewed Mar 29, 2019

View reviewed changes

JavonDavis added 4 commits April 1, 2019 10:27

code indentation cleanup and undo change in scheme config

92e359b

redo config change

0743395

remove react native install and add bundle scripts to package.json

8da08a7

remove unnecessary config owner update

339c207

JavonDavis merged commit 5cb149a into develop Apr 1, 2019

JavonDavis mentioned this pull request Apr 1, 2019

Include UI tests for blocks through Sample App #745

Closed

13 tasks

This was referenced Apr 2, 2019

Revert "Include UI tests for blocks with Appium" #803

Merged

Revert some changes to yarn.lock #805

Merged

Delete package-lock.json #806

Merged

SergioEstevao deleted the try/e2e-tests-appium branch November 8, 2019 11:50

Conversation

JavonDavis commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How it looks now

Before all

After all

Interacting with blocks

To test

Android(First time or after a change in the app code)

iOS(First time or after a change in the app code)

Android

iOS

Update

Uh oh!

hypest commented Mar 6, 2019

Uh oh!

hypest commented Mar 8, 2019

Uh oh!

JavonDavis commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JavonDavis commented Mar 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JavonDavis Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JavonDavis Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tug commented Mar 28, 2019

Uh oh!

pinarol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JavonDavis commented Mar 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

JavonDavis commented Feb 26, 2019 •

edited

Loading

JavonDavis commented Mar 11, 2019 •

edited

Loading

JavonDavis commented Mar 27, 2019 •

edited

Loading

JavonDavis Mar 28, 2019 •

edited

Loading

JavonDavis Mar 28, 2019 •

edited

Loading