tests: Provide more helpful assert

ajtowns commented at 10:39 AM on February 29, 2024: contributor

In the functional tests, we often compare dicts with assert_equal, but the output makes it very hard to tell exactly which entry in the dicts don't match when there are a lot of entries and only minor differences. Change the output to make it clearer.

DrahtBot commented at 10:42 AM on February 29, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, brunoerg, josibake, BrandonOdiwuor, achow101
Concept ACK	Empact, ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26573 (Wallet: don't underestimate the fees when spending a Taproot output by darosior)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

tests: Provide more helpful assert_equal errors

In the functional tests, we often compare dicts with assert_equal, but the
output makes it very hard to tell exactly which entry in the dicts don't
match when there are a lot of entries and only minor differences. Change
the output to make it clearer.

a3badf75f6

DrahtBot added the label Tests on Feb 29, 2024

ajtowns force-pushed on Feb 29, 2024

ajtowns commented at 10:47 AM on February 29, 2024: contributor

As an example, if I add some bugs to rpc_blockchain.py:

--- a/test/functional/rpc_blockchain.py
+++ b/test/functional/rpc_blockchain.py
@@ -200,7 +200,7 @@ class BlockchainTest(BitcoinTestFramework):
           "deployments": {
             'bip34': {'type': 'buried', 'active': True, 'height': 2},
             'bip66': {'type': 'buried', 'active': True, 'height': 3},
-            'bip65': {'type': 'buried', 'active': True, 'height': 4},
+            'bip65': False,
             'csv': {'type': 'buried', 'active': True, 'height': 5},
             'segwit': {'type': 'buried', 'active': True, 'height': 6},
             'testdummy': {
@@ -221,6 +221,7 @@ class BlockchainTest(BitcoinTestFramework):
                         'possible': True,
                     },
                     'signalling': '#'*(height-143),
+                    'dummy': True,
                 },
                 'active': False
             },
@@ -231,11 +232,11 @@ class BlockchainTest(BitcoinTestFramework):
                     'timeout': 9223372036854775807,
                     'min_activation_height': 0,
                     'status': 'active',
-                    'status_next': 'active',
-                    'since': 0,
+                    'status_next': 'failed',
+                    'since': 7,
                 },
                 'height': 0,
-                'active': True
+                'active': False,
             }
           }
         })

Then with this patch, I get the additional error information:

