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

gillichu · 2020-05-27T17:07:26Z

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:

Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py.
Extends the script.py CScriptNum test to trial larger numbers.

maflcko · 2020-05-27T17:18:59Z

Concept ACK

It would be good to motivate the change. Maybe something like this?

The scriptnum test (#14816) is a roundtrip test of the test framwork. 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

test/functional/test_framework/script.py

test/functional/mining_basic.py

glozow

Concept ACK, I suggest trying to undo some of your changes in mining_basic.py so that you aren't touching any extra lines. You could squash your commits to try to make it easier to see overall changes (you'll need to do this anyway).

There's also some description/naming conventions you could take a look at in the contributing doc as well. For example, adding [test] to the commit message and PR title.

test/functional/mining_basic.py

test/functional/test_framework/script.py

practicalswift · 2020-05-27T19:06:47Z

Concept ACK: more testing is better!

Welcome as a contributor @gillichu :)

DrahtBot · 2020-05-27T19:47:30Z

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

Conflicts

No conflicts as of last run.

maflcko

Almost ACK 9d5dd87

test/functional/mining_basic.py

laanwj · 2020-05-28T14:59:06Z

Thanks for you contribution. Looks good to me.

A small nit: please change the git commit message to a short title (up to say, 70 characters), then an empty line, then the rest of the description. This because tools (such as git pretty=oneline) will otherwise put it all on one line instead of displaying just the title:

9d5dd8752… (pull/19082/head) [test] Migrates the CScriptNum decode tests into a unit test, moving some changes made in #14816. This was made possible by the integration of test_framework unit testing in #18576.

test/functional/test_framework/script.py

jnewbery

Welcome to the project @gillichu, and thanks for your contribution ! I have a couple of small suggestions inline.

jnewbery · 2020-05-31T16:54:56Z

test/functional/mining_basic.py

        # round-trip the encoded bip34 block height commitment
        assert_equal(CScriptNum.decode(coinbase_tx.vin[0].scriptSig), next_height)


This seems to be a test of the create_coinbase() function in blocktools.py. It might be worth moving it to a unit test there.

test/functional/test_framework/script.py

jnewbery · 2020-05-31T17:00:07Z

A couple more suggestions for the commit log and PR description:

use the imperative mood (s/Moved the CScript/Move the CScript/ and s/Migrates the CScript/Migrate the CScript/).
wrap the message body width at 72 or 80 characters.

See https://chris.beams.io/posts/git-commit/ for more tips on writing great commit logs.

test/functional/test_framework/blocktools.py

glozow

Almost there

test/functional/test_framework/blocktools.py

glozow · 2020-05-31T18:23:01Z

test/functional/mining_basic.py

@@ -91,12 +91,6 @@ def assert_submitblock(block, result_str_1, result_str_2=None):
        coinbase_tx.rehash()

        # round-trip the encoded bip34 block height commitment


delete this comment?

This hasn't been deleted.

