test: create assert_not_equal util #29500

pull kevkevinpal wants to merge 1 commits into bitcoin:master from kevkevinpal:testFrameworkUtilsOne changing 41 files +137 −83
  1. kevkevinpal commented at 2:55 am on February 28, 2024: contributor

    In the functional tests there are lots of cases where we assert != which we now swap with assert_not_equal to be more readable

    This is motivated/uses logic from this PR which was closed #28528 This partially helps #23119

    I’ve broken it up to just assert_not_equal to keep the PR smaller as suggested in #28528 (comment)

    I can create follow up PR’s if this is wanted

  2. DrahtBot commented at 2:55 am on February 28, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29500.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hodlinator, ryanofsky
    Concept ACK theStack, janb84, rkrux
    Approach ACK BrandonOdiwuor
    Stale ACK vasild

    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:

    • #32155 (miner: timelock the coinbase to the mined block’s height by darosior)
    • #31492 (Execute Discover() when bind=0.0.0.0 or :: is set by andremralves)
    • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

    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.

  3. DrahtBot added the label Tests on Feb 28, 2024
  4. axizqayum686 changes_requested
  5. axizqayum686 commented at 6:47 am on February 28, 2024: none
    .
  6. Empact commented at 7:03 am on February 28, 2024: contributor

    Would be nice to structure this as a scripted diff: one commit where you add the function and requires, another where you convert the assert !=s.

    That would ensure programmatically that you hit all of the cases.

  7. maflcko requested review from theStack on Feb 28, 2024
  8. kevkevinpal force-pushed on Mar 2, 2024
  9. kevkevinpal force-pushed on Mar 2, 2024
  10. DrahtBot added the label CI failed on Mar 2, 2024
  11. kevkevinpal force-pushed on Mar 2, 2024
  12. kevkevinpal commented at 4:15 pm on March 2, 2024: contributor

    added scripted diff in dc087f1 with the following script git grep -l "assert.*!=" ./test/functional

    I will have to amend the commit as I see more spots I need to modify with the assert_not_equal util

  13. kevkevinpal force-pushed on Mar 2, 2024
  14. kevkevinpal force-pushed on Mar 2, 2024
  15. kevkevinpal force-pushed on Mar 2, 2024
  16. kevkevinpal force-pushed on Mar 2, 2024
  17. kevkevinpal force-pushed on Mar 2, 2024
  18. kevkevinpal force-pushed on Mar 2, 2024
  19. theStack commented at 8:35 pm on March 2, 2024: contributor
    Concept ACK, thanks for picking this up.
  20. kevkevinpal force-pushed on Mar 2, 2024
  21. kevkevinpal force-pushed on Mar 2, 2024
  22. kevkevinpal force-pushed on Mar 2, 2024
  23. kevkevinpal force-pushed on Mar 2, 2024
  24. kevkevinpal force-pushed on Mar 2, 2024
  25. brunoerg commented at 2:48 pm on March 4, 2024: contributor

    There are some places you are using assert_not_equal but not importing it and other ones you’re importing it twice. Check CI.

     0Running Unit Tests for Test Framework Modules
     1Traceback (most recent call last):
     2  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_runner.py", line 906, in <module>
     3    main()
     4  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_runner.py", line 539, in main
     5    run_tests(
     6  File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_runner.py", line 580, in run_tests
     7    test_framework_tests.addTest(unittest.TestLoader().loadTestsFromName("test_framework.{}".format(module)))
     8                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     9  File "/usr/lib/python3.12/unittest/loader.py", line 137, in loadTestsFromName
    10    module = __import__(module_name)
    11             ^^^^^^^^^^^^^^^^^^^^^^^
    12  File "/ci_container_base/test/functional/test_framework/address.py", line 14, in <module>
    13    from .script import (
    14  File "/ci_container_base/test/functional/test_framework/script.py", line 14, in <module>
    15    from .key import TaggedHash, tweak_add_pubkey, compute_xonly_pubkey
    16  File "/ci_container_base/test/functional/test_framework/key.py", line 16, in <module>
    17    from test_framework.crypto import secp256k1
    18  File "/ci_container_base/test/functional/test_framework/crypto/secp256k1.py", line 316, in <module>
    19    G = GE.lift_x(0x79BE667EF9DCBBAC55A06295CE870B07029BFCDB2DCE28D959F2815B16F81798)
    20        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    21  File "/ci_container_base/test/functional/test_framework/crypto/secp256k1.py", line 257, in lift_x
    22    y = (FE(x)**3 + 7).sqrt()
    23         ^^^^^
    24  File "/ci_container_base/test/functional/test_framework/crypto/secp256k1.py", line 41, in __init__
    25    assert_not_equal(den, 0)
    26    ^^^^^^^^^^^^^^^^
    27NameError: name 'assert_not_equal' is not defined
    
  26. in test/functional/p2p_blockfilters.py:24 in ad1e879351 outdated
    21@@ -21,6 +22,7 @@
    22 from test_framework.p2p import P2PInterface
    23 from test_framework.test_framework import BitcoinTestFramework
    24 from test_framework.util import (
    25+    assert_not_equal,
    


    brunoerg commented at 2:49 pm on March 4, 2024:
    Importing twice? (from util and messages)

    kevkevinpal commented at 3:20 am on March 5, 2024:

    hmm yea I was trying to use this command to find all the places but it seems to be a bit more complex than I initially thought

    0git grep -l "assert .*!=" -- ':(exclude)*script.py' ./test/functional | xargs sed -i -e "/assert .*!=/ s/assert /assert_not_equal(/g" -e "/assert_not_equal/ s/ !=/,/g" -e "/assert_not_equal/ s/$/)/g" -e "s/test_framework.util import (/test_framework.util import (\n    assert_not_equal,/g" -e "s/import assert_equal/(\n    assert_equal,\n    assert_not_equal,\n)/g"
    

    I might just manually add the imports and use the script to look for the assert ... != as suggested by @Empact

  27. kevkevinpal force-pushed on Mar 5, 2024
  28. kevkevinpal force-pushed on Mar 5, 2024
  29. kevkevinpal force-pushed on Mar 5, 2024
  30. kevkevinpal force-pushed on Mar 5, 2024
  31. kevkevinpal force-pushed on Mar 5, 2024
  32. kevkevinpal force-pushed on Mar 5, 2024
  33. kevkevinpal force-pushed on Mar 5, 2024
  34. fanquake commented at 11:05 am on March 5, 2024: member

    https://cirrus-ci.com/task/6627384883937280?logs=ci#L4021

     0 test  2024-03-05T05:30:24.555000Z TestFramework (ERROR): Unexpected exception caught during testing 
     1                                   Traceback (most recent call last):
     2                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
     3                                       self.run_test()
     4                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_accept_v3.py", line 448, in run_test
     5                                       self.test_reorg_2child_rbf()
     6                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_accept_v3.py", line 29, in wrapper
     7                                       func(self)
     8                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/mempool_accept_v3.py", line 429, in test_reorg_2child_rbf
     9                                       assert_not_equal((child_1_conflict["txid"], child_1["txid"]))
    10                                   TypeError: assert_not_equal() missing 1 required positional argument: 'thing2'
    
  35. kevkevinpal force-pushed on Mar 6, 2024
  36. kevkevinpal force-pushed on Mar 6, 2024
  37. kevkevinpal force-pushed on Mar 6, 2024
  38. kevkevinpal force-pushed on Mar 6, 2024
  39. kevkevinpal force-pushed on Mar 6, 2024
  40. kevkevinpal force-pushed on Mar 6, 2024
  41. kevkevinpal commented at 3:22 pm on March 10, 2024: contributor
    not sure why this ci is failing ASan + LSan + UBSan + integer, no depends, USDT but the rest seem to be passing
  42. maflcko commented at 8:46 am on March 11, 2024: member

    not sure why this ci is failing ASan + LSan + UBSan + integer, no depends, USDT but the rest seem to be passing

    It is a race in the test logic between the python code and the net thread, IIUC. Please file a new issue or a new pull to fix it.

  43. kevkevinpal commented at 10:07 pm on March 12, 2024: contributor

    not sure why this ci is failing ASan + LSan + UBSan + integer, no depends, USDT but the rest seem to be passing

    It is a race in the test logic between the python code and the net thread, IIUC. Please file a new issue or a new pull to fix it.

    Opened a new issue for the CI error https://github.com/bitcoin/bitcoin/issues/29643

  44. DrahtBot removed the label CI failed on Apr 5, 2024
  45. BrandonOdiwuor commented at 2:28 pm on April 23, 2024: contributor

    Approach ACK

    Good job approaching this using Scripted diffs

  46. in test/functional/test_framework/test_node.py:640 in f9ba6fd46d outdated
    637@@ -638,7 +638,7 @@ def assert_start_raises_init_error(self, extra_args=None, expected_msg=None, mat
    638                 self.start(extra_args, stdout=log_stdout, stderr=log_stderr, *args, **kwargs)
    639                 ret = self.process.wait(timeout=self.rpc_timeout)
    640                 self.log.debug(self._node_msg(f'bitcoind exited with status {ret} during initialization'))
    641-                assert ret != 0  # Exit code must indicate failure
    642+                assert_not_equal(ret, 0  ) # Exit code must indicate failure
    


    rkrux commented at 1:57 pm on May 17, 2024:
    Extra spaces after 0?

    kevkevinpal commented at 0:27 am on May 22, 2024:
    not sure why it is doing that I will check the script to see why

    kevkevinpal commented at 2:26 am on May 22, 2024:

    fixed in 4358c6e

    thanks for the review!

  47. in test/functional/p2p_blockfilters.py:97 in f9ba6fd46d outdated
    93@@ -94,7 +94,7 @@ def run_test(self):
    94         self.nodes[0].syncwithvalidationinterfacequeue()
    95 
    96         main_block_hash = self.nodes[0].getblockhash(1000)
    97-        assert main_block_hash != stale_block_hash, "node 0 chain did not reorganize"
    98+        assert_not_equal(main_block_hash, stale_block_hash, "node 0 chain did not reorganize")
    


    rkrux commented at 2:04 pm on May 17, 2024:

    "node 0 chain did not reorganize" This seems to be the 3rd argument to the assert_not_equal() call, i.e. *args. Will this piece be kicked in then? any(thing1 == arg for arg in args)

    I don’t get why would we need to search for main_block_hash in the above error string.


    kevkevinpal commented at 0:28 am on May 22, 2024:
    that is strange I would think that the 3rd arg would make the tests fail in this case

    kevkevinpal commented at 2:19 am on May 22, 2024:

    I’ve updated the function to not accept the args param since it did not seem to be needed and instead allowed an optional 3rd param as the message

    updated in 4488120

  48. rkrux commented at 2:08 pm on May 17, 2024: contributor

    crACK f9ba6fd

    I’m unable to run make successfully because of the following error on my system. I’ve noticed this in another PR as well and it is due to lack of a commit that was merged in later. Asked couple questions.

    0/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/time.h:127:12: note: 'gmtime_r' declared here
    1struct tm *gmtime_r(const time_t * __restrict, struct tm * __restrict);
    
  49. DrahtBot requested review from BrandonOdiwuor on May 17, 2024
  50. rkrux commented at 2:08 pm on May 17, 2024: contributor
    @kevkevinpal Did you use a tool to generate the verification script?
  51. DrahtBot added the label Needs rebase on May 20, 2024
  52. in test/functional/feature_dbcrash.py:278 in f9ba6fd46d outdated
    274@@ -274,7 +275,7 @@ def run_test(self):
    275         self.log.info(f"Restarted nodes: {self.restart_counts}; crashes on restart: {self.crashed_on_restart}")
    276 
    277         # If no nodes were restarted, we didn't test anything.
    278-        assert self.restart_counts != [0, 0, 0]
    279+        assert_not_equal(self.restart_counts, [0, 0, 0])
    


    ajtowns commented at 1:11 pm on May 20, 2024:

    assert_equal is helpful because when a == 3 fails, it can be helpful to know what the value of a actually is without having to add debug code and rerun. If a != 3 fails, though, you already know that a == 3, so this doesn’t seem very helpful.

    There are cases where you’re comparing a != b and may not be sure which ones getting the wrong value where this could perhaps be helpful, but those seem pretty rare.

    Concept -0 for me.


    hodlinator commented at 2:10 pm on February 24, 2025:

    In thread #29500 (review):

    Agree this is a valid argument against the PR. It is very seldom useful to print the values when they are equal.

    I still prefer assert_not_equal() for:

    • Symmetry with assert_equal().
    • Distinguishing itself from assert which is skipped when running Python with -O.

    maflcko commented at 3:11 pm on February 24, 2025:

    Distinguishing itself from assert which is skipped when running Python with -O.

    It is not possible to run the functional tests with -O (they’d fail on current master), and it is unclear what reason there could be to do so in the first place. So i think using the flag as an argument for a code change seems weak.

    If there is a reason for -O, it should be mentioned, all tests should be fixed, and it should be enforced with something like https://docs.astral.sh/ruff/rules/assert/.

    If there is no reason for -O, it should be ignored or disallowed.


    vasild commented at 1:57 pm on March 19, 2025:

    There are cases where you’re comparing a != b and may not be sure which ones getting the wrong value where this could perhaps be helpful, but those seem pretty rare

    I counted (by eyes, could be +/- a few):

    • 16 occurrences where one of the arguments is a constant, e.g. assert_not_equal(peer.nServices & NODE_BLOOM, 0), and
    • 59 occurrences where both arguments are variables, e.g. assert_not_equal(child_one_wtxid, child_two_wtxid)
  53. in test/functional/test_framework/util.py:78 in f9ba6fd46d outdated
    55@@ -56,6 +56,10 @@ def assert_equal(thing1, thing2, *args):
    56     if thing1 != thing2 or any(thing1 != arg for arg in args):
    57         raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
    58 
    59+def assert_not_equal(thing1, thing2, *args):
    60+    if thing1 == thing2 or any(thing1 == arg for arg in args):
    61+        raise AssertionError("%s" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
    


    ajtowns commented at 1:19 pm on May 20, 2024:
    This error message doesn’t make sense when *args is non-empty: if you say assert_not_equal(a, 1, 2) and a is 1 or 2, it will report 1 == 1 == 2 or 2 == 1 == 2.

    kevkevinpal commented at 0:39 am on May 22, 2024:
    Yes that would not make sense let me see if I can improve the logging here

    kevkevinpal commented at 2:18 am on May 22, 2024:

    I’ve updated this to instead only take three arguments and the last one being an optional message since I did not see anywhere we were already asserting != in a chain.

    This should clear up the message and allow the one location we do use a message to be used

    updated in 4488120

  54. kevkevinpal force-pushed on May 22, 2024
  55. kevkevinpal commented at 0:29 am on May 22, 2024: contributor

    @kevkevinpal Did you use a tool to generate the verification script?

    No I did not I just manually used git grep and sed to create the script

  56. kevkevinpal force-pushed on May 22, 2024
  57. DrahtBot added the label CI failed on May 22, 2024
  58. DrahtBot commented at 0:38 am on May 22, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/25253865668

  59. kevkevinpal commented at 0:38 am on May 22, 2024: contributor
    rebased to 4358c6e
  60. kevkevinpal force-pushed on May 22, 2024
  61. kevkevinpal force-pushed on May 22, 2024
  62. kevkevinpal force-pushed on May 22, 2024
  63. kevkevinpal force-pushed on May 22, 2024
  64. kevkevinpal force-pushed on May 22, 2024
  65. DrahtBot removed the label Needs rebase on May 22, 2024
  66. kevkevinpal force-pushed on May 22, 2024
  67. DrahtBot removed the label CI failed on May 22, 2024
  68. DrahtBot added the label Needs rebase on Jul 2, 2024
  69. glozow commented at 11:59 am on August 7, 2024: member
    Are you still working on this?
  70. kevkevinpal force-pushed on Aug 7, 2024
  71. kevkevinpal commented at 2:42 pm on August 7, 2024: contributor

    Are you still working on this?

    Yup just haven’t had much activity on it but rebased to 1d72266

  72. DrahtBot removed the label Needs rebase on Aug 7, 2024
  73. kevkevinpal requested review from axizqayum686 on Aug 7, 2024
  74. kevkevinpal requested review from rkrux on Aug 7, 2024
  75. kevkevinpal requested review from brunoerg on Aug 7, 2024
  76. kevkevinpal requested review from ajtowns on Aug 7, 2024
  77. achow101 removed review request from rkrux on Oct 15, 2024
  78. achow101 removed review request from BrandonOdiwuor on Oct 15, 2024
  79. achow101 removed review request from axizqayum686 on Oct 15, 2024
  80. achow101 requested review from vasild on Oct 15, 2024
  81. DrahtBot added the label CI failed on Oct 24, 2024
  82. DrahtBot removed the label CI failed on Oct 25, 2024
  83. vasild approved
  84. vasild commented at 10:43 am on November 5, 2024: contributor

    Almost ACK 1d722660a6, modulo squash of the two commits.

    This is a nice addition.

    It is better to squash the two commits into one because IMO it does not make sense to first add the import in one commit and in another commit to use it. That way it becomes difficult to review whether an unnecessary import was added to some file. Also some python linter that tests each commit could be upset about the unused imports in the first commit.

    I found it easier to read the diff itself instead of the “scripted diff” code (the line is 700+ chars long!). Further, I tweaked the diff to get the left and right sides align, so instead of:

    0-        assert (cblks[3]['0'][33:65] < cblks[3]['1'][33:65]) != (cblks[4]['0'][33:65] < cblks[4]['1'][33:65])
    1+        assert_not_equal((cblks[3]['0'][33:65] < cblks[3]['1'][33:65]), (cblks[4]['0'][33:65] < cblks[4]['1'][33:65]))
    

    I looked at:

    0-        assert (cblks[3]['0'][33:65] < cblks[3]['1'][33:65]) != (cblks[4]['0'][33:65] < cblks[4]['1'][33:65])
    1+        ass_ne((cblks[3]['0'][33:65] < cblks[3]['1'][33:65]),   (cblks[4]['0'][33:65] < cblks[4]['1'][33:65]))
    

    I used git show 1d722660a6 |sed -E 's/^(.*)assert_not_equal([^,]*), (.*)$/\1ass_ne\2, \3/g' for that.

    Needs rebase for CMake.

  85. DrahtBot requested review from BrandonOdiwuor on Nov 5, 2024
  86. DrahtBot requested review from rkrux on Nov 5, 2024
  87. in test/functional/test_framework/util.py:79 in b47d40bdb3 outdated
    73@@ -74,6 +74,10 @@ def assert_equal(thing1, thing2, *args):
    74     if thing1 != thing2 or any(thing1 != arg for arg in args):
    75         raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
    76 
    77+def assert_not_equal(thing1, thing2, message=""):
    


    rkrux commented at 7:31 am on November 11, 2024:
    Nit: The 3rd argument can be called error_message.
  88. rkrux commented at 7:47 am on November 11, 2024: contributor

    Concept ACK 1d722660a65522539872c09ae8a3ba8c9ca55b77 The util function looks quite different from the last time I reviewed, simpler and more readable now!

    Couple points:

    1. The PR needs a rebase so that it works with the new build system, unable to build and test it in local atm with the latest build commands. Though the PR doesn’t have any conflicts with master, it would be nice to have this PR rebased if not much overhead for the author.
    2. I agree with @vasild’s comment that the first commit should not be importing the helper function only to be used in the next commit. While reviewing the first commit, it immediately struck to me it is being imported but not used, which looked odd. However, I now wonder would squashing the 2 commits still work with the scripted diff as per the comment in the script checker?

    https://github.com/bitcoin/bitcoin/blob/master/test/lint/commit-script-check.sh#L11-L12

    The resulting script should exactly transform the previous commit into the current one. Any remaining diff signals an error.

  89. kevkevinpal force-pushed on Nov 12, 2024
  90. kevkevinpal commented at 11:04 pm on November 12, 2024: contributor

    rebased to 6590b26

    and I can address your latest comments soon

  91. kevkevinpal force-pushed on Nov 12, 2024
  92. DrahtBot added the label CI failed on Nov 12, 2024
  93. kevkevinpal force-pushed on Nov 12, 2024
  94. kevkevinpal force-pushed on Nov 12, 2024
  95. kevkevinpal force-pushed on Nov 12, 2024
  96. kevkevinpal force-pushed on Nov 13, 2024
  97. kevkevinpal force-pushed on Nov 13, 2024
  98. kevkevinpal commented at 2:43 am on November 13, 2024: contributor

    yes @rkrux you are right if I want to use a scripted diff then all the changes must be done using the script

    I can remove the scripted diff and just squash them into a single diff since right now the script is getting very large already if that is preferred by others

  99. DrahtBot removed the label CI failed on Nov 13, 2024
  100. DrahtBot added the label Needs rebase on Nov 19, 2024
  101. fanquake marked this as a draft on Feb 20, 2025
  102. fanquake commented at 5:29 pm on February 20, 2025: member
    @kevkevinpal what’s the status of this?
  103. vasild commented at 1:17 pm on February 21, 2025: contributor
    Almost ACK dcf9a45aab4c4a7f9f900d5dafcdfe0e9a598a38, modulo squash the two commits into one and rebase.
  104. DrahtBot requested review from rkrux on Feb 21, 2025
  105. kevkevinpal force-pushed on Feb 22, 2025
  106. kevkevinpal marked this as ready for review on Feb 22, 2025
  107. kevkevinpal force-pushed on Feb 22, 2025
  108. kevkevinpal commented at 2:22 am on February 22, 2025: contributor

    @kevkevinpal what’s the status of this?

    Just rebased and addressed @vasild comment


    Almost ACK dcf9a45, modulo squash the two commits into one and rebase.

    Just rebased and squashed the commits, I removed the scripted-diff because it was getting more difficult to review the diff than the actual code. But now all the changes are in d9cb032


    if you do grep -nr assert\ .*!= ./test/functional/ there should only be one result

    assert len(ss) == 175 - (in_type == SIGHASH_ANYONECANPAY) * 49 - (out_type != SIGHASH_ALL and out_type != SIGHASH_SINGLE) * 32 + (annex is not None) * 32 + scriptpath * 37

    which we don’t need to add assert_not_equal to

  109. DrahtBot removed the label Needs rebase on Feb 22, 2025
  110. in test/functional/feature_coinstatsindex.py:30 in d9cb032ec1 outdated
    26@@ -27,6 +27,7 @@
    27 )
    28 from test_framework.test_framework import BitcoinTestFramework
    29 from test_framework.util import (
    30+    assert_not_equal,
    


    hodlinator commented at 1:43 pm on February 24, 2025:

    nit: Should appear after assert_equal alphabetically.

    Same in test/functional/feature_dbcrash.py and others.

  111. in test/functional/test_framework/util.py:81 in d9cb032ec1 outdated
    75@@ -76,6 +76,10 @@ def assert_equal(thing1, thing2, *args):
    76     if thing1 != thing2 or any(thing1 != arg for arg in args):
    77         raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
    78 
    79+def assert_not_equal(thing1, thing2, error_message=""):
    80+    if thing1 == thing2:
    81+        raise AssertionError("%s == %s %s" % (str(thing1), str(thing2), str(error_message)))
    


    hodlinator commented at 1:54 pm on February 24, 2025:

    Should use f-strings: https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/test/functional/README.md?plain=1#L40

    nit: And possibly rephrase:

    0        raise AssertionError(f"Both values are {thing1}{f', {error_message}' if error_message else ''}")
    

    hodlinator commented at 10:14 pm on March 27, 2025:
    Would be more eager to ACK if code introduced by the PR followed bitcoin/test/functional/README.md as noted above. If you don’t want to change the phrasing towards what’s in the nit-part that’s fine.

    kevkevinpal commented at 11:15 pm on March 27, 2025:

    Went ahead an used your suggestion in be71af3

    it now looks like this

    02025-03-27T23:14:10.629000Z TestFramework (ERROR): Assertion failed
    1Traceback (most recent call last):
    2  File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/test_framework.py", line 182, in main
    3    self.run_test()
    4  File "/mnt/shared_drive/DEVDIR/bitcoin/./build_dir/test/functional/p2p_blockfilters.py", line 98, in run_test
    5    assert_not_equal(stale_block_hash, stale_block_hash, "node 0 chain did not reorganize")
    6  File "/mnt/shared_drive/DEVDIR/bitcoin/test/functional/test_framework/util.py", line 81, in assert_not_equal
    7    raise AssertionError(f"Both values are {thing1}, {f'{error_message}' if error_message else ''}")
    8AssertionError: Both values are 05ba865bb920e2ff5763411a9dd3c225ace8ff4393469bb25acffd56df1bfb6e, node 0 chain did not reorganize
    

    hodlinator commented at 9:43 am on March 28, 2025:
    Thread #29500 (review): Thanks for taking my suggestion!
  112. in test/functional/p2p_blockfilters.py:74 in d9cb032ec1 outdated
    69@@ -69,11 +70,11 @@ def run_test(self):
    70         assert_equal(self.nodes[1].getblockcount(), 2000)
    71 
    72         # Check that nodes have signalled NODE_COMPACT_FILTERS correctly.
    73-        assert peer_0.nServices & NODE_COMPACT_FILTERS != 0
    74+        assert_not_equal(peer_0.nServices & NODE_COMPACT_FILTERS, 0)
    75         assert peer_1.nServices & NODE_COMPACT_FILTERS == 0
    


    hodlinator commented at 2:00 pm on February 24, 2025:
    nit: Feels a bit weird to not have assert_equal() here and other directly adjacent places. Could touch up adjacent lines like this in separate commit?
  113. hodlinator commented at 2:20 pm on February 24, 2025: contributor
    Concept ACK d9cb032ec1aa7000fc3f5b9fa48405f269879497
  114. DrahtBot added the label Needs rebase on Mar 13, 2025
  115. vasild approved
  116. vasild commented at 1:44 pm on March 19, 2025: contributor
    ACK d9cb032ec1aa7000fc3f5b9fa48405f269879497
  117. DrahtBot requested review from hodlinator on Mar 19, 2025
  118. ryanofsky approved
  119. ryanofsky commented at 6:22 pm on March 27, 2025: contributor
    Code review ACK d9cb032ec1aa7000fc3f5b9fa48405f269879497. Seems like a nice change, though needs to be rebased. I think this change is good because it makes tests more readable, prints more information on failures, avoids misusing the assert keyword, and makes the test framework api more consistent internally and consistent with other frameworks.
  120. kevkevinpal force-pushed on Mar 27, 2025
  121. kevkevinpal commented at 8:40 pm on March 27, 2025: contributor
    rebased to 29fa2b9
  122. DrahtBot removed the label Needs rebase on Mar 27, 2025
  123. test: create assert_not_equal util and add to where imports are needed
    In the functional tests there are lots of cases where we assert != which
    this new util will replace, we also are adding the imports and the new assertion
    be71af3cc0
  124. kevkevinpal force-pushed on Mar 27, 2025
  125. in test/functional/test_framework/util.py:81 in be71af3cc0
    75@@ -76,6 +76,10 @@ def assert_equal(thing1, thing2, *args):
    76     if thing1 != thing2 or any(thing1 != arg for arg in args):
    77         raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
    78 
    79+def assert_not_equal(thing1, thing2, error_message=""):
    80+    if thing1 == thing2:
    81+        raise AssertionError(f"Both values are {thing1}, {f'{error_message}' if error_message else ''}")
    


    maflcko commented at 7:11 am on March 28, 2025:

    what is the point of the error_message, given that it is unused and highly fragile:

    • It is not a named arg, nor type-safe, so someone writing assert_not_equal("a", "b", "c") or assert_not_equal(1, 2, 3) will be wrong and confusing at best.
    • It is not used anywhere else in this file for another assert

    hodlinator commented at 9:42 am on March 28, 2025:

    Thread #29500 (review):

    It would be good if all assert_ functions supported error messages explaining why the condition should never fail, or what could be wrong if it has failed. (Built-in assert supports it too).

    To avoid mixing the error message up with a value to be checked, one could do:

    0def assert_not_equal(thing1, thing2, *, message=None):
    1    if thing1 == thing2:
    2        raise AssertionError(f"Both values are {thing1}{f', {message}' if message else ''}")
    
    • * enforces named parameters from that point.
    • It would be good to shorten the name for callers.
    • “, " is only printed if there is a message.
    • Feels more correct to use None as default value.

    The traceback in this case does print the source code line, so a comment on the same line could be sufficient. It depends on how the error is presented. Example failure:

    02025-03-28T09:58:37.993000Z TestFramework (ERROR): Assertion failed
    1Traceback (most recent call last):
    2  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 182, in main
    3    self.run_test()
    4  File "/home/hodlinator/bitcoin/build/test/functional/p2p_blockfilters.py", line 98, in run_test
    5    assert_not_equal(main_block_hash, stale_block_hash, message="node 0 chain did not reorganize")
    6  File "/home/hodlinator/bitcoin/test/functional/test_framework/util.py", line 81, in assert_not_equal
    7    raise AssertionError(f"Both values are {thing1}{f', {message}' if message else ''}")
    8AssertionError: Both values are 1b0ec65a9941d57790524ff90cc1cb93d2f8e70680c1c7486d9edf934d96a041, node 0 chain did not reorganize
    

    maflcko commented at 10:31 am on March 28, 2025:

    assert_not_equal(main_block_hash, stale_block_hash, message=“node 0 chain did not reorganize”)

    I’d say it is already self-explanatory to see assert_not_equal(main_block_hash, stale_block_hash) and the message “AssertionError: Both values are (the same)”. It should be clear that the chains are the same, when they should be different.

    Even more so, given that the test logs before the assert: self.log.info("Reorg node 0 to a new chain.").

    Unique and descriptive error messages make sense when it is the only piece of information given (for example in the context of an init error during startup of the program). Given that no other asserts in this file give the option, it would be best to defer this to a future where it is actually widely needed. However, given the extensive context in tests and test failures, I doubt it will ever be widely needed. Until then, it is just bike-shedding and overhead that has to be maintained and reviewed.


    hodlinator commented at 10:59 am on March 28, 2025:

    “AssertionError: Both values are (the same)”

    Don’t know if you mean literally "Both values are (the same)" or f"Both values are {thing1}". Find the latter clearly more useful; providing clues of cause of the failure, making it easier to reproduce.


    What it comes down to with custom error messages is being able to directly communicate with whoever is troubleshooting a failure. If one doesn’t even need to go to the logs to see what happened before, I consider it a small win. But agree it is bikeshedding-prone, so I would be okay with removing it.


    maflcko commented at 11:29 am on March 28, 2025:

    Don’t know if you mean literally "Both values are (the same)" or f"Both values are {thing1}". Find the latter clearly more useful; providing clues of cause of the failure, making it easier to reproduce.

    Agree. Sorry for being unclear, I meant f"Both values are {thing1}" when I said “(the same)”.


    ryanofsky commented at 3:24 pm on March 28, 2025:

    re: #29500 (review)

    I do like error_message parameter idea, fwiw. I could see these making tests more readable if they started being used for more important or more confusing checks. Tests are often written with some important checks directly related to the thing being tested, and other less important checks to see if other things are consistent. Having a place to indicate what it actually means when a check fails could be helpful for more important checks to distinguish them.


    yancyribbens commented at 6:46 pm on March 28, 2025:

    The traceback in this case does print the source code line, so a comment on the same line could be sufficient. It depends on how the error is presented. Example failure:

    This is an annoying draw back that the stack trace won’t contain the line in the test that’s in error, but instead the line in the assert_not_equal helper. In Rust, one can decorate the helper function with the track_caller macro. I don’t see anything that is similar for C++ sadly.


    hodlinator commented at 9:33 pm on March 28, 2025:

    We’re in Python here though. I spent a couple of hours with an LLM in mid-February to brute force this embryo: https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:2025/02/assert_ergonomics

    Current assert_equal()-behavior:

    02025-03-28T21:15:36.379000Z TestFramework (ERROR): Assertion failed
    1Traceback (most recent call last):
    2  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 178, in main
    3    self.run_test()
    4  File "/home/hodlinator/bitcoin/./build/test/functional/wallet_multisig_descriptor_psbt.py", line 80, in run_test
    5    assert_equal(
    6  File "/home/hodlinator/bitcoin/test/functional/test_framework/util.py", line 77, in assert_equal
    7    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
    8AssertionError: not(0 == 123)
    

    With first commit, chomping off util.py traceback entry you complained about:

    02025-03-28T21:16:19.814000Z TestFramework (ERROR): Assertion failed
    1Traceback (most recent call last):
    2  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 178, in main
    3    self.run_test()
    4  File "/home/hodlinator/bitcoin/./build/test/functional/wallet_multisig_descriptor_psbt.py", line 80, in run_test
    5    assert_equal(
    6AssertionError: not(0 == 123)
    

    With second commit, replacing another traceback entry with proper context:

    02025-03-28T21:17:39.494000Z TestFramework (ERROR): Assertion failed
    1Traceback (most recent call last):
    2  File "/home/hodlinator/bitcoin/test/functional/test_framework/test_framework.py", line 178, in main
    3    self.run_test()
    4AssertionError: not(0 == 123)
    5Failing expression: /home/hodlinator/bitcoin/./build/test/functional/wallet_multisig_descriptor_psbt.py:80 - 83
    6        assert_equal(
    7            123 if (False and
    8            True) else 0,
    9            123)
    

    The code needs more work, should be more defensive, forgot to output function name (run_test). And I wouldn’t bet on it being accepted anyway.


    maflcko commented at 12:16 pm on March 30, 2025:
    No strong opinion, but I think we shouldn’t spend too much time on the error message explaining the context because the traceback could contain too little context, or on the traceback containing too much (redundant) context by default. A full log of a test failure could be thousands of lines, so one additional line, even if could be redundant in a strict sense should be fine. Also, looking at the historic failures (in Python) at https://github.com/bitcoin/bitcoin/issues?q=is%3Aissue%20%20label%3A%22CI%20failed%22%20, it was never(?) an issue to figure out what failed where. The problem usually is to figure out why something failed, and how to reproduce and fix it.

    rkrux commented at 7:56 am on March 31, 2025:

    I don’t mind having an error message being logged and I do agree with the following points:

    It is not a named arg, nor type-safe, so someone writing assert_not_equal(“a”, “b”, “c”) or assert_not_equal(1, 2, 3) will be wrong and confusing at best.

    Unique and descriptive error messages make sense when it is the only piece of information given (for example in the context of an init error during startup of the program).

    As I see from the usages of assert_not_equal, there is only 1 case where the error message is passed out of ~77 total usages. I believe a named argument for the error message could help in avoiding unintended usages of this utility function.

  126. hodlinator approved
  127. hodlinator commented at 10:09 am on March 28, 2025: contributor

    ACK be71af3cc0b0bcb7d917cc6f2e5fda119f1b1bd6

    This change adds the missing opposite of assert_equal() and decreases usage of built-in assert for performing always-on checks during tests.

  128. DrahtBot requested review from vasild on Mar 28, 2025
  129. DrahtBot requested review from ryanofsky on Mar 28, 2025
  130. janb84 commented at 12:38 pm on March 28, 2025: contributor

    Concept ACK be71af3

    • Code review ✅
    • Build & tested ✅

    I agree with @hodlinator on the import order NIT — let’s keep it consistent and maintain clean, high-quality code. #29500 (review)

  131. ryanofsky approved
  132. ryanofsky commented at 3:25 pm on March 28, 2025: contributor
    Code review ACK be71af3cc0b0bcb7d917cc6f2e5fda119f1b1bd6. Just rebased and added f-string since last review.
  133. DrahtBot requested review from ryanofsky on Mar 28, 2025
  134. DrahtBot requested review from janb84 on Mar 28, 2025
  135. rkrux changes_requested
  136. rkrux commented at 7:57 am on March 31, 2025: contributor

    Concept ACK be71af3cc0b0bcb7d917cc6f2e5fda119f1b1bd6

    Requesting changes mainly because an additional comma is printed in case of an assertion error when the error message is not passed, which can be confusing for the reader later.

     02025-03-31T07:28:44.958000Z TestFramework (INFO): Test fundrawtxn with locked wallet and hardened derivation
     12025-03-31T07:28:46.455000Z TestFramework (ERROR): Assertion failed
     2Traceback (most recent call last):
     3  File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 182, in main
     4    self.run_test()
     5    ~~~~~~~~~~~~~^^
     6  File "/Users/rkrux/projects/bitcoin/./build/test/functional/wallet_fundrawtransaction.py", line 141, in run_test
     7    self.test_locked_wallet()
     8    ~~~~~~~~~~~~~~~~~~~~~~~^^
     9  File "/Users/rkrux/projects/bitcoin/./build/test/functional/wallet_fundrawtransaction.py", line 665, in test_locked_wallet
    10    assert_not_equal(fundedTx["changepos"], 1)
    11    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^
    12  File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/util.py", line 81, in assert_not_equal
    13    raise AssertionError(f"Both values are {thing1}, {f'{error_message}' if error_message else ''}")
    14AssertionError: Both values are 1, 
    152025-03-31T07:28:46.512000Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
    

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-03-31 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me