Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Arbitrary instances and enable corresponding roundtrip tests #1492

Merged
merged 2 commits into from May 6, 2021

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented May 6, 2021

This changes Arbitrary instances for types that had flaky JSON roundtrip tests. Here is the list of changes:

  • Milliseconds is a 64 bit integer which is converted to and from Double when going through JSON encoding. Its arbitrary instance now limits it to values that JSON can represent exactly (up to 2^53).
  • Asset, ResumableAsset, Event and ClientMismatch truncate the times they contain to millisecond precision. The Arbitrary instance now reflects that.
  • The "mute status" field of MemberUpdate never gets serialised (why?). Therefore, the arbitrary instance always sets it to Nothing.
  • OtherMemberUpdate cannot be Nothing.
  • TeamUpdateData cannot have all its fields set to Nothing.

Make sure the empty value cannot be generated.

Also re-enable roundtrip tests that were disabled because of this.
@pcapriotti pcapriotti marked this pull request as draft May 6, 2021 06:57
@pcapriotti pcapriotti changed the title Fix Arbitrary instance of TeamUpdateData Fix Arbitrary instances and enable corresponding roundtrip tests May 6, 2021
@pcapriotti pcapriotti force-pushed the pcapriotti/team-update-arbitrary branch from 843bc7e to b8fcca6 Compare May 6, 2021 07:24
@pcapriotti pcapriotti marked this pull request as ready for review May 6, 2021 07:24
@pcapriotti pcapriotti mentioned this pull request May 6, 2021
2 tasks
Copy link
Member

@akshaymankar akshaymankar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Can you please follow up with the clients about the mute status? Looks like a bug.

@pcapriotti pcapriotti merged commit 8021b7b into develop May 6, 2021
@pcapriotti pcapriotti deleted the pcapriotti/team-update-arbitrary branch May 6, 2021 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants