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

[e2e-testing][Appium] Adding support for android:id #9942

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
@jsdevel
Copy link
Contributor

jsdevel commented Sep 16, 2016

  • Calling view.setId() with the matching resource-id of an id found in R.class when BaseViewManager.setTestId is called.
  • Adding TestIdUtil in common to store mappings for testID with attributed views and their original react tag value.
  • Changing the signature for Event Classes to require the View instead of the viewTag. This reduces the number of locations where TestIdUtil.getOriginalReactTag is called.
  • Overriding BaseViewManager.onDropViewInstance to perform TestIdUtil mapping cleanup.
  • This closes #9777.
  • In order for this to work properly, an id resource needs to be manually added to android/app/src/main/res/values/ids.xml. Here's the contents of that file:
<?xml version="1.0" encoding="utf-8"?>
<resources>
<item name="something" type="id"/>
</resources>
@ghost

This comment has been minimized.

Copy link

ghost commented Sep 16, 2016

By analyzing the blame information on this pull request, we identified @kmagiera and @sebmarkbage to be potential reviewers.

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Sep 16, 2016

@ide what's the best way I could test this change?

@ghost ghost added the CLA Signed label Sep 16, 2016

@ide

This comment has been minimized.

Copy link
Collaborator

ide commented Sep 17, 2016

Building from source and running the UIExplorer + temporarily modifying its code to invoke your new code is a reasonable path forward.

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Sep 18, 2016

Thanks @ide !

I've verified locally that it works!

working

@ghost ghost added the CLA Signed label Sep 18, 2016

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Sep 18, 2016

I've also verified that non-existent ids in ids.xml don't have any side effects i.e. testID="some-id-not-in-ids.xml" doesn't break anything.

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Sep 18, 2016

cc @mkonicek @bestander

@ghost ghost added the CLA Signed label Sep 18, 2016

@jsdevel jsdevel force-pushed the kogosoftwarellc:adding-support-for-resource-id branch from 738f346 to 580888e Sep 19, 2016

@ghost ghost added the CLA Signed label Sep 19, 2016

@jsdevel jsdevel force-pushed the kogosoftwarellc:adding-support-for-resource-id branch from 580888e to ee453d3 Sep 19, 2016

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Sep 19, 2016

Looks like the tests for HMR are failing on most PRs.

@ghost ghost added the CLA Signed label Sep 19, 2016

@bestander

This comment has been minimized.

Copy link
Contributor

bestander commented Sep 19, 2016

Yeah, trunk was broken over weekend.
Should be fixed now

