Add JSON serialization test for User model (#109) #183
Conversation
|
@DanielaSfregola - on the ticket, you suggested doing the reverse: starting from JSON, then deserializing to a |
Codecov Report
@@ Coverage Diff @@
## master #183 +/- ##
=======================================
Coverage 95.63% 95.63%
=======================================
Files 70 70
Lines 1122 1122
Branches 16 16
=======================================
Hits 1073 1073
Misses 49 49Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hi @catalin-ursachi,
thanks for your contribution!
Unfortunately, this is not what we are looking for.
It happened in the past that we were thinking we were deserializing some data but we weren't because of some typos or some errors in the case class structure (see for example #96 )
You are right when you say that the test I suggested is not particularly maintainable because it is dependent on the json generated from twitter...but unfortunately this will also be the case as we are indeed dependent on twitter! :D In other words, if one day they go crazy (they won't) and change the json they return completely, we will not be able to function anymore.
Note: in the repo we already have lots of json files generated from twitter, you can just reuse one of those by cutting out the parts you are not interested in serialised/deserialised.
Cheers,
D.
There was a problem hiding this comment.
Overall awesome work! Thanks @catalin-ursachi for your contribution. We are almost ready to merge!
I just have one question on why a missing field in user, called translator_type, has not been detected from the test. I would expect the test to detect fields that we forgot to support...?
| * treat equivalent dates in different timezones as equal, and empty arrays | ||
| * as equal to nulls. | ||
| */ | ||
| trait JsonDiffSupport { this: JsonSupport => |
There was a problem hiding this comment.
Are there any licence issues with copying this over from org.json4s.Diff, or is crediting it in the class comment sufficient?
There was a problem hiding this comment.
Hi @catalin-ursachi, they use Apache 2.0 like us...so give them credit should be enough!
| implicit val arbitraryDate: Arbitrary[Date] = Arbitrary { | ||
| for { | ||
| timeInMicroseconds: Long <- Gen.chooseNum(1142899200L, 1512442349L) | ||
| } yield { |
There was a problem hiding this comment.
[Super minor] if yield is followed by a singular expression, you do not usually put parentheses.
yield new Date(timeInMicroseconds * 100)
| for { | ||
| prefix: String <- Gen.nonEmptyListOf(alphaChar).map(_.mkString) | ||
| suffix: String <- Gen.oneOf("_mini", "_normal", "_bigger", "") | ||
| } yield { |
There was a problem hiding this comment.
[Super minor] parentheses should be removed, same as above
| } | ||
| } | ||
|
|
||
| roundtripTest[User]("/twitter/rest/users/user.json") |
There was a problem hiding this comment.
[Future improvements not related to this PR] maybe we can use more json files to enrich we analyze enough json shapes
| @@ -94,5 +95,6 @@ | |||
| "default_profile_image": false, | |||
| "following": false, | |||
| "follow_request_sent": false, | |||
| "notifications": false | |||
| "notifications": false, | |||
| "translator_type": "regular" | |||
There was a problem hiding this comment.
I've checked and we do not seem to support the field translator_type in user...we should! I would expect the test to detect missing supported fields...what am I missing?
There was a problem hiding this comment.
I ran out of time last evening to add the profile_location and translator_type fields (that are present in this one: https://developer.twitter.com/en/docs/accounts-and-users/follow-search-get-users/api-reference/get-users-show). Added now. 🙂
There was a problem hiding this comment.
Hi @catalin-ursachi,
thanks for your awesome contribution! And I am glad you found a few fields we weren't supporting ;)
LGTM
| @@ -46,6 +47,7 @@ final case class User(blocked_by: Boolean = false, | |||
| status: Option[Tweet] = None, | |||
| statuses_count: Int, | |||
| time_zone: Option[String] = None, | |||
| translator_type: Option[String] = None, | |||
There was a problem hiding this comment.
I searched in the tweeter doc if maybe they provide a list of possible values for translator type, but couldn't find any. So I think using String rather than a dedicated enough is the right call
| @@ -36,6 +36,7 @@ final case class User(blocked_by: Boolean = false, | |||
| profile_image_url: ProfileImage, | |||
| profile_image_url_https: ProfileImage, | |||
| profile_link_color: String, | |||
| profile_location: Option[String], | |||
There was a problem hiding this comment.
Couldn't find any non-null representation of this either....so I guess string will do for now!
Adds one test covered by #109.