Proposal for unit testing on test_framework functions:
unittest library. Don't use test_framework to test itself.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).
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
Concept ACK! @sipa @MarcoFalke you may also be interested in this, since we all discussed this in #18378.
23 | @@ -24,6 +24,10 @@ 24 | import tempfile 25 | import re 26 | import logging 27 | +import unittest 28 | +import test_framework.script
It seems a shame to have to list the modules out multiple times. If you use the loadTestsFromName() method, you can avoid having to import the modules:
TEST_FRAMEWORK_MODULES = [
"script",
"key",
"messages",
]
...
# Test Framework Tests
for module in TEST_FRAMEWORK_MODULES:
suite = unittest.TestLoader().loadTestsFromName("test_framework.{}".format(module))
unittest.TextTestRunner(verbosity=1).run(suite)
If you wanted to be really clever, you could just search all the modules for which ones import unittest and then automatically pick them up in the test runner, so they don't need to be listed out at all. I don't think that's necessary in this PR, but you might want to consider it in a follow-up.
Ahhh yes this is what I wanted to do! Ran into a bunch of import errors when I tried to auto discover everything, will look into it further.
392 | @@ -384,6 +393,11 @@ def run_tests(*, test_list, src_dir, build_dir, tmpdir, jobs=1, enable_coverage= 393 | if os.path.isdir(cache_dir): 394 | print("%sWARNING!%s There is a cache directory here: %s. If tests fail unexpectedly, try deleting the cache directory." % (BOLD[1], BOLD[0], cache_dir)) 395 | 396 | + # Test Framework Tests 397 | + for module in TEST_FRAMEWORK_MODULES: 398 | + suite = unittest.TestLoader().loadTestsFromModule(module) 399 | + unittest.TextTestRunner(verbosity=2).run(suite)
It'd be nice if we could have less verbosity for successful unittest runs, and only have verbose output for tests that fail. Otherwise, these will take up a lot of screen space for every run.
Good point, thank you so much for all the feedback!
Verbosity=1 does the least printing, but still prints every time TestCase.run() is called, so I made it more concise by grouping all the tests into one TestSuite and calling run() just once instead of doing them each individually. These tests would probably all be very small so I figured it would be alright. On success, it's 4 lines. On fail, it's more verbose and quits early. Hopefully this is ok?
Concept and approach ACK. Thanks for tackling this, @gzhao408 .
I left a couple of comments inline. A few more ideas:
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.
Concept ACK
Thanks for tackling this. Great initiative! :)
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
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...
Tested ACK de8905adf204c42bba810802f82b98f7b3dd26dc. 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.
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.