test: use unittest for test_framework unit testing #18576

glozow · 2020-04-09T18:20:17Z

Proposal for unit testing on test_framework functions:

Use the python unittest library. Don't use test_framework to test itself.
Put the tests inside the same file as the functions they are testing.
Call the tests from test_runner.py. To include more Test Framework tests, add the filename to the list TEST_FRAMEWORK_MODULES. Don't add new files or change the list of accepted script prefixes.

Makes these changes for bn2vch (followup to this comment).

DrahtBot · 2020-04-09T21:07:50Z

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

Conflicts

No conflicts as of last run.

jnewbery · 2020-04-11T00:30:50Z

Concept ACK!

@sipa @MarcoFalke you may also be interested in this, since we all discussed this in #18378.

jnewbery

Concept and approach ACK. Thanks for tackling this, @gzhao408 .

I left a couple of comments inline. A few more ideas:

Perhaps print something immediately before the unit tests to indicate what's happening. Something like "Running test framework unit tests".
The test runner should probably exit with an error state if any of the unit tests fail (to cause travis to fail immediately).

test/functional/test_runner.py

jnewbery · 2020-04-13T23:09:57Z

Here are some tips for writing great commit messages: https://chris.beams.io/posts/git-commit/. Particularly look at section 7 (why-not-how), and make sure to include a blank line between the summary and details.

practicalswift · 2020-04-21T15:05:44Z

Concept ACK

Thanks for tackling this. Great initiative! :)

jnewbery · 2020-04-21T18:12:40Z

This is looking really good @gzhao408 . I recommend that in this PR you just add the unit test framework to test_runner.py and move the script tests to script.py, and then open follow-up PRs to add the key.py and messages.py tests.

Test the test_framework, but don't use test_framework objects or functions to test itself Use python unittest library and put test_framework's unit tests inside their respective files Add the filename to TEST_FRAMEWORK_MODULES in test_runner Aggregate all test_framework tests into one TestSuite to run before the functional tests in test_runner Delete framework_test_script, move test_bn2vch to script.py and add to TEST_FRAMEWORK_MODULES in test_runner

glozow · 2020-04-26T20:34:59Z

Ready for Review if anyone wants to take a look :) addressed jnewbery's comments.

It looks like travis failed on wallet_bumpfee.py on the last push. I just passed it locally and can't figure out why it's failing, so I'm having it run again. Will look again in a bit...

jnewbery · 2020-04-29T23:55:50Z

Tested ACK de8905a. Great stuff gzhao408 . Thanks for this!

For your next PR, try to summarize the why instead of the how in your commit log. People can look at the code to see what changes you've made, such as which files you've deleted. The log should summarize and give motivation.

glozow · 2020-04-29T23:58:22Z

Thanks so much for the review @jnewbery!

For your next PR, try to summarize the why instead of the how in your commit log. People can look at the code to see what changes you've made, such as which files you've deleted. The log should summarize and give motivation.

Ah, gotcha! Thanks for the feedback :) will do.

