874510 - [test-agent] Update mocha library (huge performance improvements)

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::TestAgent, defect, P2)

Description

[Salvador de la Puente González [:salva]]

In Firefox, iframes are indexed inside the window object as window[0], window[1], ... one by iframe in the DOM.

The mocha function filterLeaks() in charge of distinguish which global names are leaks and which not is not taking this in count so applications with iframes will report non-real memory leaks.

Comment 1

[Salvador de la Puente González [:salva]]

Attachment: Prevent mocha library from considering iframes as leaks

Comment 2

[Julien Wajsberg [:julienw]]

Salva, I think we don't want to modify a vendor library.

The easiest way to workaround is to call |mocha.setup({ globals: ['0'] })| at the start of your test, see eg https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/test/unit/thread_ui_test.js#L5

Comment 3

[Julien Wajsberg [:julienw]]

James, how do you feel about this ?

Comment 4

[Salvador de la Puente González [:salva]]

I made a PR for mocha as well (https://github.com/visionmedia/mocha/pull/854)

Comment 5

[Julien Wajsberg [:julienw]]

I'm quite sure it will be closed, mine was (https://github.com/visionmedia/mocha/pull/820). But let's see :)

Comment 6

[Julien Wajsberg [:julienw]]

Oh, it was merged, congratulations !

Therefore we should maybe wait for a new version from them and use that new version. And maybe use your PR before that.

I'll redirect the review to James then !

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 752259 [details]
Prevent mocha library from considering iframes as leaks

James is a better reviewer for this :)

Comment 8

[James Lal [:lightsofapollo]]

Now that this in mocha lets update to v1.10 which should fix this issue.

Comment 9

[James Lal [:lightsofapollo]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10222

Pointer to Github pull-request

Comment 10

[James Lal [:lightsofapollo]]

Comment on attachment 758634 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10222

Thanks for upstreaming the fix! Here is a patch that updates our mocha to the latest version.

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 758634 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10222

stealing, because I want this ;)

one question in the PR

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 758634 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10222

r=me

tried removing a mocha.globals call with numbers (which were triggered when adding iframes), and with this patch it works now.

So cool, thanks Salva and James !

Comment 13

[Julien Wajsberg [:julienw]]

since this is npotb I'd like to have this on v1-train too, could you uplift it once you'll merge it ?

Comment 14

[Julien Wajsberg [:julienw]]

James did the latest patch.

Comment 15

[Julien Wajsberg [:julienw]]

James, could we merge this please ?

Comment 16

[James Lal [:lightsofapollo]]

Looks like this is failing travis- I will take a look tomorrow morning.

Comment 17

[Julien Wajsberg [:julienw]]

I tried other travis runs :

https://travis-ci.org/mozilla-b2g/gaia/builds/8453313
https://travis-ci.org/mozilla-b2g/gaia/builds/8454246

same behavior, but the second one is way stranger.

Comment 18

[Julien Wajsberg [:julienw]]

Wow, I really thought we merged this... 

Taking to try to wrap this up next week.

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Tracing our mocha.js, I'm sure we haven't merge this yet. But mocha leaks didn't appear for a long time.
So I think we can close this bug now :)

On the other hand, our mocha version is too old and maybe it's time to update mocha for test-agent.

Comment 20

[Julien Wajsberg [:julienw]]

yeah we need to update mocha :) (see also bug 991663)

We didn't see in a long time because we used "mocha.globals" to whitelist the iframes.

Comment 21

[Ricky Chien [:rickychien] (inactive)]

oh I found these mocha "globalIgnores" in test-agent at https://github.com/mozilla-b2g/gaia/blob/e51f41c910e5d27953f256a293cd0e9036359e33/dev_apps/test-agent/common/vendor/test-agent/test-agent.js#L3031

Bug was fixed by Michael Henretty in bug 867421. 

