Unbitrot the browser-chrome tests

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 405411 [details] [diff] [review]
patch

The current set of browser-chrome tests are bitrotted and won't run very well. This patch fixes up the tests:
* Removes any unneeded test checks
* Uses waitForExplicitFinish instead of thread looping
* Adds a simple <select> test
* Minor cleanup and comments

Gavin, can you take a quick look

Run the tests by:
* cd to the objdir/mobile/_tests/testing/mochitest
* execute:

python runtests.py --browser-chrome --appname=../../../dist/bin/fennec --xre-path=../../../dist/bin/xulrunner
Attachment #405411 - Flags: review?(gavin.sharp)
Gavin doesn't like the use of setTimeout in the tests, so I am reworking them
Created attachment 405889 [details] [diff] [review]
patch

This patch adds a "waitFor" helper that can be used to wait until a condition is true to call a callback. It's a smarter setTimeout.

The patch also adds some bookmark events, which I thought I would used but don't). They kinda match the existing BookmarkRemove event, which itself is not used anywhere in our code. I'm fine with removing all the bookmark events too :)

Lastly, the patch removes 2 dead code methods we had in BrowserUI: goToBookmark and showBookmarks

NOTE: 1 test fails - open a bookmark by clicking on a bookmark in the bookmark list. The test is somehow screwed, but I can't find out how. Obviously, clicking on a bookmark in the list opens the bookmark in the main window.
Assignee: nobody → mark.finkle
Attachment #405411 - Attachment is obsolete: true
Attachment #405889 - Flags: review?(gavin.sharp)
Attachment #405411 - Flags: review?(gavin.sharp)
Comment on attachment 405889 [details] [diff] [review]
patch

>diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml

>+            let event = document.createEvent("Events");
>+            event.initEvent("BookmarkEdit", true, false);
>+            this.dispatchEvent(event);

>+            let event = document.createEvent("Events");
>+            event.initEvent("BookmarkChange", true, false);
>+            this.dispatchEvent(event);

Hold off on adding these until we have better defined use cases?

>diff --git a/chrome/tests/browser_bookmarks.js b/chrome/tests/browser_bookmarks.js

>+  if (gCurrentTest) {
>   }

Why are there a bunch of these left around?

I support porting waitFor to the browser chrome harness!

Are you going to disable/todo() the broken test before landing?
Attachment #405889 - Flags: review?(gavin.sharp) → review+
(In reply to comment #3)
> (From update of attachment 405889 [details] [diff] [review])
> >diff --git a/chrome/content/bindings.xml b/chrome/content/bindings.xml
> 
> Hold off on adding these until we have better defined use cases?

Makes sense

> >+  if (gCurrentTest) {
> >   }
> 
> Why are there a bunch of these left around?

Hmm, I think those were placeholders for a spot to put code that is executed before any test starts. But I will remove them. It should be if (!gCureentTest) too.

> I support porting waitFor to the browser chrome harness!

I'll file a bug and patch

> Are you going to disable/todo() the broken test before landing?

Yeah
pushed:
https://hg.mozilla.org/mobile-browser/rev/0dd811e35355
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
verified via private build and local test on desktop linux.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.