de8905a test: use unittest and test_runner for test framework unit testing (Gloria Zhao) Pull request description: Proposal for unit testing on test_framework functions: 1. Use the python `unittest` library. Don't use test_framework to test itself. 2. Put the tests inside the same file as the functions they are testing. 3. Call the tests from `test_runner.py`. To include more Test Framework tests, add the filename to the list `TEST_FRAMEWORK_MODULES`. Don't add new files or change the list of accepted script prefixes. Makes these changes for `bn2vch` (followup to [this comment](bitcoin#18378 (review))). ACKs for top commit: jnewbery: Tested ACK de8905a. Great stuff gzhao408 . Thanks for this! Tree-SHA512: 91572d43e203a1864765b93a9472667994115ec38b271f2b2f9fcd0f0112b393fc24ba7d2371d5a34b0a6a4522f6b934fc5164363819aa7ed8d6c6c9a60cc101

…some changes made in bitcoin#14816. This was made possible by the integration of test_framework unit testing in bitcoin#18576.

Migrates the CScriptNum decode tests into a unit test, moving some changes made in bitcoin#14816. This was made possible by the integration of test_framework unit testing in bitcoin#18576.

Migrates the CScriptNum decode tests into a unit test, moving some changes made in bitcoin#14816. This was made possible by the integration of test_framework unit testing in bitcoin#18576. Also adds a unit test testing the decode method with larger ints, similar to the scriptnum_tests.cpp file.

Migrates the CScriptNum decode tests into a unit test, moving some changes made in bitcoin#14816. This was made possible by the integration of test_framework unit testing in bitcoin#18576. Also extends the original test with larger ints, similar to the scriptnum_tests.cpp file.

Migrates the CScriptNum decode tests into a unit test, and moved some changes made in bitcoin#14816. Made possible by the integration of test_framework unit testing in bitcoin#18576. Further extends the original test with larger ints, similar to the scriptnum_tests.cpp file. Adds test to blocktools.py testing fn create_coinbase() with CScriptNum decode.

…n script.py 7daffc6 [test] CScriptNum Decode Check as Unit Tests (Gillian Chu) Pull request description: The CScriptNum test (#14816) is a roundtrip test of the test framework. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See #18576. This PR: 1. Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py. 2. Extends the script.py CScriptNum test to trial larger numbers. ACKs for top commit: laanwj: ACK 7daffc6 Tree-SHA512: 17a04a4bfff1b1817bfc167824c679455d9e06e6e0164c00a7e44f8aa5041c5f5080adcc1452fd80ba1a6d8409f976c982bc481d686c434edf97a5893a32a436

… test in script.py 7daffc6 [test] CScriptNum Decode Check as Unit Tests (Gillian Chu) Pull request description: The CScriptNum test (bitcoin#14816) is a roundtrip test of the test framework. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See bitcoin#18576. This PR: 1. Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py. 2. Extends the script.py CScriptNum test to trial larger numbers. ACKs for top commit: laanwj: ACK 7daffc6 Tree-SHA512: 17a04a4bfff1b1817bfc167824c679455d9e06e6e0164c00a7e44f8aa5041c5f5080adcc1452fd80ba1a6d8409f976c982bc481d686c434edf97a5893a32a436

Migrates the CScriptNum decode tests into a unit test, and moved some changes made in bitcoin#14816. Made possible by the integration of test_framework unit testing in bitcoin#18576. Further extends the original test with larger ints, similar to the scriptnum_tests.cpp file. Adds test to blocktools.py testing fn create_coinbase() with CScriptNum decode.

Summary: > Test the test_framework, but don't use test_framework objects or functions to test itself > > Use python `unittest` library and put test_framework's unit tests inside their respective files > Add the filename to `TEST_FRAMEWORK_MODULES` in `test_runner` > Aggregate all `test_framework` tests into one `TestSuite` to run before the functional tests in test_runner > Delete `framework_test_script`, move `test_bn2vch` to `script.py` and add to `TEST_FRAMEWORK_MODULES` in test_runner This is a backport of Core [[bitcoin/bitcoin#18576 | PR18576]] Test Plan: ninja check-functional Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9068

Summary: > Migrates the CScriptNum decode tests into a unit test, and moved some > changes made in [[bitcoin/bitcoin#14816 | core#14816]]. Made possible by the integration of > test_framework unit testing in [[bitcoin/bitcoin#18576 | core#18576]]. Further extends the original > test with larger ints, similar to the scriptnum_tests.cpp file. Adds > test to blocktools.py testing fn create_coinbase() with CScriptNum > decode. Backport note: The functional tests are usually executed individually via a `subprocess`. This means the `get_srcdir()` function in cdefs.py is able to find the proper source directory, because the parent directory of the python process is the project root that contains `src/`. But when we run the new test_framework unit test, the process currently running is `test_runner.py` itself which is located in the build directory. So cdefs.py finds the wrong `src/` directory, the one located in the build directory, which does not contain `consensus.h`. I fixed this by setting the environment variable `SRCDIR` in `test_runner.py` before calling the unittest `unittest.TestLoader().loadTestsFromName` machinery which causes `cdefs.py` to be imported. This is a backport of [[bitcoin/bitcoin#19082 | core#19082]] Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9276

DrahtBot added the Tests label Apr 9, 2020

DrahtBot mentioned this pull request Apr 9, 2020

Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #17977

Closed

4 tasks

glozow force-pushed the framework-unittests branch 3 times, most recently from e23585d to 7a5a8a7 Compare April 10, 2020 01:22

fanquake changed the title ~~wip [test] use unittest for test_framework unit testing~~ [wip] test: use unittest for test_framework unit testing Apr 11, 2020

fanquake requested a review from jnewbery April 11, 2020 00:16

glozow force-pushed the framework-unittests branch 3 times, most recently from ef8a510 to 6cc8964 Compare April 11, 2020 22:07

DrahtBot mentioned this pull request Apr 12, 2020

Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528

Merged

jnewbery reviewed Apr 13, 2020

View reviewed changes

test/functional/test_runner.py Outdated Show resolved Hide resolved

test/functional/test_runner.py Outdated Show resolved Hide resolved

glozow force-pushed the framework-unittests branch from 9a36429 to 91bbb49 Compare April 21, 2020 19:07

glozow changed the title ~~[wip] test: use unittest for test_framework unit testing~~ test: use unittest for test_framework unit testing Apr 21, 2020

glozow marked this pull request as ready for review April 21, 2020 19:13

glozow force-pushed the framework-unittests branch from 91bbb49 to be5fff5 Compare April 21, 2020 19:20

glozow force-pushed the framework-unittests branch from be5fff5 to de8905a Compare April 26, 2020 20:32

maflcko merged commit a66ba6d into bitcoin:master Apr 30, 2020

This was referenced May 13, 2020

tests: implement base58_decode #18965

Merged

test: Make scriptnum test a unit test maflcko/bitcoin-core#24

Closed

glozow deleted the framework-unittests branch May 25, 2020 20:31

maflcko mentioned this pull request May 27, 2020

test: Moved the CScriptNum asserts into the unit test in script.py #19082

Merged

jnewbery mentioned this pull request Jun 9, 2020

Add Muhash3072 implementation in Python #19105

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: use unittest for test_framework unit testing #18576

test: use unittest for test_framework unit testing #18576

Uh oh!

glozow commented Apr 9, 2020 •

edited

Loading

Uh oh!

DrahtBot commented Apr 9, 2020 •

edited

Loading

Uh oh!

jnewbery commented Apr 11, 2020

Uh oh!

jnewbery left a comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Apr 13, 2020

Uh oh!

practicalswift commented Apr 21, 2020

Uh oh!

jnewbery commented Apr 21, 2020 •

edited

Loading

Uh oh!

glozow commented Apr 26, 2020

Uh oh!

jnewbery commented Apr 29, 2020 •

edited

Loading

Uh oh!

glozow commented Apr 29, 2020 •

edited

Loading

Uh oh!

Uh oh!

test: use unittest for test_framework unit testing #18576

test: use unittest for test_framework unit testing #18576

Uh oh!

Conversation

glozow commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Apr 11, 2020

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Apr 13, 2020

Uh oh!

practicalswift commented Apr 21, 2020

Uh oh!

jnewbery commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Apr 26, 2020

Uh oh!

jnewbery commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

glozow commented Apr 9, 2020 •

edited

Loading

DrahtBot commented Apr 9, 2020 •

edited

Loading

jnewbery commented Apr 21, 2020 •

edited

Loading

jnewbery commented Apr 29, 2020 •

edited

Loading

glozow commented Apr 29, 2020 •

edited

Loading