Node RPC Tests #810

nodech · 2019-07-02T16:16:01Z

RPC tests:

Fix help help
help tests
remove getinfo.
test for getblockchaininfo.
test for getnetworkinfo.
Change verifytxoutproof to check totalTX count if possible. (performance degrades)
- If performance is an issue we could index this information separately or with headers(entry).
Add tests for gettxoutproof and verifytxoutproof.

codecov-io · 2019-07-02T16:19:38Z

Codecov Report

Merging #810 into master will increase coverage by 4.76%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master     #810      +/-   ##
==========================================
+ Coverage   62.01%   66.78%   +4.76%     
==========================================
  Files         147      147              
  Lines       25379    25384       +5     
==========================================
+ Hits        15740    16953    +1213     
+ Misses       9639     8431    -1208

Impacted Files	Coverage Δ
lib/node/rpc.js	`33.16% <93.33%> (+6.31%)`	⬆️
lib/primitives/mtx.js	`61.56% <0%> (+0.12%)`	⬆️
lib/wallet/walletdb.js	`77.7% <0%> (+0.25%)`	⬆️
lib/mempool/mempool.js	`65.76% <0%> (+0.33%)`	⬆️
lib/coins/coinview.js	`75.17% <0%> (+0.7%)`	⬆️
lib/net/bip152.js	`86.1% <0%> (+1.01%)`	⬆️
lib/primitives/abstractblock.js	`89.88% <0%> (+1.12%)`	⬆️
lib/node/node.js	`76.25% <0%> (+1.43%)`	⬆️
lib/net/packets.js	`80% <0%> (+2.04%)`	⬆️
lib/primitives/block.js	`75.1% <0%> (+2.4%)`	⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4542ec5...828a269. Read the comment docs.

braydonf · 2019-07-02T19:48:15Z

Probably worth adding tests for getblockchaininfo and other info related methods getnetworkinfo and getmininginfo that were added as replacement in addition to getinfo. As far as deprecating getinfo, it would probably be good to do, since it's mostly stubbed out currently.

nodech · 2019-07-02T19:49:24Z

I can do those test in this PR as well, I was considering doing it in separate PR.
At least deprecation can happen here

nodech · 2019-07-07T20:14:34Z

test/node-rpc-test.js


  before(async () => {
-    await node.open();
+    await fullnode.open();


We can modify/improve on test/util/node-context for managing multiple connected nodes.

nodech · 2019-07-07T20:15:09Z

test/node-rpc-test.js

+    await wclient.execute('selectwallet', [MINER_WALLET]);
+
+    // setup miner wallet
+    const address = await wclient.execute('getnewaddress');


For miner we could have MinerContext which will manage funds specifically to miner that will only act as faucet.

nodech · 2019-07-07T20:16:50Z

test/node-rpc-test.js

+        name: 'Error',
+        type: 'RPCError',
+        message: /SPV/,
+        code: -1 >>> 0


This trick here, help with the bug in bcurl: bcoin-org/bcurl#16

test/node-rpc-test.js

braydonf · 2019-07-15T16:03:39Z

test/node-rpc-test.js

@@ -236,6 +236,90 @@ describe('RPC', function() {
    });
  });

