1112605 - FenneNativeElement "unable to find view" error should log human-readable view id

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[:Margaret Leibovic]

Running a test locally, I ran into this error:

"unable to find view 2131558510"

Looks like that comes from here:
http://mxr.mozilla.org/mozilla-central/source/build/mobile/robocop/FennecNativeElement.java#47

It would be a great improvment if we could instead log the id name. It's not immedidately clear to me how we can do that from the id, hopefully there's some trick to doing this that wouldn't require refactoring FennecNativeElement.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

iirc, FennecNativeElement was a stand-in for finding views with Robotium because we couldn't do that, for some reason.

I wonder if we can't wrap some Robotium methods to mimic this behavior, which are more robust (and, yes, this is a refactor but it could fix some other bugs as well).

For a direct solution to this problem: `for (View v : mSolo.getViews()) { if (v.getId() == 2131558510) { ... } }`. I'm pretty sure Robotium getViews() captures Views that are not visible on the screen as well - check the source and make sure!

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to :Margaret Leibovic from comment #0)
> Running a test locally, I ran into this error:
> 
> "unable to find view 2131558510"
> 
> Looks like that comes from here:
> http://mxr.mozilla.org/mozilla-central/source/build/mobile/robocop/
> FennecNativeElement.java#47
> 
> It would be a great improvment if we could instead log the id name. It's not
> immedidately clear to me how we can do that from the id, hopefully there's
> some trick to doing this that wouldn't require refactoring
> FennecNativeElement.

Start with:

http://developer.android.com/reference/android/content/res/Resources.html#getResourceName%28int%29

Comment 3

[Manu Jain]

I would like to patch this bug. Do we need to change "resource id" with "resource name" ?

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Manu Jain from comment #3)
> I would like to patch this bug. Do we need to change "resource id" with
> "resource name" ?

The idea is to find the "resource name" (like "R.id.button" from [2]) corresponding to the numeric "resource id".  I think the function Resources.getResourceName will help; you can experiment.  Then, include the resource name in the error message referred to in comment 1.

Comment 5

[Manu Jain]

Attachment: bug1112605.patch

I have attached the patch. Sorry for the late response. Please check it and tell whether it is proper or not. Thanks!!

Comment 6

[:Margaret Leibovic]

Comment on attachment 8557623 [details] [diff] [review]
bug1112605.patch

