Closed Bug 1001655 Opened 6 years ago Closed 5 years ago

Replace applicable Strings in test code with StringHelper references

Categories

(Firefox for Android :: Testing, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: mcomella, Assigned: mcomella)

Details

Attachments

(1 file, 2 obsolete files)

Sometimes Strings are written verbatim in the test code [1] when they'd be better written with a StringHelper constant [2]! Find these instances and replace them with their StringHelper constants - I'd recommend automating this process using grep.

Additionally, if you find any Strings in the test code that do not have equivalent StringHelper references, you should add them (depending on how useful they are - use your judgment)!

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testHomeBanner.java?rev=523e67cf4b9e#44
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/StringHelper.java?rev=523e67cf4b9e#22
Hello, I'm interested in taking care of this as my first contribution.

Just a quick question, when a test uses a string from one of the String arrays (such as CONTEXT_MENU_ITEMS_IN_PRIVATE_TAB), can I just replace it with a hardcoded index into the array, e.g. CONTEXT_MENU_ITEMS_IN_PRIVATE_TAB[3] for "Bookmark Link"?
(In reply to Junseok from comment #1)
> Hello, I'm interested in taking care of this as my first contribution.

Fantastic and welcome to Bugzilla, Junseok! You've been assigned! :)

> Just a quick question, when a test uses a string from one of the String
> arrays (such as CONTEXT_MENU_ITEMS_IN_PRIVATE_TAB), can I just replace it
> with a hardcoded index into the array, e.g.
> CONTEXT_MENU_ITEMS_IN_PRIVATE_TAB[3] for "Bookmark Link"?

A good question! I would actually prefer it if those arrays were constructed with other constants because it ensures a single definition (the less duplication, the better) and is more readable. For example:

public static final String BOOKMARK_LINK = "Bookmark Link";

public static final String[] CONTEXNT_MENU_ITEMS_IN_PRIVATE_TAB = new String[] {
    BOOKMARK_LINK,
    ...
};

---
To get started, have you already set up a build environment? If not, you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

The tests you would be fixing are in our Robocop test framework, so you'll need to set that up too: https://wiki.mozilla.org/Auto-tools/Projects/Robocop

When you make changes in the mobile/android/base/tests/ directory, you should only need to recompile Robocop, not the entire browser, for these changes.

While you're developing, I recommend running just a single test at a time because the full robocop test suite takes a long time to run (at least 30 minutes).

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Thanks and happy coding! ^_^
Assignee: nobody → lee.junseok
Status: NEW → ASSIGNED
Good call, and thank you for the links. I just followed [1], so my environment probably isn't set up correctly. I'll go through the environment setup later tonight.

Until then, here is an "hg diff" of what I've changed so far [2]. If you're free, please look it over to make sure I'm not doing something horribly wrong. Thanks for the warm welcome and guidance!

[1]: https://developer.mozilla.org/en-US/docs/Introduction
[2]: http://pastebin.com/gk0ZgcuK
Nice! While I only gave it a skim, it is looking splendid!

Traditionally, we post patches as attachments to the bug here on Bugzilla - you can follow the guide at [1] to set up your environment to generate a patch.

By the way, nice catch on `MASTER_PASWSWORD_LABEL`. :)

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Hello! Sorry about the delay; I don't know why starting this during finals season was a good idea.

Anyway, I'm back on it. The tests build successfully, and I'm currently running through the mochitests. I seem to be getting a lot of "runTest exited with code 1" and something about a missing end of test marker. I'll let you know once they finish running.
(In reply to Junseok from comment #5)
> Hello! Sorry about the delay; I don't know why starting this during finals
> season was a good idea.

No worries - take your time! Can't say I've never done something similar before. :P

> Anyway, I'm back on it. The tests build successfully, and I'm currently
> running through the mochitests. I seem to be getting a lot of "runTest
> exited with code 1" and something about a missing end of test marker. I'll
> let you know once they finish running.

There is a precedent for occassional missing end of test marker errors (though usually on Android 2.3 devices): see bug 951181. If it doesn't happen everytime, I wouldn't worry too much about it.
(In reply to Michael Comella (:mcomella) from comment #6)
> There is a precedent for occassional missing end of test marker errors
> (though usually on Android 2.3 devices): see bug 951181. If it doesn't
> happen everytime, I wouldn't worry too much about it.

Actually, recently there was a landed patch in bug 924622 that fixed a lot of these issues, except for [1]. Make sure you pull the latest code and if you're still seeing these errors (besides [1]), perhaps it is an issue with your changes.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1008422#c95
It took me a while, but I pulled again and I finally got the tests running; turns out I didn't even have Fennec on the device while I was trying to run the tests before. Major facepalm haha.

I ran all the tests and I got 68 passes and 0 fails! I'll put a patch together using your instructions and put it up here when it's ready. Thanks for your guidance!
Cool beans! Looking forward to it! :)
I'm not sure what the best way to post patches is, so I just self hosted it here: http://jnsk.cc/test-strings.patch

Let me know if there's anything I have to change or if I should put it up somewhere else.
(In reply to Junseok from comment #10)
> I'm not sure what the best way to post patches is

You can upload patches directly to Bugzilla - click the "Add an attachment" link. Then flag me for review on the patch. You can do this by entering my bugzilla email. Since that's unreasonable to find/type in every time, you can also auto-complete names. Notably, the ":<IRC-nick>" paradigm in people's names is the best way to autocomplete. For me, use ":mcomella".
Attached patch patch for bug 1001655 (obsolete) — Splinter Review
Attachment #8433689 - Flags: review?(michael.l.comella)
Comment on attachment 8433689 [details] [diff] [review]
patch for bug 1001655

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

Looking really good - just a few small, non-functionality fixes to make!

I've pushed your patch to our test server to make sure I didn't miss anything: https://tbpl.mozilla.org/?tree=Try&rev=94c9fd0b819c

::: mobile/android/base/tests/StringHelper.java
@@ +49,5 @@
>      public static final String[] CONTEXT_MENU_ITEMS_IN_PRIVATE_TAB = new String[] {
> +        CONTEXT_MENU_OPEN_LINK_IN_PRIVATE_TAB,
> +        CONTEXT_MENU_COPY_LINK,
> +        CONTEXT_MENU_SHARE_LINK,
> +        CONTEXT_MENU_BOOKMARK_LINK 

nit: We don't like to leave excess whitespace at the end of lines - do you mind removing it?

If you're curious about some of the motivations, please ask.

@@ +57,5 @@
> +        CONTEXT_MENU_OPEN_LINK_IN_NEW_TAB,
> +        CONTEXT_MENU_OPEN_LINK_IN_PRIVATE_TAB,
> +        CONTEXT_MENU_COPY_LINK,
> +        CONTEXT_MENU_SHARE_LINK,
> +        CONTEXT_MENU_BOOKMARK_LINK 

nit: whitespace

@@ +150,4 @@
>      public static final String CLEAR_PRIVATE_DATA_LABEL = "Clear private data";
>  
>      // Mozilla
> +    public static final String BRAND_NAME = "(Fennec|Nightly|Aurora|Firefox Beta|Firefox)";

Nice cleanup!

::: mobile/android/base/tests/testAddonManager.java
@@ +87,3 @@
>  
>          // Addons Manager is not opened 2 separate times when opened from the menu
> +        selectMenuItem(StringHelper.ADDONS_LABEL);        

nit: Since you're touching this line, can you remove the EOL whitespace?

::: mobile/android/base/tests/testBookmarksPanel.java
@@ +60,5 @@
>      * @param values String array with the new values for all fields
>      */
>      private void editBookmark(String bookmarkUrl, String[] values) {
>          openBookmarkContextMenu(bookmarkUrl);
> +        mSolo.clickOnText(StringHelper.BOOKMARK_CONTEXT_MENU_ITEMS[2]);

I apologize as it seems I misinterpreted your question from comment 1! I would prefer if this used the constant directly (i.e. CONTEXT_MENU_EDIT).

@@ +80,5 @@
>      * @param values String array with the new values for all fields
>      */
>      private void checkBookmarkEdit(String bookmarkUrl, String[] values) {
>          openBookmarkContextMenu(bookmarkUrl);
> +        mSolo.clickOnText(StringHelper.BOOKMARK_CONTEXT_MENU_ITEMS[2]);

Same.

::: mobile/android/base/tests/testMasterPassword.java
@@ +21,5 @@
>  
>      public void enableMasterPassword(String password, String badPassword) {
>  
>          // Look for the 'Settings' menu if this device/OS uses it
> +        selectSettingsItem(StringHelper.PRIVACY_SECTION_LABEL, "Use master password");

You can use MASTER_PASSWORD_LABEL here.

@@ +81,5 @@
>  
>      public void disableMasterPassword(String password, String badPassword) {
>  
>          // Look for the 'Settings' menu if this device/OS uses it
> +        selectSettingsItem(StringHelper.PRIVACY_SECTION_LABEL, "Use master password");

MASTER_PASSWORD_LABEL.

@@ +138,5 @@
>  
>      public void clearPrivateData() {
>  
>          // Look for the 'Settings' menu if this device/OS uses it
> +        selectSettingsItem(StringHelper.PRIVACY_SECTION_LABEL, "Clear private data");

CLEAR_PRIVATE_DATA_LABEL
Attachment #8433689 - Flags: review?(michael.l.comella) → feedback+
Hey, Junseok.

I just want to warn you that if you wait too long, your patch will start to bitrot (the code underneath changes before your patch gets into the repository so that patch does not apply cleanly). This will likely be a pain to fix for your patch because you're touching so much code!
Mentor: michael.l.comella
Whiteboard: [mentor=mcomella][lang=java] → [lang=java]
Hey, Junseok. Are you still working on this?
Flags: needinfo?(lee.junseok)
I'd like for this to land so I'm going to take this over.
Assignee: lee.junseok → michael.l.comella
Mentor: michael.l.comella
Flags: needinfo?(lee.junseok)
Whiteboard: [lang=java]
I rebased and updated for the nits in the review in comment 13. I made two extra changes to testMasterPassword before I realized it was futile to do by hand. This is not meant to be comprehensive (hence part 1) and does not take into account changes since this patch was initially made. Note that I have no intention of continuing part 2 and will likely open it up as a mentored bug.
Attachment #8461048 - Flags: review?(wjohnston)
The try run in comment 17 is for the patch in comment 18.
Attachment #8433689 - Attachment is obsolete: true
Comment on attachment 8461048 [details] [diff] [review]
Part 1: Replace applicable Strings in test code with StringHelper references

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

This is pretty great. There's lots of little things we could do, but none that couldn't be done in a separate bug.

::: mobile/android/base/tests/SessionTest.java
@@ +140,5 @@
>              final PageInfo[] pages = tab.getItems();
>  
>              // New tabs always start with about:home, so make sure about:home
>              // is always the first entry.
> +            mAsserter.is(pages[0].url, StringHelper.ABOUT_HOME_URL, "first page in tab is about:home");

You should probably use the string in the message. i.e.

"first page in tab is " + StringHelper.ABOUT_HOME_URL

::: mobile/android/base/tests/StringHelper.java
@@ +150,5 @@
>      public static final String CLEAR_PRIVATE_DATA_LABEL = "Clear now";
>  
>      // Mozilla
> +    public static final String BRAND_NAME = "(Fennec|Nightly|Aurora|Firefox Beta|Firefox)";
> +    public static final String ABOUT_LABEL = "About " + BRAND_NAME;

I wonder if these regexy-strings should have that in their name. i.e.

BRAND_NAME_REGEX

Then again, maybe that's just added noise.

::: mobile/android/base/tests/testBookmarksPanel.java
@@ +134,1 @@
>          waitForText("Edit Bookmark");

Any reason we skipped this one?

@@ +139,5 @@
>              mSolo.clickOnEditText(i);
>              mActions.sendKeys(values[i]);
>          }
>  
>          mSolo.clickOnButton("OK");

Or this

@@ +154,1 @@
>          waitForText("Edit Bookmark");

Look! its even repeated!

::: mobile/android/base/tests/testMasterPassword.java
@@ +81,5 @@
>  
>      public void disableMasterPassword(String password, String badPassword) {
>  
>          // Look for the 'Settings' menu if this device/OS uses it
> +        selectSettingsItem(StringHelper.PRIVACY_SECTION_LABEL, "Use master password");

MASTER_PASSWORD_LABEL

::: mobile/android/base/tests/testPromptGridInput.java
@@ +36,5 @@
>  
>      public void test(final int num) {
>          // Load about:blank between each test to ensure we reset state
> +        loadUrl(StringHelper.ABOUT_BLANK_URL);
> +        mAsserter.ok(waitForText(StringHelper.ABOUT_BLANK_URL), "Loaded blank page", "page title match");

While you're here :) I have messages like "page title match". This should probably just be StringHelper.ABOUT_BLANK_URL.

::: mobile/android/base/tests/testReaderMode.java
@@ +103,5 @@
>          contentEventExpecter.blockForEvent();
>          contentEventExpecter.unregisterListener();
>  
>          // Check if the page is present in the Reading List
> +        mAsserter.ok(mSolo.waitForText(StringHelper.ROBOCOP_TEXT_PAGE_TITLE), "Verify if the page is added to your Reading List", "The page is present in your Reading List");

Same "The page is present in your Reading List" should probably be StringHelper.ROBOCOP_TEXT_PAGE_TITLE which gives some helpful info in the log. i.e. "This is the text that never appeared" and saves some digging around.

::: mobile/android/base/tests/testSettingsMenuItems.java
@@ +142,5 @@
>              String customizeString = "^Customize$";
>              waitForEnabledText(customizeString);
>              mSolo.clickOnText(customizeString);
>          }
> +        mAsserter.ok(mSolo.waitForText(StringHelper.SYNC_LABEL), "Waiting for Sync option", "The Sync option is present");

Same.

::: mobile/android/base/tests/testSystemPages.java
@@ +67,5 @@
>              contentEventExpecter = mActions.expectGeckoEvent("DOMContentLoaded");
>              selectMenuItemByPath(pathToItem);
>  
>              // Wait for the new tab and page to load
> +            if (StringHelper.ABOUT_URL.equals(expectedUrl)) {

ABOUT_URL should probably be ABOUT_SCHEME since its not a url, but this includes a ":" character, which makes it a bit more confusing. Maybe its just ABOUT_URL_PREFIX.
Attachment #8461048 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #20)
> Any reason we skipped this one?

Probably overlooked by the original patch author, and I merely rebased the changes given to me. That being said, for the upcoming patch (waiting on try), I made the changes you recommended.
Attachment #8461048 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/b0d56decbf83
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.