Closed Bug 1470251 Opened 6 years ago Closed 6 years ago

Split test_group_touchevents into multiple files

Categories

(Core :: Panning and Zooming, enhancement)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: u608768, Assigned: u608768)

References

Details

Attachments

(1 file)

Bug 1470251 - Split test_group_touchevents into multiple files.
59 bytes, text/x-review-board-request
botond
: review+
Details 
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 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 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 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.
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37c19e77f78d
Split test_group_touchevents into multiple files. r=botond
https://hg.mozilla.org/mozilla-central/rev/37c19e77f78d
Status: NEW → RESOLVED
Closed: 6 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: