Details

Reviewers

jasonbcox
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGINGb141c65a6aaa: Change signrawtransaction to signrawtransactionwithwallet in tests, add a test…
rABCb141c65a6aaa: Change signrawtransaction to signrawtransactionwithwallet in tests, add a test…

Summary

This diff updates the signrawtransaction() (to be deprecated) calls with the new signrawtransactionwithwallet() RPC in functional tests.
Because the signrawtransaction is still a valid RPC, also add a test to ensure that there is no regression with it. Not adding this test would left this RPC untested.

From commit eefff65:

scripted-diff:
-BEGIN VERIFY SCRIPT-
sed -i 's/\<signrawtransaction\>/signrawtransactionwithwallet/g'
test/functional/*.py
sed -i 's/\<signrawtransaction\>/signrawtransactionwithwallet/g'
test/functional/test_framework/*.py
-END VERIFY SCRIPT-

From commit d602348:
Add a test for signrawtransaction

Backport 2/3 of core PR10579
(commit eefff65 and part of commit d602348)
This commit arrangement makes it possible to merge the diff:

without breaking the tests or leaving code untested
without deprecating signrawtransaction

Depends on D2007
Part of T438

Test Plan

./test/functional/test_runner.py --extended

Diff Detail

Repository

rABC Bitcoin ABC

Branch

PR10579_part2

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4477
Build 7017: Bitcoin ABC Buildbot (legacy)
Build 7016: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 6 2018, 13:17

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Fabien added a child revision: D2009: [Target v0.19] Deprecate and add test for signrawtransaction.Nov 6 2018, 13:23

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3783: Diff 5666.Nov 6 2018, 13:28

jasonbcox accepted this revision.Nov 9 2018, 19:20

This revision is now accepted and ready to land.Nov 9 2018, 19:20

Rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4259: Diff 6265.Dec 3 2018, 18:01

Rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Fabien edited the summary of this revision. (Show Details)Dec 9 2018, 20:44

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4290: Diff 6311.Dec 9 2018, 20:51

Rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4294: Diff 6315.Dec 9 2018, 21:10

Request review again since now contains the "deprecatedrpc=signrawtransaction" removal from D2184

Rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4298: Diff 6322.Dec 10 2018, 16:14

deadalnix requested changes to this revision.Jan 3 2019, 22:11

deadalnix added a subscriber: deadalnix.

deadalnix added inline comments.

test/functional/abandonconflict.py
22 ↗	(On Diff #6322)	Do one thing per diff. If you are doing a scripted change, you do the scripted change. See D2159 .

This revision now requires changes to proceed.Jan 3 2019, 22:11

deadalnix added inline comments.Jan 3 2019, 22:15

test/functional/abandonconflict.py
22 ↗	(On Diff #6322)	Alternatively, you can do both in a given test, as the refactoring also remove the need for the deprecated flag, but then don't do it on all the test at once as it's very hard to review.

Diff has to be splitted

Fabien mentioned this in D2184: Prepare functional tests for deprecation of signrawtransaction rpc.Jan 4 2019, 18:14

Rebase, removing the -deprecated deletion (no longer added)

Fabien edited the summary of this revision. (Show Details)Jan 6 2019, 12:11

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4425: Diff 6526.Jan 6 2019, 13:47

Rebase

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4434: Diff 6536.Jan 7 2019, 10:02

This cannot be merged without d60234885bcc07b1a7f85ded7731549ec2fcfefa as it would leave code untested. Back on your queue.

This revision now requires changes to proceed.Jan 8 2019, 16:47

Include the tests for legacy signrawtransaction

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Fabien edited the summary of this revision. (Show Details)Jan 11 2019, 10:52

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4477: Diff 6599.Jan 11 2019, 10:59

You need to update the diff's description to reflect what the change is.

This revision now requires changes to proceed.Jan 14 2019, 17:21

deadalnix added inline comments.Jan 14 2019, 17:24

test/functional/signrawtransactions.py
128	Because this is not affect by the automated changes, then maybe it is better to commit it on its own. Usually it is best to not mix automated changes with manually made ones, as it forces the reviewer to play where is Waldo, which is both error prone and time consuming.

Fabien requested review of this revision.Jan 16 2019, 10:18

Fabien retitled this revision from Change signrawtransaction to signrawtransactionwithwallet in tests to Change signrawtransaction to signrawtransactionwithwallet in tests, add a test for legacy signrawtransaction.

Fabien added inline comments.

test/functional/signrawtransactions.py
128	If I add it to a later diff, `signrawtransaction` is left untested. If I add it to a preceding diff, I will have to make it an exception anyway because the scripted diff will update it unexpectedly. I fail to see a solution that cover all the requirements here. I updated the diff description to explain why it is like this.