Use blank blocklist xml with addon manager tests

VERIFIED FIXED in mozilla2.0

Status

()

VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: joe, Assigned: joe)

Tracking

Trunk
mozilla2.0
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [hardblocker])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Bug 637008 was caused by us adding a <gfxItems> tag to the xml we ship with Firefox. Dave says in bug 637008 comment 11:

"In order to tell what add-ons have been newly blocked we load the old list
first to compare it to the new. This has the side effect of notifying about the
GFX entries.

You could fix this by creating an empty blocklist.xml file in the profile
directory before starting the load."

I'm going to do exactly that.
(Assignee)

Updated

8 years ago
Blocks: 636254
No longer depends on: 636254
Blocks: 635044
Note that I fixed another test in exactly this way in bug 614868
(Assignee)

Comment 2

8 years ago
Created attachment 515770 [details] [diff] [review]
move the creation of an empty blocklist file to head_addons.js

Well, that's pretty awesome! I don't even have to write the code.

This is currently going through try with an updated blocklist xml file to ensure it fixes this bug.
Attachment #515770 - Flags: review?(dtownsend)
(Assignee)

Updated

8 years ago
OS: Mac OS X → All
Hardware: x86 → All
Version: 1.9.2 Branch → Trunk
(Assignee)

Comment 3

8 years ago
Blocks a hardblocker => blocks.
blocking2.0: --- → final+
Whiteboard: [hardblocker][has patch]
Comment on attachment 515770 [details] [diff] [review]
move the creation of an empty blocklist file to head_addons.js

Assuming it passes tests this is fine.
Attachment #515770 - Flags: review?(dtownsend) → review+
OK.  This might be what we need to get to release, but it would seem to me that a test having to do with addon blocklisting shoud be completely ignoring blocklist entries it does not understand (like gfx ones or whatever else we make up later on) or this is just going to come back to bite us later on.

I really think the test should be fixed at some point so that this issue does not recur later.

Making everyone else who wants to add a new blocklist have to workaround the way this test works just seems wrong to me.
(In reply to comment #5)
> OK.  This might be what we need to get to release, but it would seem to me that
> a test having to do with addon blocklisting shoud be completely ignoring
> blocklist entries it does not understand (like gfx ones or whatever else we
> make up later on) or this is just going to come back to bite us later on.
> 
> I really think the test should be fixed at some point so that this issue does
> not recur later.
> 
> Making everyone else who wants to add a new blocklist have to workaround the
> way this test works just seems wrong to me.

Not sure what you're saying here. The patch makes it so all tests in the add-ons manager directory ignore the shipped blocklist so this issue won't occur again in those cases. What more would you want?
OK. Sorry, I did not follow completely.  I thought this was a fix just for the GFX blocklist.  if it works for that and all future added blocklists, then that is exactly what I thought was required.  Guess I should have made sure I knew what I was taking about before commenting.
(Assignee)

Comment 8

8 years ago
Created attachment 515835 [details] [diff] [review]
v2

Insanely, nsIFile.copyFile throws an error if the file exists already. So, in every test that needs to overwrite blocklist.xml in the profile directory, we have to delete that file if it already exists.

This revised patch is making its way through try.
Attachment #515770 - Attachment is obsolete: true
Attachment #515835 - Flags: review?(dtownsend)
(Assignee)

Comment 9

8 years ago
This patch passed try!
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][needs review mossop]
Attachment #515835 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/21b0f3548852
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker][has patch][needs review mossop] → [hardblocker]
Marking as verified fixed based on no test failures.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-
Target Milestone: --- → mozilla2.0
You need to log in before you can comment on or make changes to this bug.