856458 - Story - Fixup all failing mochitest-metro-chrome tests

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Tests, defect, P2)

Description

[Jim Mathies [:jimm]]

Once we have these running, we'll need to fix up unreliable tests.

Comment 1

[Marco Mucci [:MarcoM]]

Hey Asa, this requires a new story.

Comment 2

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Context menu and keyboard tests

WIP

Comment 3

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Added fixes to browser tiles (grid.xml) and test hangs

WIP

Fixed some grid.xml bugs that were uncovered by browser_tiles test.

Fixed test hangs - When we reject a promise (say a waitForEvent timeout), taskjs throws an exception: https://mxr.mozilla.org/mozilla-central/source/toolkit/content/Task.jsm#193

Because we were not catching the exception, finish() was never called at runTests, hanging test execution until the test itself timed out. I've added a try/catch block to runTests to fix this.

The only non-intermittent failures I'm seeing with this patch are on download tests. Seems like a binding issue with XUL, I'm investigating.

Comment 4

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v1

There is a call to getBoundingClientRect() from richgrid's arrangeItems() that is triggering the richgrid-item constructor.
http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/grid.xml?rev=e4b06af6ce99#343

But for some reason the bindings are not fully done and getAnonymousElementByAttribute() is returning null for the internal elements like anon-richgrid-item-icon. I've added a check for this case that will skip refresh() instead of throwing. 

Also fixed one last issue with download tests.

Now I get all test passing pretty frequently. The intermittent failures I get are timeouts, most on context menu test. We probably need something similar to what Jim is doing on Bug 859154 for those cases too, then I think we can consider enabling the tests in automation.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Nice work. Yay for tests.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 735534 [details] [diff] [review]
Patch v1

We also have bug 849015.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 735534 [details] [diff] [review]
Patch v1

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

Thanks!

::: browser/metro/base/content/bindings/grid.xml
@@ +56,3 @@
>                anItem.removeAttribute("selected");
>              } else {
>                anItem.setAttribute("selected", true);

You can now change this to "anItem.selected = !wasSelected;"

::: browser/metro/base/tests/mochitest/browser_onscreen_keyboard.js
@@ +44,5 @@
> +    let promise = waitForEvent(window, "MozDeckOffsetChanged");
> +    yield promise;
> +
> +    promise.then(function (event){
> +      is(event.detail, 100, "deck offset by keyboard height");

Wouldn't it be equivalent to write:

  let event = waitForEvent(window, "MozDeckOffsetChanged");
  is(event.detail, 100, "deck offset by keyboard height");

@@ -46,5 @@
>      MetroUtils.keyboardVisible = false;
>      Services.obs.notifyObservers(null, "metro_softkeyboard_hidden", null);
>  
> -    let rect2 = text.getBoundingClientRect();
> -    is(rect2.top, rect0.top, "text field moves back to the original position");

Is it possible to keep this assertion, after the final waitForEvent?

Comment 8

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> Wouldn't it be equivalent to write:
> 
>   let event = waitForEvent(window, "MozDeckOffsetChanged");

Sorry, I meant:

  let event = yield waitForEvent(window, "MozDeckOffsetChanged");

Comment 9

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to Jim Mathies [:jimm] from comment #6)
> We also have bug 849015.

Thanks! Strangely enough, I'm consistently seeing either all pass or the same 5 failures. 4 wait for popupshow and 1 timeout on browser_plugin_input_mouse. I'm curious as to why this connection is happening, might even be a product issue.

(In reply to Matt Brubeck (:mbrubeck) from comment #8)
> (In reply to Matt Brubeck (:mbrubeck) from comment #7)
> > Wouldn't it be equivalent to write:
> > 
> >   let event = waitForEvent(window, "MozDeckOffsetChanged");
> 
> Sorry, I meant:
> 
>   let event = yield waitForEvent(window, "MozDeckOffsetChanged");

Thanks for the review and for pointing this one out, much cleaner :). I've made the other updates too.

Comment 10

[Jim Mathies [:jimm]]

If the plugin tests get in the way, just disable them in the makefile. We worked on those back before adobe told us they weren't going to develop a flash plugin for metro.

Comment 11

[Rodrigo Silveira [:rsilveira] [:rodms]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/855fe8cf74d8

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/855fe8cf74d8

Comment 13

[Asa Dotzler [:asa]]

Story: As a Metro Developer, I can run mochitest-metro-chrome without failures because we've fixed them up ;-)