@@ -84,6 +87,13 @@ public void setRenderToHardwareTexture(T view, boolean useHWTexture) {

@ReactProp(name = PROP_TEST_ID)
public void setTestId(T view, String testId) {
if (!testIDs.containsKey(testId)) {

This comment has been minimized.

@AaaChiuuu

AaaChiuuu Sep 19, 2016

Contributor

isn't this and line 94 checking the same thing? can you combine this logic?

This comment has been minimized.

@jsdevel

jsdevel Sep 19, 2016

Contributor

view.getResources().getIdentifier(testId, "id", view.getContext().getPackageName()) will return 0 if no matching identifier is found. I wanted to avoid calling it more than once for a given testID if possible which is why there are 2 checks:

  • The first check is for caching purposes.
  • The second check is to avoid setting the testId on the View if the user didn't define it.
@AaaChiuuu

This comment has been minimized.

Copy link
Contributor

AaaChiuuu commented Sep 19, 2016

I'm pretty wary of setting the ids on views because we at least use the ids of the ReactRootViews to track the ReactRootViews so this can break something in a hard to track way...
I can't remember off the top of my head if we use the ids of non-ReactRootViews else where though...

@jsdevel jsdevel force-pushed the kogosoftwarellc:adding-support-for-resource-id branch 2 times, most recently from 4a9cf22 to e406eb9 Sep 19, 2016

@ghost ghost added the CLA Signed label Sep 19, 2016

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Sep 19, 2016

I'm pretty wary of setting the ids on views because we at least use the ids of the ReactRootViews to track the ReactRootViews so this can break something in a hard to track way...

  • Is there an example of where a ReactRootView would ever have a testID? I'd like to test that in JSX if possible.
  • I could add a check and only set the id if view.getId() == 0.

@jsdevel jsdevel force-pushed the kogosoftwarellc:adding-support-for-resource-id branch from e406eb9 to 5a61912 Sep 19, 2016

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Sep 19, 2016

@AaaChiuuu I added an additional check to avoid setting the id on the View if it currently has an Id.

@ghost ghost added the CLA Signed label Sep 19, 2016

@jsdevel jsdevel force-pushed the kogosoftwarellc:adding-support-for-resource-id branch from 5a61912 to 62f66db Sep 19, 2016

@ghost ghost added the CLA Signed label Sep 19, 2016

@jsdevel jsdevel force-pushed the kogosoftwarellc:adding-support-for-resource-id branch from 62f66db to 85dee1d Sep 19, 2016

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Apr 26, 2017

what does the element look like in uiautomatorviewer?

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Apr 26, 2017

I fail to see how that works, given that BaseViewManager doesn't do anything fancy with accessibilityLabel.

@jonesdar

This comment has been minimized.

Copy link

jonesdar commented Apr 26, 2017

It's quite simple really - the string in the accessibilityLabel prop ends up in the Android View's content description ('content-desc' field in uiautomatorviewer).
Appium's waitForElementById uses several locator strategies internally (you can see them in the log I posted) and one of them (accessibility id) searches View content descriptions.

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Apr 26, 2017

Appium's waitForElementById uses several locator strategies internally (you can see them in the log I posted) and one of them (accessibility id) searches View content descriptions.

OK. I see that now! That's cool. So waitForElementById will work, but what about actions like .elementById?

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Apr 26, 2017

Your tests would likely run faster if you used waitForElementByAccessibilityId instead would they not?

@jonesdar

This comment has been minimized.

Copy link

jonesdar commented Apr 26, 2017

elementById works the same way.
Yes waitForElementByAccessibilityId might be slightly faster - but it wouldn't work on iOS and I don't really want to make my tests platform-specific.

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Apr 26, 2017

I see. That's pretty cool in the sense that it works. I'd say your workaround is probably the best approach that we have at the moment. It would be much easier if testID was all that was needed though. Per the documentation, accessibilityLabel affects the behavior of the view when present, so it's possible the app works during testing when the workaround adds the label, but it doesn't work in production when the label is not present.

@jonesdar

This comment has been minimized.

Copy link

jonesdar commented Apr 27, 2017

@jsdevel Good that you now understand it but painful that I had to spend so much time convincing you. I suggest next time you keep more of an open mind - you could easily have spent a few minutes trying the workaround for yourself rather than repeatedly shooting it down with unhelpful comments like "There's no way that works" just because you didn't understand it.
Agree on the need to get testID working properly though. If I understand correctly uiautomator 2 has view tag support so to me the best solution would be for Appium to add that as a locator strategy in waitForElementById etc.

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Apr 27, 2017

painful that I had to spend so much time convincing you

@jonesdar It's quite possible that others will have similar questions. I wasn't trying to be difficult. Sorry if my questions caused you some consternation. You helped me to understand something about appium that I haven't seen documented anywhere and for that I am grateful.

If I understand correctly uiautomator 2 has view tag support so to me the best solution would be for Appium to add that as a locator strategy in waitForElementById etc.

In case any testing library authors are reading this, the method is findViewWithTag.

@skovhus

This comment has been minimized.

Copy link

skovhus commented May 3, 2017

@mkonicek is this still down prioritized?

@curioustechizen

This comment has been minimized.

Copy link

curioustechizen commented May 17, 2017

@jonesdar Thanks for this work-around! Just to point out, I had to use the plural forms of the locator methods for Appium to be able to find my view with the accessibilityLabel on Android: i.e., waitForElementsById instead of waitForElementById and elementsById instead of elementById.

Using the plural form is apparently what tells Appium to try multiple strategies. The clue was in the Appium server debug logs. When I used the singular form, the log had the equivalent of

[debug] [AndroidBootstrap] Sending command to android: {"cmd":"action","action":"find","params":{"strategy":"id","selector":"Email","context":"","multiple":false}}

The "multiple":false prompted me to explore this a bit and I finally found the answer here

jsdevel referenced this pull request in EdtechFoundry/react-native May 24, 2017

feat: Adding full support for testID with uiautomator.
* Calling view.setId() with the matching resource-id of an id found in R.class.  Added TestIdUtil to facilitate this.
* Updating the android sample project to include a testID example.  Updating the e2e test to use it.
* Changing the signature for virtually all Event Classes to require the View instead of the viewTag.  This reduces the number of locations where TestIdUtil.getOriginalReactTag is called.
* Minimizing the impact in non __DEV__ environments where testID should not be set by simply returning view.getId() in TestIdUtil.getOriginalReactTag.
* This closes facebook#9777.

@itsmepetrov itsmepetrov referenced this pull request May 25, 2017

Closed

Android Support #96

@voidgit

This comment has been minimized.

Copy link

voidgit commented Sep 26, 2017

@mkonicek Are there any plans to proceed with this issue?
Because tag is not available via Appium on Android (and will be not available in foreseeable futurte) due to UIAutomator2 limitations (appium/appium#6025 (comment))
and only AccessibilityLabel is left and given this is not an id and we are going to use AccessibilityLabel for, actually, accessibility there is no viable way to test react-native applications on Android.

@marlenabowen

This comment has been minimized.

Copy link

marlenabowen commented Dec 13, 2017

@mkonicek any ETA for when FB might open-source the internal e2e framework?

@kholiavko-roman

This comment has been minimized.

Copy link

kholiavko-roman commented Jan 22, 2018

Can I now to set resource-id for android from react-native ?
I need it for running pre-launch tests on Goolge Play.

@ThaJay

This comment has been minimized.

Copy link

ThaJay commented Jan 23, 2018

I'm afraid not. This should have been handled / merged months ago.

@mkonicek

This comment has been minimized.

Copy link
Contributor

mkonicek commented Feb 4, 2018

Hey everyone, so sorry about the long radio silence about this. I left fb in the meantime and went traveling for 4 months and then joined a startup where I'm doing some native Android dev for now, currently playing with integrating React Native to an existing native app.

I feel very sorry about closing this PR and seeing it disappointed so many people. I thought someone from the community would provide a way to look up elements by Android view tags like we did at fb and the issue would be solved. It looks like it's harder to add support to Appium than I thought, but will happen eventually (see appium-espresso-driver, found via a comment on the Appium issue).

At the same time, I'm glad there is the workaround by @jonesdar (#9942 (comment)). To me it looks like a reasonable workaround and I'd use it when testing my app with Appium. I'll copy it here to make it easier to discover (at the end of the comments section). Quoting @jonesdar:

I defined a testProps(id) function which returns an object {testID: id, accessibilityLabel: id}.
It's used in all our RN views e.g.
<View {...testProps('idForThisView')}>

Appium can then find elements by id on both iOS and Android. Yes, the accessibilityLabel is user-visible but testProps is conditional on build type so we only include these props for a test build anyway.
It means we're not quite testing with the exact build we ship, but it's worked well for the last 9-10 months.

@jonesdar

This comment has been minimized.

Copy link

jonesdar commented Feb 4, 2018

Worth pointing out that the workaround needs a few tweaks on newer versions of Android.
Firstly you'll need to specify automationName: 'uiautomator2' in Appium's desired capabilities (I think the default 'uiautomator' has been deprecated from Android 5.0 onwards).

Then you'll notice that waitForElementById, elementById etc. no longer do multiple locator strategy fallbacks (so elementById won't fall back to elementByAcessibilityId etc.) - I fixed that as follows:

    if ('android' === process.env.APPIUM_PLATFORM) {
      // uiautomator2 doesn't seem to do multiple locator strategy fallbacks
      // so map id to accessibility id directly
      wd.addPromiseChainMethod('waitForElementById', id => {
        return this._driver.waitForElementByAccessibilityId(id);
      });

      wd.addPromiseChainMethod('elementById', id => {
        return this._driver.elementByAccessibilityId(id);
      });

      wd.addPromiseChainMethod('elementByIdOrNull', id => {
        return this._driver.elementByAccessibilityIdOrNull(id);
      });

      wd.addElementPromiseChainMethod('elementById', function() {
        return this.elementByAccessibilityId(id);
      });
    }

i.e. centrally map waitForElementById() to waitForElementByAccessibilityId() etc. - to ensure that Appium finds the testIDs we've put into the accessibilityLabels.
Note that you could just use the accessibility id methods on Android and the id methods on iOS but I've always used the same test code on both platforms and I don't want it to diverge now.

I think I read somewhere that you can do multiple locator strategy fallbacks by using plurals (waitForElementsById, elementsById etc.) but I haven't tried that - and it would still require a global find/replace in all test code (or a central mapping as above) anyway to get existing tests working again.

On the plus side, uiautomator2 seems to run my tests twice as fast as uiautomator.

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Feb 8, 2018

I'd happily get this working again 😄 any takers from the RN team?

@jribeiro

This comment has been minimized.

Copy link

jribeiro commented Feb 22, 2018

I've written a babel plugin to address this issue in the meantime. The idea is to add a property accessibilityLabel every time a testId jsx property is found. Essentially it'll allow you to provide meaningful accesibilityLabels in your application and on an Android specific build replace this by the defined testIDs.
https://github.com/jribeiro/babel-plugin-jsx-property-alias

Hope it helps

@lightboys22

This comment has been minimized.

Copy link

lightboys22 commented Mar 23, 2018

I realize that my pull request is very similar to this pull request.

@jsdevel

This comment has been minimized.

Copy link
Contributor

jsdevel commented Mar 23, 2018

@lightboys22 I'm sorry that you went down this road. I feel your pain 😢

@Mishan999

This comment has been minimized.

Copy link

Mishan999 commented Mar 29, 2018

Good

@tylermurry

This comment has been minimized.

Copy link

tylermurry commented May 1, 2018

We use Appium to test our applications and I was able to solve this problem very elegantly with react-native-testid.

@lightboys22

This comment has been minimized.

Copy link

lightboys22 commented May 1, 2018

@tylermurry react-native-testid will change the accessibilityLabel on Android platform. Our accessibilityLabels won't be unique.

@Frank1234

This comment has been minimized.

Copy link

Frank1234 commented Jul 4, 2018

A copy and paste solution using Espresso and the solution from #9942 in this article:

Espresso testing React Native

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment