279150 - RSS/Atom parsers should have unit tests

Status: RESOLVED WONTFIX

(MailNews Core :: Feed Reader, enhancement)

Description

[Robert Sayre]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; rv:1.7.3) Gecko/20040913 Firefox/0.10

It's my experience that the only way to deal with variations in RSS feeds is to
have a large amount of unit tests. I've asked around a little, and have gotten
donations of a couple huge unit test libraries. Plus, there's the brand new
RSS1.1, which has unit tests in the public domain.

Right now, Feed.js is structured in a way that makes it difficult to test the
parsers. I was thinking that the parsers could be moved out of Feed, and into
their own classes. That way, they could be fed test feeds from xpcshell. Thoughts?

I'm up for doing this.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Robert Sayre]

Attachment: FeedParser.js -- new file

couldn't figure out the CVS incantation to let me add new file w/ anon access.
For use with the following patch.

Comment 2

[Robert Sayre]

Attachment: Make feed parser testable as standalone

Right now, the parsing code is very tied up with the download code. This patch
makes it possible to unit test the parsers from the command line (I have
hundreds of test files ready to add...).

Comment 3

[Scott MacGregor]

Comment on attachment 172058 [details]
FeedParser.js -- new file

I think you meant to make these question marks not plusses since your asking
for a review

Comment 4

[Robert Sayre]

(In reply to comment #3)
> (From update of attachment 172058 [details] [edit])
> I think you meant to make these question marks not plusses since your asking
> for a review
> 

Oops. Yes, that's what I meant.
Knowing is half the battle. I'm surprised it let me do that...

Comment 5

[Scott MacGregor]

robert, did you make this patch from a 1.0 branch or from the mozilla trunk.
That effects what I should do with this change since the parser is slighty
different in the two places.

Comment 6

[Robert Sayre]

(In reply to comment #5)
> robert, did you make this patch from a 1.0 branch or from the mozilla trunk.
> That effects what I should do with this change since the parser is slighty
> different in the two places.

I made it against the trunk.

Comment 7

[Scott MacGregor]

Attachment: updated patch with some changes

You might want to take a look at the changes I made to your original patch.
While I was here I did a lot of general cleanup / code re-organization that was
unrelated to what you were trying to do. 

You can now create a Feed parser instance by doing something like the
following:

// create a feed parser which will parse the feed for us
var parser = new FeedParser();
this.itemsToStore = parser.parseFeed(this, this.request.responseText,
this.request.responseXML, this.request.channel.URI);

Comment 8

[Scott MacGregor]

I just checked my patch in.

Comment 9

[Scott MacGregor]

Attachment: re-organizing Feed.js to look like an object class

Comment 10

[Scott MacGregor]

Attachment: re-organize FeedItem.js to look like an object class

Comment 11

[Robert Sayre]

(In reply to comment #7)

> 
> You can now create a Feed parser instance by doing something like the
> following...

OK, how would you like the unit tests structured? It would be great if you could point me at some part 
of the tree that's structured the way you'd prefer to do this. I saw that /js/ has a unit test approach, but 
I'm not sure that's appropriate.

Comment 12

[Scott MacGregor]

Attachment: fix regression in date field handling

Comment 13

[Robert Sayre]

Attachment: Change comment style on license text so XPCShell can parse

Did you mean to close this? The parsers don't have unit tests yet :).

Comment 14

[Robert Sayre]

Attachment: messageID accessor typo fixed

the trunk is giving every message an id of <undefined> right now.

Comment 15

[Robert Sayre]

Attachment: Make Atom links use xml:base, change commenting style

http://intertwingly.net/blog/index.atom uses relative references. This patch
makes that work.

Comment 16

[Scott MacGregor]

Comment on attachment 174311 [details] [diff] [review]
Make Atom links use xml:base, change commenting style

i checked this in

Comment 17

[Robert Sayre]

Attachment: Change comment style on utils.js

Change to '*' on the license block so xpcshell can load the file. sr bienvenu as mscott is on vacation.

Comment 18

[Robert Sayre]

Got a test harness set up, but XPCShell is acting strange with DOM classes. See bug319968.

Comment 19

[Robert Sayre]

Attachment: shell.js -- jsDriver.pl test environment setup

This file is setup code for use with jsDriver.pl, the JS unit testing script. It scans a directory full of XML files that lead with a comment like this:

<!--
Description: dc:date is the date of the entry
Expect:      feed.items[0].date = '2005-12-07T14:48:03-05:00'
-->

That way the test code can eval() the Expect: value to see if it's correct, and you don't actually have to write test js files for the parser.

Comment 20

[Robert Sayre]

related bug 320967: add tests for content:encoded in all feed formats.

Comment 21

[Robert Sayre]

related bug 324179: make sure we don't match elements at the wrong nesting level

Comment 22

[Robert Sayre]

the new toolkit feedparser has ~150 unit tests. We should move Thunderbird to that ASAP.