Split test_group_touchevents into multiple files

RESOLVED FIXED in Firefox 62

Status

()

RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: kashav, Assigned: kashav)

Tracking

Trunk
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
There are currently too many subtests in test_group_touchevents [1], which is causing test-verify to consistently timeout on Android builds (see [2] for an example Try push).

Increasing the timeout factor [3] isn't a viable solution, as the test still fails after hitting a different time limit [4]. The optimal solution is to split this file up into multiple smaller files.

[1] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/gfx/layers/apz/test/mochitest/test_group_touchevents.html
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=18df9e3faecc61e255908f745321d558c7028a75
[3] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/gfx/layers/apz/test/mochitest/test_group_touchevents.html#95
[4] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4058813161e29063ee07217c95b54e97e65b2d81&selectedJob=183807886
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

9 months ago
mozreview-review
Comment on attachment 8986856 [details]
Bug 1470251 - Split test_group_touchevents into multiple files.

https://reviewboard.mozilla.org/r/252104/#review258792

Thanks!

A Try push for this looks pretty good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=999dc8f5bbb2820170ad97950ac6d1350624ca8c&filter-tier=1

::: gfx/layers/apz/test/mochitest/test_group_touchevents-3.html:12
(Diff revision 3)
> +  <script type="application/javascript" src="apz_test_native_event_utils.js"></script>
> +  <script type="application/javascript" src="apz_test_utils.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +  <script type="application/javascript">
> +
> +var touch_action_prefs = getPrefs("TOUCH_EVENTS:TOUCH_ACTIONS");

Since the TOUCH_ACTIONS prefs are only used in this file, we can define them here, and simply getPrefs() to just always return the basic pan prefs.
Comment hidden (mozreview-request)

Comment 6

9 months ago
mozreview-review
Comment on attachment 8986856 [details]
Bug 1470251 - Split test_group_touchevents into multiple files.

https://reviewboard.mozilla.org/r/252104/#review258804

Thanks! Just one more thing I thought of, to make sure the next person looking at this isn't confused about the split.

::: gfx/layers/apz/test/mochitest/test_group_touchevents-3.html:13
(Diff revisions 3 - 4)
>    <script type="application/javascript" src="apz_test_utils.js"></script>
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>    <script type="application/javascript">
>  
> -var touch_action_prefs = getPrefs("TOUCH_EVENTS:TOUCH_ACTIONS");
> +var touch_action_prefs = [
> +  ...getPrefs("TOUCH_EVENTS:PAN"),

Huh, didn't know JS could do that. Neat!

::: gfx/layers/apz/test/mochitest/test_group_touchevents-2.html:40
(Diff revision 4)
> +                                                ["apz.test.fails_with_native_injection", isWindows]]},
> +
> +  {'file': 'helper_tap_default_passive.html', 'prefs': [["apz.content_response_timeout", 24 * 60 * 60 * 1000],
> +                                                        ["apz.test.fails_with_native_injection", isWindows],
> +                                                        ["dom.event.default_to_passive_touch_listeners", true]]},
> +];

Please add a comment:

  // Add new subtests to test_group_touchevents-3.html, not this file.

::: gfx/layers/apz/test/mochitest/test_group_touchevents-3.html:26
(Diff revision 4)
> +  // More complex touch-action tests, with overlapping regions and such
> +  {'file': 'helper_touch_action_complex.html', 'prefs': touch_action_prefs},
> +  // Tests that touch-action CSS properties are handled in APZ without waiting
> +  // on the main-thread, when possible
> +  {'file': 'helper_touch_action_regions.html', 'prefs': touch_action_prefs},
> +];

Please add a comment:

  // Add new subtests here. If this starts timing out because it's taking too long,
  // create a test_group_touchevents-4.html file.

::: gfx/layers/apz/test/mochitest/test_group_touchevents.html:29
(Diff revision 4)
> -  // More complex touch-action tests, with overlapping regions and such
> -  {'file': 'helper_touch_action_complex.html', 'prefs': touch_action_prefs},
> -  // Tests that touch-action CSS properties are handled in APZ without waiting
> -  // on the main-thread, when possible
> -  {'file': 'helper_touch_action_regions.html', 'prefs': touch_action_prefs},
>  ];

Please add a comment:

  // Add new subtests to test_group_touchevents-3.html, not this file.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

9 months ago
mozreview-review
Comment on attachment 8986856 [details]
Bug 1470251 - Split test_group_touchevents into multiple files.

https://reviewboard.mozilla.org/r/252104/#review258814

Thanks!
Attachment #8986856 - Flags: review?(botond) → review+
I did one more Try push to be safe:

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

If that's green we can go ahead and land this.

Comment 11

9 months ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37c19e77f78d
Split test_group_touchevents into multiple files. r=botond

Comment 12

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37c19e77f78d
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.