Filed by: archaeopteryx [at] coole-files.de https://treeherder.mozilla.org/logviewer.html#?job_id=8052634&repo=autoland https://queue.taskcluster.net/v1/task/BsDMypWjQe6oNM13QQwbgQ/runs/0/artifacts/public/logs/live_backing.log

this test is perma fail on linux64 debug when running in an upgraded environment (upgrading from 12.04 to 16.04 - we need to green the tests up prior to upgrading). I see that we typically fail this test and rerun it in production, we fail because it doesn't finish in 30 seconds, then we rerun and it takes 180+ seconds to complete. here is a log from a failed run: https://public-artifacts.taskcluster.net/eOcuDn-1QJuPSJxoKiNHnQ/0/public/logs/live_backing.log you can see 3 failures in a row on the 10th chunk in linux64 debug here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=442aa421aa40a69a124413b6546c486210b728c1&selectedJob=65308889&group_state=expanded&filter-searchStr=xpcshell :mossop, can you shed some light on what this test is doing and how we can get it to pass the first time in debug mode since it appears to take 3+ minute to run typically

We talked about this on IRC and decided it just needs to be split up. Probably a job for rhelmer or aswan.

I agree this needs to be split up, it's trying to do a lot. I'll take a look.

:rhelmer, checking in here as it has been a few weeks...

Thanks for the reminder - have been out unexpectedly quite a bit the last few weeks.

checking in here as it has been a couple more week...

Test durations for xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_system_update.js on mozilla-central,mozilla-inbound,autoland between 2017-02-21 and 2017-02-28 android-4-2-x86/opt: 0.00 s (0.00 s - 0.00 s over 117 runs) android-4-3-armv7-api15/debug: 0.00 s (0.00 s - 0.00 s over 134 runs) android-4-3-armv7-api15/opt: 0.00 s (0.00 s - 0.00 s over 123 runs) linux32/debug: 287.05 s (240.32 s - 300.04 s over 84 runs) linux32/opt: 91.24 s (71.44 s - 297.92 s over 120 runs) linux64-ccov/opt: 107.43 s (95.31 s - 115.52 s over 4 runs) linux64-jsdcov/opt: 83.29 s (76.86 s - 89.74 s over 4 runs) linux64/asan: 164.05 s (134.60 s - 196.17 s over 123 runs) linux64/debug: 233.96 s (193.17 s - 264.72 s over 127 runs) linux64/opt: 90.36 s (68.70 s - 300.11 s over 133 runs) linux64/pgo: 117.90 s (69.66 s - 298.79 s over 124 runs) macosx64/debug: 100.59 s (96.93 s - 104.26 s over 237 runs) macosx64/opt: 155.09 s (150.65 s - 159.81 s over 103 runs) win32/debug: 129.70 s (120.42 s - 299.74 s over 106 runs) win32/opt: 77.33 s (38.02 s - 88.96 s over 256 runs) win32/pgo: 65.62 s (32.04 s - 75.10 s over 73 runs) win64/debug: 137.38 s (104.08 s - 299.99 s over 102 runs) win64/opt: 93.13 s (81.56 s - 108.33 s over 98 runs) win64/pgo: 86.26 s (73.91 s - 96.84 s over 72 runs) ...so it runs long on all platforms, and too long on linux32-debug. I'd like to see the test split up, but if that's not happening, consider: - skip on linux32-debug only - request a longer timeout (requesttimeoutfactor in xpcshell.ini)

Rob, can you pick this up or help find someone else to work on this?

Sorry for the delay, keeps getting pushed off by other things. I should be able to get to it this week.

Comment on attachment 8842991 [details] Bug 1324192 - move common system add-on test functions to head_addons.js https://reviewboard.mozilla.org/r/116732/#review118388 ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1346 (Diff revision 1) > return xml; > } > + > +// system add-on test functions > + > +let dir = FileUtils.getDir("ProfD", ["sysfeatures", "hidden"], true); this is in head, doesn't it clash with tests that also declare a variable called `dir`? ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1347 (Diff revision 1) > +do_get_file("data/system_addons/system1_1.xpi").copyTo(dir, "system1@tests.mozilla.org.xpi"); > +do_get_file("data/system_addons/system2_1.xpi").copyTo(dir, "system2@tests.mozilla.org.xpi"); > + > +dir = FileUtils.getDir("ProfD", ["sysfeatures", "prefilled"], true); > +do_get_file("data/system_addons/system2_2.xpi").copyTo(dir, "system2@tests.mozilla.org.xpi"); > +do_get_file("data/system_addons/system3_2.xpi").copyTo(dir, "system3@tests.mozilla.org.xpi"); Can you put this in a function and only call it from system update tests rather than doing it for every test in this directory? ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1357 (Diff revision 1) > +do_get_file("data/system_addons/system3_2.xpi").copyTo(dir, "system3@tests.mozilla.org.xpi"); > + > +const distroDir = FileUtils.getDir("ProfD", ["sysfeatures", "empty"], true); > + > +function getCurrentUpdatesDir() { > + let dir = updatesDir.clone(); I don't see updatesDir defined? ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1398 (Diff revision 1) > + }, > + } > + })); > +} > + > +function* check_installed(conditions) { Since this is shared throughout this directory, maybe give it a more specific name? Similarly with the other functions below

