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

test: Write a generic jsonEquals check and deepToJson function #259

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Aug 9, 2023

As discussed at #256 (comment) .

@@ -23,21 +15,9 @@ extension ReactionsChecks on Subject<List<Reaction>> {
void deepEquals(_) {
throw UnimplementedError('Tried to call [Subject<List<Reaction>>.deepEquals]. Use jsonEquals instead.');
}
Copy link
Collaborator

@chrisbobbe chrisbobbe Aug 9, 2023

Choose a reason for hiding this comment

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

Is it intentional to leave in this ReactionsChecks on Subject<List<Reaction>>, with its deepEquals implementation that throws UnimplementedError?

The error still does apply, I think. It's jsonEquals, not deepEquals, that actually looks deeply enough to be useful in a test, given that Reaction doesn't have an == override. So the error is likely to be helpful when you encounter it.

But if the error is needed, then probably the need won't be unique to List<Reaction>s, right? Does it look odd to you to have an extension on something as specific as Subject<List<Reaction>>?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this would be equally appropriate on List of any type that we know doesn't have a == that you would want in a test, and that has a toJson that you'd want instead.

It does look a bit odd having it on this one type. But it was already written, and could be useful, so I didn't feel like deleting it. I probably wouldn't go around trying to systematically add these for other types that could equally use them.

@chrisbobbe
Copy link
Collaborator

Thanks! One small comment above, then please merge at will.

@gnprice
Copy link
Member Author

gnprice commented Aug 9, 2023

Thanks for the review! Replied above; merging.

@gnprice gnprice merged commit 3cba058 into zulip:main Aug 9, 2023
@gnprice gnprice deleted the pr-test-json branch August 9, 2023 23:35
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.

2 participants