Closed Bug 709531 Opened 11 years ago Closed 11 years ago

Addons Manager tests should have AMO-related prefs reset after each test to disallow network access

Categories

(Toolkit :: Add-ons Manager, defect, P1)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla11
Tracking Flags:
Tracking Status
firefox10 --- fixed

People

(Reporter: Unfocused, Assigned: Unfocused)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Patch v2
14.84 KB, patch
mossop
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
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.
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.
Blocks: 617414
No longer blocks: 617414
Removed two more tests which depended on the test directory removed in comment 9 and 10.
(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
Attached patch patch rev 1 (obsolete) — — Splinter Review 
Most of the prefs are already reset correctly, caching isn't though
Assignee: bmcbride → dtownsend
Attachment #581780 - Flags: review?(bmcbride)
Attached patch Patch v2 — — Splinter Review 
New and improved!

And yes, the stuff in head.js is being paranoid on purpose.
Assignee: dtownsend → bmcbride
Attachment #581780 - Attachment is obsolete: true
Attachment #581780 - Flags: review?(bmcbride)
Attachment #581817 - Flags: review?(dtownsend)
And I couldn't finds anything in the xpinstall tests that actually set any relevant prefs, so AFAIK they're fine.
Attachment #581817 - Flags: review?(dtownsend) → review+
http://hg.mozilla.org/mozilla-central/rev/243ff1422033
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
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.
Attachment #581817 - Flags: approval-mozilla-aurora?
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.
Blocks: 710956
Comment on attachment 581817 [details] [diff] [review]
Patch v2

[Triage Comment]
Stop the insanity :)! Approved for Aurora.
Attachment #581817 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
status-firefox10: --- → fixed
Marking qa- as I don't think this is something QA needs to verify. Please mark qa+ if this is not the case.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.