+  describe('getblockchaininfo', function() {
+    const check = (info, options) => {
+      assert.ok(info);


nit: assert() === assert.ok(), shorter to use assert() and more consistent.

yeah, but .ok kinda felt for testing was more descriptive. That's the only reason for .oks.
But can go back to asserts.

braydonf · 2019-07-15T16:05:17Z

test/node-rpc-test.js

+      const info = await nclient.execute('getblockchaininfo');
+      const height = fullnode.chain.height;
+
+      await check(info, {


nit: unnecessary await?

yep, it was async before and forgot to remove.

braydonf · 2019-07-16T16:24:54Z

test/node-rpc-test.js

  });
 });
+
+function isHex(string) {
+  const hexCheck = /^([0-9a-f]{2})*$/i;


nit: Could be a one-liner:

return /^([0-9a-f]{2})*$/i.test(string);

I mostly prefer non-one-liners, if they are not arrow functions. It's easier to quickly modify and debug what's going on.

This case can be rewritten in one-liner it wont change (it can even move to test/utils), it's just a habit/convention at this point. :)

braydonf · 2019-07-16T16:27:23Z

test/node-rpc-test.js

+
+      const tweaked = mblock.toRaw().toString('hex');
+
+      // should fail in fullnode or in pruned mode (if there's block available).


nit: Comments generally have capitalization of comments (especially for sentences).

braydonf · 2019-07-16T16:39:27Z

test/node-rpc-test.js

+      await wclient.execute('selectwallet', [MINER_WALLET]);
+      const minerAddr = await wclient.execute('getnewaddress');
+
+      { // block 0 - This block will have 2 txs (including coinbase)


These are generating blocks on the chain shared by all tests. Should the setup be a part of the before for all tests? It could potentially be generic enough. Currently this is the last test, so no other tests depend on it, however as more tests are added, they would always need to be added before the test to not depend state changes within these tests. Also, I don't think these tests require having all sets of different nodes, spv and etc.

Even though chain is same for the testsuite, this test generates its own blocks.

I was having several conventions that I have not described in the document.

Blocks that are shared are only coinbase blocks and is managed by miner wallet/address/mining api.

If test needs to generate blocks, or specific blocks are necessary to test the api then it can generate blocks in its context and only use general blocks as faucet (miner).

I even considered doing reset in the test case in after, but have not used it so far because there was no apparent problem with current approach. Still any test messes up another test case we can use reset.

This is highly unlikely though. Each describe can figure out relevant state before starting the test and describe it as a starting point and do tests relative to the current state regardless which tests were skipped or chosen.

Even though SPV and Prune node tests currently have small usage, for some APIs difference gets bigger so described it in the beginning with minor tests and other tests can use it. We can remove if after everything is done, they don't provide value.

nodech added 2 commits July 2, 2019 17:53

rpc-test: add help rpc tests.

df3e55c

rpc-test: add getinfo tests.

4e694f9

nodech force-pushed the rpc-node-up1 branch from 99a73a7 to 4e694f9 Compare July 2, 2019 16:17

nodech added api HTTP, RPC, authentication ready for review Ready to be reviewed tests Has tests for code change is just an addtional test labels Jul 2, 2019

nodech changed the title ~~rpc test - getinfo, help~~ Node RPC Tests Jul 3, 2019

node-rpc: remove getinfo.

d30e4da

nodech force-pushed the rpc-node-up1 branch from fd048ec to d30e4da Compare July 3, 2019 18:59

nodech added 2 commits July 8, 2019 00:06

node-rpc: detect mempool txs in gettxoutproof.

f381218

rpc-test: add gettxoutproof rpc call tests.

4ae5aee

nodech commented Jul 7, 2019

View reviewed changes

braydonf added rpc and removed api HTTP, RPC, authentication labels Jul 9, 2019

pinheadmz reviewed Jul 9, 2019

View reviewed changes

test/node-rpc-test.js Outdated Show resolved Hide resolved

pinheadmz reviewed Jul 9, 2019

View reviewed changes

test/node-rpc-test.js Show resolved Hide resolved

pinheadmz reviewed Jul 9, 2019

View reviewed changes

test/node-rpc-test.js Show resolved Hide resolved

pinheadmz reviewed Jul 9, 2019

View reviewed changes

test/node-rpc-test.js Outdated Show resolved Hide resolved

pinheadmz reviewed Jul 10, 2019

View reviewed changes

test/node-rpc-test.js Show resolved Hide resolved

nodech added 2 commits July 11, 2019 23:36

test-rpc: verifytxoutproof param tests.

1eecac0

node-rpc: check tx count in block if it's available in verifytxoutproof.

41d15de

nodech force-pushed the rpc-node-up1 branch from 40922ea to 41d15de Compare July 11, 2019 20:27

nodech removed the ready for review Ready to be reviewed label Jul 12, 2019

nodech added 2 commits July 14, 2019 19:56

rpc-test: getblockchaininfo test.

ee0ff93

rpc-test: getnetworkinfo test.

ff1f797

braydonf reviewed Jul 15, 2019

View reviewed changes

braydonf reviewed Jul 16, 2019

View reviewed changes

rpc-test: minor.

828a269

nodech mentioned this pull request Oct 17, 2019

RPC signmessage #802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node RPC Tests #810

Node RPC Tests #810

nodech commented Jul 2, 2019 •

edited

Loading

codecov-io commented Jul 2, 2019 •

edited

Loading

braydonf commented Jul 2, 2019

nodech commented Jul 2, 2019

nodech Jul 7, 2019

nodech Jul 7, 2019

nodech Jul 7, 2019

braydonf Jul 15, 2019

nodech Jul 19, 2019

braydonf Jul 15, 2019

nodech Jul 19, 2019

braydonf Jul 16, 2019

nodech Jul 19, 2019

braydonf Jul 16, 2019

braydonf Jul 16, 2019

nodech Jul 19, 2019


		const tweaked = mblock.toRaw().toString('hex');

		// should fail in fullnode or in pruned mode (if there's block available).

Node RPC Tests #810

Are you sure you want to change the base?

Node RPC Tests #810

Conversation

nodech commented Jul 2, 2019 • edited Loading

codecov-io commented Jul 2, 2019 • edited Loading

Codecov Report

braydonf commented Jul 2, 2019

nodech commented Jul 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodech commented Jul 2, 2019 •

edited

Loading

codecov-io commented Jul 2, 2019 •

edited

Loading