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

Include UI tests for blocks with Appium #676

Merged
merged 146 commits into from
Apr 1, 2019
Merged

Conversation

JavonDavis
Copy link
Contributor

@JavonDavis JavonDavis commented Feb 26, 2019

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

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

@JavonDavis JavonDavis changed the title Try/e2e tests appium Appium Tests Feb 26, 2019
@JavonDavis JavonDavis changed the title Appium Tests Appium UI Tests Feb 26, 2019
@JavonDavis JavonDavis changed the title Appium UI Tests Include UI tests for blocks with Appium Feb 26, 2019
@hypest
Copy link
Contributor

hypest commented Mar 6, 2019

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
Copy link
Contributor

hypest commented Mar 8, 2019

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
Copy link
Contributor Author

JavonDavis commented Mar 11, 2019

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.

@JavonDavis JavonDavis marked this pull request as ready for review March 13, 2019 15:55
@JavonDavis JavonDavis requested review from pinarol and hypest and removed request for hypest March 27, 2019 16:25
@JavonDavis JavonDavis self-assigned this Mar 27, 2019
@JavonDavis JavonDavis added the Testing Anything related to automated tests label Mar 27, 2019
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right! I'll make that change, thanks!

@@ -70,6 +71,14 @@
"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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏽

@@ -7,198 +7,151 @@ export default `
<!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image -->

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@JavonDavis JavonDavis Mar 28, 2019

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@JavonDavis JavonDavis Mar 28, 2019

Choose a reason for hiding this comment

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

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

@Tug
Copy link
Contributor

Tug commented Mar 28, 2019

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?

Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

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 🎉

@@ -48,29 +48,38 @@ export class BlockToolbar extends Component<PropsType> {
} = this.props;

return (
<View style={ styles.container }>
<View style={ styles.container } accessible={ false } accessibilityLabel="View Add block" >
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<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>
Copy link
Contributor

Choose a reason for hiding this comment

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

we can beautify indentation here a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I can make this update!

<ToolbarButton
accessibilityLabel="toolbar"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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, toolbar here makes no sense and I think was there as placeholder when I was testing something else, I can change this name

@JavonDavis
Copy link
Contributor Author

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.

@@ -36,10 +36,10 @@
</BuildActionEntry>
<BuildActionEntry
buildForTesting = "YES"
buildForRunning = "YES"
buildForRunning = "NO"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing Anything related to automated tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants