Tests: Add test for getdifficulty #10229

jimmysong · 2017-04-19T02:58:09Z

Test added to blockchain.py as adding a new test to reduce test run time.

paveljanik · 2017-04-19T14:21:44Z

test/functional/blockchain.py

@@ -57,7 +64,8 @@ def _test_gettxoutsetinfo(self):
    def _test_getblockheader(self):
        node = self.nodes[0]

-        assert_raises_jsonrpc(-5, "Block not found", node.getblockheader, "nonsense")
+        assert_raises_jsonrpc(-5, "Block not found",


Why this change?

flake8 warning: E501 line too long (85 > 79 characters)

My personal preference is to ignore the 'line too long' warning. Nothing wrong with going over 79 character lines in my opinion (but we don't have a style guide for functional tests, so entirely up to you)

paveljanik · 2017-04-19T14:27:08Z

test/functional/blockchain.py

        self.nodes[0].verifychain(4, 0)

+    def _test_getdifficulty(self):
+        res = self.nodes[0].getdifficulty()


Mention the getdifficulty RPC test in the top level comment or remove the list of tested methods there.

adding to the comment, thanks!

paveljanik · 2017-04-19T14:42:03Z

test/functional/blockchain.py

        self.nodes[0].verifychain(4, 0)

+    def _test_getdifficulty(self):


Can you move the def after the _test_getblockheader so it is ordered the same as it is called?

Yep, will do.

paveljanik · 2017-04-19T14:50:55Z

test/functional/blockchain.py

+        res = self.nodes[0].getdifficulty()
+        # 1 hash in 2 should be valid, so difficulty should be 1/2**31
+        # binary => decimal => binary math is why we round
+        assert_equal(round(res * 2**31, 4), 1.0)


Is there a cleverer way to do this?

jimmysong · 2017-04-19T15:03:09Z

Fixed issues, rebased to master.

paveljanik

ACK 821dd5e

jnewbery

Looks good. Tested ACK ba2b295093d5f5cad4701f9ca86b2c83e60c3ef9.

A few nits which you can take or leave.

jnewbery · 2017-04-19T21:39:24Z

test/functional/blockchain.py

@@ -6,6 +6,7 @@

 Test the following RPCs:
    - gettxoutsetinfo
+    - getdifficulty
    - verifychain


Might as well fix this docstring while you're at it. The following RPCs are also tested:

getbestblockhash

getblockhash

getblockheader

jnewbery · 2017-04-19T21:40:32Z

test/functional/blockchain.py

@@ -57,7 +64,8 @@ def _test_gettxoutsetinfo(self):
    def _test_getblockheader(self):
        node = self.nodes[0]

-        assert_raises_jsonrpc(-5, "Block not found", node.getblockheader, "nonsense")
+        assert_raises_jsonrpc(-5, "Block not found",


My personal preference is to ignore the 'line too long' warning. Nothing wrong with going over 79 character lines in my opinion (but we don't have a style guide for functional tests, so entirely up to you)

jnewbery · 2017-04-19T21:41:21Z

test/functional/blockchain.py

@@ -79,5 +82,11 @@ def _test_getblockheader(self):
        assert isinstance(int(header['versionHex'], 16), int)
        assert isinstance(header['difficulty'], Decimal)

+    def _test_getdifficulty(self):
+        res = self.nodes[0].getdifficulty()


I'd prefer a more descriptive name than res, perhaps difficulty? (or even combine with the line below)

Makes sense.

Test added to blockchain.py as adding a new test to reduce test run time.

jimmysong · 2017-04-19T21:54:22Z

Fixed nits and squashed.

821dd5e Tests: Add test for getdifficulty (Jimmy Song) Tree-SHA512: 3da78c4f88efdaab8374582cda606620beb2f1e9a93119a72b67572702c17c36b68c3abf9d466e8c7fb8ba9e8afa9a172e454c553df10d3054f19b3282d3097b

fanquake added the Tests label Apr 19, 2017

paveljanik suggested changes Apr 19, 2017

View reviewed changes

jimmysong force-pushed the test_getdifficulty branch from bfd615c to a75c80e Compare April 19, 2017 15:02

jimmysong force-pushed the test_getdifficulty branch 2 times, most recently from 8fd75de to ba2b295 Compare April 19, 2017 15:19

paveljanik approved these changes Apr 19, 2017

View reviewed changes

jnewbery reviewed Apr 19, 2017

View reviewed changes

jnewbery approved these changes Apr 19, 2017

View reviewed changes

Tests: Add test for getdifficulty

821dd5e

Test added to blockchain.py as adding a new test to reduce test run time.

jimmysong force-pushed the test_getdifficulty branch from ba2b295 to 821dd5e Compare April 19, 2017 21:53

laanwj merged commit 821dd5e into bitcoin:master Apr 21, 2017

dagurval mentioned this pull request Mar 5, 2018

blockchain.py cherries bitcoinxt/bitcoinxt#347

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		self.nodes[0].verifychain(4, 0)

		def _test_getdifficulty(self):

Tests: Add test for getdifficulty #10229

Tests: Add test for getdifficulty #10229

Conversation

jimmysong commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmysong Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmysong commented Apr 19, 2017

Uh oh!

paveljanik left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmysong commented Apr 19, 2017

Uh oh!

Uh oh!

jimmysong Apr 19, 2017 •

edited

Loading

paveljanik left a comment •

edited

Loading