Page MenuHomePhabricator

Change signrawtransaction to signrawtransactionwithwallet in tests, add a test for legacy signrawtransaction
ClosedPublic

Authored by Fabien on Nov 6 2018, 13:17.

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Nov 6 2018, 13:17
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 6 2018, 13:17
jasonbcox accepted this revision.Nov 9 2018, 19:20
This revision is now accepted and ready to land.Nov 9 2018, 19:20
Fabien updated this revision to Diff 6265.Dec 3 2018, 17:50

Rebase

Fabien updated this revision to Diff 6311.Dec 9 2018, 20:43

Rebase

Fabien edited the summary of this revision. (Show Details)Dec 9 2018, 20:44
Fabien updated this revision to Diff 6315.Dec 9 2018, 20:59

Rebase

Fabien requested review of this revision.Dec 10 2018, 08:29

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

Fabien updated this revision to Diff 6322.Dec 10 2018, 16:03

Rebase

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.

Fabien planned changes to this revision.Jan 4 2019, 09:25

Diff has to be splitted

Fabien updated this revision to Diff 6526.Jan 6 2019, 12:10

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

Fabien edited the summary of this revision. (Show Details)Jan 6 2019, 12:11
Fabien updated this revision to Diff 6536.Jan 7 2019, 09:51

Rebase

deadalnix requested changes to this revision.Jan 8 2019, 16:47

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
Fabien updated this revision to Diff 6599.Jan 11 2019, 10:48

Include the tests for legacy signrawtransaction

Fabien edited the summary of this revision. (Show Details)Jan 11 2019, 10:52
deadalnix requested changes to this revision.Jan 14 2019, 17:21

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 ↗(On Diff #6599)

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 ↗(On Diff #6599)

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.

Fabien edited the summary of this revision. (Show Details)Feb 11 2019, 12:14
Fabien updated this revision to Diff 7295.Feb 11 2019, 12:40

Rebase

jasonbcox accepted this revision.Feb 12 2019, 23:50
deadalnix requested changes to this revision.Mon, Feb 25, 23:20

rebase

This revision now requires changes to proceed.Mon, Feb 25, 23:20
Fabien updated this revision to Diff 7517.Thu, Feb 28, 09:23

Rebase

deadalnix accepted this revision.Thu, Feb 28, 22:12
This revision is now accepted and ready to land.Thu, Feb 28, 22:12
This revision was automatically updated to reflect the committed changes.