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
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
This revision is now accepted and ready to land.Nov 9 2018, 19:20
Fabien requested review of this revision.Dec 10 2018, 08:29

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

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
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

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

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

Include the tests for legacy signrawtransaction

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
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.

This revision now requires changes to proceed.Feb 25 2019, 23:20
This revision is now accepted and ready to land.Feb 28 2019, 22:12
This revision was automatically updated to reflect the committed changes.