Closed Bug 1112605 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox for Android Graveyard :: Testing, defect)

35 Branch
All
Android
defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: Margaret, Assigned: bhargavch, Mentored)

Details

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

Attachments

(1 file, 3 obsolete files)

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
Mentor: margaret.leibovic, nalexander
Whiteboard: [lang=java]
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.
Whiteboard: [lang=java] → [lang=java][good first bug]
Attached patch bug1112605.patch (obsolete) — Splinter Review
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)
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-
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)
(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)
Attachment #8557623 - Attachment is obsolete: true
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+
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)
Attachment #8571464 - Attachment is obsolete: false
Attachment #8573214 - Attachment is obsolete: true
Attachment #8573214 - Flags: review?(margaret.leibovic)
Flags: needinfo?(margaret.leibovic)
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)
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+
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)
(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
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 39
(In reply to :Margaret Leibovic from comment #15)
Thank you Margaret for mentoring me fix my first bug which got landed :)
(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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.