1273654 - Refactor common APZ mochitest code

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Bug 1264017 did a little bit of refactoring, by creating a runSubtestsSeriallyInFreshWindows function that was reused across some tests. However there's a lot more common code that can be extracted and refactored. I haz patches.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f625b1b47456

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Try is looking green, patches incoming.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1273654 - Merge the subtests from test_basic_pan into test_group_touchevents. r?botond

Review commit: https://reviewboard.mozilla.org/r/53406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53406/

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1273654 - Remove redundant function wrapper. r?botond

Review commit: https://reviewboard.mozilla.org/r/53408/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53408/

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1273654 - Extract a waitUntilApzStable helper function for tests. r?botond

Review commit: https://reviewboard.mozilla.org/r/53410/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53410/

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1273654 - Extract a pushPrefs helper function to simplify test setup. r?botond

The set of changes in this patch removes all waitForFocus calls in the test
folder, except for the one in apz_test_utils.js. This results in less
boilerplate and more consistent test state initialization.

Review commit: https://reviewboard.mozilla.org/r/53412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53412/

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1273654 - Extract a helper to check if APZ is enabled or not. r?botond

Review commit: https://reviewboard.mozilla.org/r/53414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53414/

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1273654 - Reuse the apz test utils in a few more places. r?botond

Review commit: https://reviewboard.mozilla.org/r/53416/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53416/

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1273654 - Extract a helper function to run continuation-style tests. r?botond

Review commit: https://reviewboard.mozilla.org/r/53418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53418/

Comment 10

[Kartikaya Gupta (email:[e-mail])]

https://reviewboard.mozilla.org/r/53412/#review50252

::: gfx/layers/apz/test/mochitest/apz_test_utils.js:213
(Diff revision 1)
> +  return new Promise(function(resolve, reject) {
> +    SpecialPowers.pushPrefEnv({'set': prefs}, resolve);
> +  });

In light of bug 1197310 I'll just change this to be:

function pushPrefs(prefs) {
  return SpecialPowers.pushPrefEnv({'set': prefs});
}

or I can just inline it to the call sites if you don't mind the extra 'set' boilerplate.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1273654 - Update a new test to use the new helpers. r?botond

Review commit: https://reviewboard.mozilla.org/r/55118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55118/

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8753623 [details]
MozReview Request: Bug 1273654 - Merge the subtests from test_basic_pan into test_group_touchevents. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53406/diff/1-2/

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8753624 [details]
MozReview Request: Bug 1273654 - Remove redundant function wrapper. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53408/diff/1-2/

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8753625 [details]
MozReview Request: Bug 1273654 - Extract a waitUntilApzStable helper function for tests. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53410/diff/1-2/

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8753626 [details]
MozReview Request: Bug 1273654 - Extract a pushPrefs helper function to simplify test setup. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53412/diff/1-2/

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8753627 [details]
MozReview Request: Bug 1273654 - Extract a helper to check if APZ is enabled or not. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53414/diff/1-2/

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8753628 [details]
MozReview Request: Bug 1273654 - Reuse the apz test utils in a few more places. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53416/diff/1-2/

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8753629 [details]
MozReview Request: Bug 1273654 - Extract a helper function to run continuation-style tests. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53418/diff/1-2/

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8753623 [details]
MozReview Request: Bug 1273654 - Merge the subtests from test_basic_pan into test_group_touchevents. r?botond

https://reviewboard.mozilla.org/r/53406/#review51814

I'm fine with this, but I have a general concern about this consolidation of tests into test groups: when we get a test failure, is it sufficiently clear which subtest failed? (In particular, does the error output mention the subtest by name?)

For example, I'd view it as an unfortunate consequence of this change if failures in a subtest formerly part of test_basic_pan will now start to get starred on Treeherder as the same bug as failures in another subtest in test_group_touchevents.

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8753624 [details]
MozReview Request: Bug 1273654 - Remove redundant function wrapper. r?botond

https://reviewboard.mozilla.org/r/53408/#review51816

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8753625 [details]
MozReview Request: Bug 1273654 - Extract a waitUntilApzStable helper function for tests. r?botond

https://reviewboard.mozilla.org/r/53410/#review51820

So we're switching from "window.onload = " to "SimpleTest.waitForFocus()". I assume the latter is equivalent (or better)?

::: gfx/layers/apz/test/mochitest/helper_basic_pan.html:30
(Diff revision 2)
>  function checkScroll() {
>    is(window.scrollY, 50, "check that the window scrolled");
>    subtestDone();
>  }
>  
> -window.onload = function() {
> +waitUntilApzStable().then(scrollPage);

I can see why people like Promises so much: they make code read very nicely ("wait until APZ is stable, then scroll the page").

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 8753626 [details]
MozReview Request: Bug 1273654 - Extract a pushPrefs helper function to simplify test setup. r?botond

https://reviewboard.mozilla.org/r/53412/#review51830

Very nice :)

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8753627 [details]
MozReview Request: Bug 1273654 - Extract a helper to check if APZ is enabled or not. r?botond

https://reviewboard.mozilla.org/r/53414/#review51836

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8753628 [details]
MozReview Request: Bug 1273654 - Reuse the apz test utils in a few more places. r?botond

https://reviewboard.mozilla.org/r/53416/#review51838

Comment 25

[Botond Ballo [:botond]]

Comment on attachment 8753629 [details]
MozReview Request: Bug 1273654 - Extract a helper function to run continuation-style tests. r?botond

https://reviewboard.mozilla.org/r/53418/#review51840

Getting nicer and nicer!

::: gfx/layers/apz/test/mochitest/apz_test_utils.js:254
(Diff revision 2)
> +  // if we didn't have this extra function, the promise would start running
> +  // during construction of the promise chain, concurrently with the first
> +  // promise in the chain.
> +  return function() {
> +    return new Promise(function(resolve, reject) {
> +      var gTestContinuation = null;

Not much point prefixing this with 'g' if it's not a global any more.

Comment 26

[Botond Ballo [:botond]]

Comment on attachment 8756372 [details]
MozReview Request: Bug 1273654 - Update a new test to use the new helpers. r?botond

https://reviewboard.mozilla.org/r/55118/#review51848

Comment 27

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #19)
> Comment on attachment 8753623 [details]
> MozReview Request: Bug 1273654 - Merge the subtests from test_basic_pan into
> test_group_touchevents. r?botond
> 
> https://reviewboard.mozilla.org/r/53406/#review51814
> 
> I'm fine with this, but I have a general concern about this consolidation of
> tests into test groups: when we get a test failure, is it sufficiently clear
> which subtest failed? (In particular, does the error output mention the
> subtest by name?)
> 
> For example, I'd view it as an unfortunate consequence of this change if
> failures in a subtest formerly part of test_basic_pan will now start to get
> starred on Treeherder as the same bug as failures in another subtest in
> test_group_touchevents.

Ah, that's a good point. I can fix this easily enough, by injecting the subtest name into the message passed to the is/ok functions at [1]. I just verified locally that it works as intended, I'll update the patch to do that.

(In reply to Botond Ballo [:botond] from comment #21)
> Comment on attachment 8753625 [details]
> MozReview Request: Bug 1273654 - Extract a waitUntilApzStable helper
> function for tests. r?botond
> 
> https://reviewboard.mozilla.org/r/53410/#review51820
> 
> So we're switching from "window.onload = " to "SimpleTest.waitForFocus()". I
> assume the latter is equivalent (or better)?

Yeah, I looked at the code for waitForFocus [2] and that also waits for load, as well as waits for focus. So it's strictly better I think.

(In reply to Botond Ballo [:botond] from comment #25)
> > +      var gTestContinuation = null;
> 
> Not much point prefixing this with 'g' if it's not a global any more.

True, I'll rename that.

I have more refactorings in mind, but they're much smaller and sometimes local to specific tests. I'll do those too at some point, in another bug, since I wanted to get these larger-scope refactorings done first.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/test/mochitest/apz_test_utils.js?rev=47b83c8478a0#192
[2] http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js?rev=8710b9ecc282#665

Comment 28

[Kartikaya Gupta (email:[e-mail])]

The try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8a80c00d94d&group_state=expanded has the rebased patches (the ones posted to this bug, plus some WIPs for bug 1275604), I'll wait for that before landing.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c11ccdcbb7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/40bfcafa28c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f40e481b9348
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb15e7ac2e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/90341d286f4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f163ba08afc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac523b371f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e096709134af

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c11ccdcbb7b
https://hg.mozilla.org/mozilla-central/rev/40bfcafa28c2
https://hg.mozilla.org/mozilla-central/rev/f40e481b9348
https://hg.mozilla.org/mozilla-central/rev/dcb15e7ac2e2
https://hg.mozilla.org/mozilla-central/rev/90341d286f4d
https://hg.mozilla.org/mozilla-central/rev/f163ba08afc6
https://hg.mozilla.org/mozilla-central/rev/1ac523b371f2
https://hg.mozilla.org/mozilla-central/rev/e096709134af