test: replace bare asserts with assertion helpers (assert_equal() etc.) #23119

1. 
   theStack commented at 1:06 pm on September 28, 2021: contributor

   #23117 replaced asserts with the test framework’s internal helpers (see https://github.com/bitcoin/bitcoin/blob/dccf3d25f9e78909eb7b3143e89a7c87fac25ab5/test/functional/test_framework/util.py#L47-L59) for a single test, in order to see the expected and failed values if such an assertion fails. The same should be done for all the remaining tests. Potential candidates can be found via

   0$ cd ./test/functional
   1$ git grep assert.*==
   2$ git grep "assert.*<="
   3$ git grep "assert.*>="
   4$ git grep "assert.*<"
   5$ git grep "assert.*>"
   

   Useful skills:

   basic Python3 knowledge

   Want to work on this issue?

   For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

2. maflcko added the label good first issue on Sep 28, 2021
3. 
   vincenzopalazzo commented at 8:15 pm on September 28, 2021: none

   I can take the lock on this issue?
4. 
   meshcollider commented at 1:51 am on September 29, 2021: contributor

   @vincenzopalazzo feel free to open a pull request :)
5. bitcoin deleted a comment on Sep 30, 2021
6. 
   fawkesG5 commented at 12:22 pm on October 8, 2021: none

   Hi ! Can I take up this issue?
7. 
   vincenzopalazzo commented at 12:33 pm on October 8, 2021: none

   @fawkesG5 https://github.com/bitcoin/bitcoin/pull/23127
8. 
   fawkesG5 commented at 1:02 pm on October 8, 2021: none

   Hey @vincenzopalazzo Thanks for replying and the reference. But can you elaborate a little on the same? This will be my first PR and I am looking to make a positive contribution. Can I work on the files which you haven’t touched yet?
9. 
   vincenzopalazzo commented at 3:03 pm on October 8, 2021: none

   Hi @vincenzopalazzo,

   IMO the change of the same issue in different PR is very confusing for the people that will make a review, and also work in a different file.

   As discussed in this issue #23135#pullrequestreview-771245637 I will make all the changes in one PR in the different commit, however some review of the works is very welcome.

   P.S: The PR will include also an additional method inside the test framework, to know more you can look inside https://github.com/bitcoin/bitcoin/pull/23127

10. 
    dougEfresh commented at 9:04 am on October 22, 2021: contributor

    I’m taking a look at this
11. 
    dougEfresh commented at 10:44 am on October 22, 2021: contributor

    oh, now I see @vincenzopalazzo remarks.
12. 
    viniciusdepadua commented at 5:23 am on November 6, 2021: none

    Is this issue still open? Could I help?
13. 
    vincenzopalazzo commented at 7:52 am on November 6, 2021: none

    Is this issue still open? Could I help?

    Maybe with some PR review?

14. 
    stickies-v commented at 8:29 pm on November 11, 2021: contributor

    Building on theStack’s initial script, instead of focusing on the (in)equality operators which are also present in the test_framework’s assert functions, I think it might be easier to just rely on the limited set of forms that Python’s built in assert statement can come in. To my knowledge, this is either assert <expression>... or assert (<expression>..., with a flexible number of spaces after assert.

    The below script should capture more problematic cases (e.g. this also includes boolean asserts) and exclude all of the false positives that arise when (in)equality operators are used in the test_framework assert functions.

    0cd ./test/functional
    1git grep "assert\( *\)("
    2git grep "assert \( *\)"
    

    edit: I assumed test_framework exposed assert_true and assert_false functions, similar to what e.g. unittest does which is needed for boolean asserts. @theStack would it make sense to add those to the test_framework? At the moment, if implemented in the same way as the other test_framework assert functions, it wouldn’t really add any value over the native assert. However, I think it would be better to have all asserts come from the same library so we can more easily and uniformly upgrade the way we assert going forward.

15. bitcoin deleted a comment on Nov 27, 2021
16. bitcoin deleted a comment on Mar 25, 2022
17. bitcoin deleted a comment on Mar 26, 2022
18. 
    NorrinRadd commented at 6:19 pm on October 3, 2022: none

    I will take this up to finish some of the last suggestions on the most recent PR
19. NorrinRadd referenced this in commit d3b7250e8f on Oct 20, 2022
20. NorrinRadd referenced this in commit ce71207e09 on Oct 20, 2022
21. NorrinRadd referenced this in commit 3630f3aed9 on Oct 27, 2022
22. NorrinRadd referenced this in commit 6534222196 on Oct 27, 2022
23. osagie98 referenced this in commit 1c67375a64 on Sep 25, 2023
24. osagie98 referenced this in commit 8c6d2f16db on Sep 25, 2023
25. 
    osagie98 commented at 5:18 am on September 25, 2023: none

    It looks like there hasn’t been any movement on this issue for a year, so I decided to give it a shot. Would really appreciate a review!
26. osagie98 referenced this in commit 14fd710d7b on Oct 6, 2023
27. osagie98 referenced this in commit 995cc4771e on Oct 6, 2023
28. osagie98 referenced this in commit ad6d9e2219 on Oct 12, 2023
29. osagie98 referenced this in commit 8f00a52b34 on Oct 24, 2023
30. osagie98 referenced this in commit 146d75c8d2 on Oct 28, 2023
31. osagie98 referenced this in commit 2103e12699 on Dec 11, 2023
32. 
    Numbers0689 commented at 10:52 am on May 30, 2024: none

    is the issue still relevant? can i work on it?

Contributors
theStack vincenzopalazzo meshcollider fawkesG5 dougEfresh viniciusdepadua stickies-v NorrinRadd osagie98 Numbers0689

Labels
good first issue