904893 - Investigate snapped view testing

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Firefox Start, defect)

Description

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

Snapped view has been regressing quite regularly, it would be nice to be able to add basic tests to it.

We can't programatically set snapped [1], we need to find a way to simulate snapped.

[1] http://social.msdn.microsoft.com/Forums/windowsapps/en-US/263b39dd-89d4-4f39-96dc-596a500fa10a/is-there-any-api-to-snap-an-application

Comment 1

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

Attachment: WIP

Using notifyObservers to set the viewstate and a thick border to simulate snapped/portrait size and trigger the media queries. I'll add more tests.

Comment 2

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

Comment on attachment 793786 [details] [diff] [review]
WIP

I see where you are going here. Looks good. The tests should describe our understanding of what it means to be snapped/portrait/full/landscape so that anything that disagrees is a bug, right? That should help document how we deal with these states - particularly as we shift to supporting 8.1. The viewstate test helpers will want to move to head.js eventually I think, so that we can evaluate support for different viewstates in the feature tests.

Comment 3

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

Attachment: Patch v1

Added some basic tests to start with. I want to include at least a couple more tests before closing this bug:
* Test URL bar context buttons
* test scrolling
* More tests for portrait mode

As a side note, it looks like we can actually get to portrait mode programmatically[1] (we can't snap though). We can look into that if needed, but for now this approach should be enough.

[1] http://msdn.microsoft.com/en-us/library/ms812499.aspx

Comment 4

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

Attachment: multiHead.patch

This moves the viewstate helper functions into it's own file that get loaded from head.js. I'm planning on moving the code I have to create mock bookmark and history tiles from tests to separate files. Once I have that it will be easier to create more tests for snapped/portrait.

Comment 5

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

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

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

Looks great.

Comment 6

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

Comment on attachment 794906 [details] [diff] [review]
multiHead.patch

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

Thanks for doing this!

::: browser/metro/base/tests/mochitest/head.js
@@ +13,5 @@
>    Useful constants
>  =============================================================================*/
>  const serverRoot = "http://example.com/browser/metro/";
>  const baseURI = "http://mochi.test:8888/browser/metro/";
>  const chromeRoot = getRootDirectory(gTestPath);

chromeRoot? This gets populated with the path to the test directory, right?

Comment 7

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

(In reply to Sam Foster [:sfoster] from comment #6)
> Comment on attachment 794906 [details] [diff] [review]
> multiHead.patch
> 
> Review of attachment 794906 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for doing this!
> 
> ::: browser/metro/base/tests/mochitest/head.js
> @@ +13,5 @@
> >    Useful constants
> >  =============================================================================*/
> >  const serverRoot = "http://example.com/browser/metro/";
> >  const baseURI = "http://mochi.test:8888/browser/metro/";
> >  const chromeRoot = getRootDirectory(gTestPath);
> 
> chromeRoot? This gets populated with the path to the test directory, right?

Yes, it generates a chrome path like chrome://mochitests/content/metro/browser/metro/base/tests/mochitest/

Comment 8

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

Attachment: multiHead.patch v2

The path pointed by chromeRoot is different for mochiperf tests and it was causing them to fail because the helper file wasn't found. Added some logic to look for the helper methods in chromeRoot/../mochitest.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=72b200fb3bea

Comment 9

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

https://hg.mozilla.org/integration/fx-team/rev/4a1bb4573d35
https://hg.mozilla.org/integration/fx-team/rev/d6b360e5cc5d

Comment 10

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

Attachment: MoarTests

Moved BookmarksTestHelper to a helper file to use it to fill mock bookmarks and test snapped/portrait scrolling. Also added a test for navbar contextual buttons in snapped.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=83b9fa942732

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a1bb4573d35
https://hg.mozilla.org/mozilla-central/rev/d6b360e5cc5d

Comment 12

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

Comment on attachment 795726 [details] [diff] [review]
MoarTests

Try push came out with a failure on the portrait scrolls vertically test. IIRC the resolution on the test machines was pretty high. I'll make the test less screen height dependent.

Comment 13

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

Attachment: MoarTests v2

Added some mock history items to the portrait scroll test for good measure. I was also notifying the viewstate change too early, changed to notify after setting the viewport size.

try again: https://tbpl.mozilla.org/?tree=Try&rev=4fd6bc83d6d1

Comment 14

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

Try results came green.

Comment 15

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

Comment on attachment 796105 [details] [diff] [review]
MoarTests v2

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

As discussed in #windev, I'm getting a timing/animation related test failure in browser_bookmarks, but it exists without this patch. Lets track that elsewhere, this is good to go.

Comment 16

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

Created bug 910002 to track other issue. Thanks!

https://hg.mozilla.org/integration/fx-team/rev/7c71879e1171

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c71879e1171