852537 - [Settings] Create performance test for the WiFi network list displayed in WiFi subpanel.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[José Antonio Olivera Ortega [:jaoo]]

This bug is about measuring the performance in displaying the WiFi network list in the WiFi subpanel from the setting app.

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

This patch adds a test for measuring the performance while rendering the list of WiFi networks available in the WiFi sub panel from the Setting app. Could you guys take a look at it and give some feedback please? Thanks.

Comment 2

[Anthony Ricaud (:rik)]

Comment on attachment 726669 [details] [diff] [review]
v1

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

I'm a bit worried that this test depends highly on the network situation.

What are we trying to measure exactly?

::: apps/settings/manifest.webapp
@@ +7,4 @@
>      "name": "The Gaia Team",
>      "url": "https://github.com/mozilla-b2g/gaia"
>    },
> +  "entry_points": {

That shouldn't be necessary.

::: apps/settings/test/integration/app.js
@@ +8,5 @@
> +}
> +
> +SettingsIntegration.prototype = {
> +  __proto__: AppIntegration.prototype,
> +  appName: 'Phone',

Copy paste fail :)
appName: 'Settings',

@@ +10,5 @@
> +SettingsIntegration.prototype = {
> +  __proto__: AppIntegration.prototype,
> +  appName: 'Phone',
> +  manifestURL: 'app://settings.gaiamobile.org/manifest.webapp',
> +  entryPoint: 'settings',

You can remove the entryPoint.

::: apps/settings/test/performance/rendering_wifi_list_test.js
@@ +22,5 @@
> +    this.timeout(500000);
> +    yield device.setScriptTimeout(50000);
> +
> +    var lastEvent = 'wifi-network-list-finish';
> +    var eventTitles = {

We've removed eventTitles in another bug, you don't need this anymore.

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

Address comments from comment #2

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Anthony Ricaud (:rik) from comment #2)
> I'm a bit worried that this test depends highly on the network situation.

Yes, that's true. The time reported depends on how many WiFi networks are available in the area.

> 
> What are we trying to measure exactly?

The time the user waits for opening the WiFi subpanel and seeing the list of WiFi networks completely rendered. Somehow it measure the time the user has to wait until he/she has the WiFi subpanel ready to be used.

Actually this test puts in practice what you guys talked about in the performance work week we held in Paris. If this test does not make sense could you suggest what to measure better in the setting app please? I'll be happy to change the test and measure another thing.

Comment 5

[Anthony Ricaud (:rik)]

OK so what we could do is report two events. 1) When the panel is visible |settings-panel-wifi-visible| 2) When the list of Wifi is displayed |settings-panel-wifi-ready|.
1) should be fairly stable.

You should also dispatch a 'start' event when you receive the tap event. The time of this event will be used as the starting point for the other events, making them more reliable. https://github.com/mozilla-b2g/gaia/blob/master/tests/performance/performance_helper.js#L104

And thanks so much for submitting those performance tests!

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v3

Adding 'start', 'settings-panel-wifi-visible' and 'settings-panel-wifi-ready' events.

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Anthony Ricaud (:rik) from comment #5)

Thanks for your help and comments.

> OK so what we could do is report two events. 1) When the panel is visible
> |settings-panel-wifi-visible| 2) When the list of Wifi is displayed
> |settings-panel-wifi-ready|.
> 1) should be fairly stable.
>
> You should also dispatch a 'start' event when you receive the tap event.

I dispatch a 'start' event as you suggested but I'm wondering if I did it well, I mean, If what I've done is what you suggested or I'm not understanding what you said about adding a 'start' event.

Comment 8

[Anthony Ricaud (:rik)]

Comment on attachment 727927 [details] [diff] [review]
v3

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

>I dispatch a 'start' event as you suggested but I'm wondering if I did it well, I mean, If what I've done is what you suggested or I'm not understanding what you said about adding a 'start' event.
You dispatch it correctly but not the 'settings-panel-wifi-visible' event.

This looks good and I think we'll be able to use this as the basis to get performance measurements for every settings panel.

::: apps/settings/index.html
@@ +32,5 @@
>      <script defer src="shared/js/l10n.js"></script>
>      <script defer src="shared/js/l10n_date.js"></script>
>  
> +    <!-- For perf-measurement related utilities --> 
> +    <script defer src="/shared/js/performance_testing_helper.js"></script> 

Those two lines have a trailing whitespace.

::: apps/settings/js/settings.js
@@ +668,5 @@
>                             'current peek' : 'peek current forward';
>      oldHash = hash;
>  
> +    if (hash === '#wifi') {
> +      PerformanceTestingHelper.dispatch('settings-panel-wifi-visible');

This is dispatched too early. The transitions have not been done yet, the panel is not visible. You should move this in the second transitionend callback, with the "Workaround for bug 825622".

::: apps/settings/js/wifi.js
@@ +363,5 @@
>        req.onerror = function onScanError(error) {
>          // always try again.
>          scanning = false;
> +
> +        PerformanceTestingHelper.dispatch('wifi-subpanel-network-list-ready');

Forgot to rename this event.

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v4

Review comments from comment #8 addressed.

Comment 10

[Anthony Ricaud (:rik)]

Comment on attachment 728938 [details] [diff] [review]
v4

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

Good to go!

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

https://github.com/mozilla-b2g/gaia/commit/12174ec886fbf7ae29cbd2e88d70eb2fa0ef4568

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Anthony Ricaud (:rik) from comment #10)
> Comment on attachment 728938 [details] [diff] [review]
> v4
> 
> Review of attachment 728938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good to go!

Thanks for your help rik!