1440164 - APZ test hangs if subtest file is not added to mochitest.ini

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Botond Ballo [:botond]]

STR:

  1. Write a new APZ mochitest helper_*.html file.
  2. Add it as a subtest to a test group like test_group_wheelevents.html.
  3. Forget to add it to support-files mochitest.ini
  4. Run the test group

Expected results:

  You get some sort of error message about the missing file.

Actual results:

  The test run hangs.

I just spent a lot of time debugging such a hang, only to realize that its cause is the sort of thing you'd expect to be caught as an immediate error.

Comment 1

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #0)
>   3. Forget to add it to support-files mochitest.ini

That should say "to the support-files section in mochitest.ini".

Comment 2

[Botond Ballo [:botond]]

Attachment: Bug 1440164 - Call SimpleTest.finish() in runSubtestsSeriallyInFreshWindows(), instead of relying on its caller to call it.

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

Comment 3

[Botond Ballo [:botond]]

Attachment: Bug 1440164 - Call SimpleTest.finish() in runSubtestsSeriallyInFreshWindows() even if running subtests failed.

Not calling SimpleTest.finish() causes the test to hang, which isn't helpful.

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

Comment 4

[Botond Ballo [:botond]]

Attachment: Bug 1440164 - Detect a subtest URL not resolving and give an error message.

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

Comment 5

[Botond Ballo [:botond]]

(Going back to MozReview for the time being. There's no Autoland for Phabricator yet, and posting a series of patches is too much work.)

Comment 6

[Botond Ballo [:botond]]

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

Comment 7

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

Comment on attachment 8953706 [details]
Bug 1440164 - Call SimpleTest.finish() in runSubtestsSeriallyInFreshWindows(), instead of relying on its caller to call it.

https://reviewboard.mozilla.org/r/222938/#review228836

::: gfx/layers/apz/test/mochitest/test_group_hittest.html:35
(Diff revision 1)
>    SimpleTest.waitForExplicitFinish();
>    window.onload = function() {
> -    runSubtestsSeriallyInFreshWindows(subtests)
> +    runSubtestsSeriallyInFreshWindows(subtests);
> -    .then(SimpleTest.finish);

I don't like that the waitForExplicitFinish() call happens in the test, but the finish call happens elsewhere. Although I don't feel too strongly about it - maybe if we renamed the function to runSubtestsSeriallyInFreshWindowsAndFinish it would be a little better? Or we could drop this part 1 and just update all the test files to do .then(SimpleTest.finish, SimpleTest.finish)?

I kind of prefer the latter in case we write one of these tests that has additional cleanup to do after running the subtests but before calling SimpleTest.finish.

Comment 8

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

Comment on attachment 8953707 [details]
Bug 1440164 - Call SimpleTest.finish() in runSubtestsSeriallyInFreshWindows() even if running subtests failed.

https://reviewboard.mozilla.org/r/222940/#review228838

Comment 9

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

Comment on attachment 8953708 [details]
Bug 1440164 - Detect a subtest URL not resolving and give an error message.

https://reviewboard.mozilla.org/r/222942/#review228840

Comment 10

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #6)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3b1e04616876f1b037bbd366b37c49b0c35104d9

Looks like I missed a call to runSubtestsSeriallyInFreshWindows() in dom/events/test/pointerevents/test_touch_action.html.

Comment 11

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> I don't like that the waitForExplicitFinish() call happens in the test, but
> the finish call happens elsewhere. 

Good point, that's not great.

> Or
> we could drop this part 1 and just update all the test files to do
> .then(SimpleTest.finish, SimpleTest.finish)?
> 
> I kind of prefer the latter in case we write one of these tests that has
> additional cleanup to do after running the subtests but before calling
> SimpleTest.finish.

I don't love the repetition but I don't have a better idea, so let's do that.

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8953707 [details]
Bug 1440164 - Call SimpleTest.finish() in runSubtestsSeriallyInFreshWindows() even if running subtests failed.

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

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8953708 [details]
Bug 1440164 - Detect a subtest URL not resolving and give an error message.

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da952030e9e4
Call SimpleTest.finish() in runSubtestsSeriallyInFreshWindows() even if running subtests failed. r=kats
https://hg.mozilla.org/integration/autoland/rev/bbceec48f282
Detect a subtest URL not resolving and give an error message. r=kats

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/da952030e9e4
https://hg.mozilla.org/mozilla-central/rev/bbceec48f282