Skip to content

[tests] Add getmininginfo test#11150

Merged
maflcko merged 1 commit intobitcoin:masterfrom
mess110:test_rpc_getmininginfo
Aug 29, 2017
Merged

[tests] Add getmininginfo test#11150
maflcko merged 1 commit intobitcoin:masterfrom
mess110:test_rpc_getmininginfo

Conversation

@mess110
Copy link
Contributor

@mess110 mess110 commented Aug 25, 2017 •
edited
Loading

Add getmininginfo functional test in mining.py

promag
promag reviewed Aug 25, 2017
View reviewed changes
test/functional/rpc_getmininginfo.py Outdated
Copy link
Contributor

@promag promag Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline.

test/functional/rpc_getmininginfo.py Outdated
Copy link
Contributor

@promag promag Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline.

test/functional/rpc_getmininginfo.py Outdated
Copy link
Contributor

@promag promag Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline.

test/functional/rpc_getmininginfo.py Outdated
Copy link
Contributor

@promag promag Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline.

test/functional/rpc_getmininginfo.py Outdated
Copy link
Contributor

@promag promag Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline.

test/functional/rpc_getmininginfo.py Outdated
Copy link
Contributor

@promag promag Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline.

test/functional/rpc_getmininginfo.py Outdated
Copy link
Contributor

@promag promag Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't create self.mining_info, pass to functions below?

test/functional/rpc_getmininginfo.py Outdated
Copy link
Contributor

@promag promag Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are few with _ prefix, remove?

test/functional/rpc_getmininginfo.py Outdated
Copy link
Contributor

@promag promag Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline below?

test/functional/rpc_getmininginfo.py Outdated
Copy link
Contributor

@promag promag Aug 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same a above?

@fanquake fanquake added the Tests label Aug 25, 2017
@mess110
Copy link
Contributor Author

mess110 commented Aug 25, 2017 •
edited
Loading

@promag thanks for taking a look. Obviously won't leave 2 new lines in the future 😄

jnewbery
jnewbery reviewed Aug 28, 2017
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @mess110 . As an alternative to making this an individual test case (which adds a few seconds overhead to running the test suite), you could add this as a sub-test at the top of the mining.py test script.

Whether or not to add a new test script or extend an old one is a judgement call. In this case, I think the getmininginfo RPC can be run at the start of mining.py without any side-effects so it's a good candidate for extending the existing test.

test/functional/rpc_getmininginfo.py Outdated
Copy link
Contributor

@jnewbery jnewbery Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supernit: place imports in PEP-8 ordering (std library first, then project imports)

test/functional/rpc_getmininginfo.py Outdated
Copy link
Contributor

@jnewbery jnewbery Aug 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The This is a pre-release test build - use at your own risk error is only presented on non-release branches, so this assert will fail on all release branches. Just omit it.

maflcko
maflcko reviewed Aug 28, 2017
View reviewed changes
test/functional/rpc_getmininginfo.py Outdated
Copy link
Member

@maflcko maflcko Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to assert for keys in our tests. Python has you covered by throwing a KeyError in case you access a key that does not exist.

@mess110 mess110 changed the title [tests] Add getmininginfo functional tests [tests] Add getmininginfo test Aug 28, 2017
@mess110
Copy link
Contributor Author

mess110 commented Aug 28, 2017

@jnewbery thanks. I initially started in mining.py but I decided not to put it there because it would be "cleaner" to have its own test. In this particular case, I agree, the extra time to setup the test is not worth it

@MarcoFalke thanks for the review and hint

@jnewbery
Copy link
Contributor

jnewbery commented Aug 28, 2017

Looks good to me. We could test the actual values returned by getmininginfo more thoroughly (ie are the values for size, weight, hashps, etc correct under different circumstances) in a different PR. Testing the interface correctness in this PR is better than no testing at all.

Tested ACK 4f2905b

promag
promag reviewed Aug 28, 2017
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 4f2905b.

test/functional/mining.py
import copy
from binascii import b2a_hex
from decimal import Decimal

Copy link
Contributor

@promag promag Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, remove empty lines?

Copy link
Contributor

@promag promag Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore as per PEP-8.

@maflcko maflcko merged commit 4f2905b into bitcoin:master Aug 29, 2017
maflcko pushed a commit that referenced this pull request Aug 29, 2017
Merge #11150: [tests] Add getmininginfo test
a90e6d2 
4f2905b Add getmininginfo functional test (Cristian Mircea Messel)

Pull request description:

  Add `getmininginfo` functional test in `mining.py`

Tree-SHA512: 12be9cfb37e9ac4c6625fc06051704c8a8dfd7271c2654f994c7659c8810e4b7a4335105ae159315308bcd45b65589bab1829bd134d2f4cabf74d63f2e5d22fe
@mess110 mess110 deleted the test_rpc_getmininginfo branch August 29, 2017 08:44
@sipa sipa mentioned this pull request Sep 11, 2017
Merged
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 3, 2017
@mess110
Add getmininginfo functional test
847c75e 
Github-Pull: bitcoin#11150
Rebased-From: 4f2905b
codablock pushed a commit to codablock/dash that referenced this pull request Sep 24, 2019
@codablock
Merge bitcoin#11150: [tests] Add getmininginfo test
37250c0 
4f2905b Add getmininginfo functional test (Cristian Mircea Messel)

Pull request description:

  Add `getmininginfo` functional test in `mining.py`

Tree-SHA512: 12be9cfb37e9ac4c6625fc06051704c8a8dfd7271c2654f994c7659c8810e4b7a4335105ae159315308bcd45b65589bab1829bd134d2f4cabf74d63f2e5d22fe
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@jnewbery jnewbery jnewbery left review comments

@promag promag promag left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mess110 @jnewbery @promag @maflcko @fanquake