See also PR https://github.com/mozilla-b2g/gaia/commit/5d6623018670a7d5c4d37bf27bcb56395c3dfc4d#diff-28e938f73741aae68b3acb9d387d5132R2926

Because new version mocha has fixed this iframe leaks. So we could remove this workaround after upgrading mocha successfully. Let me close this bug. :)

Comment 22

[Julien Wajsberg [:julienw]]

Ricky, sorry, I don't understand why you resolve/fixed the bug?

We still didn't upgrade the mocha library?

Comment 23

[Ricky Chien [:rickychien] (inactive)]

Sorry for doing this too quick.

Because this bug has been fixed in this PR https://github.com/mozilla-b2g/gaia/commit/5d6623018670a7d5c4d37bf27bcb56395c3dfc4d#diff-28e938f73741aae68b3acb9d387d5132R2926 although mocha is still old but this workaround has given us an effect we want.

Comment 24

[Julien Wajsberg [:julienw]]

ok, let me reopen then so that we remove this workaround and upgrade mocha :)

Comment 25

[Fernando Campo (:fcampo)]

Another vote for upgrading mocha. It's messing with the postMessage tests from bug 992193 (thanks Julien for finding it)

Comment 26

[Julien Wajsberg [:julienw]]

Attachment: github PR

Comment 27

[Julien Wajsberg [:julienw]]

So, with this PR, we see that an awful lot of test are breaking.

The main difference is because how the new mocha is moving from one test to the next one. In the mocha lib we use now, it's using a postMessage as a 'nextTick' emulation. With the new lib, it uses a simple setTimeout call, which is really faster.

And that's where most of the issues are: timing issues. Some unit tests are not reliable enough and are subject to timing issues.

Here are the errors:
https://travis-ci.org/mozilla-b2g/gaia/builds/25368412

I could fix one of them, the others are more difficult.

Comment 28

[Julien Wajsberg [:julienw]]

Current status: https://travis-ci.org/mozilla-b2g/gaia/builds/25393990

David, there are a lot of failure in the camera test, I tried to make sense of them but I don't know the app enough. Could you or someone from the media team possibly help here?

One of the main change is that the "this" in suiteSetup/setup is not the same than in tests. I haven't checked exactly but I think the one in test() has the same properties, but copied from the parent.

Comment 29

[Anthony Ricaud (:rik)]

This cut the test suite in half (~20 minutes instead of 40), we should really land the new mocha as soon as possible.

I think the strategy here is to open a bug for each application to fix the tests and then land the new mocha. Julien, do you mind opening those bugs?

Comment 30

[David Flanagan [:djf]]

Julien,

I'm not familar with the camera tests myself, but Wilson is.  Wilson please see comment 28

Comment 31

[Julien Wajsberg [:julienw]]

Another source of compatibility issues is with the mocha commit [1]. I made this change [2] to work around this change (another possibility would be to use a shared variable which would not depend on how mocha works internally).

[1] makes nested suites inherit the context using prototype inheritance. That means that if you assign `this.variable` in a setup, then in the same setup or suiteSetup you make something with this variable, then in a nested suite you override it using `this.variable` again, the nested suite's `this.variable` is not the same object than the parent suite's `this.variable`.

This can be worked around using a `this.state` object like in [2], or using shared variables instead.

[1] https://github.com/visionmedia/mocha/commit/6c705cd4d4bde87d735fcf6d8726af720bf392a5
[2] https://github.com/julienw/gaia/commit/3060e61ed9d914ea5de7a440a18e01f60d10147a

Comment 32

[Wilson Page [:wilsonpage]]

julienw: I've fixed the camera unit-tests in my branch 'julienw-874510-upgrade-mocha'[1]. You can probably just cherry-pick my commit across onto your existing pull-request.

I found the majority of the of the failure were related to SinonJS. As combination of `.useFakeTimers()` and `.callsArgAsync` seems to no longer work as it did. `.callsArgAsync` will no longer works when using fake-timers, requiring a `clock.tick(1)` call to.

