Last Comment Bug 709531 - Addons Manager tests should have AMO-related prefs reset after each test to disallow network access
: Addons Manager tests should have AMO-related prefs reset after each test to d...
Status: RESOLVED FIXED
[qa-]
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: unspecified
: All All
: P1 blocker (vote)
: mozilla11
Assigned To: Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
:
Mentors:
: 711084 (view as bug list)
Depends on:
Blocks: 710956
  Show dependency treegraph
 
Reported: 2011-12-10 15:06 PST by Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
Modified: 2011-12-28 13:17 PST (History)
10 users (show)
bmcbride: in‑testsuite-
bmcbride: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch rev 1 (6.20 KB, patch)
2011-12-14 14:13 PST, Dave Townsend [:mossop]
no flags Details | Diff | Splinter Review
Patch v2 (14.84 KB, patch)
2011-12-14 15:35 PST, Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not)
dtownsend: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-10 15:06:53 PST
We've had a few cases where a test doesn't correctly reset all AMO-related prefs, and therefore subsequent tests end up hitting the network, which is not only a bad idea, but it can lead to test failures. eg:

https://tbpl.mozilla.org/php/getParsedLog.php?id=7870276&tree=Firefox&full=1

The head.js file should just reset these each time.
Comment 1 Phil Ringnalda (:philor, back in August) 2011-12-13 10:36:52 PST
https://tbpl.mozilla.org/php/getParsedLog.php?id=7907285&tree=Firefox
https://tbpl.mozilla.org/php/getParsedLog.php?id=7907246&tree=Firefox
https://tbpl.mozilla.org/php/getParsedLog.php?id=7907240&tree=Firefox

Hitting the network is pretty much the last thing on earth you want to be doing right now :)
Comment 2 Phil Ringnalda (:philor, back in August) 2011-12-13 22:03:45 PST
https://tbpl.mozilla.org/php/getParsedLog.php?id=7921482&tree=Mozilla-Inbound

Even more so right now.
Comment 3 Phil Ringnalda (:philor, back in August) 2011-12-13 22:34:57 PST
https://tbpl.mozilla.org/php/getParsedLog.php?id=7921925&tree=Mozilla-Inbound
Comment 6 Phil Ringnalda (:philor, back in August) 2011-12-14 09:13:26 PST
https://tbpl.mozilla.org/php/getParsedLog.php?id=7929139&tree=Firefox
Comment 7 Phil Ringnalda (:philor, back in August) 2011-12-14 09:15:31 PST
https://tbpl.mozilla.org/php/getParsedLog.php?id=7929867&tree=Firefox
Comment 8 Ben Hearsum (:bhearsum) 2011-12-14 09:36:43 PST
Yes, please. Our tests should be runnable without internet access. And they should continue to work if AMO goes down temporarily, or changes location permanently.

We're planning on shutting off outside network access to the build and test machines in the near future, to help enforce things like this.
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-14 10:44:51 PST
Tests disabled

https://hg.mozilla.org/mozilla-central/rev/ce998a59ee1d
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-14 10:49:31 PST
And the rest of them too, for good measure

https://hg.mozilla.org/mozilla-central/rev/a07143afa2d0
Comment 11 :Ehsan Akhgari 2011-12-14 12:33:49 PST
Removed two more tests which depended on the test directory removed in comment 9 and 10.
Comment 12 :Ehsan Akhgari 2011-12-14 12:33:59 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #11)
> Removed two more tests which depended on the test directory removed in
> comment 9 and 10.

https://hg.mozilla.org/mozilla-central/rev/4d2921df8f34
Comment 13 Dave Townsend [:mossop] 2011-12-14 14:13:20 PST
Created attachment 581780 [details] [diff] [review]
patch rev 1

Most of the prefs are already reset correctly, caching isn't though
Comment 15 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-14 15:35:42 PST
Created attachment 581817 [details] [diff] [review]
Patch v2

New and improved!

And yes, the stuff in head.js is being paranoid on purpose.
Comment 16 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-14 15:38:29 PST
And I couldn't finds anything in the xpinstall tests that actually set any relevant prefs, so AFAIK they're fine.
Comment 17 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-14 17:04:23 PST
http://hg.mozilla.org/mozilla-central/rev/243ff1422033
Comment 18 Phil Ringnalda (:philor, back in August) 2011-12-14 19:11:04 PST
https://tbpl.mozilla.org/php/getParsedLog.php?id=7942014&tree=Mozilla-Aurora - Hello from Aurora's xpinstall/ tests!
Comment 19 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-14 19:15:57 PST
Comment on attachment 581817 [details] [diff] [review]
Patch v2

Lets fix this on Aurora too :) No change to any code we ship, just test-related fixed.
Comment 20 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-14 20:35:42 PST
This may or may not have caused an android test orange (bug 710956), which showed up on this push, but no one knows why/how.
Comment 21 Mihai Sucan [:msucan] 2011-12-15 08:38:34 PST
*** Bug 711084 has been marked as a duplicate of this bug. ***
Comment 22 Alex Keybl [:akeybl] 2011-12-15 08:42:10 PST
Comment on attachment 581817 [details] [diff] [review]
Patch v2

[Triage Comment]
Stop the insanity :)! Approved for Aurora.
Comment 23 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2011-12-16 23:37:25 PST
Merged in the followup fix to the mobile browser_addons.js test too, to avoid that going orange.

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7ca1d99832b
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 13:17:33 PST
Marking qa- as I don't think this is something QA needs to verify. Please mark qa+ if this is not the case.

Note You need to log in before you can comment on or make changes to this bug.