qa: Add getdescriptorinfo functional test #15443

pull promag wants to merge 1 commits into bitcoin:master from promag:2019-02-qa-feature-descriptor changing 2 files +66 −0

promag commented at 3:21 PM on February 19, 2019: member

The getdescriptorinfo RPC was added in #15368, this PR adds some tests.
promag commented at 3:23 PM on February 19, 2019: member

@sipa not sure if it makes sense to test these examples. Initially I was only testing the interface.
in test/functional/test_runner.py:193 in 11778c8fd5 outdated
```
 189 | @@ -190,6 +190,7 @@
 190 |      'rpc_help.py',
 191 |      'feature_help.py',
 192 |      'feature_shutdown.py',
 193 | +    'feature_descriptor.py',
```
MarcoFalke commented at 3:25 PM on February 19, 2019:

According to the naming guide, this should be rpc_descriptor?

promag commented at 3:30 PM on February 19, 2019:

Descriptor is not a RPC thing is it?

Sjors commented at 4:22 PM on February 19, 2019:

Or rpc_getdescriptorinfo

promag commented at 8:46 PM on July 1, 2019:

Ok, will rename to @Sjors suggestion.

MarcoFalke commented at 3:10 AM on March 10, 2020:

@promag Are you going to rename this or not. If not I will merge as is.

promag commented at 8:14 AM on March 10, 2020:

Renamed to rpc_getdescriptorinfo.py.
Sjors approved
Sjors commented at 4:33 PM on February 19, 2019: member
tACK 11778c on macOS 10.14.3.

We should distinguish two reasons for having test descriptors:
1. To test the command output (make sure no fields are missing, all error RPC error conditions are covered)
2. To test descsum_create i.e. the Python implementation of descriptor checksums.
(2) probably deserves a seperate test that is called by test_runner but otherwise doesn't need any of the framework. On the other hand, just adding it this test file seems fine too.

It could be useful to put a bunch of test descriptors in a single JSON file to reuse between tests and to make it easier for other projects to borrow.
fanquake added the label Tests on Feb 19, 2019
jonasschnelli commented at 5:55 AM on February 20, 2019: contributor

More tests are always good. Thanks for adding. Concept AcK
practicalswift commented at 7:18 PM on February 20, 2019: contributor

Concept ACK

Thanks for adding!
DrahtBot commented at 1:16 AM on June 9, 2019: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

No conflicts as of last run.
jachiang commented at 11:39 AM on July 1, 2019: contributor
@promag I looked into this PR in preparation for the PR Review Club IRC session later this week hosted by John and Harding and had a couple questions or thoughts:
1. I believe the RPC test does not cover a case where no checksum is appended to the output descriptor? Currently, test_desc will append a checksum to every descriptor literal. Could we extend test_desc parameters to allow for such a test: e.g. def test_desc(self, desc, *appendchecksum*, isrange, issolvable, hasprivatekeys): EDIT: I realize the test covers descriptors with and without checksum.
2. It would seem that we could cover a case with hasprivatekeys=True e.g. wpkh(WIF)#checksum or wsh(multi(1, XPRV/1/0/*, XPUB/0/0/*))#checksum
3. ~~How would a issolvable=False return differ from an Invalid descriptor error? I am sure there is a distinction which eludes me. In any case, would it make sense to cover an unsolvable descriptor?~~ EDIT: Perhaps add addr(ADDR)/raw(HEX) inputs for issolvable=False coverage?
Thanks for adding this test.
promag commented at 8:47 PM on July 1, 2019: member

@jachiang FYI these were taken from doc/descriptors.md.
meshcollider commented at 1:33 AM on January 17, 2020: contributor

utACK 11778c8fd5cc13fcbcbc746ea1ca7ddef13a7de3

As mentioned above, it would be good to include some tests where hasprivatekeys=True
DrahtBot cross-referenced this on Mar 2, 2020 from issue Serve BIP 157 compact filters by jimpo
DrahtBot closed this on Mar 9, 2020
DrahtBot commented at 8:34 PM on March 9, 2020: contributor

The last travis run for this pull request was 384 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
DrahtBot reopened this on Mar 9, 2020
promag force-pushed on Mar 10, 2020
qa: Add getdescriptorinfo functional test cbf2d75d8f
promag force-pushed on Mar 10, 2020
MarcoFalke merged this on Mar 10, 2020
MarcoFalke closed this on Mar 10, 2020
promag deleted the branch on Mar 10, 2020
MarkLTZ cross-referenced this on Apr 4, 2020 from issue Bitcoin PR tracking by MarkLTZ
Fabcien referenced this in commit a34d598eb4 on Jan 6, 2021
bitcoin locked this on Feb 15, 2022