837139 - Performance tests framework built on top of the integration tests

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Etienne Segonzac (:etienne)]

The two-fold goal is:

- to help the automation team get reliable performance data
- to revitalize (resuscitate?) the integration tests at the same time

Comment 1

[Etienne Segonzac (:etienne)]

Attachment: WIP

WIP - Performance tests:
 
* launch time for sms, dialer, contacts and gallery - they work very well
* custom rendering time tests for the contacts app - timeouts half the time :/

The |tests/js/performance_helper.js| file is where some constants can be tweaked.

Comment 2

[Etienne Segonzac (:etienne)]

Julien, take good care of this patch, a lot of sweat went into it :)

Comment 3

[Julien Wajsberg [:julienw]]

So, to begin, I have this error when I try to run a test:

     Error: done() invoked with non-Error: unrecognizedPacketType

I'm sure I did something wrong but I can't find what. If someone has a solution I'd be grateful.

Comment 4

[James Lal [:lightsofapollo]]

Sounds like you have he remote debugger enabled and not marionette

Comment 5

[James Lal [:lightsofapollo]]

+ a-team

We need to cross review the changes for migration back to gaia-ui-tests...

Comment 6

[James Lal [:lightsofapollo]]

Julien, your digging through mocha/test-agent could help. It seems like we should report the time in JSON out of stdout rather then console.log... It should be really easy to do this with a custom mocha reporter... There is already an example of a custom reporter in tests/js which :mdas wrote.

Comment 7

[Etienne Segonzac (:etienne)]

(In reply to James Lal [:lightsofapollo] from comment #6)
> Julien, your digging through mocha/test-agent could help. It seems like we
> should report the time in JSON out of stdout rather then console.log... It
> should be really easy to do this with a custom mocha reporter... There is
> already an example of a custom reporter in tests/js which :mdas wrote.

Oh yes! let's do this!

Comment 8

[James Lal [:lightsofapollo]]

Also- I landed a patch for :mdas yesterday that fixes the app launching issues so the changes to launching apps and the window manager for |frame.id| should/can be discarded.

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 709067 [details] [diff] [review]
WIP

I did a PR with Etienne's patch > https://github.com/mozilla-b2g/gaia/pull/7929

Comment 10

[Julien Wajsberg [:julienw]]

Attachment: patch v2

apply with git am, or use the PR https://github.com/mozilla-b2g/gaia/pull/7929

- Infrastructure for performance tests
- New Mocha reporter for perf tests
- Startup tests for sms, dialer, contacts and gallery
  (will be improved by bug 838527)
- Custom rendering time tests for the contacts app
  (will be continued in bug 837673)

Also, it makes the integration tests work again :)

Work done by @etiennesegonzac, @julienw and @Rik
---
 Makefile                                           |   13 ++++
 apps/clock/test/integration/app.js                 |   11 +++
 apps/clock/test/performance/startup_test.js        |   35 +++++++++
 apps/communications/contacts/js/contacts_list.js   |   15 ++++
 .../test/atoms/contacts_rendering_performance.js   |   43 +++++++++++
 .../contacts/test/integration/app.js               |   23 +++++-
 .../contacts/test/performance/rendering_test.js    |   37 ++++++++++
 .../contacts/test/performance/startup_test.js      |   35 +++++++++
 apps/communications/dialer/test/integration/app.js |   11 +++
 .../dialer/test/performance/startup_test.js        |   35 +++++++++
 apps/gallery/test/integration/app.js               |   11 +++
 apps/gallery/test/performance/startup_test.js      |   35 +++++++++
 apps/sms/test/integration/app.js                   |   11 +++
 apps/sms/test/performance/startup_test.js          |   35 +++++++++
 apps/system/js/window_manager.js                   |    4 +-
 build/preferences.js                               |    2 +
 tests/atoms/gaia_apps.js                           |   39 ++++++----
 tests/js/app_integration.js                        |   15 ++--
 tests/js/integration_helper.js                     |   22 ++++++
 tests/js/performance_helper.js                     |   57 +++++++++++++++
 tests/js/xpc.js                                    |    1 +
 tests/reporters/jsonmozperf.js                     |   75 ++++++++++++++++++++
 22 files changed, 543 insertions(+), 22 deletions(-)
 create mode 100644 apps/clock/test/integration/app.js
 create mode 100644 apps/clock/test/performance/startup_test.js
 create mode 100644 apps/communications/contacts/test/atoms/contacts_rendering_performance.js
 create mode 100644 apps/communications/contacts/test/performance/rendering_test.js
 create mode 100644 apps/communications/contacts/test/performance/startup_test.js
 create mode 100644 apps/communications/dialer/test/integration/app.js
 create mode 100644 apps/communications/dialer/test/performance/startup_test.js
 create mode 100644 apps/gallery/test/integration/app.js
 create mode 100644 apps/gallery/test/performance/startup_test.js
 create mode 100644 apps/sms/test/integration/app.js
 create mode 100644 apps/sms/test/performance/startup_test.js
 create mode 100644 tests/js/performance_helper.js
 create mode 100644 tests/reporters/jsonmozperf.js

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 710848 [details] [diff] [review]
patch v2

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

