Improve content scanning

[Abestanis]

This includes #145 and is based on top of #148.

This makes content scanning more robust and prevents failing the entire content search because one item fails by catching exceptions on an item level and reporting them, which implements the first checkbox on #127.

I think it is valuable to report these as non-fatal errors, so we can see if we are too strict and need to make a content attribute nullable.

[nt4f04uNd]

General comment: the solution is fine, but tests are hard to read. If we could make it a bit more simple, or more readable, that would be great

[nt4f04uNd]

If we add a new property, this will likely not be updated

[Abestanis]

The way I see it, that's not going to be a problem. The tests verify the current behaviour, any change will (and should) break them. If we add a nullable property, the subSets function below will generate albums maps where that property is null/missing, so the test will fail when we expect parsing of these to fail but they get accepted.

[nt4f04uNd]

What does it do?

[nt4f04uNd]

Generally it's hard to understand what is the output of these function calls, how they work, and why we use them

[Abestanis]

Generally it's hard to understand what is the output of these function calls, how they work, and why we use them

I agree.

The point of this is to automatically create all possible bad variations of inputs to the parse function.
There is two types of problems that this covers:

• Missing data: The subSets function gives us variations of the validSong map (without the values that are allowed to be null) with one or more missing elements. We generate all possible subsets instead of just checking if it's ok if individual elements are missing because sometimes we fall back to another element in that case (like Song.dateModified falling back to Song.dateAdded).
• Invalid data types: The withWrongTypes gives us variations of the validSong map where each element of the map is a different type from the expected one. For example, the title property has a type String, so withWrongTypes will generate one variation where the title is null, one where the title is true, one where it's 0 and so on. We only generate variations where one of the elements is changed and don't bother with variations where multiple element types have changed because there would be a large amount of variations and because I don't believe it is very relevant.

I agree that this is not very easy to parse from the code, but I'm not quite sure how to improve the situation other than adding comments and maybe changing the name of withWrongTypes and subSets. Do you have any recomendations?

[Abestanis]

I've added some helper extension methods to improve the clarity of the tests.

[Abestanis]

I think splitting the tests into the valid and invalid cases improves clarity a lot.

[nt4f04uNd]

What is the point of generating a model, then removing by hand all of its properties?

[Abestanis]

subSets generates all possible variations of the map where one or more elements are missing to generate a list of invalid maps that can't be parsed into a valid album. The copyWithout removes the elements of the map that are allowed to be missing and would therefore successfully parse.

The reason to not hard code the list of properties is that if we add a new property to Album it will be automatically included here.

[nt4f04uNd]

The copyWithout removes the elements of the map that are allowed to be missing and would therefore successfully parse.

But if it removes those fields from map, subSets gets the map with those removed fields. How would it know it needs to generate some variations of those non-existent fields, am I missing something here?

[Abestanis]

How would it know it needs to generate some variations of those non-existent fields, am I missing something here?

The idea is for it to not generate any variations with the removed fields. We want to generate invalid maps here, but since these field are nullable it's valid for them to be missing.