637413 - Use blank blocklist xml with addon manager tests

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Joe Drew (not getting mail)]

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.

Comment 1

[Dave Townsend [:mossop]]

Note that I fixed another test in exactly this way in bug 614868

Comment 2

[Joe Drew (not getting mail)]

Attachment: 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.

Comment 3

[Joe Drew (not getting mail)]

Blocks a hardblocker => blocks.

Comment 4

[Dave Townsend [:mossop]]

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.

Comment 5

[Bill Gianopoulos [:WG9s]]

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.

Comment 6

[Dave Townsend [:mossop]]

(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?

Comment 7

[Bill Gianopoulos [:WG9s]]

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.

Comment 8

[Joe Drew (not getting mail)]

Attachment: 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.

Comment 9

[Joe Drew (not getting mail)]

This patch passed try!

Comment 10

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/21b0f3548852

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marking as verified fixed based on no test failures.