I have some questions - most of them comes from my ignorance of the test framework so I would like to take this to r? opportunity to learn from it!
The biggest questions I have are:
 - Can we have a way to run a single performance test
 - Does it works with l10n build? So can we test pt-BR for example?
 - A helper in shared/ can be useful for emitting test events. 
 - Average time is good. min/max/variance would be useful too.

More concerning the framework:
 - There are sometimes some calls to some abstractions and sometimes some raw calls. I would prefer raw calls over a pretty window manager object but this is out of the scope of this bug.
 - I know you are going to remove some file duplications in a followup. But I feel like the setup and teardown of tests will share a some lines in common too - the ones related to start marionnette. Can it be factorize too?

::: Makefile
@@ +396,5 @@
> +ifneq ($(APP),)
> +	TESTS_PERF=$(shell find apps/$(APP)/test/performance/ -name "*_test.js" -type f )
> +else
> +	TESTS_PERF=$(shell find apps -name "*_test.js" -type f | grep performance)
> +endif

I wonder if you also want to let a chance to TEST_PERFS to be overidden? That would let someone interested into a particular perf test to launch only this one.

@@ +402,3 @@
>  .PHONY: test-integration
>  test-integration:
> +	adb forward tcp:2828 tcp:2828

I wonder if we should make sure that adb is on the machine to not thrown on travis machine if we want to run some integration tests there?

::: apps/clock/test/performance/startup_test.js
@@ +1,2 @@
> +require('apps/clock/test/integration/app.js');
> +require('/tests/js/performance_helper.js');

really small nit: I would have inverted both lines - to have the generic before the particular.

@@ +1,4 @@
> +require('apps/clock/test/integration/app.js');
> +require('/tests/js/performance_helper.js');
> +
> +suite('Clock', function() {

If the name l1l0n dependent? I would like to make sure the performance tests can be runned against Portuguese.

@@ +3,5 @@
> +
> +suite('Clock', function() {
> +  var device;
> +  var app;
> +

nit: 'use strict'; ?

@@ +10,5 @@
> +    device = app.device;
> +  });
> +
> +  setup(function() {
> +    yield IntegrationHelper.unlock(device); // it affects the first run otherwise

Out of curiosity: Is IntegrationHelper.unlock different from GaiaLockScreen.unlock?

@@ +28,5 @@
> +      yield app.close();
> +    }
> +
> +    var results = yield PerformanceHelper.getLoadTimes(device);
> +

nit: not sure the extra lines is needed.

@@ +31,5 @@
> +    var results = yield PerformanceHelper.getLoadTimes(device);
> +
> +    PerformanceHelper.reportDuration(results);
> +  });
> +});

I have been told that you have some plans to not repeat this file for every apps. That sounds good since it is very redundant.

::: apps/communications/contacts/js/contacts_list.js
@@ +226,5 @@
> +    // Performance testing
> +    function dispatchPerfEvent(name) {
> +      var evt = new CustomEvent(name, null);
> +      window.dispatchEvent(evt);
> +    }

I won't be against a file in shared/

