Add tests for sorting behaviour and make album year optional #152
Conversation
Abestanis
force-pushed
the
feature/sorting_tests
branch
from
6beb6bd to
98bb949
Compare
|
Nice, that increase in test coverage is what I was hoping to see. 😄 |
| /// Returns sort feature values for a given content. | ||
| static List<SortFeature<T>> getValuesForContent<T extends Content>(ContentType<T> contentType) { | ||
| // TODO: Remove ContentType cast, see https://github.com/dart-lang/language/issues/2315 |
There was a problem hiding this comment.
Because dart-lang/language#2315 has gotten no traction in the last two years, and since with the new Flutter update there is no longer a lint against this, I think it's acceptable to leave the cast. The original intent of the todo was to be able to get rid of the lint ignore below.
lib/logic/models/sort.dart
Outdated
|
|
||
| /// Interface for other sort feature enums. | ||
| abstract class SortFeature<T extends Content> { |
There was a problem hiding this comment.
nit: can also make it abstract interface class
There was a problem hiding this comment.
Ah, the CI reminded me why I didn't do it (yet). The dart version we use on master doesn't support that combination yet. We'll have to wait for #130 to be merged.
There was a problem hiding this comment.
And then a moment later I see that you approved that PR, so we can use abstract interface class. Sorry for the noise.
lib/logic/models/sort.dart
Outdated
| String get id => this.name; | ||
|
|
||
| @override | ||
| final SortOrder defaultSortingOrder; | ||
| } | ||
|
|
||
| abstract class Sort<T extends Content> extends Equatable { | ||
| const Sort({ | ||
| required this.feature, | ||
| required this.orderAscending, |
There was a problem hiding this comment.
Yeah, I guess it's clearer with the enum.
There was a problem hiding this comment.
Done, I had to add a fallback when deserializing the sort configuration from a map to not break any existing installations.
| SongSort.defaultOrder(feature) : super.defaultOrder(feature); | ||
|
|
||
| factory SongSort.fromMap(Map map) => SongSort( | ||
| feature: EnumToString.fromString( |
There was a problem hiding this comment.
should probably refactor all such places and delete this dependency
There was a problem hiding this comment.
Yes, I have a follow-up PR in the works that deals with the null-sorting issue where I get rid of it.
test/logic/models/sort_test.dart
Outdated
| expect( | ||
| songs.toList() | ||
| ..sort(SongSort(feature: feature, orderAscending: sortOrder == SortOrder.ascending).comparator), | ||
| expectedContentList.map((i) => songs[i]).toList(), |
There was a problem hiding this comment.
Those test cases would be much more readable, if instead of IDs we would use the objects directly
You could so see the initial list vs expected list easily
Right now it's very hard to understand them
There was a problem hiding this comment.
Let's say one of those tests start to fail
I imagine determining what went wrong will be a real pain, but with expect(list, expectedList) it will show you the diff right in the error message
There was a problem hiding this comment.
I'm not sure I understand what you mean. Here we are expecting on List<Song>, not List<int>, so a failure looks like this:
Expected: [
Song:Song(16),
Song:Song(14),
Song:Song(13),
Song:Song(15),
Song:Song(8),
Song:Song(6),
Song:Song(5),
Song:Song(7),
Song:Song(0),
Song:Song(1),
Song:Song(2),
Song:Song(3),
Song:Song(4),
Song:Song(9),
Song:Song(10),
Song:Song(12),
Song:Song(11)
]
Actual: [
Song:Song(16),
Song:Song(14),
Song:Song(13),
Song:Song(15),
Song:Song(8),
Song:Song(6),
Song:Song(5),
Song:Song(7),
Song:Song(0),
Song:Song(1),
Song:Song(2),
Song:Song(3),
Song:Song(4),
Song:Song(9),
Song:Song(10),
Song:Song(11),
Song:Song(12)
]
Which: at location [15] is Song:<Song(11)> instead of Song:<Song(12)>
Maybe you are saying that testcases should be a Map<SortOrder, Map<SongSortFeature, List<Song>>>, but I'm not really convinced that either creating a new song with songWith to match one we created in the setUp or referencing the song created in setUp will be any more readable.
There was a problem hiding this comment.
About the error message - yeah, it looks good. I thought it would look different.
Maybe you are saying that testcases should be a Map<SortOrder, Map<SongSortFeature, List>>, but I'm not really convinced that either creating a new song with songWith to match one we created in the setUp or referencing the song created in setUp will be any more readable.
Yes, that's what I meant. I would like for me to be able to humanly read those test cases and verify that the expectations have the songs actually in the order that I would expect (by seeing the song parameters)
Thinking about it more, maybe we should even compare song.toMaps, because now it only compares songs by IDs - it would also make the error message more detailed, and disallow any discrepancies that might happen if we implement what I suggested above
What do you think?
There was a problem hiding this comment.
I think you are right. My fear was that by creating these huge lists of song constructors it would be just overwhelming and less readable, and while I still think it's a huge wall of text, I agree that it helps to understand how we are actually sorting.
Example of a album test failure
Expected: [
{
'id': 10,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': -1,
'numberOfSongs': 0
},
{
'id': 13,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': -1,
'lastYear': null,
'numberOfSongs': 0
},
{
'id': 0,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 5,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': 'A',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 6,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': 'AA',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 7,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': 'a',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 8,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': 'z',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 11,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': null,
'numberOfSongs': 0
},
{
'id': 14,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': null,
'lastYear': null,
'numberOfSongs': 0
},
{
'id': 15,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 1
},
{
'id': 16,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': -1
},
{
'id': 9,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 1,
'numberOfSongs': 0
},
{
'id': 12,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 1,
'lastYear': null,
'numberOfSongs': 0
},
{
'id': 1,
'album': 'A',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 3,
'album': 'a',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 4,
'album': 'z',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 2,
'album': 'AA',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
}
]
Actual: [
{
'id': 10,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': -1,
'numberOfSongs': 0
},
{
'id': 13,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': -1,
'lastYear': null,
'numberOfSongs': 0
},
{
'id': 0,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 5,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': 'A',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 6,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': 'AA',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 7,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': 'a',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 8,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': 'z',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 11,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': null,
'numberOfSongs': 0
},
{
'id': 14,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': null,
'lastYear': null,
'numberOfSongs': 0
},
{
'id': 15,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 1
},
{
'id': 16,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': -1
},
{
'id': 9,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 1,
'numberOfSongs': 0
},
{
'id': 12,
'album': '',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 1,
'lastYear': null,
'numberOfSongs': 0
},
{
'id': 1,
'album': 'A',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 3,
'album': 'a',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 2,
'album': 'AA',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
},
{
'id': 4,
'album': 'z',
'albumArt': 'albumArt_albumArt_albumArt',
'artist': '',
'artistId': 0,
'firstYear': 0,
'lastYear': 0,
'numberOfSongs': 0
}
]
Which: at location [15]['id'] is <2> instead of <4>
There was a problem hiding this comment.
Thanks, that is now so much better
Abestanis
force-pushed
the
feature/sorting_tests
branch
from
ddeb4df to
98bb949
Compare
Abestanis
force-pushed
the
feature/sorting_tests
branch
from
98bb949 to
94cad8f
Compare
| descending; | ||
|
|
||
| /// The inversion of the sort order. | ||
| SortOrder get inverted => switch (this) { |
There was a problem hiding this comment.
Wow Dart 3 really shines with this, very neat
|
Also created #158 |
This is part of my quest to make the app more robust by accepting more
nullvalues in the content (aka the third point of #127). If we want to make certain content fields nullable we will have to change how sorting works, so I wanted to add some tests for the existing sorting behaviour.When adding the tests I noticed that the album
yeargetter falls back to the year of the current system time. This does not make sense to me, because that means that sorting depends on the current year (or if the device is not syncing time online, whatever year it thinks it is).I also changed the
*SortFeatures into regular enums. Whatever prevented them from being regular enums in the past seems to be part of the dart language now, so it seems to be better to use actual enums.