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

tests: Use test framework utils where possible #23127

Closed

Conversation

vincenzopalazzo
Copy link

@vincenzopalazzo vincenzopalazzo commented Sep 29, 2021

This PR removes when it is possible the native python assert, and use the test utils function, like assert_equal, assert_greater_than, assert_greater_than_or_equal.

Update

In addition, this PR includes a new operator assert_not_equal, which IMO it is useful to test other conditions and add the error message as a parameter of the function to give more flexibility.

Fixes #23119

Edit laanwj: removed HTML comment from post

@fanquake fanquake added the Tests label Sep 29, 2021
@meshcollider
Copy link
Contributor

Thanks for contributing and helping out @vincenzopalazzo!

@brunoerg
Copy link
Contributor

Concept ACK

@brunoerg
Copy link
Contributor

Will it be created new PRs to update other tests or should we concentrate all the changes here?

@vincenzopalazzo
Copy link
Author

Will it be created new PRs to update other tests or should we concentrate all the changes here?

The plan is to divide the work into small chunks to make the price of PR review easy and fast.

@maflcko maflcko changed the title tests: Use test utils each time that it is possible tests: Use assert_equal* utils where possible Sep 29, 2021
@brunoerg
Copy link
Contributor

Will it be created new PRs to update other tests or should we concentrate all the changes here?

The plan is to divide the work into small chunks to make the price of PR review easy and fast.

Cool, thanks!

@brunoerg
Copy link
Contributor

I just created #23135 to help on it

@vincenzopalazzo
Copy link
Author

I just created #23135 to help on it

Thanks, the important think it that we don't make the double work :-)

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 29, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25732 (tests: Use test utils each time that it is possible by vincenzopalazzo)
  • #25291 (test: Refactor rpc_fundrawtransaction.py by akankshakashyap)
  • #23502 (wallet: Avoid underpaying transaction fees when signing taproot spends by achow101)
  • #20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
  • #17794 (test: Use assert_greater_than_or_equal helper method to aid debugging. by jbampton)
  • #16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

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.

@michaelfolkson
Copy link
Contributor

Concept ACK

I think generally splitting across multiple pull requests works best for the monster sub-projects (AssumeUTXO, deglobalizing ChainstateManager, Process Separation etc).

In this case splitting across multiple commits within the same PR would probably have been sufficient to make it easily reviewable but not a big deal and if it encourages multiple contributors to self-organize around completing it then that's great.

@vincenzopalazzo vincenzopalazzo force-pushed the tests/use_testutils branch 2 times, most recently from a83d95b to 6fbb7e1 Compare October 5, 2021 09:40
@vincenzopalazzo
Copy link
Author

Thanks @michaelfolkson,

In this case splitting across multiple commits within the same PR would probably have been sufficient to make it easily reviewable but not a big deal and if it encourages multiple contributors to self-organize around completing it then that's great.

It sounds great to me, I will go with this solution. Thansk!

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>

# Title: 

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@fanquake
Copy link
Member

fanquake commented Oct 3, 2022

@vincenzopalazzo Given #25732, did you want to leave this one open as well, or close for now?

@vincenzopalazzo
Copy link
Author

better close this and hope that someone will finish the job :)

@bitcoin bitcoin locked and limited conversation to collaborators Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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