Sitelet https://web.archive.org/web/20211227114953/https://github.com/bitcoin/bitcoin/issues/23119
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

Jump to bottom

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

Open
theStack opened this issue Sep 28, 2021 · 11 comments · May be fixed by #23127
Open

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

theStack opened this issue Sep 28, 2021 · 11 comments · May be fixed by #23127
Labels
good first issue

Comments

@theStack
Copy link
Contributor

@theStack theStack commented Sep 28, 2021 •
edited

#23117 replaced asserts with the test framework's internal helpers (see

bitcoin/test/functional/test_framework/util.py

Lines 47 to 59 in dccf3d2

def assert_equal(thing1, thing2, *args):
if thing1 != thing2 or any(thing1 != arg for arg in args):
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
def assert_greater_than(thing1, thing2):
if thing1 <= thing2:
raise AssertionError("%s <= %s" % (str(thing1), str(thing2)))
def assert_greater_than_or_equal(thing1, thing2):
if thing1 < thing2:
raise AssertionError("%s < %s" % (str(thing1), str(thing2)))
) 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

$ cd ./test/functional
$ git grep assert.*==
$ git grep "assert.*<="
$ git grep "assert.*>="
$ git grep "assert.*<"
$ 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.
@vincenzopalazzo
Copy link

@vincenzopalazzo vincenzopalazzo commented Sep 28, 2021

I can take the lock on this issue?

@meshcollider
Copy link
Member

@meshcollider meshcollider commented Sep 29, 2021

@vincenzopalazzo feel free to open a pull request :)

@vincenzopalazzo vincenzopalazzo linked a pull request that will close this issue Sep 29, 2021
Open
@brunoerg brunoerg mentioned this issue Sep 29, 2021
Closed
@bitcoin bitcoin deleted a comment from 123jc1 Sep 30, 2021
@fawkesG5
Copy link

@fawkesG5 fawkesG5 commented Oct 8, 2021

Hi ! Can I take up this issue?

@vincenzopalazzo
Copy link

@vincenzopalazzo vincenzopalazzo commented Oct 8, 2021

@fawkesG5 #23127

@fawkesG5
Copy link

@fawkesG5 fawkesG5 commented Oct 8, 2021

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?

@vincenzopalazzo
Copy link

@vincenzopalazzo vincenzopalazzo commented Oct 8, 2021

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 (review) 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 #23127

@dougEfresh
Copy link
Contributor

@dougEfresh dougEfresh commented Oct 22, 2021

I'm taking a look at this

@dougEfresh
Copy link
Contributor

@dougEfresh dougEfresh commented Oct 22, 2021

oh, now I see @vincenzopalazzo remarks.

@dougEfresh dougEfresh mentioned this issue Oct 22, 2021
Closed
@viniciusdepadua
Copy link

@viniciusdepadua viniciusdepadua commented Nov 6, 2021

Is this issue still open? Could I help?

@vincenzopalazzo
Copy link

@vincenzopalazzo vincenzopalazzo commented Nov 6, 2021

Is this issue still open? Could I help?

Maybe with some PR review?

@stickies-v
Copy link

@stickies-v stickies-v commented Nov 11, 2021 •
edited

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.

cd ./test/functional
git grep "assert\( *\)("
git 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.

@HelpCombot HelpCombot mentioned this issue Nov 24, 2021
Closed
@bitcoin bitcoin deleted a comment from Wonders11 Nov 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

tests: Use test framework utils where possible
8 participants
@theStack @dougEfresh @meshcollider @MarcoFalke @vincenzopalazzo @fawkesG5 @viniciusdepadua @stickies-v