Tests for lastgenre (cleaned up)

[sampsyo]

This new PR supersedes #717. It contains only the refactoring and new tests. I also restored the min_weight option, which was lost in one of the refactoring commits.

Does this look mostly good, @Kraymer? Looks like we have one failing test, which I'm guessing was the whole goal of this exercise in the first place. 😃

[sampsyo]

I think the build failed above because of teardown issues with the config. Each test just adds to the configuration in setUp rather than constructing a new plugin class. setUp should probably construct LastGenrePlugin and we should use _common.TestCase (or equivalent) to clear out the configuration before every test.

[Kraymer]

Looks like we have one failing test, which I'm guessing was the whole goal of this exercise in the first place

We don't have a test that reveals the incompatibility between c14n and lastfm weighings (which was the original issue met by @marlemion), because the tests are too high level : when we reach resolve_genres (towards which the tests are aimed), weighings filtering is supposed to already been done by _tags_for.
(that's a weakness of my tests that have been written without considerations to lastfm weighings 😬 )

I added the test_c14n test (the one that fails) to evaluate config with c14n activated (default c14n tree). I think we could expect that in such a case, genres are funneled to a restricted set of popular genres because that's what c14n is about.
To make the test pass the default whitelist must be restricted as in #717.

[Kraymer]

Ok I realized that a restricted whitelist brings an obvious problem too, hence the commit above ^^
Who would have thought de-intermixing these options would be so tedious ? 😣

---

Sum-up of conclusions I've reached so far :

1. weighings don't mix well with c14n because genres retrieved by c14n don't come from lastfm api and so have no weighings attached
2. if kept, weighings filtering should be done after c14n as the principle of c14n is exactly to transform unpopular genres into popular ones
3. with default config files (genres.txt, genres-tree.yaml) + whitelist off + c14n off => no genre should be filtered out
4. with default config files + whitelist on + c14n on => genres should be funneled to a restricted set
5. setting whitelist off and c14n on should funnel each genre to root genres (the roots of the c14n tree branches)

And regarding these facts, the problem I see with the plugin is that there is no way to turn the whitelist off (as opposed to c14n option), which makes it hard to handle the main use cases nicely.

So let's imagine now that whitelist can be turned off and let's enumerate all the combinations :

• (default) whitelist:off, c14n:off => genres are written as returned by lastfm api
• whitelist:on, c14n:off => because user has explicitely set the whitelist on, I think it's acceptable now that it contains only a small subset of genres. It would be elegant (see below) that the whitelist contains only the roots of the c14n tree (as of now it's blues, country, electronic, folk, hip-hop & rap, latin, pop, r&b, rock, ska)
• whitelist:on|off, c14n:on => funnel genres returned by lastfm api into a small subset of genres. Setting whitelist on or off would have no incidence if default whitelist is created as stated above.
• whitelist:custom_path, c14n:custom_path => funnel genres returned by lastfm into the given subset of genres

tldr; sorry for the long post. Allowing whitelist to be off, enable us to fix issues 3, 4, and 5 listed at the top of this post, and make the current tests suite pass. Does that seem reasonable ?

[sampsyo]

Sure! Allowing the whitelist to be disabled (e.g., whitelist: null) seems like a fine idea.

Is there anything wrong with the very simple policy of applying canonicalization (if enabled) and then the whitelist (if enabled)? This would allow:

• The default behavior (whitelist only), where you just get good genres but maybe lots of very specific ones.
• A more paranoid behavior (canonicalization only), where you get more common genres.
• A custom and restricted behavior (canonicalization + whitelisting), where you can eliminate some of the genres in the default canonicalization tree that you don't like.

Also, I still think the weight filtering via min_weight should be done before any other processing (whitelisting or canonicalization). This is to prevent tags that are just plain wrong from being used, even if they name popular genres.

[sampsyo]

Great; I've now merged @Kraymer's additions.

@Kraymer, I also restored whitelisting by default. It seems like the default configuration should avoid tags like "seen live" and stuff that obviously isn't a genre. The tests (_setup_config) have whitelisting off by default, though, for readability.

The tests pass, I think, so maybe it's time to merge! We can of course add more later too.

[sampsyo]

Oops, spoke too soon. Looks like we're still having some teardown issues causing problems with other tests. Nearly there, though.

[Kraymer]

Now that the extensive whitelist is the default, what edit should be done to config.yaml if one wants to disable whitelist ?
We could pick whitelist: null as you noted before, but it is not symmetric with c14n option. Currently we have :

• whitelist : default:activated, null to deactivate
• c14n: default:deactivated, '' to activate

Why not replace null by false and '' by true to homogenize the two options?

[sampsyo]

Sure! The booleans seem good to me. No reason why we can't also keep the older, weirder None and '' aliases for compatibility with existing configurations.

[sampsyo]

I changed the options to use real YAML booleans (true, false, yes, no, etc.) as opposed to strings containing the letters "true" and "false". Let me know if this looks okay.

And the tests all pass now! Think this is ready to merge, @Kraymer?

[Kraymer]

Let me know if this looks okay

We lost a call to strip() when getting filenames, but i doubt it was useful in the first place?
Ok to merge.

[sampsyo]

Ah, good catch. Let's see if anyone complains. 😃 This was left over from the INI config file days, when whitespace was harder to control.

Thanks for all the work on this! Merging.