1122374 - [music] Improve testability of the metadata parser

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Music, defect)

Description

[Jim Porter (:squib)]

I've got a patch in progress that breaks up the metadata parser into smaller chunks to make it easier to test each piece. Some spoilers:

* Each metadata format can be tested completely independently (no relying on metadata_scripts.js anymore)
* The default metadata values (filename as title, rating/playcount) are now done in a post-processing step, which simplifies parsing Vorbis comments

Comment 1

[Jim Porter (:squib)]

Attachment: Does what it says on the tin

Comment 2

[Jim Porter (:squib)]

Comment on attachment 8550126 [details] [review]
Does what it says on the tin

This patch looks big, but it's not really that large (although there are a bunch of new tests). Most of the diff is because I moved some files around, but the contents are unchanged, aside from a couple minor renamings.

The main change here is that we fill in the default metadata values (e.g. the filename as the song title) *after* parsing the file, not before. This removes the hack I wrote for bug 877474, simplifies the parsers' API, and also makes it easier to test them in isolation.

I've also added a bunch of new tests, including tests for storing album art on deviceStorage, metadata format detection, and fixed some bugs in the existing tests, like how some failures could cause test timeouts.

In working on this, I also noticed some actual bugs (e.g. I'm pretty sure ForwardLocked files can't currently have album art), so I'll file those shortly.

Comment 3

[David Flanagan [:djf]]

Comment on attachment 8550126 [details] [review]
Does what it says on the tin

Thanks for explaining that most of this patch is just reorganizing existing code. I took a quick look and didn't try testing anything.  See my question on github, though: I think you either need a comment in your code or you need a return statement.

Filling in the missing metadata at the end of the parsing process makes a lot of sense. Breaking the album art processing out as a completely separate step after parsing also makes sense. And it is nice to have a clean AlbumArtCache class.

Comment 4

[Jim Porter (:squib)]

Landed: https://github.com/mozilla-b2g/gaia/commit/3982146c6fc66e38bf404674da8b796920aee355

There's a suspicious test failure in the playlist tests for music, but that happens for me on master too, so I don't think that's my fault. (It looks like the tests are timing out while launching the music app, so it's probably a marionette issue.)

Comment 5

[Jim Porter (:squib)]

Attachment: Followup fix

I forgot to make one change. Here it is. r+ via IRC.