@@ +232,3 @@
>      function renderChunks(index) {
> +      if (index === 0) {
> +        dispatchPerfEvent('contacts-first-chunk');

nit: maybe contacts-list-first-chunk-rendered. And actually is the first chunk rendered at that point or are we just entering the function? (sorry for beeing contacts_list.js agnostic here).

::: apps/communications/contacts/test/atoms/contacts_rendering_performance.js
@@ +2,5 @@
> +
> +(function(window) {
> +  window.ContactsRenderingPerformance = {
> +    register: function() {
> +      window.cStart = window.performance.now();

what means 'c' in this context?

@@ +5,5 @@
> +    register: function() {
> +      window.cStart = window.performance.now();
> +
> +      window.addEventListener('contacts-first-chunk', this.firstChunkHandler.bind(this));
> +      window.addEventListener('contacts-last-chunk', this.lastChunkHandler.bind(this));

It is not clear to me why you need to use bind for those?

@@ +7,5 @@
> +
> +      window.addEventListener('contacts-first-chunk', this.firstChunkHandler.bind(this));
> +      window.addEventListener('contacts-last-chunk', this.lastChunkHandler.bind(this));
> +
> +      marionetteScriptFinished(true);

Just curious: There are sometimes some marionetteScriptFinished(true) and sometimes some marionetteScriptFinished(false). What it is for? (Thanks for teaching me! :))

::: apps/communications/contacts/test/performance/rendering_test.js
@@ +1,2 @@
> +require('apps/communications/contacts/test/integration/app.js');
> +require('/tests/js/performance_helper.js');

really small nit: invert the 2 lines
nit: the name of the does not make it clear what this rendering test is about. Do you expect this file to be a generic rendering test file for contact?

@@ +15,5 @@
> +  });
> +
> +  test('average rendering time', function() {
> +    this.timeout(100000);
> +    yield app.device.setScriptTimeout(50000);

It should be me. But why is it 50000 for device.setScriptTimeout? I could have not understood what is setScriptTimeout too :)
nit: you have a global device variable - app.device -> device.

@@ +28,5 @@
> +      var results = yield app.observeRendering();
> +      firstPaints.push((results.first - results.start));
> +      lastPaints.push((results.last - results.start));
> +      yield app.close();
> +    }

This block of code will deserve some line breaks to breath a little.

::: apps/system/js/window_manager.js
@@ +1177,5 @@
>      // Give a name to the frame for differentiating between main frame and
>      // inline frame. With the name we can get frames of the same app using the
>      // window.open method.
>      iframe.name = 'main';
> +    iframe.id = 'appiframe' + nextAppId++;

I think I have seen a comment where James says this has been fixed. Can you check if this is still needed?

::: build/preferences.js
@@ +31,5 @@
>  }
>  
>  if (DEBUG) {
>    prefs.push(["marionette.defaultPrefs.enabled", true]);
> +  prefs.push(["marionette.defaultPrefs.port", 2828]);

Any reason to do that? I seems to be the default value already. http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#447

::: tests/atoms/gaia_apps.js
@@ +224,5 @@
>        }
>      });
>    },
>  
> +  // Closes app with the specified name (e.g., 'Calculator'); returns nothing

nit: grrrr!!! :)

@@ +236,5 @@
> +            window.wrappedJSObject.WindowManager.kill(origin, marionetteScriptFinished);
> +          },
> +          // wait until the app is found in the running apps list
> +          function() {
> +            origin = GaiaApps.getRunningAppOrigin(appName);

What does GaiaApps.getRunningAppOrigin do exactly? Is there a way to map this call to WindowManager method or does it abstracts things in a way that it is hard? If that's the case, maybe you want to add GaiaApps.kill too to abstract the internal objects from people writing tests? I not 100% of fan of that since it seems like this could be simpler for third party developers writing tests and harder for core developers since they will have to learn a new set of helpers...

@@ +242,5 @@
> +          }
> +        );
> +      } else {
> +        marionetteScriptFinished(false);
> +      }

if (!app) {
  marionetteScriptFinished(false);
  return;
}

::: tests/js/app_integration.js
@@ +196,5 @@
>          yield device.switchToFrame();
>  
> +        var result = yield device.executeAsyncScript(
> +          'GaiaApps.closeWithName("' + self.appName + '");'
> +        );