in particular not({'deployments': {'bip65': {'type': 'buried', 'active': True, 'height': 4}, 'taproot': {'active': True, 'bip9': {'since': 0, 'status_next': 'active'}}, 'testdummy': {'bip9': {}}}} == {'deployments': {'bip65': False, 'taproot': {'active': False, 'bip9': {'since': 7, 'status_next': 'failed'}}, 'testdummy': {'bip9': {'dummy': True}}}})```

which only tells me about each error.

vasild approved

vasild commented at 10:53 AM on February 29, 2024: contributor

ACK 89b586b413a14590dd4b00c37286b47c802084d3

Tested this myself:

AssertionError: not({'id': 2, 'network': 'not_publicly_routable', 'services': '0000000000000000', 'servicesnames': [], 'relaytxes': False, 'lastsend': 1709203943, 'lastrecv': 1709203943, 'last_transaction': 0, 'last_block': 0, 'conntime': 1709203943, 'timeoffset': 0, 'version': 0, 'subver': '', 'inbound': True, 'bip152_hb_to': False, 'bip152_hb_from': False, 'startingheight': -1, 'presynced_headers': -1, 'synced_headers': -1, 'synced_blocks': -1, 'inflight': [], 'addr_relay_enabled': False, 'addr_processed': 0, 'addr_rate_limited': 0, 'permissions': [], 'minfeefilter': Decimal('0E-8'), 'bytessent_per_msg': {}, 'bytesrecv_per_msg': {}, 'connection_type': 'inbound', 'transport_protocol_type': 'v2', 'session_id': '04c4eba0893b099c923beb8b1a1e08a23f98fa6e825fcc0100bfe6dadd858272'} == {'addr_processed': 0, 'addr_rate_limited': 0, 'addr_relay_enabled': False, 'bip152_hb_from': False, 'bip152_hb_to': False, 'bytesrecv_per_msg': {}, 'bytessent_per_msg': {}, 'connection_type': 'inbound', 'conntime': 1709203943, 'id': 2, 'inbound': True, 'inflight': [], 'last_block': 0, 'last_transaction': 0, 'lastrecv': 1709203943, 'lastsend': 1709203943, 'minfeefilter': Decimal('0E-8'), 'network': 'not_publicly_routable', 'permissions': [], 'presynced_headers': -1, 'foo': 0, 'relaytxes': False, 'services': '0000000000000000', 'servicesnames': [], 'session_id': '04c4eba0893b099c923beb8b1a1e08a23f98fa6e825fcc0100bfe6dadd858272', 'startingheight': -1, 'subver': '', 'synced_blocks': -1, 'synced_headers': -1, 'timeoffset': 0, 'transport_protocol_type': 'v22', 'version': 0})
  in particular not({'transport_protocol_type': 'v2'} == {'transport_protocol_type': 'v22', 'foo': 0})

Thanks!

vasild approved

vasild commented at 10:56 AM on February 29, 2024: contributor

ACK a3badf75f6fd88d465e59f46f0336a0c1eacb7de

in test/functional/test_framework/util.py:71 in a3badf75f6

  66 | +            d2[k] = thing2[k]
  67 | +    return d1, d2
  68 | +
  69 |  def assert_equal(thing1, thing2, *args):
  70 | +    if thing1 != thing2 and not args and isinstance(thing1, dict) and isinstance(thing2, dict):
  71 | +        d1,d2 = summarise_dict_differences(thing1, thing2)

maflcko commented at 10:59 AM on February 29, 2024:

        diff_str = '\n'.join(difflib.unified_diff(*[json.dumps(obj, indent=4, sort_keys=True).splitlines() for obj in [thing1, thing2]], lineterm=''))

nit: Possibly, an alternative, needing two imports

vasild commented at 3:25 PM on February 29, 2024:

What would the output look like?

maflcko commented at 9:44 AM on March 1, 2024:

The output will put each error on a separate line, which is helpful if there is more than one diff. However, it may also hide the parent key names (nesting) in the unified diff. I guess the context can be increased, if this is important.

    raise AssertionError("not(%s == %s)\n  in particular(%s)" % (thing1, thing2, diff_str))
AssertionError: not({'hash': '62f73cf9d4e2eaabe3fd04b426f0c05f236b0bd38a0015ac20d7060bf8080b25', 'height': 208, 'deployments': {'bip34': {'type': 'buried', 'active': True, 'height': 2}, 'bip66': {'type': 'buried', 'active': True, 'height': 3}, 'bip65': {'type': 'buried', 'active': True, 'height': 4}, 'csv': {'type': 'buried', 'active': True, 'height': 5}, 'segwit': {'type': 'buried', 'active': True, 'height': 6}, 'testdummy': {'type': 'bip9', 'active': False, 'bip9': {'bit': 28, 'start_time': 0, 'timeout': 9223372036854775807, 'min_activation_height': 0, 'status': 'started', 'since': 144, 'status_next': 'started', 'statistics': {'period': 144, 'elapsed': 65, 'count': 65, 'threshold': 108, 'possible': True}, 'signalling': '#################################################################'}}, 'taproot': {'type': 'bip9', 'height': 0, 'active': True, 'bip9': {'start_time': -1, 'timeout': 9223372036854775807, 'min_activation_height': 0, 'status': 'active', 'since': 0, 'status_next': 'active'}}}} == {'hash': '62f73cf9d4e2eaabe3fd04b426f0c05f236b0bd38a0015ac20d7060bf8080b25', 'height': 208, 'deployments': {'bip34': {'type': 'buried', 'active': True, 'height': 2}, 'bip66': {'type': 'buried', 'active': True, 'height': 3}, 'bip65': False, 'csv': {'type': 'buried', 'active': True, 'height': 5}, 'segwit': {'type': 'buried', 'active': True, 'height': 6}, 'testdummy': {'type': 'bip9', 'bip9': {'bit': 28, 'start_time': 0, 'timeout': 9223372036854775807, 'min_activation_height': 0, 'status': 'started', 'status_next': 'started', 'since': 144, 'statistics': {'period': 144, 'threshold': 108, 'elapsed': 65, 'count': 65, 'possible': True}, 'signalling': '#################################################################', 'dummy': True}, 'active': False}, 'taproot': {'type': 'bip9', 'bip9': {'start_time': -1, 'timeout': 9223372036854775807, 'min_activation_height': 0, 'status': 'active', 'status_next': 'failed', 'since': 7}, 'height': 0, 'active': False}}})
  in particular(--- 
+++ 
@@ -5,11 +5,7 @@
             "height": 2,
             "type": "buried"
         },
-        "bip65": {
-            "active": true,
-            "height": 4,
-            "type": "buried"
-        },
+        "bip65": false,
         "bip66": {
             "active": true,
             "height": 3,
@@ -26,13 +22,13 @@
             "type": "buried"
         },
         "taproot": {
-            "active": true,
+            "active": false,
             "bip9": {
                 "min_activation_height": 0,
-                "since": 0,
+                "since": 7,
                 "start_time": -1,
                 "status": "active",
-                "status_next": "active",
+                "status_next": "failed",
                 "timeout": 9223372036854775807
             },
             "height": 0,
@@ -42,6 +38,7 @@
             "active": false,
             "bip9": {
                 "bit": 28,
+                "dummy": true,
                 "min_activation_height": 0,
                 "signalling": "#################################################################",
                 "since": 144,)

maflcko commented at 9:56 AM on March 1, 2024:

Forgot the serialization_fallback in my patch. Fixed:

diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index b4b05b1597..5e1ca4628e 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -7,6 +7,7 @@
 from base64 import b64encode
 from decimal import Decimal, ROUND_DOWN
 from subprocess import CalledProcessError
+import difflib
 import hashlib
 import inspect
 import json
@@ -18,7 +19,7 @@ import re
 import time
 
 from . import coverage
-from .authproxy import AuthServiceProxy, JSONRPCException
+from .authproxy import AuthServiceProxy, JSONRPCException, serialization_fallback
 from collections.abc import Callable
 from typing import Optional
 
@@ -53,6 +54,14 @@ def assert_fee_amount(fee, tx_size, feerate_BTC_kvB):
 
 
 def assert_equal(thing1, thing2, *args):
+    if thing1 != thing2 and not args and isinstance(thing1, dict) and isinstance(thing2, dict):
+        diff_str = "\n".join(difflib.unified_diff(*[json.dumps(
+            obj,
+            indent=4,
+            default=serialization_fallback,
+            sort_keys=True,
+        ).splitlines() for obj in [thing1, thing2]], lineterm=""))
+        raise AssertionError("not(%s == %s)\n  in particular(%s)" % (thing1, thing2, diff_str))
     if thing1 != thing2 or any(thing1 != arg for arg in args):
         raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))

maflcko commented at 9:57 AM on March 1, 2024:

(In any case, my comment was just a nit. In the common case of just a single difference, both solutions are equally good)

maflcko approved

maflcko commented at 10:59 AM on February 29, 2024: member

lgtm, thanks!

brunoerg commented at 11:34 AM on February 29, 2024: contributor

nice!

utACK a3badf75f6fd88d465e59f46f0336a0c1eacb7de

josibake commented at 11:49 AM on February 29, 2024: member

ACK https://github.com/bitcoin/bitcoin/pull/29514/commits/a3badf75f6fd88d465e59f46f0336a0c1eacb7de

I've also written at least 2 versions of this function locally when faced with this.

in test/functional/test_framework/util.py:72 in a3badf75f6

  67 | +    return d1, d2
  68 | +
  69 |  def assert_equal(thing1, thing2, *args):
  70 | +    if thing1 != thing2 and not args and isinstance(thing1, dict) and isinstance(thing2, dict):
  71 | +        d1,d2 = summarise_dict_differences(thing1, thing2)
  72 | +        raise AssertionError("not(%s == %s)\n  in particular not(%s == %s)" % (thing1, thing2, d1, d2))

Empact commented at 9:11 PM on February 29, 2024:

How about moving this into the block below, as that would maintain a hierarchy of checks and avoid unintended divergence? I.e. such that the below checks: "are they equal" and the new code within checks "how should we best communicate the difference".

in test/functional/test_framework/util.py:57 in a3badf75f6

  51 | @@ -52,7 +52,24 @@ def assert_fee_amount(fee, tx_size, feerate_BTC_kvB):
  52 |          raise AssertionError("Fee of %s BTC too high! (Should be %s BTC)" % (str(fee), str(target_fee)))
  53 |  
  54 |  
  55 | +def summarise_dict_differences(thing1, thing2):
  56 | +    if not isinstance(thing1, dict) or not isinstance(thing2, dict):
  57 | +        return thing1, thing2

Empact commented at 9:14 PM on February 29, 2024:

How about raising a TypeError error here, which will help detect unexpected calling patterns?

maflcko commented at 9:45 AM on March 1, 2024:

This would break the recursion, no?

Empact commented at 9:14 PM on February 29, 2024: contributor

Concept ACK

BrandonOdiwuor approved

BrandonOdiwuor commented at 4:45 AM on March 2, 2024: contributor

Code Review ACK a3badf75f6fd88d465e59f46f0336a0c1eacb7de

DrahtBot requested review from Empact on Mar 2, 2024

ismaelsadeeq commented at 6:47 PM on March 7, 2024: member

Concept ACK

achow101 commented at 11:51 AM on March 11, 2024: member

ACK a3badf75f6fd88d465e59f46f0336a0c1eacb7de

DrahtBot requested review from ismaelsadeeq on Mar 11, 2024

achow101 merged this on Mar 11, 2024

achow101 closed this on Mar 11, 2024

PastaPastaPasta referenced this in commit 49e3719cbe on Oct 24, 2024

PastaPastaPasta referenced this in commit f9933f0ba5 on Oct 24, 2024

PastaPastaPasta referenced this in commit 045fa5f57e on Oct 24, 2024

PastaPastaPasta referenced this in commit aaccc9ea51 on Oct 24, 2024

bitcoin locked this on Mar 11, 2025

tests: Provide more helpful assert_equal errors #29514

Code Coverage

Reviews

Conflicts