Review of attachment 8557623 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/mobile/robocop/FennecNativeElement.java
@@ +18,4 @@
>  
>      public FennecNativeElement(Integer id, Activity activity) {
>          mId = id;
> +        mName = getResources().getResourceName(id);

I haven't tested this locally, but does this actually work? getResources isn't a method on Element or FennecNativeElement, I think you'll need to call activity.getResources().

Please also confirm that this patch actually works by running a robocop test locally. You should just modify an existing test to try to get a view that doesn't exist, and then you should see the new error message appear in the log.

@@ +45,5 @@
>                                  "Robocop called click on an element with no listener");
>                          }
>                      } else {
>                          FennecNativeDriver.log(FennecNativeDriver.LogLevel.ERROR,
> +                            "click: unable to find view "+mName);

Nit: please put spaces around the +.

Also, I see these error message declared twice in this file, let's fix it in both places.

Comment 7

[Bhargav Chippada [:bhargavch]]

Attachment: Patch_[Bug_1112605]_FennecNativeElement_view_name_in_log_msg.patch

Hi, I would like to be assigned to this bug if no one is working on it. I created a patch but i don't know how to test it by modifying one of the existing tests and running them. 
This is what i did: I am using intellij so i right click the robocop folder containing org.mozilla.gecko.tests and select 'Run "All test"'. As suggested i am modifying one of the tests to access a view which doesn't exist before running the tests. Is this the correct way?

Comment 8

[:Margaret Leibovic]

(In reply to Bhargav Chippada [:bhargavch] from comment #7)

> Hi, I would like to be assigned to this bug if no one is working on it.

Hi Bhargav, it looks like Manu was already working on this bug, but he hasn't been back in a while to update it, so I think it's fine if you take over here :)

> I created a patch but i don't know how to test it by modifying one of the
> existing tests and running them. 
> This is what i did: I am using intellij so i right click the robocop folder
> containing org.mozilla.gecko.tests and select 'Run "All test"'. As suggested
> i am modifying one of the tests to access a view which doesn't exist before
> running the tests. Is this the correct way?

Ah, this can be confusing. I don't believe we support running robocop tests from inside IntelliJ, so you'll need to run them from the command line. You can do that by following the instructions here:
https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop

Feel free to join #mobile on irc.mozilla.org if you have more questions about running the tests locally, that's where the developers on our team hang out and should be able to help you.

Comment 9

[:Margaret Leibovic]

Comment on attachment 8571464 [details] [diff] [review]
Patch_[Bug_1112605]_FennecNativeElement_view_name_in_log_msg.patch

Review of attachment 8571464 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like this patch doesn't match the format we normally expect. You should follow the instructions here to make sure to generate a git style patch:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

But overall this looks like it's on the right track! You'll just need to update the format of the patch, and get tests running locally to verify that this works as expected.

::: ../../../../build/mobile/robocop/FennecNativeElement.java
@@ +42,4 @@
>                              mClickSuccess = true;
>                          } else {
>                              FennecNativeDriver.log(FennecNativeDriver.LogLevel.WARN,
> +                                "Robocop called click on an element with no listener "+mId+" "+mName);

I know you're following the pattern that already exists in this file, but please add spaces around the + operators, since that's the normal style we follow in our Java code.

Comment 10

[Bhargav Chippada [:bhargavch]]

Attachment: Patch_[Bug_1112605]_FennecNativeElement_view_name_in_log_msg_r=Margaret_Leibovic.patch

I added spaces around + as you suggested. Is the format of this patch correct? Is the commit message proper?

Comment 11

[Bhargav Chippada [:bhargavch]]

Attachment: Patch_[Bug_1112605]_FennecNativeElement_view_name_in_log_msg_r=Margaret_Leibovic.patch

I added spaces around + as you suggested. Is the format of this patch correct? Is the commit message proper?
PS: by mistake i submitted the wrong patch before but this is the one..

Comment 12

[:Margaret Leibovic]

Comment on attachment 8573219 [details] [diff] [review]
Patch_[Bug_1112605]_FennecNativeElement_view_name_in_log_msg_r=Margaret_Leibovic.patch

Review of attachment 8573219 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Were you able to get the tests running locally to verify the output? I can also make a try run for you to make sure this doesn't cause any issues for existing tests.

Comment 13

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=341ecd253164

Comment 14

[Bhargav Chippada [:bhargavch]]

I was able to run the robocop tests but i am not able to verify the patch. I am building the robocop tests using cmd './mach build build/mobile/robocop' and running a single test using ./mach robocop <testFilename>. Am i doing it right? tell me which line to modify inside some test file and how to run it, i tried but i am not able to verify :/
is treeherder used for testing the patch for existing tests? the patch didn't cause any issues right?

Comment 15

[:Margaret Leibovic]

(In reply to Bhargav Chippada [:bhargavch] from comment #14)
> I was able to run the robocop tests but i am not able to verify the patch. I
> am building the robocop tests using cmd './mach build build/mobile/robocop'
> and running a single test using ./mach robocop <testFilename>. Am i doing it
> right? tell me which line to modify inside some test file and how to run it,
> i tried but i am not able to verify :/

I'm sorry this wasn't very well defined. To reproduce this, you would need to explicitly create some test that fails to find a view. While it would be nice to verify that this patch does indeed do what it sets out to do, it's pretty straightforward, so I trust that it works, and we can just land it.

> is treeherder used for testing the patch for existing tests? the patch
> didn't cause any issues right?

Yes, this looks good, we can land it.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/0f761f2a95be

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0f761f2a95be

Comment 18

[Bhargav Chippada [:bhargavch]]

(In reply to :Margaret Leibovic from comment #15)
Thank you Margaret for mentoring me fix my first bug which got landed :)

Comment 19

[:Margaret Leibovic]

(In reply to Bhargav Chippada [:bhargavch] from comment #18)
> (In reply to :Margaret Leibovic from comment #15)
> Thank you Margaret for mentoring me fix my first bug which got landed :)

You're welcome! Let me know if you need help finding more bugs to work on. Here's a list of our mentor bugs:
http://www.joshmatthews.net/bugsahoy/?mobileandroid=1