Comment on attachment 8842991 [details] Bug 1324192 - move common system add-on test functions to head_addons.js https://reviewboard.mozilla.org/r/116732/#review118394 Oh hm, I see you handled some of the things I commented on in the next commit. Disregard that last round of feedback until I read the whole thing...

Comment on attachment 8842992 [details] Bug 1324192 - use better names and document common system addon test functions https://reviewboard.mozilla.org/r/116734/#review118398 Some little nits, the biggest concern is avoiding doing the copies directly from head.js in tests that don't use system addons. Is there any reason not to roll this together with the previous commit? I think the state between the two is actually broken and it wouldn't lose anything to merge the two. ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1345 (Diff revision 1) > +do_get_file("data/system_addons/system1_1.xpi").copyTo(hiddenSystemAddonDir, "system1@tests.mozilla.org.xpi"); > +do_get_file("data/system_addons/system2_1.xpi").copyTo(hiddenSystemAddonDir, "system2@tests.mozilla.org.xpi"); > + > +let prefilledSystemAddonDir = FileUtils.getDir("ProfD", ["sysfeatures", "prefilled"], true); > +do_get_file("data/system_addons/system2_2.xpi").copyTo(prefilledSystemAddonDir, "system2@tests.mozilla.org.xpi"); > +do_get_file("data/system_addons/system3_2.xpi").copyTo(prefilledSystemAddonDir, "system3@tests.mozilla.org.xpi"); Can you put the actual file copeis into a funciton that only gets called from system update tests to avoid unneeded work before other tests? ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1361 (Diff revision 1) > -const distroDir = FileUtils.getDir("ProfD", ["sysfeatures", "empty"], true); > > -function getCurrentUpdatesDir() { > +/** > + * Returns current system add-on update directory (stored in pref). > + */ > +function getCurrentSystemAddonUpdatesDir() { this appears to only be used one place below, just inline it there? ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1410 (Diff revision 1) > } > })); > } > > -function* check_installed(conditions) { > +/** > + * Check currently installed ssystem add-ons against a set of conditions. ssystem -> system ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1411 (Diff revision 1) > })); > } > > -function* check_installed(conditions) { > +/** > + * Check currently installed ssystem add-ons against a set of conditions. > + * trailing space ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1494 (Diff revision 1) > return subdirs; > } > > -function* setup_conditions(setup) { > +/** > + * Sets up initial system add-on update conditions. > + * trailing space ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1517 (Diff revision 1) > - yield check_installed(setup.initialState); > + yield checkInstalledSystemAddons(setup.initialState); > } > > -function* verify_state(initialState, finalState = undefined, alreadyUpgraded = false) { > +/** > + * Verify state of system add-ons after installation. > + * space

Comment on attachment 8842993 [details] Bug 1324192 - split system addon update tests into expected pass / expected fail to avoid timeouts https://reviewboard.mozilla.org/r/116736/#review118402 ::: toolkit/mozapps/extensions/test/xpcshell/test_system_update_fail.js:1 (Diff revision 1) > +// Tests that system add-on upgrades fail to upgrade in expected cases. can you "hg copy" this file so we keep the original history and the diff is easier to read?

Comment on attachment 8842991 [details] Bug 1324192 - move common system add-on test functions to head_addons.js https://reviewboard.mozilla.org/r/116732/#review118388 > Can you put this in a function and only call it from system update tests rather than doing it for every test in this directory? Oops - meant to remove these actually, at least a few of these are redundant.

Comment on attachment 8842991 [details] Bug 1324192 - move common system add-on test functions to head_addons.js https://reviewboard.mozilla.org/r/116732/#review118804 ::: toolkit/mozapps/extensions/test/xpcshell/head_addons.js:1355 (Diff revision 3) > + do_get_file("data/system_addons/system2_2.xpi").copyTo(prefilledSystemAddonDir, "system2@tests.mozilla.org.xpi"); > + do_get_file("data/system_addons/system3_2.xpi").copyTo(prefilledSystemAddonDir, "system3@tests.mozilla.org.xpi"); > +} > + > +const distroDir = FileUtils.getDir("ProfD", ["sysfeatures", "empty"], true); > +registerDirectory("XREAppFeat", distroDir); looks like you're doing this in the test so don't need to do it here?

Comment on attachment 8843092 [details] Bug 1324192 - split system addon update tests into expected pass / expected fail to avoid timeouts https://reviewboard.mozilla.org/r/116846/#review118806

Pushed by rhelmer@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/513015fce0ee move common system add-on test functions to head_addons.js r=aswan https://hg.mozilla.org/integration/autoland/rev/62163a007dd5 split system addon update tests into expected pass / expected fail to avoid timeouts r=aswan

https://hg.mozilla.org/mozilla-central/rev/513015fce0ee https://hg.mozilla.org/mozilla-central/rev/62163a007dd5

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b71135a7500 https://hg.mozilla.org/releases/mozilla-aurora/rev/949d1800c7f2

https://hg.mozilla.org/releases/mozilla-beta/rev/0ba3df0bf6c9 https://hg.mozilla.org/releases/mozilla-beta/rev/782ebf2dfc40