Sorry about that :( I'll watch my changes more carefully next time, and look for a chance to take this out in the future.

test/functional/test_runner.py

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.

gillichu · 2020-06-03T20:31:18Z

rebased the branch, addressed review comments and updated tests. thanks for the review @laanwj @gzhao408 @jnewbery comments have been addressed!

jnewbery · 2020-06-04T03:03:24Z

Thanks @gillichu. I think the PR title, commit summary and commit details could be made a bit clearer:

switch to indicative mood (eg "test: Moved the CScriptNum asserts into the unit test in script.py" -> "Add CScriptNum and create_coinbase unit tests", "Migrates the CScriptNum decode tests" -> "Migrate the CScriptNum decode tests", etc)
no need to include the complete history ("... changes made in Add CScriptNum decode python implementation in functional suite #14816", "... integration of test_framework unit testing in test: use unittest for test_framework unit testing #18576.").

laanwj · 2020-06-04T15:26:23Z

ACK 7daffc6
Looks good to me now!
Please keep the additional suggestions in mind for a next PR, but I don't think we should keep iterating on the commit message too much.

… 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

fa98e10 test: Remove leftover comment in mining_basic (MarcoFalke) faedb50 test: pep-8 mining_basic (MarcoFalke) Pull request description: Remove an accidental leftover comment from #19082, which no longer applies and thus might be confusing ACKs for top commit: adamjonas: code review ACK fa98e10 Tree-SHA512: c7f7f8f579b3c6e92f45769be0a7af1a421438a3f5524db5278b2269511a9e0e08f44e3836afb26727644035897ee51ff8296d13ce23030549e7403f57b40e40

fa98e10 test: Remove leftover comment in mining_basic (MarcoFalke) faedb50 test: pep-8 mining_basic (MarcoFalke) Pull request description: Remove an accidental leftover comment from bitcoin#19082, which no longer applies and thus might be confusing ACKs for top commit: adamjonas: code review ACK fa98e10 Tree-SHA512: c7f7f8f579b3c6e92f45769be0a7af1a421438a3f5524db5278b2269511a9e0e08f44e3836afb26727644035897ee51ff8296d13ce23030549e7403f57b40e40

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

gillichu changed the title ~~Moved the CScriptNum asserts into the unit test in script.py~~ [WIP]. Moved the CScriptNum asserts into the unit test in script.py May 27, 2020

gillichu changed the title ~~[WIP]. Moved the CScriptNum asserts into the unit test in script.py~~ [WIP] Moved the CScriptNum asserts into the unit test in script.py May 27, 2020

maflcko reviewed May 27, 2020

View reviewed changes

test/functional/test_framework/script.py Outdated Show resolved Hide resolved

gillichu marked this pull request as draft May 27, 2020 17:24

maflcko reviewed May 27, 2020

View reviewed changes

test/functional/mining_basic.py Show resolved Hide resolved

DrahtBot added the Tests label May 27, 2020

glozow reviewed May 27, 2020

View reviewed changes

test/functional/mining_basic.py Show resolved Hide resolved

test/functional/test_framework/script.py Outdated Show resolved Hide resolved

test/functional/test_framework/script.py Outdated Show resolved Hide resolved

test/functional/test_framework/script.py Outdated Show resolved Hide resolved

gillichu force-pushed the issue24 branch from fba67c6 to 8a04208 Compare May 27, 2020 22:41

gillichu changed the title ~~[WIP] Moved the CScriptNum asserts into the unit test in script.py~~ [WIP] [test] Moved the CScriptNum asserts into the unit test in script.py May 27, 2020

gillichu force-pushed the issue24 branch from 8a04208 to 9d5dd87 Compare May 27, 2020 22:50

DrahtBot mentioned this pull request May 27, 2020

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

Closed

4 tasks

gillichu changed the title ~~[WIP] [test] Moved the CScriptNum asserts into the unit test in script.py~~ test: Moved the CScriptNum asserts into the unit test in script.py May 28, 2020

gillichu marked this pull request as ready for review May 28, 2020 04:09

maflcko approved these changes May 28, 2020

View reviewed changes

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

maflcko mentioned this pull request May 28, 2020

Write linter to check that commit messages have a new line before the body (or no body) #19091

Closed

gillichu force-pushed the issue24 branch 4 times, most recently from c368d24 to 401487d Compare May 28, 2020 23:53

glozow reviewed May 30, 2020

View reviewed changes

test/functional/test_framework/script.py Outdated Show resolved Hide resolved

gillichu force-pushed the issue24 branch from 401487d to ef4cce1 Compare May 31, 2020 02:58

jnewbery reviewed May 31, 2020

View reviewed changes

gillichu force-pushed the issue24 branch from ef4cce1 to 58c4a30 Compare May 31, 2020 17:42

gillichu commented May 31, 2020

View reviewed changes

test/functional/test_framework/blocktools.py Outdated Show resolved Hide resolved

glozow reviewed May 31, 2020

View reviewed changes

bitcoin deleted a comment from kantornnam Jun 1, 2020

jnewbery reviewed Jun 1, 2020

View reviewed changes

test/functional/test_runner.py Show resolved Hide resolved

gillichu force-pushed the issue24 branch from 58c4a30 to 63cba4b Compare June 1, 2020 22:19

DrahtBot added the Needs rebase label Jun 1, 2020

gillichu force-pushed the issue24 branch 2 times, most recently from 91c0820 to 6092784 Compare June 2, 2020 05:30

DrahtBot removed the Needs rebase label Jun 2, 2020

gillichu force-pushed the issue24 branch from 6092784 to 7daffc6 Compare June 3, 2020 14:18

laanwj merged commit 39afe5b into bitcoin:master Jun 4, 2020

maflcko mentioned this pull request Jun 8, 2020

test: Remove leftover comment in mining_basic #19206

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

		# round-trip the encoded bip34 block height commitment
		assert_equal(CScriptNum.decode(coinbase_tx.vin[0].scriptSig), next_height)

		@@ -91,12 +91,6 @@ def assert_submitblock(block, result_str_1, result_str_2=None):
		coinbase_tx.rehash()

		# round-trip the encoded bip34 block height commitment

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

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

Uh oh!

Conversation

gillichu commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented May 27, 2020

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented May 27, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented May 28, 2020

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery May 31, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented May 31, 2020

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glozow May 31, 2020

Choose a reason for hiding this comment

Uh oh!

jnewbery Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

gillichu Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gillichu commented Jun 3, 2020

Uh oh!

jnewbery commented Jun 4, 2020

Uh oh!

laanwj commented Jun 4, 2020

Uh oh!

Uh oh!

gillichu commented May 27, 2020 •

edited

Loading

DrahtBot commented May 27, 2020 •

edited

Loading