Skip to content

[qa] Bug fixes and refactor #7778

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
merged 4 commits into from
Apr 3, 2016
Merged

[qa] Bug fixes and refactor #7778

merged 4 commits into from
Apr 3, 2016

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Mar 31, 2016

It is not possible to switch to py3 without fixing those bugs or doing the refactor. (E.g. python2 will silently cast floats to ints where needed, but I'd rather explicitly use ints)

To aid review, each commit in this pull will either fix bugs or do some refactor.

@maflcko maflcko force-pushed the Mf1604-qaFixesRefactor branch from fa1d203 to faaa3c9 Compare April 1, 2016 19:24
@maflcko
Copy link
Member Author

maflcko commented Apr 1, 2016

Extended tests pass and enabling python deprecation warnings only give the following "false positive" warnings:

$ qa/pull-tester/rpc-tests.py -extended | grep -C 1 DeprecationWarning
/home/marco/workspace/bitcoin/qa/rpc-tests/test_framework/blocktools.py:49: DeprecationWarning: classic int division
  halvings = int(height/150) # regtest
/home/marco/workspace/bitcoin/qa/rpc-tests/replace-by-fee.py:33: DeprecationWarning: classic long division
  while node.getbalance() < satoshi_round((amount + fee)/COIN):
/home/marco/workspace/bitcoin/qa/rpc-tests/replace-by-fee.py:39: DeprecationWarning: classic long division
  txid = node.sendtoaddress(new_addr, satoshi_round((amount+fee)/COIN))
/home/marco/workspace/bitcoin/qa/rpc-tests/replace-by-fee.py:396: DeprecationWarning: classic long division
  split_value = int((initial_nValue-fee)/(MAX_REPLACEMENT_LIMIT+1))

laanwj
laanwj reviewed Apr 2, 2016
View reviewed changes
qa/rpc-tests/test_framework/mininode.py
else:
r = chr(255) + struct.pack("<Q", len(l))
r = struct.pack("B", 255) + struct.pack("<Q", len(l))
Copy link
Member

Choose a reason for hiding this comment

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

What about:

r = struct.pack("<BQ", 255, len(l))

etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, this makes sense. I've tried to make the diff easier to read but I will address you nit, as it makes the code smaller and possibly easier to read.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, yes I agree with regard to keeping the minimum diff, but indeed it's better to do this small cleanup (which someone is bound to do later) at the same time.

@laanwj
Copy link
Member

laanwj commented Apr 3, 2016

Tested ACK 4444806

@laanwj laanwj merged commit 4444806 into bitcoin:master Apr 3, 2016
laanwj added a commit that referenced this pull request Apr 3, 2016
@laanwj
Merge #7778: [qa] Bug fixes and refactor
ff5874b 
4444806 [qa] mininode: Combine struct.pack format strings (MarcoFalke)
faaa3c9 [qa] mininode: Catch exceptions in got_data (MarcoFalke)
fa2cea1 [qa] rpc-tests: Properly use integers, floats (MarcoFalke)
fa524d9 [qa] Use python2/3 syntax (MarcoFalke)
@maflcko maflcko deleted the Mf1604-qaFixesRefactor branch April 3, 2016 13:56
@btcdrak btcdrak mentioned this pull request Apr 3, 2016
Merged
btcdrak added a commit to btcdrak/bitcoin that referenced this pull request Apr 3, 2016
@btcdrak
[qa] rpc-tests: Properly use integers, floats
640666b 
partial backport from bitcoin#7778 using fa2cea1
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 5, 2016
[qa] Bug fixes and refactor
ff9b436 
Github-Pull: bitcoin#7778
Rebased-From: fa524d9 fa2cea1 faaa3c9 4444806
rebroad pushed a commit to rebroad/bitcoin that referenced this pull request Dec 7, 2016
@btcdrak @dgenr8
[qa] rpc-tests: Properly use integers, floats
2d05b4f 
partial backport from bitcoin#7778 using fa2cea1
@dagurval dagurval mentioned this pull request Aug 21, 2017
Merged
@str4d str4d mentioned this pull request Nov 19, 2020
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Dec 2, 2020
@zkbot
Auto merge of #4874 - str4d:rpc-tests-py, r=str4d
7a0a268 
Backport migration from rpc-tests.sh to rpc-tests.py

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#6567
- bitcoin/bitcoin#6523
- bitcoin/bitcoin#6616
- bitcoin/bitcoin#6788
  - Only the commit fixing `rpc-tests.py`
- bitcoin/bitcoin#6791
  - Only the fix to `qa/rpc-tests/README.md`
- bitcoin/bitcoin#6827
- bitcoin/bitcoin#6930
- bitcoin/bitcoin#6804
- bitcoin/bitcoin#7029
- bitcoin/bitcoin#7028
- bitcoin/bitcoin#7027
- bitcoin/bitcoin#7135
- bitcoin/bitcoin#7209
- bitcoin/bitcoin#7635
- bitcoin/bitcoin#7778
- bitcoin/bitcoin#7851
- bitcoin/bitcoin#7814
  - Only the changes to the new .py files in this PR.
- bitcoin/bitcoin#7971
- bitcoin/bitcoin#7972
- bitcoin/bitcoin#8056
  - Only the first commit.
- bitcoin/bitcoin#8098
- bitcoin/bitcoin#8104
- bitcoin/bitcoin#8133
  - Only the `rpc-tests.py` commit.
- bitcoin/bitcoin#8066
- bitcoin/bitcoin#8216
  - Only the last two commits.
- bitcoin/bitcoin#8254
- bitcoin/bitcoin#8400
- bitcoin/bitcoin#8482
  - Excluding the first commit (only affects RPC tests we don't have).
- bitcoin/bitcoin#8551
- bitcoin/bitcoin#8607
  - Only the pull-tester commit, for conflict removal.
- bitcoin/bitcoin#8625
- bitcoin/bitcoin#8713
- bitcoin/bitcoin#8750
- bitcoin/bitcoin#8789
- bitcoin/bitcoin#9098
- bitcoin/bitcoin#9276
  - Excluding the second commit (we don't have the changes it requires).
- bitcoin/bitcoin#9657
- bitcoin/bitcoin#9807
- bitcoin/bitcoin#9766
- bitcoin/bitcoin#9823
zkbot added a commit to zcash/zcash that referenced this pull request Dec 2, 2020
@zkbot
Auto merge of #4874 - str4d:rpc-tests-py, r=str4d
c8896f9 
Backport migration from rpc-tests.sh to rpc-tests.py

Cherry-picked from the following upstream PRs:
- bitcoin/bitcoin#6567
- bitcoin/bitcoin#6523
- bitcoin/bitcoin#6616
- bitcoin/bitcoin#6788
  - Only the commit fixing `rpc-tests.py`
- bitcoin/bitcoin#6791
  - Only the fix to `qa/rpc-tests/README.md`
- bitcoin/bitcoin#6827
- bitcoin/bitcoin#6930
- bitcoin/bitcoin#6804
- bitcoin/bitcoin#7029
- bitcoin/bitcoin#7028
- bitcoin/bitcoin#7027
- bitcoin/bitcoin#7135
- bitcoin/bitcoin#7209
- bitcoin/bitcoin#7635
- bitcoin/bitcoin#7778
- bitcoin/bitcoin#7851
- bitcoin/bitcoin#7814
  - Only the changes to the new .py files in this PR.
- bitcoin/bitcoin#7971
- bitcoin/bitcoin#7972
- bitcoin/bitcoin#8056
  - Only the first commit.
- bitcoin/bitcoin#8098
- bitcoin/bitcoin#8104
- bitcoin/bitcoin#8133
  - Only the `rpc-tests.py` commit.
- bitcoin/bitcoin#8066
- bitcoin/bitcoin#8216
  - Only the last two commits.
- bitcoin/bitcoin#8254
- bitcoin/bitcoin#8400
- bitcoin/bitcoin#8482
  - Excluding the first commit (only affects RPC tests we don't have).
- bitcoin/bitcoin#8551
- bitcoin/bitcoin#8607
  - Only the pull-tester commit, for conflict removal.
- bitcoin/bitcoin#8625
- bitcoin/bitcoin#8713
- bitcoin/bitcoin#8750
- bitcoin/bitcoin#8789
- bitcoin/bitcoin#9098
- bitcoin/bitcoin#9276
  - Excluding the second commit (we don't have the changes it requires).
- bitcoin/bitcoin#9657
- bitcoin/bitcoin#9807
- bitcoin/bitcoin#9766
- bitcoin/bitcoin#9823
@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
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maflcko @laanwj @jonasschnelli