Intermittent TEST-UNEXPECTED-TIMEOUT | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_system_update.js | Test timed out

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: intermittent-bug-filer, Assigned: rhelmer)

Tracking

({intermittent-failure})

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [stockwell fixed])

Attachments

(2 attachments, 2 obsolete attachments)

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
Blocks: 1326400
Flags: needinfo?(dtownsend)
We talked about this on IRC and decided it just needs to be split up. Probably a job for rhelmer or aswan.
Flags: needinfo?(rhelmer)
Flags: needinfo?(dtownsend)
Flags: needinfo?(aswan)
I agree this needs to be split up, it's trying to do a lot. I'll take a look.
Assignee: nobody → rhelmer
Flags: needinfo?(rhelmer)
Flags: needinfo?(aswan)
:rhelmer, checking in here as it has been a few weeks...
Flags: needinfo?(rhelmer)
Thanks for the reminder - have been out unexpectedly quite a bit the last few weeks.
Status: NEW → ASSIGNED
Flags: needinfo?(rhelmer)
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)
Whiteboard: [stockwell needswork]
Rob, can you pick this up or help find someone else to work on this?
Flags: needinfo?(rhelmer)
Sorry for the delay, keeps getting pushed off by other things. I should be able to get to it this week.
Flags: needinfo?(rhelmer)
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
Attachment #8842992 - Flags: review?(aswan) → review+
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?
Attachment #8842992 - Attachment is obsolete: true
Attachment #8842993 - Attachment is obsolete: true
Attachment #8842993 - Flags: review?(aswan)
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?
Attachment #8842991 - Flags: review?(aswan) → review+
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
Attachment #8843092 - Flags: review?(aswan) → review+
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
Whiteboard: [stockwell needswork] → [stockwell fixed]
https://hg.mozilla.org/mozilla-central/rev/513015fce0ee
https://hg.mozilla.org/mozilla-central/rev/62163a007dd5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.