search

dart-lang / test

A library for writing unit tests in Dart.
https://pub.dev/packages/test
494 stars 213 forks source link

deepEquals on Map should give details on the value that didn't match #2256

Open gnprice opened 1 month ago

gnprice commented 1 month ago

The deepEquals method is extremely handy for comparing some actual data to an expected list (or iterable), because when some item doesn't match it will drill in to pinpoint specifically why it didn't match, recursively.

Currently it's a lot less helpful when the expected data is a map, though. If some detail deep within a big complicated value didn't match, it'll only print the whole expected value at that key, and say that something somewhere in there didn't match. (In fact it won't even point out the actual value at that key; so if the actual map as a whole is big, the user has to read through that to pick out the relevant key and its value.)

A real-world example is here: https://github.com/zulip/zulip-flutter/pull/762#discussion_r1678510731

For an isolated demo of the issue: take some blob of JSON, corrupt it, and use deepEquals to compare the decodings of the corrupted and original versions. Taking one such blob I had handy:

  test('demo', () {
    final json = r'{"result":"success","msg":"","authentication_methods":{"password":true,"dev":false,"email":true,"ldap":false,"remoteuser":false,"github":true,"azuread":false,"gitlab":true,"google":true,"apple":true,"saml":false,"openid connect":false},"zulip_version":"9.0-dev-3981-ga5f034fe46","zulip_merge_base":"9.0-dev-3975-gbdd39f4c67","zulip_feature_level":273,"push_notifications_enabled":true,"is_incompatible":false,"email_auth_enabled":true,"require_email_format_usernames":true,"realm_url":"https://chat.zulip.org","realm_name":"Zulip Community","realm_icon":"/user_avatars/2/realm/icon.png?version=3","realm_description":"<p>Welcome to the Zulip development and user community!  </p>\n<p>Join to get a quick Zulip demo, observe a Zulip community, offer feedback to the Zulip core team, or get involved as a contributor.  </p>\n<ul>\n<li><a href=\"https://zulip.com/development-community/#community-norms\">Community conventions</a></li>\n<li><a href=\"https://zulip.readthedocs.io/en/latest/code-of-conduct.html\">Code of Conduct</a></li>\n</ul>\n<p>Note that this server runs a bleeding-edge version of Zulip, so you may encounter bugs.  Please report them!</p>","realm_web_public_access_enabled":true,"external_authentication_methods":[{"name":"google","display_name":"Google","display_icon":"https://chat.zulip.org/static/images/authentication_backends/googl_e-icon.png","login_url":"/accounts/login/social/google","signup_url":"/accounts/register/social/google"},{"name":"github","display_name":"GitHub","display_icon":"https://chat.zulip.org/static/images/authentication_backends/github-icon.png","login_url":"/accounts/login/social/github","signup_url":"/accounts/register/social/github"},{"name":"gitlab","display_name":"GitLab","display_icon":"https://chat.zulip.org/static/images/authentication_backends/gitlab-icon.png","login_url":"/accounts/login/social/gitlab","signup_url":"/accounts/register/social/gitlab"},{"name":"apple","display_name":"Apple","display_icon":"https://chat.zulip.org/static/images/authentication_backends/apple-icon.png","login_url":"/accounts/login/social/apple","signup_url":"/accounts/register/social/apple"}],"realm_uri":"https://chat.zulip.org"}';
    final badJson = json.replaceFirst('GitHub', 'Github');
    check(jsonDecode(badJson) as Map)
      .deepEquals(jsonDecode(json) as Map);
  });

the error output is long (full version below), and the most specific part is:

  Which: has no entry to match 'external_authentication_methods': [{'name': 'google',
  'display_name': 'Google',
  'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/googl_e-icon.png',
  'login_url': '/accounts/login/social/google',
  'signup_url': '/accounts/register/social/google'},
  {'name': 'github',
  'display_name': 'GitHub',
  'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/github-icon.png',
  'login_url': '/accounts/login/social/github',
  'signup_url': '/accounts/register/social/github'},
  {'name': 'gitlab',
  'display_name': 'GitLab',
  'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/gitlab-icon.png',
  'login_url': '/accounts/login/social/gitlab',
  'signup_url': '/accounts/register/social/gitlab'},
  {'name': 'apple',
  'display_name': 'Apple',
  'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/apple-icon.png',
  'login_url': '/accounts/login/social/apple',
  'signup_url': '/accounts/register/social/apple'}]

Ideally the end of the output would look more like:

  Which: at ['external_authentication_methods'][<1>]['display_name'] is 'Github'
    which does not equal 'GitHub'
Complete demo output ``` 00:01 +0 -1: demo [E] Expected: a Map that: is deeply equal to {'result': 'success', 'msg': '', 'authentication_methods': {'password': true, 'dev': false, 'email': true, 'ldap': false, 'remoteuser': false, 'github': true, 'azuread': false, 'gitlab': true, 'google': true, 'apple': true, 'saml': false, 'openid connect': false}, 'zulip_version': '9.0-dev-3981-ga5f034fe46', 'zulip_merge_base': '9.0-dev-3975-gbdd39f4c67', 'zulip_feature_level': 273, 'push_notifications_enabled': true, 'is_incompatible': false, 'email_auth_enabled': true, 'require_email_format_usernames': true, 'realm_url': 'https://chat.zulip.org', 'realm_name': 'Zulip Community', 'realm_icon': '/user_avatars/2/realm/icon.png?version=3', 'realm_description': '

Welcome to the Zulip development and user community!

Join to get a quick Zulip demo, observe a Zulip community, offer feedback to the Zulip core team, or get involved as a contributor.

Note that this server runs a bleeding-edge version of Zulip, so you may encounter bugs. Please report them!

', 'realm_web_public_access_enabled': true, 'external_authentication_methods': [{'name': 'google', 'display_name': 'Google', 'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/googl_e-icon.png', 'login_url': '/accounts/login/social/google', 'signup_url': '/accounts/register/social/google'}, {'name': 'github', 'display_name': 'GitHub', 'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/github-icon.png', 'login_url': '/accounts/login/social/github', 'signup_url': '/accounts/register/social/github'}, {'name': 'gitlab', 'display_name': 'GitLab', 'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/gitlab-icon.png', 'login_url': '/accounts/login/social/gitlab', 'signup_url': '/accounts/register/social/gitlab'}, {'name': 'apple', 'display_name': 'Apple', 'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/apple-icon.png', 'login_url': '/accounts/login/social/apple', 'signup_url': '/accounts/register/social/apple'}], 'realm_uri': 'https://chat.zulip.org'} Actual: {'result': 'success', 'msg': '', 'authentication_methods': {'password': true, 'dev': false, 'email': true, 'ldap': false, 'remoteuser': false, 'github': true, 'azuread': false, 'gitlab': true, 'google': true, 'apple': true, 'saml': false, 'openid connect': false}, 'zulip_version': '9.0-dev-3981-ga5f034fe46', 'zulip_merge_base': '9.0-dev-3975-gbdd39f4c67', 'zulip_feature_level': 273, 'push_notifications_enabled': true, 'is_incompatible': false, 'email_auth_enabled': true, 'require_email_format_usernames': true, 'realm_url': 'https://chat.zulip.org', 'realm_name': 'Zulip Community', 'realm_icon': '/user_avatars/2/realm/icon.png?version=3', 'realm_description': '

Welcome to the Zulip development and user community!

Join to get a quick Zulip demo, observe a Zulip community, offer feedback to the Zulip core team, or get involved as a contributor.

Note that this server runs a bleeding-edge version of Zulip, so you may encounter bugs. Please report them!

', 'realm_web_public_access_enabled': true, 'external_authentication_methods': [{'name': 'google', 'display_name': 'Google', 'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/googl_e-icon.png', 'login_url': '/accounts/login/social/google', 'signup_url': '/accounts/register/social/google'}, {'name': 'github', 'display_name': 'Github', 'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/github-icon.png', 'login_url': '/accounts/login/social/github', 'signup_url': '/accounts/register/social/github'}, {'name': 'gitlab', 'display_name': 'GitLab', 'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/gitlab-icon.png', 'login_url': '/accounts/login/social/gitlab', 'signup_url': '/accounts/register/social/gitlab'}, {'name': 'apple', 'display_name': 'Apple', 'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/apple-icon.png', 'login_url': '/accounts/login/social/apple', 'signup_url': '/accounts/register/social/apple'}], 'realm_uri': 'https://chat.zulip.org'} Which: has no entry to match 'external_authentication_methods': [{'name': 'google', 'display_name': 'Google', 'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/googl_e-icon.png', 'login_url': '/accounts/login/social/google', 'signup_url': '/accounts/register/social/google'}, {'name': 'github', 'display_name': 'GitHub', 'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/github-icon.png', 'login_url': '/accounts/login/social/github', 'signup_url': '/accounts/register/social/github'}, {'name': 'gitlab', 'display_name': 'GitLab', 'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/gitlab-icon.png', 'login_url': '/accounts/login/social/gitlab', 'signup_url': '/accounts/register/social/gitlab'}, {'name': 'apple', 'display_name': 'Apple', 'display_icon': 'https://chat.zulip.org/static/images/authentication_backends/apple-icon.png', 'login_url': '/accounts/login/social/apple', 'signup_url': '/accounts/register/social/apple'}] package:checks/src/checks.dart 85:9 check. package:checks/src/checks.dart 708:12 _TestContext.expect package:checks/src/extensions/map.dart 103:12 MapChecks.deepEquals … ```
gnprice commented 1 month ago

I think the core of what's happening is that when comparing maps, deepEquals uses unorderedCompare to do the work, comparing the two Map.entries iterables.

In general it'd fundamentally be complex for unorderedCompare to do better than this, because if some expected element doesn't match any actual element, it's not clear which actual element in particular it was intended to match. Doing better would require a fancier diff algorithm, like some sort of minimal-edit-distance computation rather than comparing elements only for equality.

But a map has extra structure which should enable deepEquals to drill down to the specific mismatch in much the same way it already does for iterables. If some expected entry doesn't match any actual entry, then either:

It does get complicated (as I realize now, thinking through this further) if the expected key is a Condition or Iterable or Map, rather than a value meant to be compared for equality.

I expect it's pretty uncommon to want a Condition callback for the key of an expected map, though, and similarly to have an Iterable or Map for a map key. After all, the normal semantics of Map are all about == comparisons on the keys. So it should be fine if the nicer error output only appears when the keys are all values meant to be compared for equality, with a fallback to the current behavior if some keys are Condition callbacks or Iterable or Map.

natebosch commented 1 month ago

It does get complicated (as I realize now, thinking through this further) if the expected key is a Condition or Iterable or Map, rather than a value meant to be compared for equality.

Yes, it's possible to write a check where multiple keys can match. I agree that this is worth specializing for the sake of more specific error messages.

natebosch commented 1 month ago

@gnprice - Can you try out the map-paths branch and let me know how it works for you?

I'm a little nervous about how the behavior of some value key could change when you add a different non-value key into the expectation. I looked a little bit at unifying the behavior and choosing key by key which way to test it, but I haven't found a satisfying approach yet.

gnprice commented 1 month ago

I just tried it out, and it works great for the use cases I had. Thanks for the swift implementation!

I'm a little nervous about how the behavior of some value key could change when you add a different non-value key into the expectation.

Yeah, that's reasonable. If I were maintaining this library, I think the key fact I'd try to convince myself of for reassurance is that if you add a non-value key into the expectation, and it matches either zero or one of the actual keys, then the check will pass (or fail) just if it would have passed (or failed) with a value key that matched the same zero or one of the actual keys.

As long as that's true, then it seems like this really is just a failure-message improvement, as intended, and can't cause subtle breakage. After all, if a non-value key matches multiple actual keys, then the test behavior may be counterintuitive but that's fundamental to what the test is choosing to do.

And though I haven't studied this closely enough to feel certain of it, I think that fact is true… as long as neither the actual nor expected maps do anything that breaks the Map contract, anyway, like having containsKey and [] and entries behave inconsistently from each other.

Which seems fine. If someone's working with collections that subvert the collection contracts, there's ultimately nothing that a general-purpose test-assertions framework can do to clear that up. It's really up to the user in that case to figure out what assumptions they can still rely on to make sense of those collections, and then to write the assertions they need given that.

gnprice commented 1 month ago

There's one kind of collection I know of in the SDK that subverts the collection contracts to some degree: SplayTreeMap. Because it uses compareTo instead of ==, it can give surprising results on types where those don't agree, like num.

So here's a fun pair of demos:

  test('odd map: match, compared as maps', () {
    final m = SplayTreeMap<num, String>()
      ..[0.0] = 'zero'
      ..[-0.0] = 'zero'
      ..[1] = 'one';
    check(m).deepEquals({0.0: 'zero', 1.0: 'one'});
  });

  test('odd map: mismatch, compared as sets of entries', () {
    final m = SplayTreeMap<num, String>()
      ..[0.0] = 'zero'
      ..[-0.0] = 'zero'
      ..[1] = 'one';
    check(m).deepEquals({0.0: 'zero', // FAIL
      ((Subject<Object?> it) => it.equals(1.0)): 'one'});
  });

With your PR branch the first test passes, and the second fails. Both of them compare the same actual Map (a SplayTreeMap) against nearly the same Map, except that in the second test one of the keys is a condition (it) => it.equals(1.0) instead of just the value 1.0; this matters only in that it causes the "ambiguous" comparison to be used instead of the "unambiguous", and the former finds a mismatch while the latter finds a match.

I think this behavior is fine, though, just as I said above. The root of the confusion here is that there's a Map where keys has two values that are equal according to ==. Both "it matches" and "it doesn't match" are equally valid answers when compared against those expected maps, because the actual map is inconsistent when queried through different parts of the Map interface.