895716 - [fig] TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

This was failing on my try run, and seems to still be failing on fig. I can reproduce locally, so I'll investigate.

Comment 1

[:Margaret Leibovic]

It appears the issue here is that the second call to addTab is failing to enter a URL. The problem happens here:
https://hg.mozilla.org/projects/fig/file/f248fe823002/mobile/android/base/tests/BaseTest.java.in#l640

I tried using focusUrlBar() instead of this call to manually click on the EditText, but that caused addTab to just always fail :/

Maybe Adrian or Geoff can help me figure out what's going wrong here.

Comment 2

[Adrian Tamas (:AdrianT)]

I can look into this and help investigate

Comment 3

[Ed Morley [:emorley]]

(Adding keyword so this shows in TBPL's bug suggestions)

Comment 4

[Adrian Tamas (:AdrianT)]

Attachment: testNewTab fix v1

When clicking on the element I see a warning about the element not having a registered observer. I changed it to clickOnText and since every new tab will load about:home the title, "Enter Search or Address", will always be displayed so the click should not fail. I also added a delay to wait for the text "BOOKMARKS" after clicking add_tab in order to make sure that about:home was loaded.

I tested this locally and it works every time. Please run a tryserver run since I don't know and couldn't find how to run this on FIG on the tryserver

Comment 5

[:Margaret Leibovic]

Comment on attachment 778381 [details] [diff] [review]
testNewTab fix v1

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

Thanks for the patch! This does work, but I'd really like to see us move away from hard-coding strings like this. Could you pull them into constants at the top of the file?

::: mobile/android/base/tests/BaseTest.java.in
@@ +638,2 @@
>  
> +        // Since the Awesomescreen is about:home every new tab will have this title

We shouldn't talk about the "awesomescreen" in comments, since that doesn't actually exist anymore. Also, this isn't actually the title, but placeholder text that's shown because the title is empty. So maybe a better comment would be "The new tab will have a blank title, so click on the urlbar placeholder text".

Comment 6

[Adrian Tamas (:AdrianT)]

Attachment: newTabFigFix.patch v1.1

Defined the strings as variables and changed the comment to your suggestion

Comment 7

[:Margaret Leibovic]

Comment on attachment 779172 [details] [diff] [review]
newTabFigFix.patch v1.1

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

Thanks for the updated patch! I just have one more small comment.

::: mobile/android/base/tests/BaseTest.java.in
@@ +42,5 @@
>  
>      private static final String TARGET_PACKAGE_ID = "org.mozilla.gecko";
> +    private static final String LAUNCH_ACTIVITY_FULL_CLASSNAME ="@ANDROID_PACKAGE_NAME@.App";
> +    public static final String TITLE_PLACEHOLDER_TEXT = "Enter Search or Address";
> +    public static final String ABOUT_HOME_BOOKMARKS_TAB_TITLE = "BOOKMARKS";

These can be private for now, since they're not used anywhere else.

Nit: Could you put these variables in their own newline-separated section, with a comment saying they are strings that are in the UI? It would be similar to how there's a section down below for IDs of UI views:
https://hg.mozilla.org/projects/fig/file/f248fe823002/mobile/android/base/tests/BaseTest.java.in#l63

Comment 8

[Adrian Tamas (:AdrianT)]

(In reply to :Margaret Leibovic from comment #7)
> Comment on attachment 779172 [details] [diff] [review]
> newTabFigFix.patch v1.1
> 
> Review of attachment 779172 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the updated patch! I just have one more small comment.
> 
> ::: mobile/android/base/tests/BaseTest.java.in
> @@ +42,5 @@
> >  
> >      private static final String TARGET_PACKAGE_ID = "org.mozilla.gecko";
> > +    private static final String LAUNCH_ACTIVITY_FULL_CLASSNAME ="@ANDROID_PACKAGE_NAME@.App";
> > +    public static final String TITLE_PLACEHOLDER_TEXT = "Enter Search or Address";
> > +    public static final String ABOUT_HOME_BOOKMARKS_TAB_TITLE = "BOOKMARKS";
> 
> These can be private for now, since they're not used anywhere else.

I would not make these private since it's useful to wait for the tabs titles to check the new about:home is loaded and also for the title placeholder in some cases

> Nit: Could you put these variables in their own newline-separated section,
> with a comment saying they are strings that are in the UI? It would be
> similar to how there's a section down below for IDs of UI views:
> https://hg.mozilla.org/projects/fig/file/f248fe823002/mobile/android/base/
> tests/BaseTest.java.in#l63

I'll fix this and submit a new patch

Comment 9

[Adrian Tamas (:AdrianT)]

Attachment: testNewTab fix v1.2

Separated the strings in a new section and decided to make them private and if someone needs them in other tests they can just set them public then.

Comment 10

[:Margaret Leibovic]

Comment on attachment 779239 [details] [diff] [review]
testNewTab fix v1.2

Thanks!

Do you have level 3 commit access? If not, I can land this on fig for you.

Comment 11

[Adrian Tamas (:AdrianT)]

I don't have level 3 commit access. I can only add patches to the tryserver.

Comment 12

[:Margaret Leibovic]

(In reply to Adrian Tamas (:AdrianT) from comment #11)
> I don't have level 3 commit access. I can only add patches to the tryserver.

Okay, I'm working on merging m-c into fig right now, but I'll land your patch after I'm done. Thanks!

Comment 13

[Adrian Tamas (:AdrianT)]

(In reply to :Margaret Leibovic from comment #12)
> (In reply to Adrian Tamas (:AdrianT) from comment #11)
> > I don't have level 3 commit access. I can only add patches to the tryserver.
> 
> Okay, I'm working on merging m-c into fig right now, but I'll land your
> patch after I'm done. Thanks!

Thanks. BTW: is there anything special I should add to the commit message in order to push a patch to try for fig or do I have to push it to a special try address for fig? I only pushed to try patches on m-c

Comment 14

[:Margaret Leibovic]

(In reply to Adrian Tamas (:AdrianT) from comment #13)
> (In reply to :Margaret Leibovic from comment #12)
> > (In reply to Adrian Tamas (:AdrianT) from comment #11)
> > > I don't have level 3 commit access. I can only add patches to the tryserver.
> > 
> > Okay, I'm working on merging m-c into fig right now, but I'll land your
> > patch after I'm done. Thanks!
> 
> Thanks. BTW: is there anything special I should add to the commit message in
> order to push a patch to try for fig or do I have to push it to a special
> try address for fig? I only pushed to try patches on m-c

You don't need to change the commit message for fig. You can push to try like normal from fig, and your push will just include all the parent changesets on fig that aren't on m-c already (that's what I did when I was working on bug 880060).

The patch here is pretty low-risk, and I verified it made the test pass locally for me, so I just pushed it to fig:

https://hg.mozilla.org/projects/fig/rev/31d437a61ce1

Comment 15

[Legacy TBPL/Treeherder Robot]

margaret.leibovic%gmail.com
https://tbpl.mozilla.org/php/getParsedLog.php?id=25572151&tree=Fig
Android 4.0 Panda fig opt test robocop-1 on 2013-07-22 12:19:06
revision: 3ce8681e5602
slave: panda-0772

12:19:58    ERROR - Return code: 1
12:20:00    ERROR - Return code: 1
12:35:13     INFO -  11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
12:35:13     INFO -  junit.framework.AssertionFailedError: 11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
12:35:13     INFO -  12 INFO TEST-UNEXPECTED-FAIL | testNewTab | Exception caught - junit.framework.AssertionFailedError: 11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
12:35:15     INFO -  07-22 12:35:10.640 I/Robocop ( 2686): 11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
12:35:15     INFO -  07-22 12:35:10.828 I/Robocop ( 2686): junit.framework.AssertionFailedError: 11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
12:35:15     INFO -  07-22 12:35:10.828 I/Robocop ( 2686): 12 INFO TEST-UNEXPECTED-FAIL | testNewTab | Exception caught - junit.framework.AssertionFailedError: 11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
12:35:15     INFO -  07-22 12:35:11.945 I/TestRunner( 2686): junit.framework.AssertionFailedError: 12 INFO TEST-UNEXPECTED-FAIL | testNewTab | Exception caught - junit.framework.AssertionFailedError: 11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
12:40:37    ERROR - Return code: 1

Comment 16

[Legacy TBPL/Treeherder Robot]

margaret.leibovic%gmail.com
https://tbpl.mozilla.org/php/getParsedLog.php?id=25575711&tree=Fig
Android 4.0 Panda fig opt test robocop-1 on 2013-07-22 13:41:06
revision: a985831478dd
slave: panda-0761

13:42:00    ERROR - Return code: 1
13:42:02    ERROR - Return code: 1
14:03:45     INFO -  11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
14:03:45     INFO -  junit.framework.AssertionFailedError: 11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
14:03:45     INFO -  12 INFO TEST-UNEXPECTED-FAIL | testNewTab | Exception caught - junit.framework.AssertionFailedError: 11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
14:03:46     INFO -  07-22 14:03:41.320 I/Robocop ( 2728): 11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
14:03:46     INFO -  07-22 14:03:41.468 I/Robocop ( 2728): junit.framework.AssertionFailedError: 11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
14:03:46     INFO -  07-22 14:03:41.476 I/Robocop ( 2728): 12 INFO TEST-UNEXPECTED-FAIL | testNewTab | Exception caught - junit.framework.AssertionFailedError: 11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
14:03:46     INFO -  07-22 14:03:42.562 I/TestRunner( 2728): junit.framework.AssertionFailedError: 12 INFO TEST-UNEXPECTED-FAIL | testNewTab | Exception caught - junit.framework.AssertionFailedError: 11 INFO TEST-UNEXPECTED-FAIL | testNewTab | GeckoEventExpecter - blockForEvent timeout: DOMContentLoaded
14:09:09    ERROR - Return code: 1

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/31d437a61ce1

Comment 18

[Paul Feher]

Since there are no more fails, I’m setting this as verified.