Closed Bug 837139 Opened 7 years ago Closed 7 years ago

Performance tests framework built on top of the integration tests

Categories

(Firefox OS Graveyard :: Gaia, defect)

x86_64
Linux
defect
Not set

Tracking

(b2g18 fixed)

RESOLVED FIXED
Tracking Status
b2g18 --- fixed

People

(Reporter: etienne, Assigned: julienw)

References

Details

Attachments

(1 file, 5 obsolete files)

The two-fold goal is:

- to help the automation team get reliable performance data
- to revitalize (resuscitate?) the integration tests at the same time
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #709067 - Flags: feedback?(jlal)
Julien, take good care of this patch, a lot of sweat went into it :)
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.
Assignee: nobody → felash
Sounds like you have he remote debugger enabled and not marionette
+ a-team

We need to cross review the changes for migration back to gaia-ui-tests...
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.
(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!
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.
Blocks: 837673
Blocks: 837658
Blocks: 837651
Blocks: 837662
Blocks: 837681
Blocks: 837656
Blocks: 837657
Blocks: 837678
Blocks: 837663
Blocks: 837660
Blocks: 837683
Blocks: 837697
Blocks: 837675
Blocks: 837703
Blocks: 837701
Blocks: 837689
Blocks: 837670
Blocks: 837669
Blocks: 837686
Blocks: 837659
Blocks: 837696
Blocks: 837671
Blocks: 837677
Blocks: 837674
Blocks: 837676
Blocks: 837654
Blocks: 837666
Blocks: 837655
Blocks: 837672
Blocks: 837691
Blocks: 837668
Attached patch patch v2 (obsolete) — Splinter Review
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
Attachment #709067 - Attachment is obsolete: true
Attachment #709067 - Flags: feedback?(jlal)
Attachment #710848 - Flags: review?(21)
Attachment #710848 - Flags: feedback?(jlal)
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?
Attachment #710848 - Flags: review?(21)
(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.
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #710848 - Attachment is obsolete: true
Attachment #710848 - Flags: feedback?(jlal)
Attachment #711816 - Flags: review?(21)
Attachment #711816 - Flags: feedback?(jlal)
Attachment #711816 - Flags: review?(anthony)
Comment on attachment 711816 [details] [diff] [review]
patch v3

will post an update in a few minutes
Attachment #711816 - Attachment is obsolete: true
Attachment #711816 - Flags: review?(anthony)
Attachment #711816 - Flags: review?(21)
Attachment #711816 - Flags: feedback?(jlal)
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attachment #711897 - Flags: review?(21)
Attachment #711897 - Flags: feedback?(jlal)
Attachment #711897 - Flags: review?(anthony)
Attached patch patch v5 (obsolete) — Splinter Review
sorry, I forgot one file in my patch.
Attachment #711897 - Attachment is obsolete: true
Attachment #711897 - Flags: review?(anthony)
Attachment #711897 - Flags: review?(21)
Attachment #711897 - Flags: feedback?(jlal)
Comment on attachment 712257 [details] [diff] [review]
patch v5

PR was updated too : https://github.com/mozilla-b2g/gaia/pull/7929
Attachment #712257 - Flags: review?(21)
Attachment #712257 - Flags: feedback?(jlal)
Attachment #712257 - Flags: review?(anthony)
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!!! :)
Attachment #712257 - Flags: review?(anthony)
Attachment #712257 - Flags: review?(21)
Attachment #712257 - Flags: review+
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 ? :)
Attached patch patch v6Splinter Review
fixed latest nit, carrying r=vingtetun
Attachment #712257 - Attachment is obsolete: true
Attachment #712257 - Flags: feedback?(jlal)
Attachment #712409 - Flags: review+
Attachment #712409 - Flags: feedback?(jlal)
https://github.com/mozilla-b2g/gaia/commit/d7fad119beddf112208bb8fcfb7654d0a5cec347
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attachment #712409 - Flags: feedback?(jlal)
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.
Attachment #712409 - Flags: approval-gaia-v1?(21)
(note to uplifters: two commits here)
Attachment #712409 - Flags: approval-gaia-v1?(21) → approval-gaia-v1+
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
Flags: needinfo?(felash)
v1-train: d7fad119beddf112208bb8fcfb7654d0a5cec347 -> 1ed47a4ebac1f383c57e4e77ed482c999b7aa08f
v1-train: 66659e24e442a55c46c90cbdddd409069df761a8 -> 53db58f074e89c22112ae2f3d79cea0e13ce8fec
Flags: needinfo?(felash)
You need to log in before you can comment on or make changes to this bug.