Anyway, seems all green now.

[1]https://github.com/wilsonpage/gaia/tree/julienw-874510-upgrade-mocha

Comment 33

[Julien Wajsberg [:julienw]]

(In reply to Wilson Page [:wilsonpage] from comment #32)
> julienw: I've fixed the camera unit-tests in my branch
> 'julienw-874510-upgrade-mocha'[1]. You can probably just cherry-pick my
> commit across onto your existing pull-request.
> 
> I found the majority of the of the failure were related to SinonJS. As
> combination of `.useFakeTimers()` and `.callsArgAsync` seems to no longer
> work as it did. `.callsArgAsync` will no longer works when using
> fake-timers, requiring a `clock.tick(1)` call to.

A clear mocha bug from my point of view.
I'll dig deeper and possibly report an issue there.

Thanks so much for the investigation. Maybe it would make sense to have a separate bug and patch so that it can be reviewed by another peer?

Comment 34

[Anthony Ricaud (:rik)]

It would also make sense to have a separate bug to land the fixes asap so that the patch doesn't bitrot while we wait for all the fixes.

Comment 35

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #33)
> (In reply to Wilson Page [:wilsonpage] from comment #32)
> > julienw: I've fixed the camera unit-tests in my branch
> > 'julienw-874510-upgrade-mocha'[1]. You can probably just cherry-pick my
> > commit across onto your existing pull-request.
> > 
> > I found the majority of the of the failure were related to SinonJS. As
> > combination of `.useFakeTimers()` and `.callsArgAsync` seems to no longer
> > work as it did. `.callsArgAsync` will no longer works when using
> > fake-timers, requiring a `clock.tick(1)` call to.
> 
> A clear mocha bug from my point of view.
> I'll dig deeper and possibly report an issue there.

I tried every mocha version since callsArgAsync exists, and no version made this work (you can play with [1]). So something's missing in this explanation :)

[1] http://jsbin.com/yiyegoqa/2/edit

> 
> Thanks so much for the investigation. Maybe it would make sense to have a
> separate bug and patch so that it can be reviewed by another peer?

I filed several bugs for several apps.

Current status (with Wilson's commit) is: https://travis-ci.org/mozilla-b2g/gaia/builds/26140588

Comment 37

[Julien Wajsberg [:julienw]]

Current status: https://travis-ci.org/mozilla-b2g/gaia/builds/27437564

Comment 38

[Julien Wajsberg [:julienw]]

new current status: https://travis-ci.org/mozilla-b2g/gaia/builds/27710536

Comment 39

[Julien Wajsberg [:julienw]]

Current status with patches for bug 1026992, bug 1016429, bug 1016431, bug 1016407:

https://travis-ci.org/mozilla-b2g/gaia/builds/27853301

Comment 40

[Julien Wajsberg [:julienw]]

Current status with patches for bug 1027586, bug 1026992, bug 1016429: 
https://travis-ci.org/mozilla-b2g/gaia/builds/27947852

Comment 41

[Julien Wajsberg [:julienw]]

I think I'm fairly ready now.

Rebased to latest master, added back all the travis jobs, and pushed. New travis run is:
https://travis-ci.org/mozilla-b2g/gaia/builds/28036374

Comment 42

[Julien Wajsberg [:julienw]]

Comment on attachment 8423983 [details] [review]
github PR

Hey Yuren,

please move forward and land yourself if the changes looks fine to you and both travis and gaia try looks right.

Otherwise, I don't mind that you do the needed changes yourself when I'm away :)

Comment 43

[Julien Wajsberg [:julienw]]

Note that a run is about 8min faster with this change.

Comment 44

[Julien Wajsberg [:julienw]]

Hey Jonathan, with the changes in this pull request, the unit tests are failing in TBPL. I believe this is because we need node_modules for unit tests now, and we copy things in [1]. There is a dependency in both make test-agent-server and make test-agent-test, but this needs to be done before B2G-Desktop is launched, so maybe "make update-common" needs to be done explicitely on TBPL, with the same environment variables we use for the marionette-js tests ?

Thanks for your help !

[1] https://github.com/mozilla-b2g/gaia/pull/19330/files#diff-b67911656ef5d18c4ae36cb6741b7965R790

Comment 45

[Jonathan Griffin (:jgriffin)]

Yes, we'll need to add a call to self.make_node_modules() to the gaia_unit mozharness script, similar to what we do for gaia-integration:  http://hg.mozilla.org/build/mozharness/file/ac8e4ce7775e/scripts/gaia_integration.py#l37

Keeping the needinfo for myself to remind me to do this.

Comment 46

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8423983 [details] [review]
github PR

we should use update-common instead of common-install in GAIA/tests/travis_ci/unit-tests-in-firefox/install since this pr removed install-common in Makefile.

Comment 47

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

steal.

Comment 48

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Attachment: github PR: https://github.com/mozilla-b2g/gaia/pull/20871

waiting travis & try server result.

Comment 49

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

travis:
https://travis-ci.org/mozilla-b2g/gaia/builds/28194802

try server:
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=66ab96437b67d74a696eadf9d8079d458f19c7d0

Comment 50

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #45)
> Yes, we'll need to add a call to self.make_node_modules() to the gaia_unit
> mozharness script, similar to what we do for gaia-integration: 
> http://hg.mozilla.org/build/mozharness/file/ac8e4ce7775e/scripts/
> gaia_integration.py#l37
> 
> Keeping the needinfo for myself to remind me to do this.

I'm trying this out on ash right now.  I'm not sure it will be this simple, but maybe we'll be lucky.

Comment 51

[Jonathan Griffin (:jgriffin)]

Landed a change to mozharness to make it install node modules the same way we currently do for gaia-integration tests:

https://hg.mozilla.org/build/mozharness/rev/29a640b027ad

I just retriggered the gaia-unit tests at https://tbpl.mozilla.org/?tree=Gaia-Try&rev=66ab96437b67d74a696eadf9d8079d458f19c7d0 to see if this will resolve the problem.

Comment 52

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #51)
> Landed a change to mozharness to make it install node modules the same way
> we currently do for gaia-integration tests:
> 
> https://hg.mozilla.org/build/mozharness/rev/29a640b027ad
> 
> I just retriggered the gaia-unit tests at
> https://tbpl.mozilla.org/?tree=Gaia-
> Try&rev=66ab96437b67d74a696eadf9d8079d458f19c7d0 to see if this will resolve
> the problem.

Oh this won't help us with gaia-try, since gaia-try uses a copy of mozharness from https: //hg.mozilla.org/users/jford_mozilla.com/mozharness

Comment 53

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #51)
> Landed a change to mozharness to make it install node modules the same way
> we currently do for gaia-integration tests:
> 
> https://hg.mozilla.org/build/mozharness/rev/29a640b027ad
> 
> I just retriggered the gaia-unit tests at
> https://tbpl.mozilla.org/?tree=Gaia-
> Try&rev=66ab96437b67d74a696eadf9d8079d458f19c7d0 to see if this will resolve
> the problem.

John, can you help merge this mozharness patch into the jhford_mozharness repo?  I don't understand the state of that repo and don't want to mess anything up.

Comment 54

[Justin Wood (:Callek)]

something here is In production

Comment 55

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

:jgriffin, can I merge my pr or should I wait your pr for mozharness?

Comment 56

[Julien Wajsberg [:julienw]]

(In reply to Yuren [:yurenju] from comment #46)
> Comment on attachment 8423983 [details] [review]
> github PR
> 
> we should use update-common instead of common-install in
> GAIA/tests/travis_ci/unit-tests-in-firefox/install since this pr removed
> install-common in Makefile.

Actually, you can even remove the call because I adjusted the dependencies in the Makefile.

See :
https://github.com/mozilla-b2g/gaia/commit/2f17d91076c19f6c52ab69a4a42aa93ba26ce7fd#diff-12

(I don't know why I missed these changes in my latest squashed commit).

Comment 57

[Julien Wajsberg [:julienw]]

John, please see comment 53. Thanks !

Comment 58

[Julien Wajsberg [:julienw]]

Stealing back now that I'm back :)

Comment 59

[Julien Wajsberg [:julienw]]

Comment on attachment 8423983 [details] [review]
github PR

Just pushed the missing changes in my PR.

Comment 60

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

I've updated my mozharness from hg.m.o/build/mozharness.  Gaia-try will resume using regular /builds/mozharness as soon as possible.

Comment 61

[Julien Wajsberg [:julienw]]

The latest push showed a remaining issue in the Settings app, so I filed bug 1032327 and provided a patch.

Comment 62

[Jonathan Griffin (:jgriffin)]

The mozharness bits seem to work now; Yuren's originally push to gaia-try now fails with:

10:46:52     INFO -  JavaScript error: app://test-agent.gaiamobile.org/common/vendor/blanket/blanket.js, line 7396: mocha.reporter is not a function
10:46:53     INFO -  JavaScript error: app://test-agent.gaiamobile.org/common/test/agent.js, line 69: TestAgent is not defined

https://tbpl.mozilla.org/php/getParsedLog.php?id=42780403&tree=Gaia-Try&full=1

Comment 63

[Julien Wajsberg [:julienw]]

Hey Jonathan, we really need "make update-common", not only "make node_modules" :)
"make update-common" will run "make node_modules" so you don't need to call it explicitely.

Comment 64

[Julien Wajsberg [:julienw]]

(In reply to Jonathan Griffin (:jgriffin) from comment #62)
> The mozharness bits seem to work now; Yuren's originally push to gaia-try
> now fails with:
> 
> 10:46:52     INFO -  JavaScript error:
> app://test-agent.gaiamobile.org/common/vendor/blanket/blanket.js, line 7396:
> mocha.reporter is not a function
> 10:46:53     INFO -  JavaScript error:
> app://test-agent.gaiamobile.org/common/test/agent.js, line 69: TestAgent is
> not defined
> 

Actually, it's the same error than before :)

Comment 65

[Jonathan Griffin (:jgriffin)]

Attachment: Use make update-common instead of make node_modules,

I'll try this out on cedar to make sure it doesn't break anything.

Comment 66

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #65)
> Created attachment 8448176 [details] [diff] [review]
> Use make update-common instead of make node_modules,
> 
> I'll try this out on cedar to make sure it doesn't break anything.

On cedar, this patch fails with:

12:30:57     INFO -  mkdir -p dev_apps/test-agent/common/vendor/test-agent/
12:30:57     INFO -  rm -f dev_apps/test-agent/common/vendor/test-agent/test-agent.js
12:30:57     INFO -  rm -f dev_apps/test-agent/common/vendor/test-agent/test-agent.css
12:30:57     INFO -  cp node_modules/test-agent/test-agent.js dev_apps/test-agent/common/vendor/test-agent/
12:30:57     INFO -  cp: cannot stat `node_modules/test-agent/test-agent.js': No such file or directory

https://tbpl.mozilla.org/php/getParsedLog.php?id=42784750&tree=Cedar&full=1#error0

Comment 67

[Julien Wajsberg [:julienw]]

Right, because it needs the new patch to work. Kind of chicken and egg.

So, to break the evil circle, you can do both "make node_modules" first and only then "make update-common". It should work with both the current master (and thus the various branches) and the future master.

Thanks for your work.

Comment 68

[Jonathan Griffin (:jgriffin)]

Attachment: Run 'make node_modules' then 'make update-common',

Comment 69

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #68)
> Created attachment 8448334 [details] [diff] [review]
> Run 'make node_modules' then 'make update-common',

This patch doesn't break on cedar.

Comment 70

[Julien Wajsberg [:julienw]]

Comment on attachment 8448334 [details] [diff] [review]
Run 'make node_modules' then 'make update-common',

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

I don't feel competent to review patches for the test harness, especially that I don't really know how it works.

As I understand, "make update-common" will be run for every test job that call the "make_node_modules" method. While it's not necessary (only the unit test harness uses it) it also doesn't do any harm (as it's not a slow operation).

Adding a comment could be useful though, like: "This command copies the necessary files from node_modules to the test-agent's main directory."

Comment 71

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Julien, I tried your pr and I got a test-agent webpage without any test cases, but it works on master, can you check that?

> make clean && DEBUG=1 make && nightly -profile /home/yurenju/src/gaia/profile-debug app://test-agent.gaiamobile.org

Comment 72

[Julien Wajsberg [:julienw]]

Yuren, I think you need to run "make update-common" because this patch deletes some test-agent files in favor of copying them from node_modules.

Both "make test-agent-test" and "make test-agent-server" run the "update-common" goal (using a Makefile dependency).

This will remove the need to commit new versions of test-agent files each time we do a change. The inconvenient is that each time we'll change branches (from a moment before this patch to a moment after this patch) we'll need to run "make update-common".

Comment 73

[Jonathan Griffin (:jgriffin)]

My rationale for applying it to all Gaia jobs was that with the name "update-common", other logic may creep in that isn't unit-test specific.  If that won't ever be the case, I'll change it just to run during gaia_unit tests.

Comment 74

[Julien Wajsberg [:julienw]]

Jonathan, I follow your rationale and honestly I don't know if that's true or wrong :) The name "update-common" comes from a time when we only had unit tests. I don't foresee that we'll use this goal for anything else (it didn't changed once in 2 years), but I may be wrong.

In the end of the day, it doesn't really matter if it always runs or if it runs for unit tests only, so maybe better be safe?

Comment 75

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Attachment: make.patch

we should also make $(PROFILE_FOLDER) depend on update-common if DEBUG=1 since it will confuse if someone just wants open test-agent in browser for unit test without make test-agent-test or make test-agent-server.

Comment 76

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

discussed with Julien on IRC and because update-common depends on node-module and we don't want to $(PROFILE_FOLDER) depends on node-modules, we won't apply patch on comment 75 to julien's pr.

Comment 77

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8423983 [details] [review]
github PR

okay then it looks good to me, r=yurenju 
thanks!

and we have talked about an error message on test-agent, will you add in this pr or you will file another bug?

Comment 78

[Julien Wajsberg [:julienw]]

I'll file a separate bug but mark this bug as a blocker bug. I'll wait for all dependencies to land before landing this.

Thanks !

Comment 79

[Julien Wajsberg [:julienw]]

New status with the patch for bug 1032327:
https://travis-ci.org/mozilla-b2g/gaia/builds/29308895

(Gaia-try seems to be off right now)

Comment 80

[Julien Wajsberg [:julienw]]

Travis is fine, but Gaia-try has more failures: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=8dcacd7ad6f2

Comment 83

[Julien Wajsberg [:julienw]]

Attachment: github PR

Carrying over r=yurenju

Comment 84

[Ricky Chien [:rickychien] (inactive)]

Hi! What's mocha version we used?

BTW, mocha 2.0 was released a few days ago.

https://github.com/mochajs/mocha/blob/master/History.md

Comment 85

[Julien Wajsberg [:julienw]]

I think we use mocha 1.7 for unit tests currently, given the history.
For marionette tests we use mocha 1.20 (the one in package.json).

Comment 86

[BMO Automation]

Firefox OS is not being worked on