Skip to content

Tests: Add test for getdifficulty #10229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 1 commit into from
Apr 21, 2017
Merged

Tests: Add test for getdifficulty #10229

laanwj merged 1 commit into from
Apr 21, 2017

Conversation

@jimmysong
Copy link
Contributor

@jimmysong jimmysong commented Apr 19, 2017

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

@fanquake fanquake added the Tests label Apr 19, 2017
paveljanik
paveljanik suggested changes Apr 19, 2017
View reviewed changes
test/functional/blockchain.py Outdated
Copy link
Contributor

@paveljanik paveljanik Apr 19, 2017

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

@jimmysong jimmysong Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jnewbery jnewbery Apr 19, 2017

Choose a reason for hiding this comment

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

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)

test/functional/blockchain.py Outdated
Copy link
Contributor

@paveljanik paveljanik Apr 19, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jimmysong jimmysong Apr 19, 2017

Choose a reason for hiding this comment

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

adding to the comment, thanks!

test/functional/blockchain.py Outdated
Copy link
Contributor

@paveljanik paveljanik Apr 19, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jimmysong jimmysong Apr 19, 2017

Choose a reason for hiding this comment

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

Yep, will do.

test/functional/blockchain.py Outdated
Copy link
Contributor

@paveljanik paveljanik Apr 19, 2017

Choose a reason for hiding this comment

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

Ugh.

Copy link
Contributor Author

@jimmysong jimmysong Apr 19, 2017

Choose a reason for hiding this comment

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

Is there a cleverer way to do this?

@jimmysong jimmysong force-pushed the test_getdifficulty branch from bfd615c to a75c80e Compare April 19, 2017 15:02
@jimmysong
Copy link
Contributor Author

jimmysong commented Apr 19, 2017

Fixed issues, rebased to master.

@jimmysong jimmysong force-pushed the test_getdifficulty branch 2 times, most recently from 8fd75de to ba2b295 Compare April 19, 2017 15:19
paveljanik
paveljanik approved these changes Apr 19, 2017
View reviewed changes
Copy link
Contributor

@paveljanik paveljanik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK 821dd5e

jnewbery
jnewbery reviewed Apr 19, 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.

Looks good. Tested ACK ba2b295093d5f5cad4701f9ca86b2c83e60c3ef9.

A few nits which you can take or leave.

test/functional/blockchain.py Outdated
Copy link
Contributor

@jnewbery jnewbery Apr 19, 2017

Choose a reason for hiding this comment

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

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

  • getbestblockhash
  • getblockhash
  • getblockheader

Copy link
Contributor Author

@jimmysong jimmysong Apr 19, 2017

Choose a reason for hiding this comment

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

good idea.

test/functional/blockchain.py Outdated
Copy link
Contributor

@jnewbery jnewbery Apr 19, 2017

Choose a reason for hiding this comment

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

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)

test/functional/blockchain.py Outdated
Copy link
Contributor

@jnewbery jnewbery Apr 19, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jimmysong jimmysong Apr 19, 2017

Choose a reason for hiding this comment

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

Makes sense.

@jimmysong
Tests: Add test for getdifficulty
821dd5e 
Test added to blockchain.py as adding a new test to reduce test run time.
@jimmysong jimmysong force-pushed the test_getdifficulty branch from ba2b295 to 821dd5e Compare April 19, 2017 21:53
@jimmysong
Copy link
Contributor Author

jimmysong commented Apr 19, 2017

Fixed nits and squashed.

@laanwj laanwj merged commit 821dd5e into bitcoin:master Apr 21, 2017
laanwj added a commit that referenced this pull request Apr 21, 2017
@laanwj
Merge #10229: Tests: Add test for getdifficulty
f3db4c6 
821dd5e Tests: Add test for getdifficulty (Jimmy Song)

Tree-SHA512: 3da78c4f88efdaab8374582cda606620beb2f1e9a93119a72b67572702c17c36b68c3abf9d466e8c7fb8ba9e8afa9a172e454c553df10d3054f19b3282d3097b
@dagurval dagurval mentioned this pull request Mar 5, 2018
Merged
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 8, 2019
@laanwj @PastaPastaPasta
Merge bitcoin#10229: Tests: Add test for getdifficulty
6fb2866 
821dd5e Tests: Add test for getdifficulty (Jimmy Song)

Tree-SHA512: 3da78c4f88efdaab8374582cda606620beb2f1e9a93119a72b67572702c17c36b68c3abf9d466e8c7fb8ba9e8afa9a172e454c553df10d3054f19b3282d3097b
@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

2 more reviewers

@jnewbery jnewbery jnewbery approved these changes

@paveljanik paveljanik paveljanik approved these changes

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

@jimmysong @jnewbery @paveljanik @laanwj @fanquake