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

Hi, I would like to work on this. Can I get assigned to this one? Thanks!

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!

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

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?

(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

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

(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

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

checkin-needed Alright, thanks for the clarification!

Thanks for the patch! https://hg.mozilla.org/integration/fx-team/rev/183f367cda86