Seems to be the abstraction of kill. I have seen some calls to kill somewhere, maybe you want to replace it by this call? (And have an equivalent to closeWithName such as closeWithOrigin.

::: tests/js/integration_helper.js
@@ +101,5 @@
> +      this.importScript(
> +        device,
> +        '/tests/atoms/gaia_lock_screen.js',
> +        function() {
> +          device.executeAsyncScript('GaiaLockScreen.unlock();', null,

I have the same question for this GaiaLockScreen.unlock call than the one I have for the GaiaApps.getRunningAppByOrigin. Should we really have an abstraction layer on top of our objects?

::: tests/js/performance_helper.js
@@ +2,5 @@
> +
> +var PerformanceHelper = (function() {
> +
> +  var PerformanceHelper = {
> +    kSpawnInterval: 6000, // Time before gecko spawns a new template process

You may want to add a reference to http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#577 so it be clear that it is mapped to a pref. Also the reason why it is 6000 instead of 5000 here is because it takes some times for the background task to finished preloading.

@@ +3,5 @@
> +var PerformanceHelper = (function() {
> +
> +  var PerformanceHelper = {
> +    kSpawnInterval: 6000, // Time before gecko spawns a new template process
> +    kRuns: 3,

on datazilla this is 30 runs, that's maybe too much but 3 seems to few when you are working on small optimizations. Can we raise this number. I would say 5 times is a minimum.

@@ +38,5 @@
> +
> +      return sum / arr.length;
> +    },
> +
> +    reportDuration: function(arr_value, title) {

nit: arr_value does not seems to match with the other variables names.

@@ +41,5 @@
> +
> +    reportDuration: function(arr_value, title) {
> +      title = title || '';
> +      if (window.mozPerfDurations === null) {
> +        window.mozPerfDurations = Object.create(null);

Here this is window.mozPerfDurations. Somewhere else is is global or no prefix. Let's unify those calls.

@@ +48,5 @@
> +      if (title in window.mozPerfDurations) {
> +        throw new Error('reportDuration was called twice with the same title');
> +      }
> +      var value = this.average(arr_value);
> +      window.mozPerfDurations[title] = value;

I feel like it could be useful to report the min/max run time as well as the variance.

::: tests/reporters/jsonmozperf.js
@@ +1,4 @@
> +/**
> + * Initialize a new `JSON` reporter for MozTest.
> + *
> + * @param {Runner} runner

That's really a use...[ful|less] ... comment. Pick the value you want.

@@ +6,5 @@
> + */
> +
> +(function(global) {
> +
> +function JSONMozPerfReporter(runner) {

nit: not really sure why there is a Moz in the variable name?
Do we expect those reporters to run on something else some day. If yes do we expect them to have different reporters?

@@ +7,5 @@
> +
> +(function(global) {
> +
> +function JSONMozPerfReporter(runner) {
> +  var self = this;

Let's move this declaration closer to where it is used.

@@ +9,5 @@
> +
> +function JSONMozPerfReporter(runner) {
> +  var self = this;
> +  global.Mocha.reporters.Base.call(this, runner);
> +  global.mocha.options.ignoreLeaks = true;

Having global.mocha and global.Mocha seems error prone to me but I guess this is not part of your patch.

Also ignoreLeaks? What is that and why do we want to ignore leaks?

@@ +18,5 @@
> +  runner.on('test', function(test) {
> +    global.mozPerfDurations = null;
> +  });
> +
> +  runner.on('pass', function(test){

nit: space between the ) and the {

@@ +30,5 @@
> +      passes.push({
> +        title: test.title + ' ' + title,
> +        fullTitle: test.fullTitle() + ' ' + title,
> +        duration: test.duration,
> +        mozPerfDuration: mozPerfDurations[title]

There is sometimes |global.mozPerfDurations| and |mozPerfDurations|. Not sure what is |global| in this context though?

@@ +39,5 @@
> +  runner.on('fail', function(test, err) {
> +    failures.push(test);
> +  });
> +
> +  runner.on('end', function(){

nit: space between the ) and the {

@@ +61,5 @@
> +  var expected = err.expected;
> +
> +  return {
> +    title: test.title,
> +    fullTitle: test.fullTitle(),

Is there a reason to not have the same title and same fullTitle than when there is a success?

Comment 12

[Julien Wajsberg [:julienw]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #11)
> Comment on attachment 710848 [details] [diff] [review]
> patch v2
> 
> Review of attachment 710848 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some questions - most of them comes from my ignorance of the test
> framework so I would like to take this to r? opportunity to learn from it!
> The biggest questions I have are:
>  - Can we have a way to run a single performance test

I believe you can run a single file but this is maybe not important for this specific patch as this is would be a build change and would not impact the tests themselves. So this could be done in a follow up.

>  - Does it works with l10n build? So can we test pt-BR for example?

I don't know how l10n builds work but I don't see why it shouldn't. Maybe the fact that we're using the app names could be a problem ? It may be better to use the manifestURL to find them, we'll try to find out.

>  - A helper in shared/ can be useful for emitting test events. 

why not !

>  - Average time is good. min/max/variance would be useful too.

I agree but this could be done in a follow-up thanks to the abstraction we have.


> 
> More concerning the framework:
>  - There are sometimes some calls to some abstractions and sometimes some
> raw calls. I would prefer raw calls over a pretty window manager object but
> this is out of the scope of this bug.

We'll have to see exactly what you call abstractions here; maybe we can remove some ?

>  - I know you are going to remove some file duplications in a followup. But
> I feel like the setup and teardown of tests will share a some lines in
> common too - the ones related to start marionnette. Can it be factorize too?

yeah we feel that too. I'd say we should wait a little, start to write some tests, and then maybe we'll see better how and what to factorize.

> 
> ::: Makefile
> @@ +396,5 @@
> > +ifneq ($(APP),)
> > +	TESTS_PERF=$(shell find apps/$(APP)/test/performance/ -name "*_test.js" -type f )
> > +else
> > +	TESTS_PERF=$(shell find apps -name "*_test.js" -type f | grep performance)
> > +endif
> 
> I wonder if you also want to let a chance to TEST_PERFS to be overidden?
> That would let someone interested into a particular perf test to launch only
> this one.

agreed, that's what we started to do in https://github.com/Rik/gaia/commit/6a8fd9adcb6a2dd76bc18cf1d0af2944f41db9f0 for Bug 838527. We felt that we could wait for Bug 838527 to do that.

> 
> @@ +402,3 @@
> >  .PHONY: test-integration
> >  test-integration:
> > +	adb forward tcp:2828 tcp:2828
> 
> I wonder if we should make sure that adb is on the machine to not thrown on
> travis machine if we want to run some integration tests there?

I'm sure there will be more to do if we want travis to run integration tests, so let's do that in the same time ?

> 
> ::: apps/clock/test/performance/startup_test.js
> @@ +1,2 @@
> > +require('apps/clock/test/integration/app.js');
> > +require('/tests/js/performance_helper.js');
> 
> really small nit: I would have inverted both lines - to have the generic
> before the particular.

agreed.

> 
> @@ +1,4 @@
> > +require('apps/clock/test/integration/app.js');
> > +require('/tests/js/performance_helper.js');
> > +
> > +suite('Clock', function() {
> 
> If the name l1l0n dependent? I would like to make sure the performance tests
> can be runned against Portuguese.

The name of the suite is irrelevant but I know we do a "getAppByName" (or smthg like that) elsewhere so I'll check.

> 
> @@ +3,5 @@
> > +
> > +suite('Clock', function() {
> > +  var device;
> > +  var app;
> > +
> 
> nit: 'use strict'; ?

yep !

> 
> @@ +10,5 @@
> > +    device = app.device;
> > +  });
> > +
> > +  setup(function() {
> > +    yield IntegrationHelper.unlock(device); // it affects the first run otherwise
> 
> Out of curiosity: Is IntegrationHelper.unlock different from
> GaiaLockScreen.unlock?

Actually, this is "call GaiaLockScreen.unlock on the device" :-)

> 
> @@ +28,5 @@
> > +      yield app.close();
> > +    }
> > +
> > +    var results = yield PerformanceHelper.getLoadTimes(device);
> > +
> 
> nit: not sure the extra lines is needed.
> 
> @@ +31,5 @@
> > +    var results = yield PerformanceHelper.getLoadTimes(device);
> > +
> > +    PerformanceHelper.reportDuration(results);
> > +  });
> > +});
> 
> I have been told that you have some plans to not repeat this file for every
> apps. That sounds good since it is very redundant.

Yep, that's bug 838527. I'd like to land this first so that we can start work on app-specific tests in addition in parallel.

> 
> ::: apps/communications/contacts/js/contacts_list.js
> @@ +226,5 @@
> > +    // Performance testing
> > +    function dispatchPerfEvent(name) {
> > +      var evt = new CustomEvent(name, null);
> > +      window.dispatchEvent(evt);
> > +    }
> 
> I won't be against a file in shared/

Ok, why not !

> 
> @@ +232,3 @@
> >      function renderChunks(index) {
> > +      if (index === 0) {
> > +        dispatchPerfEvent('contacts-first-chunk');
> 
> nit: maybe contacts-list-first-chunk-rendered. And actually is the first
> chunk rendered at that point or are we just entering the function? (sorry
> for beeing contacts_list.js agnostic here).

I agree with you here. But we decided to not do more here as this will be handled in Bug 837673 in parallel.
I think like you that the first chunk is not rendered at all (why, I don't know ? a missing setTimeout maybe ?). And also that the event name is wrong.

But let's land this first !

> ::: apps/communications/contacts/test/atoms/contacts_rendering_performance.js
> @@ +2,5 @@
> > +
> > +(function(window) {
> > +  window.ContactsRenderingPerformance = {
> > +    register: function() {
> > +      window.cStart = window.performance.now();
> 
> what means 'c' in this context?

yeah, well, I don't know, that was etienne's stuff ;)
Maybe "contact" ?

> 
> @@ +5,5 @@
> > +    register: function() {
> > +      window.cStart = window.performance.now();
> > +
> > +      window.addEventListener('contacts-first-chunk', this.firstChunkHandler.bind(this));
> > +      window.addEventListener('contacts-last-chunk', this.lastChunkHandler.bind(this));
> 
> It is not clear to me why you need to use bind for those?

right !

> 
> @@ +7,5 @@
> > +
> > +      window.addEventListener('contacts-first-chunk', this.firstChunkHandler.bind(this));
> > +      window.addEventListener('contacts-last-chunk', this.lastChunkHandler.bind(this));
> > +
> > +      marionetteScriptFinished(true);
> 
> Just curious: There are sometimes some marionetteScriptFinished(true) and
> sometimes some marionetteScriptFinished(false). What it is for? (Thanks for
> teaching me! :))

It seems this is the value to returns to the caller (via yield). I'll try to not return anything and see if this still works, because these times we just don't care of the value.

> 
> ::: apps/communications/contacts/test/performance/rendering_test.js
> @@ +1,2 @@
> > +require('apps/communications/contacts/test/integration/app.js');
> > +require('/tests/js/performance_helper.js');
> 
> really small nit: invert the 2 lines
> nit: the name of the does not make it clear what this rendering test is
> about. Do you expect this file to be a generic rendering test file for
> contact?

Yep this is not clear, let's do that in Bug 837673 :)

> 
> @@ +15,5 @@
> > +  });
> > +
> > +  test('average rendering time', function() {
> > +    this.timeout(100000);
> > +    yield app.device.setScriptTimeout(50000);
> 
> It should be me. But why is it 50000 for device.setScriptTimeout? I could
> have not understood what is setScriptTimeout too :)

We're not really sure here. But we may find out in Bug 837673 ;)

> nit: you have a global device variable - app.device -> device.

cool :)

> 
> @@ +28,5 @@
> > +      var results = yield app.observeRendering();
> > +      firstPaints.push((results.first - results.start));
> > +      lastPaints.push((results.last - results.start));
> > +      yield app.close();
> > +    }
> 
> This block of code will deserve some line breaks to breath a little.

fine

> 
> ::: apps/system/js/window_manager.js
> @@ +1177,5 @@
> >      // Give a name to the frame for differentiating between main frame and
> >      // inline frame. With the name we can get frames of the same app using the
> >      // window.open method.
> >      iframe.name = 'main';
> > +    iframe.id = 'appiframe' + nextAppId++;
> 
> I think I have seen a comment where James says this has been fixed. Can you
> check if this is still needed?

There was no comment on this specific line but we can try to revert this change.

> 
> ::: build/preferences.js
> @@ +31,5 @@
> >  }
> >  
> >  if (DEBUG) {
> >    prefs.push(["marionette.defaultPrefs.enabled", true]);
> > +  prefs.push(["marionette.defaultPrefs.port", 2828]);
> 
> Any reason to do that? I seems to be the default value already.
> http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#447

good :)

> ::: tests/atoms/gaia_apps.js
> @@ +224,5 @@
> >        }
> >      });
> >    },
> >  
> > +  // Closes app with the specified name (e.g., 'Calculator'); returns nothing
> 
> nit: grrrr!!! :)

yeah 

> 
> @@ +236,5 @@
> > +            window.wrappedJSObject.WindowManager.kill(origin, marionetteScriptFinished);
> > +          },
> > +          // wait until the app is found in the running apps list
> > +          function() {
> > +            origin = GaiaApps.getRunningAppOrigin(appName);
> 
> What does GaiaApps.getRunningAppOrigin do exactly? Is there a way to map
> this call to WindowManager method or does it abstracts things in a way that
> it is hard? If that's the case, maybe you want to add GaiaApps.kill too to
> abstract the internal objects from people writing tests? I not 100% of fan
> of that since it seems like this could be simpler for third party developers
> writing tests and harder for core developers since they will have to learn a
> new set of helpers...

The main problem comes from the fact we use appName.

Also, GaiaApps do stuff like waiting for the event to happen (like for kill) but this is not the case for getRunningAppOrigin. We should kill this.


> 
> @@ +242,5 @@
> > +          }
> > +        );
> > +      } else {
> > +        marionetteScriptFinished(false);
> > +      }
> 
> if (!app) {
>   marionetteScriptFinished(false);
>   return;
> }

ok

> 
> ::: tests/js/app_integration.js
> @@ +196,5 @@
> >          yield device.switchToFrame();
> >  
> > +        var result = yield device.executeAsyncScript(
> > +          'GaiaApps.closeWithName("' + self.appName + '");'
> > +        );
> 
> Seems to be the abstraction of kill. I have seen some calls to kill
> somewhere, maybe you want to replace it by this call? (And have an
> equivalent to closeWithName such as closeWithOrigin.

yep

> 
> ::: tests/js/integration_helper.js
> @@ +101,5 @@
> > +      this.importScript(
> > +        device,
> > +        '/tests/atoms/gaia_lock_screen.js',
> > +        function() {
> > +          device.executeAsyncScript('GaiaLockScreen.unlock();', null,
> 
> I have the same question for this GaiaLockScreen.unlock call than the one I
> have for the GaiaApps.getRunningAppByOrigin. Should we really have an
> abstraction layer on top of our objects?


GaiaLockScreen does a "execute on the device this stuff". Maybe we could have something more generic but I doubt this will be clearer.


> 
> ::: tests/js/performance_helper.js
> @@ +2,5 @@
> > +
> > +var PerformanceHelper = (function() {
> > +
> > +  var PerformanceHelper = {
> > +    kSpawnInterval: 6000, // Time before gecko spawns a new template process
> 
> You may want to add a reference to
> http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#577 so it be
> clear that it is mapped to a pref. Also the reason why it is 6000 instead of
> 5000 here is because it takes some times for the background task to finished
> preloading.

ok

> 
> @@ +3,5 @@
> > +var PerformanceHelper = (function() {
> > +
> > +  var PerformanceHelper = {
> > +    kSpawnInterval: 6000, // Time before gecko spawns a new template process
> > +    kRuns: 3,
> 
> on datazilla this is 30 runs, that's maybe too much but 3 seems to few when
> you are working on small optimizations. Can we raise this number. I would
> say 5 times is a minimum.

Ok. We also plan to abstract the iteration, hopefully in Bug 838527.

> 
> @@ +38,5 @@
> > +
> > +      return sum / arr.length;
> > +    },
> > +
> > +    reportDuration: function(arr_value, title) {
> 
> nit: arr_value does not seems to match with the other variables names.

That comes from some zealot pythonist.

> 
> @@ +41,5 @@
> > +
> > +    reportDuration: function(arr_value, title) {
> > +      title = title || '';
> > +      if (window.mozPerfDurations === null) {
> > +        window.mozPerfDurations = Object.create(null);
> 
> Here this is window.mozPerfDurations. Somewhere else is is global or no
> prefix. Let's unify those calls.

yep right let's use "global" everywhere we can.

> 
> @@ +48,5 @@
> > +      if (title in window.mozPerfDurations) {
> > +        throw new Error('reportDuration was called twice with the same title');
> > +      }
> > +      var value = this.average(arr_value);
> > +      window.mozPerfDurations[title] = value;
> 
> I feel like it could be useful to report the min/max run time as well as the
> variance.

let's do that in a follow-up.

> 
> ::: tests/reporters/jsonmozperf.js
> @@ +1,4 @@
> > +/**
> > + * Initialize a new `JSON` reporter for MozTest.
> > + *
> > + * @param {Runner} runner
> 
> That's really a use...[ful|less] ... comment. Pick the value you want.

Yep that's not very use[ful|less].

> 
> @@ +6,5 @@
> > + */
> > +
> > +(function(global) {
> > +
> > +function JSONMozPerfReporter(runner) {
> 
> nit: not really sure why there is a Moz in the variable name?
> Do we expect those reporters to run on something else some day. If yes do we
> expect them to have different reporters?

Nope but I expect that some new reporters might be done by the mocha team ? Agreed this is more a speculation.

> 
> @@ +7,5 @@
> > +
> > +(function(global) {
> > +
> > +function JSONMozPerfReporter(runner) {
> > +  var self = this;
> 
> Let's move this declaration closer to where it is used.

ok

> 
> @@ +9,5 @@
> > +
> > +function JSONMozPerfReporter(runner) {
> > +  var self = this;
> > +  global.Mocha.reporters.Base.call(this, runner);
> > +  global.mocha.options.ignoreLeaks = true;
> 
> Having global.mocha and global.Mocha seems error prone to me but I guess
> this is not part of your patch.

Mocha is the "class", mocha is an instance.

> Also ignoreLeaks? What is that and why do we want to ignore leaks?

We're really ignoring leaks in the environment that executes the test, we really don't care and it makes our like harder because we're using global variable. A comment could be useful though.

> 
> @@ +18,5 @@
> > +  runner.on('test', function(test) {
> > +    global.mozPerfDurations = null;
> > +  });
> > +
> > +  runner.on('pass', function(test){
> 
> nit: space between the ) and the {

ok

> 
> @@ +30,5 @@
> > +      passes.push({
> > +        title: test.title + ' ' + title,
> > +        fullTitle: test.fullTitle() + ' ' + title,
> > +        duration: test.duration,
> > +        mozPerfDuration: mozPerfDurations[title]
> 
> There is sometimes |global.mozPerfDurations| and |mozPerfDurations|. Not
> sure what is |global| in this context though?

right, we'll check this.

> 
> @@ +39,5 @@
> > +  runner.on('fail', function(test, err) {
> > +    failures.push(test);
> > +  });
> > +
> > +  runner.on('end', function(){
> 
> nit: space between the ) and the {

ok

> 
> @@ +61,5 @@
> > +  var expected = err.expected;
> > +
> > +  return {
> > +    title: test.title,
> > +    fullTitle: test.fullTitle(),
> 
> Is there a reason to not have the same title and same fullTitle than when
> there is a success?

yep, when it fails, we don't have another text.

Comment 13

[Julien Wajsberg [:julienw]]

Attachment: patch v3

fixed most comments from your review. Especially we know use the manifestURL and the entry point id instead of the app/entry point name that can be localized.

I know some stuff is still not working (the contacts rendering test timeouts after some iterations, we can't run all the perf tests sequentially) but I think this works good enough so that we can start working on the measurements, and we can file follow-ups for the various problems that are left.

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 711816 [details] [diff] [review]
patch v3

will post an update in a few minutes

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: patch v4

see also PR https://github.com/mozilla-b2g/gaia/pull/7929

now I added a helper for applications.

There are some things that I find strange with wrappedJSObject but let's say that the current code is good enough for me.

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: patch v5

sorry, I forgot one file in my patch.

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 712257 [details] [diff] [review]
patch v5

PR was updated too : https://github.com/mozilla-b2g/gaia/pull/7929

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 712257 [details] [diff] [review]
patch v5

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

Let's land that. I removed Ryk's review? since you wrote the patch with him :)

::: apps/system/js/window_manager.js
@@ +1171,5 @@
>      // Create the <iframe mozbrowser mozapp> that hosts the app
>      var frame =
>          createFrame(origFrame, origin, url, name, manifest, manifestURL);
>      var iframe = frame.firstChild;
> +

extra line!!! :)

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 712257 [details] [diff] [review]
patch v5

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

::: apps/system/js/window_manager.js
@@ +1171,5 @@
>      // Create the <iframe mozbrowser mozapp> that hosts the app
>      var frame =
>          createFrame(origFrame, origin, url, name, manifest, manifestURL);
>      var iframe = frame.firstChild;
> +

don't you think it's nicer like that ? :)

Comment 20

[Julien Wajsberg [:julienw]]

Attachment: patch v6

fixed latest nit, carrying r=vingtetun

Comment 21

[Julien Wajsberg [:julienw]]

https://github.com/mozilla-b2g/gaia/commit/d7fad119beddf112208bb8fcfb7654d0a5cec347

Comment 22

[Julien Wajsberg [:julienw]]

follow-up to fix unit tests (I feel bad :) ):
https://github.com/mozilla-b2g/gaia/commit/66659e24e442a55c46c90cbdddd409069df761a8

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 712409 [details] [diff] [review]
patch v6

This landed a long time ago without causing regression. Only the contacts app has changes in the actual code and it's small.

I believe we need these perf measurements also in v1-train.

Comment 24

[Julien Wajsberg [:julienw]]

(note to uplifters: two commits here)

Comment 25

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x  66659e24e442a55c46c90cbdddd409069df761a8
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1-train
  git cherry-pick -x  d7fad119beddf112208bb8fcfb7654d0a5cec347
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 26

[Julien Wajsberg [:julienw]]

v1-train: d7fad119beddf112208bb8fcfb7654d0a5cec347 -> 1ed47a4ebac1f383c57e4e77ed482c999b7aa08f
v1-train: 66659e24e442a55c46c90cbdddd409069df761a8 -> 53db58f074e89c22112ae2f3d79cea0e13ce8fec