Spinning up a new bug as I'm not sure if this is the desired fix or not for bug 1013999. The global leak checks are not really needed now that we have JSHint. Since they are causing problems, perhaps we should remove them?

What do you guys think? Another quick fix could be to add it to globalIgnores. I guess my thoughts are that I just don't think we really need this now that we have JSHint. It seems to do a really good job at detecting leakage. Also having to specify globals twice in tests can suck (once for mocha and once for JSHint).

jshint doesn't find everything (eg: window.XXX = 'YYY', or using a jshintignored or xfailed file). I'd tend to agree with your proposal but I know we have some developers doing bad code and I'm afraid :(

(In reply to Julien Wajsberg [:julienw] from comment #2) > jshint doesn't find everything (eg: window.XXX = 'YYY', or using a > jshintignored or xfailed file). > > I'd tend to agree with your proposal but I know we have some developers > doing bad code and I'm afraid :( What situation are you envisioning? In general bad code is a per-app type of problem and our apps operate in independent global namespaces from each other. Intentional global contamination via window.XXX is more of a library problem where the library may run in a situation where its user may just do everything in the global scope itself. I think our main concern would be about accidental memory leaks due to leaving a "var" off and retaining a reference on the global object. But not only will jshint find that, "use strict" (which jshint yells about) makes that impossible. If someone is doing window.XXX they likely mean it and we are now gaining memory metrics elsewhere on our automated tests. (And with Tarako, leaks become somewhat more obvious :)

Comment on attachment 8426478 [details] Github pull request I have no objections but julienw has been the lead on TA changes lately so I would wait for him to respond before landing unless this is an emergency.

I kind of agree that assigning to window.XXX is a fairly deliberate action. I also don't think that this is going to help to prevent bad code, as the if this is a problem - they probably won't be writing unit tests in the first place :) If we don't want to land this, I suppose I would be ok with adding all of the globals in bug 1013999 to our pre-defined globals. I just thought that this might be more future-proof and make our test files a bit cleaner. I cry everytime I look at bootstrap_test.js, https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/bootstrap_test.js#L47

Comment on attachment 8426478 [details] Github pull request ok, let's do this, r=me Andrew convinced me here: global leaks is really a library issue and here we're mainly working with applications.

Thanks guys. If we run into this in the future, or have a few troublesome apps, perhaps we can look at some alternatives, set this on a per-app/test basis.

Landed in js-test-agent: https://github.com/mozilla-b2g/js-test-agent/commit/59e03c9da4097e2151ed79e90d014ef5c221b271 Follow-up to update test-agent version: https://github.com/mozilla-b2g/js-test-agent/commit/591c94a27a7c6fdaa4c007cef4c141fac0955e73 Update to node_modules: https://github.com/mozilla-b2g/gaia-node-modules/commit/dbccfa4909974b3f4a48bdcd54f60e95991f78d9 And finally a pull request open to gaia: https://github.com/mozilla-b2g/gaia/pull/19525 Everything will probably be fine, but I'm concerned about the marionette-apps update to ~0.3.5. I need to follow-up there to understand why it's bumped on node_modules but not master yet. (The two package.json files should stay in sync)

Hi Kevin, Could you please sync the test-agent.js from js-test-agent to gaia by using "make update-common" after you updated node_modules. Thanks!

Thanks for the reminder! :)

Landed: https://github.com/mozilla-b2g/gaia/commit/56162d96620aee365a463d56dd346984344f1d0c

This isn't totally fixed, while trying to land bug 789261 we're still running into this, see https://tbpl.mozilla.org/php/getParsedLog.php?id=40371072&tree=Mozilla-Inbound 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 | system/test/unit/app_window_factory_test.js | system/AppWindowFactory handle event classic app launch | global leak detected: dispatchEvent gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/screenshot_test.js | system/Screenshot Receive home+sleep event with available device storage. | 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/notifications_test.js | system/NotificationScreen > resending notifications > on restart > "before each" hook | global leak detected: dispatchEvent 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/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/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/system_nfc_connect_dialog_test.js | NfcConnectSystemDialog Dialog lifecycle "before each" hook | global leak detected: dispatchEvent

This change has yet to be merged over to m-i yet though, right?

Ah, yes, I thought it'd already be merged.

Wow! Did you guys notice this patch reduce a lot of time on Travis? I looked into Travis and noticed that our unit test duration reduce from 35~40min to 20~25min after this patch!! Thanks Kevin accidentally improve our test-agent. LOL

Awesome! Let's bring that time back with coverage checks maybe? :)