If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Update findListViewWithTag consumers to use strings from HomePager

RESOLVED FIXED in Firefox 33

Status

()

Firefox for Android
Testing
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Margaret, Assigned: Franz Sarmiento, Mentored)

Tracking

Trunk
Firefox 33
All
Android
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Right now they just specify strings themselves, but these tags can change.

This issue was raised in bug 1004850, but it existed before the tests were updated for that bug.
To be more specific, the strings at [1] should be updated to use the associated constants defined in the HomePager class [2].

[1]: https://mxr.mozilla.org/mozilla-central/search?string=findlistviewwithtag&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomePager.java?rev=9801180cd3b7#67
Whiteboard: [lang=java] → [lang=java][good first bug]
(Assignee)

Comment 2

3 years ago
Hi, I would like to work on this. Can I get assigned to this one? Thanks!
(Assignee)

Comment 3

3 years ago
Created attachment 8445008 [details] [diff] [review]
0001-Bug-1028728-Update-findListViewWithTag-consumers-to-.patch

After applying this patch, I rebuilt Fennec and have tested it on an actual device to make sure there are no regressions.
Sure, Franz - you've been assigned!
Assignee: nobody → franzks
Status: NEW → ASSIGNED
Comment on attachment 8445008 [details] [diff] [review]
0001-Bug-1028728-Update-findListViewWithTag-consumers-to-.patch

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

After posting patches, don't forget to r? a reviewer! Since I just reviewed this, you don't need to worry about that for this patch.

That being said, this looks good to me. Here's a run on our Try test server: https://tbpl.mozilla.org/?tree=Try&rev=e6692e6e397c
Attachment #8445008 - Flags: review+
Comment on attachment 8445008 [details] [diff] [review]
0001-Bug-1028728-Update-findListViewWithTag-consumers-to-.patch

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

As per the try run in comment 5, this doesn't build because the access to the values in HomePager is not public. Did you test your changes?
Attachment #8445008 - Flags: review+ → review-
(In reply to Michael Comella (:mcomella) from comment #6)
> Did you test your changes?

Ah, I see what happened! The robocop test framework is independent of the browser - you need to run the tests too! To set it up, see [1].

I recommend running one test at a time, as per [2].

[1]: https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop
[2]: https://wiki.mozilla.org/Auto-tools/Projects/Robocop#Running_a_single_test
(Assignee)

Comment 8

3 years ago
Created attachment 8445667 [details] [diff] [review]
0002-Bug-1028728-Update-findListViewWithTag-consumers-to-.patch

Ah, now I know what this dropdown for "review" was for. Thanks for the tip.

I did not notice the issue with the access modifiers. After applying the first patch, I built Fennec and it says build successful. Is it because the tests are separate from the browser, that's why it told me the build was successful anyway?

To test it, I read the links you gave me and tried to build just robocop using:
./mach build build/mobile/robocop
Sure thing, it showed me the errors you pointed out. After applying this patch #2, I rebuilt fennec once again and rebuilt robocop as well. Build was fine, and I could run the tests.

One thing though: out of all the affected tests, only testAddSearchEngine and testBookmarkFolders can be ran. The others are giving me an invalid TEST_PATH error. I did a grep and found this robocop.ini file and saw that the others tests were actually commented out:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/robocop.ini#25
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/robocop.ini#43
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/robocop.ini#77
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/robocop.ini#89

Same with the AboutHomeTest, but this one I can't find specifically in robocop.ini. Is it this one (also commented out):
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/robocop.ini#120
Attachment #8445008 - Attachment is obsolete: true
Attachment #8445667 - Flags: review?(michael.l.comella)
(In reply to Franz Sarmiento from comment #8)
> I did not notice the issue with the access modifiers. After applying the
> first patch, I built Fennec and it says build successful. Is it because the
> tests are separate from the browser, that's why it told me the build was
> successful anyway?

Correct - we don't build the tests when we build the browser, and your changes (mostly) only modified test code.

> One thing though: out of all the affected tests, only testAddSearchEngine
> and testBookmarkFolders can be ran. The others are giving me an invalid
> TEST_PATH error. I did a grep and found this robocop.ini file and saw that
> the others tests were actually commented out:

The tests sometimes get commented out because they fail too frequently (because they're poorly written, the code changed underneath and the tests were not updated, etc.). If you follow the bug numbers in the comments next to the disabled test, you can usually see an explanation.

For now, update the code, but don't worry too much about them since they're disabled - the compiler will likely catch any issues related to your changes.

> Same with the AboutHomeTest, but this one I can't find specifically in
> robocop.ini. Is it this one (also commented out):
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/
> robocop.ini#120

AboutHomeTest is actually a used base class and is never run directly. For example, see [1], where `testAddSearchEngine extends AboutHomeTest`.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testAddSearchEngine.java?rev=523e67cf4b9e#20
Comment on attachment 8445667 [details] [diff] [review]
0002-Bug-1028728-Update-findListViewWithTag-consumers-to-.patch

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

lgtm.

try: https://tbpl.mozilla.org/?tree=Try&rev=fec7582c7932
Attachment #8445667 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 11

3 years ago
Alright, thanks for the clarifications. It all makes sense now.

So, what actions do I need to take right now? Or should I simply wait for more people to review this to have it finally checked in?
(In reply to Franz Sarmiento from comment #11)
> Alright, thanks for the clarifications. It all makes sense now.
> 
> So, what actions do I need to take right now? Or should I simply wait for
> more people to review this to have it finally checked in?

You need to use the "checkin-needed" keyword to get your patch landed [1]. We require all patches with "checkin-needed" to have a try run (a test server run) associated with it, which will usually be taken care of by the mentor of your bug until you gain such commit access. Once the tests pass (are green), you can add the "checkin-needed" keyword.

The run in comment 10 is green, so please add "checkin-needed" keyword whenever you are ready!

Note that we generally only need one reviewer per patch that gets checked in.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Flags: needinfo?(franzks)
(Assignee)

Comment 13

3 years ago
checkin-needed

Alright, thanks for the clarification!
Flags: needinfo?(franzks)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Thanks for the patch!

https://hg.mozilla.org/integration/fx-team/rev/183f367cda86
Flags: in-testsuite+
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/183f367cda86
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 33
You need to log in before you can comment on or make changes to this bug.