385859 - comprehensive tests for bookmark html import / export code

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect, P4)

Description

[(not reading, please use [e-mail] instead)]

comprehensive tests for bookmark html import / export code

for some starting points, see bug #383840 comment #22.

The goal would be to test nsPlacesImportExportService.cpp code (through nsIPlacesImportExportService interface).

There is a lot we could do.

Comment 1

[Mike Connor [:mconnor]]

Needs an owner, at this point I think M9 is the sane target, as much as it pains me to say that.

Comment 2

[Dietrich Ayala (:dietrich)]

(In reply to comment #0)
> comprehensive tests for bookmark html import / export code
> 
> for some starting points, see bug #383840 comment #22.
> 

Copying the referenced comment here:

> again, a test case could be written here to test these changes.
> 
> for nsIPlacesImportExportService, we could check that:
> 
> a) with "importHTMLFromFile(x, false);", importing appends
> b) with "importHTMLFromFile(x, true);", importing replaces
> c) exportHTMLToFile() doesn't write out HTML with ITEM_IDs.
> 
> In fact, there are a lot of import / export tests that we should be doing. 
> I'll log a spin of bug on that.
> 

A and B are valid. C is overkill, IMO.

Comment 3

[Mike Connor [:mconnor]]

Not going to continue to block on this.  If you disagree with this decision, please renominate with reasons why we can't ship with this in final

Comment 4

[Ray Kiddy]

This seems to be a common fate for "do the right thing" bugs in MoCo.

Are there more specific suggestions that could be made to enable this to move forward? Are there certain risks that the organization is more concerned about? Perhaps a bug or bugs that address those concerns will have more effect.

Comment 5

[Benjamin Smedberg]

I think all we need is somebody to actually write the tests.

Comment 6

[Dietrich Ayala (:dietrich)]

fwiw, import/export is under test here:

http://mxr.mozilla.org/seamonkey/source/browser/components/places/tests/unit/test_bookmarks_html.js

IMO, this bug is not a blocker because we already have some level of automated testing, as well as a lack of reported bugs for specific problems. 

(Unfortunately, drivers don't always explain the reason why they +'d or -'d in the bug comments... hint, hint)

Ray, saying "you should fix this" doesn't advance us towards a fix. We clearly understand that it needs to be fixed, or the bug would've been WONTFIXed :) Digging in and answering the questions that you posed, and then executing on the results would be awesome

Comment 7

[Dietrich Ayala (:dietrich)]

(In reply to comment #5)
> I think all we need is somebody to actually write the tests.
> 

a couple of things that need to be done:

1. would be to fix the commented out parts of the existing tests: figure out why they're failing, and make sure bugs are filed for each issue.

2. write new tests for the scenarios in comment #2.

Comment 8

[Ondrej Brablc]

(In reply to comment #2)
> (In reply to comment #0)
> > again, a test case could be written here to test these changes.
> > 
> > for nsIPlacesImportExportService, we could check that:
> > 
> > ...
> > c) exportHTMLToFile() doesn't write out HTML with ITEM_IDs.
> > 
> 
> A and B are valid. C is overkill, IMO.

The C test would fail anyway, neither export nor import use it - see bug 420729.

Comment 9

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 10

[Marco Bonardo [:mak]]

we have tests for importHTMLFromFile, fwiw.