Adding tests, tweaking built_value code. #5
Conversation
|
Please review, @filiph! |
| @@ -23,14 +23,19 @@ abstract class BuiltComplexObject | |||
| @nullable | |||
| double get aDouble; | |||
|
|
|||
| @nullable | |||
There was a problem hiding this comment.
This makes me sad, but I know you're doing it for a reason.
There was a problem hiding this comment.
Yeah, I did the other two first, so it's been changed to match. I'm annoyed that I didn't catch it earlier, but that's why one writes tests, I guess. 😄
| import 'package:jsonexample/json_serializable/serializable_simple_object.dart'; | ||
|
|
||
| void main() { | ||
| const Map<String, dynamic> typicalObjectJson = { |
There was a problem hiding this comment.
[nit] I think the Dart style, if you want to type-annotate, is:
const typicalObjectJson = <String,dynamic>{
// ...
}
And in this particular case, you don't even need to do that, since it's going to be inferred.
Types on the left are encourages for top-level members, but local variables tend to rely on inferred types.
https://www.dartlang.org/guides/language/effective-dart/design#prefer-type-annotating-public-fields-and-top-level-variables-if-the-type-isnt-obvious
If the second parameter wasn't dynamic, you could just leave generics altogether. But because of this rule, you want to use it.
|
|
||
| test('Empty object', () { | ||
| final simpleObject = | ||
| SerializableSimpleObject.fromJson(<String, dynamic>{}); |
There was a problem hiding this comment.
[nit] Why not have this empty map among the constants as well?
There was a problem hiding this comment.
Originally it was just the brackets {}, so it felt silly. Makes more sense now, though, so fixed.
|
|
||
| test('Extra properties', () { | ||
| final simpleObject = | ||
| ConvertedSimpleObject.fromJson(unexpectedPropertyjson); |
There was a problem hiding this comment.
Not sure what you're testing with the unexpectedPropertyjson. Are you testing that it returnsNormally? In that case, the test should say "Extra properties are ignored".
Is there a way to force the deserialization to throw when the object has extra properties? In that case, we'd want to test that.
There was a problem hiding this comment.
I'm just testing that an extra, unexpected property is ignored and doesn't break anything. Updated the test description to better state this.
| 'aListOfDoubles': [] | ||
| }; | ||
|
|
||
| const Map<String, dynamic> unexpectedPropertyjson = { |
| }; | ||
|
|
||
| group('ConvertedSimpleObject unit tests', () { | ||
| test('Typical object', () { |
There was a problem hiding this comment.
[nit] The concatenated message should ideally read like a sentence. For example: "ConvertedSimpleObject deserializes a typical object".
group('ConvertedSimpleObject', () {
test('deserializes a typical object', () {
There was a problem hiding this comment.
Rewrote all of 'em.
| test('Typical object', () { | ||
| final simpleObject = ConvertedSimpleObject.fromJson(typicalObjectJson); | ||
|
|
||
| expect(simpleObject, isNotNull); |
There was a problem hiding this comment.
[nit] Purists would say that there should be one expect statement per unit test. I'm not a purist.
I do think these tests may be unnecessarily thorough. Especially below, where you're using json_serializable. Those tests look like something I could find in the json_serializable package. Here we could just make sure that the things don't throw, or that one of the members is what we expect it to be.
Definitely a nit, though.
There was a problem hiding this comment.
You're right, but I'm not sure what other tests would go in here beyond the type of thing I have. I do like that it effectively compares all three approaches to ensure that they all pass the same tests, which is nice.
|
|
||
| void main() { | ||
| group('SimpleObjectView widget test', () { | ||
| testWidgets('Typical object', (WidgetTester tester) async { |
There was a problem hiding this comment.
dtto about test descriptions reading like sentences:
"SimpleObjectView shows JSON contents" or something like that
| expect(find.text('[1, 2, 3]'), findsOneWidget); | ||
| expect(find.text('[1.0, 2.0, 3.0]'), findsOneWidget); | ||
|
|
||
| for (int i = 1; i <= 4; i++) { |
There was a problem hiding this comment.
[nit] I think that logic like this in unit tests is generally frowned upon. No big deal.
There was a problem hiding this comment.
I was torn about it myself. I don't like having loops in the tests, but these get enormous otherwise. Leaving as-is for now, but I'll more than likely come back and clean this up.
No description provided.