Closed Bug 1122374 Opened 8 years ago Closed 8 years ago

[music] Improve testability of the metadata parser


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

Windows 7
Not set


(Not tracked)



(Reporter: squib, Assigned: squib)




(2 files)

46 bytes, text/x-github-pull-request
: review+
Details | Review
46 bytes, text/x-github-pull-request
: review+
Details | Review
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 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.
Attachment #8550126 - Flags: review?(dflanagan)
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.
Attachment #8550126 - Flags: review?(dflanagan) → review+

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.)
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Attached file Followup fix
I forgot to make one change. Here it is. r+ via IRC.
Attachment #8555459 - Flags: review+
You need to log in before you can comment on or make changes to this bug.