1013999 - Various gaia-unit-test permafails on b2g30 from "global leak detected" errors

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Almost certainly fallout from bug 976920 merging over from mozilla-beta. Not sure if this is exposing bugs that should be fixed or if we should just backout bug 976920 from b2g30 and get on with life.

https://tbpl.mozilla.org/php/getParsedLog.php?id=40084747&tree=Mozilla-B2g30-v1.4

b2g_ubuntu64_vm mozilla-b2g30_v1_4 opt test gaia-unit on 2014-05-21 02:04:22 PDT for push 12fe2b67a099
slave: tst-linux64-spot-182

gaia-unit-tests TEST-UNEXPECTED-FAIL | communications/dialer/test/unit/mmi_test.js | dialer/mmi Should handle network initiated messages properly | global leak detected: postMessage
gaia-unit-tests TEST-UNEXPECTED-FAIL | communications/contacts/test/unit/image_loader_test.js | Image Loader Test Suite > imgsLoading balance > "before each" hook | global leak detected: stop
gaia-unit-tests TEST-UNEXPECTED-FAIL | camera/test/unit/controllers/preview-gallery_test.js | controllers/preview-gallery PreviewGalleryController() Should add and remove a resize handler when opening and closing view | global leaks detected: addEventListener, removeEventListener
gaia-unit-tests TEST-UNEXPECTED-FAIL | sms/test/unit/attachment_test.js | attachment_test.js view attachment with open activity Activity errors > "before each" hook | global leak detected: alert
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/hardware_buttons_test.js | system/HardwareButtons press and release home (screen enabled) | global leak detected: dispatchEvent
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/radio_test.js | Radio >  set enabled to true conn0 is enabling, conn1 is enabled "before each" hook | global leak detected: dispatchEvent
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/fxa_manager_test.js | system/FxAccountManager > "before each" hook | global leak detected: addEventListener
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/fxa_manager_test.js | system/FxAccountManager > On openFlow mozFxAccountsRPChromeEvent "before each" hook | global leak detected: dispatchEvent
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/update_manager_test.js | system/UpdateManager "after each" hook | TypeError: self.toaster is null (http://system.gaiamobile.org:8080/js/update_manager.js?time=1400664443911:406)
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/fxa_client_test.js | system/FxAccountsClient > "before each" hook | global leaks detected: addEventListener, removeEventListener, dispatchEvent
gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/app_window_factory_test.js | system/AppWindowFactory handle event classic app launch | global leak detected: dispatchEvent

Comment 1

[Ed Morley [:emorley]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=40108731&tree=Mozilla-B2g30-v1.4

Comment 2

[Boris Zbarsky [:bzbarsky]]

We don't want to back bug 976920 out (except temporarily), because there are known web compat issues with the state without that bug.

Peter, this sounds similar to the issue you ran into with the Window patches on inbound, right?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  See bug 789261 comment 10.

Basically, the gaia unit tests are making busted assumptions.  We should just add every single one of these to the globals whitelist, imho.  Going to do a try run to verify this.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Also, who the heck owns this stuff?  Trying some people who have blame for the relevant files, because while I can add this stuff to the whitelist, any time someone starts using a new function it will need to be added there too, which doesn't scale.  So we really need to fix the tests to not be broken.

Comment 5

[James Lal [:lightsofapollo]]

I don't own these tests but I put together some of the bits used to write the tests... We are using some off the shelf test framework (mocha) which is doing some assertions after each test to ensure no new globals are exposed...

We can disable the global checks and upstream a patch to make these globally acceptable... I guess there was some change that effects the visibility of window properties which is why this is cropping up.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> which is doing some assertions after each test to ensure no new globals are exposed...

Right, but you're also using sinon.js, which adds window properties all the time, is the point.

> I guess there was some change that effects the visibility of window properties

More precisely, such a change was reverted for now on beta.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

I was only updating sinon to 1.9.1 (bug 995425). I don't think I changed any code in tests.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So I guess the problem is fundamentally with any test that does sinon.spy(window, foo) where foo comes from the proto chain, not the window object itself.

The smart thing to do if we want to keep this mocha "find props that got randomly added" feature would be to just teach sinon.spy to tell mocha about the properties it's adding... or to have sinon remove the props it adds at the end of the test.

Comment 9

[Kevin Grandon :kgrandon]

Gaia now uses JSHint which does a pretty good job of finding unintentional globals. I think we should just remove this feature.

Comment 10

[Julien Wajsberg [:julienw]]

see also bug 1013816

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yep, that's the trunk version.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=688db6ec9aaf is green. Can we get it reviewed and landed soon please? :)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Created a pull request at https://github.com/mozilla-b2g/gaia/pull/19512

Not sure how to request review on that...

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Attachment: Link to pull request

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8426677 [details] [review]
Link to pull request

Ah right - if this is for 1.4 I think something like this is fine. Going forward I'll try to land bug 1014180, or get this patched upstream.

I'll give F+, but someone like Julien or James should probably put the official R+ on the patch.

Comment 16

[Julien Wajsberg [:julienw]]

I think we'll try to land bug 1014180 on the branch instead.

Kevin, you can take care of this too?

Comment 17

[Kevin Grandon :kgrandon]

This requires uplifting all of our node_module dependencies to 1.4. I'm not sure how feasible this is, but I will open a pull request to see what kind of breakage we get. We'll likely need to disable a bunch of tests in 1.4

Comment 18

[Julien Wajsberg [:julienw]]

Another possibility could be to branch node_modules ?
(BTW that's what Travis does when caching stuff)

Although I'm optimistic here :)

If we need to disable tests to uplift 1014180, then I'd rather go with Boris' simple solution.

Comment 19

[Kevin Grandon :kgrandon]

Ok, let's try it. Branched node_modules in 1.4 and updated test-agent: https://github.com/mozilla-b2g/gaia-node-modules/commit/00b8ace9941a74105597beefdbed8b0bb9c3fc00

And here is the pull request to gaia: https://github.com/mozilla-b2g/gaia/pull/19532

It's pretty self-contained. If it errors out, let's go with the simple approach.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Talked it over with Julien and Kevin and decided to go with bz's PR for now to at least get b2g30 green again. Leaving the bug open until we see if comment 19 is viable or not.

v1.4: https://github.com/mozilla-b2g/gaia/commit/8990a6b8ee6a2c8f9668ef24e6d86e37aecad750

Comment 21

[Kevin Grandon :kgrandon]

Turns out we can't do a straight update of test-agent for 1.4, and I don't want to spend too much longer investigating. I'll use this patch for master, and we can let 1.4 ship as-is.

Comment 22

[Kevin Grandon :kgrandon]

Comment on attachment 8426677 [details] [review]
Link to pull request

This was irc-reviewed.