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

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: bhargavch, Mentored)

Tracking

35 Branch
Firefox 39
All
Android
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [lang=java][good first bug])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.
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!
(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
(Reporter)

Updated

4 years ago
Mentor: margaret.leibovic, nalexander
Whiteboard: [lang=java]

Comment 3

4 years ago
I would like to patch this bug. Do we need to change "resource id" with "resource name" ?
(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.
(Reporter)

Updated

4 years ago
Whiteboard: [lang=java] → [lang=java][good first bug]

Comment 5

4 years ago
Created attachment 8557623 [details] [diff] [review]
bug1112605.patch

I have attached the patch. Sorry for the late response. Please check it and tell whether it is proper or not. Thanks!!
Attachment #8557623 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 6

4 years ago
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.
Attachment #8557623 - Flags: review?(margaret.leibovic) → review-
(Assignee)

Comment 7

4 years ago
Created attachment 8571464 [details] [diff] [review]
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?
Flags: needinfo?(margaret.leibovic)
Attachment #8571464 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 8

4 years ago
(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.
Assignee: nobody → bhargav.chippada19
Flags: needinfo?(margaret.leibovic)
(Reporter)

Updated

4 years ago
Attachment #8557623 - Attachment is obsolete: true
(Reporter)

Comment 9

4 years ago
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.
Attachment #8571464 - Flags: review?(margaret.leibovic) → feedback+
(Assignee)

Comment 10

4 years ago
Created attachment 8573214 [details] [diff] [review]
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?
Attachment #8571464 - Attachment is obsolete: true
Flags: needinfo?(margaret.leibovic)
Attachment #8573214 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

4 years ago
Attachment #8571464 - Attachment is obsolete: false
(Assignee)

Updated

4 years ago
Attachment #8573214 - Attachment is obsolete: true
Attachment #8573214 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

4 years ago
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 11

4 years ago
Created attachment 8573219 [details] [diff] [review]
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..
Attachment #8571464 - Attachment is obsolete: true
Flags: needinfo?(margaret.leibovic)
Attachment #8573219 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 12

4 years ago
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.
Attachment #8573219 - Flags: review?(margaret.leibovic) → review+
(Reporter)

Comment 13

4 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=341ecd253164
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 14

4 years ago
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?
Flags: needinfo?(margaret.leibovic)
(Reporter)

Comment 15

4 years ago
(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.
Flags: needinfo?(margaret.leibovic)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/0f761f2a95be
Keywords: checkin-needed
Whiteboard: [lang=java][good first bug] → [lang=java][good first bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0f761f2a95be
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 39
(Assignee)

Comment 18

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

Comment 19

4 years ago
(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
You need to log in before you can comment on or make changes to this bug.