add rpc_misc.py, mv test getmemoryinfo, add test mallocinfo #15485

adamjonas commented at 7:43 PM on February 26, 2019: member

Creating the rpc_misc.py functional test file to add space for adding tests to a file that doesn't have a lot of coverage.

Removing the getmemoryinfo() smoke test from wallet basic rather than moving it to keep the wallet decoupled. Feel like testing for reasonable memory allocation values should suffice.
Adding coverage for mallocinfo(). Introduced standard lib XML parser since the function exports an XML string that describes the current state of the memory-allocation implementation in the caller.

MarcoFalke commented at 7:49 PM on February 26, 2019: member

utACK 2e957b6ccaa41d13190bc5197865979f5f5265bf

Good to split up the overly large wallet_basic tests into independent tests, that are easier to debug in case of failure.

in test/functional/rpc_misc.py:11 in 2e957b6cca outdated

   6 | +import xml.etree.ElementTree as ET
   7 | +
   8 | +from test_framework.test_framework import BitcoinTestFramework
   9 | +from test_framework.util import (
  10 | +    assert_raises_rpc_error,
  11 | +    assert_equal

MarcoFalke commented at 7:51 PM on February 26, 2019:

style-nit: Could add trailing comma, so that git blame works properly when something is added.

in test/functional/rpc_misc.py:26 in 2e957b6cca outdated

  21 | +    def run_test(self):
  22 | +        node0 = self.nodes[0]
  23 | +
  24 | +        self.log.info("test getmemoryinfo")
  25 | +        memory = node0.getmemoryinfo()['locked']
  26 | +        assert memory['used'], memory['free'] > 0

MarcoFalke commented at 7:52 PM on February 26, 2019:

Please do not assert on tuples. This will never fail

assert (0, False) # will never fail

MarcoFalke changes_requested

MarcoFalke commented at 7:53 PM on February 26, 2019: member

Oops, one assert issue I missed

in test/functional/rpc_misc.py:2 in 2e957b6cca outdated

   0 | @@ -0,0 +1,46 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2018 The Bitcoin Core developers

jnewbery commented at 7:57 PM on February 26, 2019:

nit: 2019

in test/functional/rpc_misc.py:22 in 2e957b6cca outdated

  17 | +class RpcMiscTest(BitcoinTestFramework):
  18 | +    def set_test_params(self):
  19 | +        self.num_nodes = 1
  20 | +
  21 | +    def run_test(self):
  22 | +        node0 = self.nodes[0]

jnewbery commented at 7:58 PM on February 26, 2019:

nit: consider naming this node, since there's only one of them

in test/functional/rpc_misc.py:28 in 2e957b6cca outdated

  22 | +        node0 = self.nodes[0]
  23 | +
  24 | +        self.log.info("test getmemoryinfo")
  25 | +        memory = node0.getmemoryinfo()['locked']
  26 | +        assert memory['used'], memory['free'] > 0
  27 | +        assert memory['total'] > 0

jnewbery commented at 7:59 PM on February 26, 2019:

nit: consider using assert_greater_than(), which gives more useful logging output when it fails. Same for all other asserts.

in test/functional/rpc_misc.py:44 in 2e957b6cca outdated

  38 | +            assert_equal(tree.tag, 'malloc')
  39 | +        except JSONRPCException:
  40 | +            self.log.info('getmemoryinfo(mode="mallocinfo") not available')
  41 | +            assert_raises_rpc_error(-8, 'mallocinfo is only available when compiled with glibc 2.10+', node0.getmemoryinfo, mode="mallocinfo")
  42 | +
  43 | +        assert_raises_rpc_error(-8, "unknown mode " + "foobar", node0.getmemoryinfo, mode="foobar")

jnewbery commented at 8:01 PM on February 26, 2019:

nit: no need to concat two string literals. Do "unknown mode foobar" instead of "unknown mode " + "foobar"

adamjonas force-pushed on Feb 26, 2019

jnewbery commented at 8:03 PM on February 26, 2019: member

Looks great. Thanks for doing this.

You'll need to update the file mode bits so this is executable. Run chmod 755 rpc_misc.py, then commit the file and push.

I've left some style nits inline. Up to you whether you take them or not.

add rpc_misc.py, mv test getmemoryinfo, add test mallocinfo 2fa85ebd1c

adamjonas force-pushed on Feb 26, 2019

MarcoFalke approved

MarcoFalke commented at 8:12 PM on February 26, 2019: member

re-utACK 2fa85ebd1c2d7579ea005300e9101fbb48805c2f

jnewbery commented at 8:13 PM on February 26, 2019: member

ACK 2fa85ebd1c2d7579ea005300e9101fbb48805c2f. Nice work!

practicalswift commented at 8:51 PM on February 26, 2019: contributor

Concept ACK

What an excellent first-time contribution! Hope I'll see more contributions from you going forward! :-)

DrahtBot commented at 9:19 PM on February 26, 2019: member

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

Coverage

Coverage	Change (pull 15485, 26ffd3c77fd871825d20a2cdd07528770b04dacd)	Reference (master, d88f7f8764b1f93e3992c714f62446b6ed5cfd1e)
Lines	+0.0231 %	87.3445 %
Functions	+0.0147 %	84.7625 %
Branches	+0.0069 %	51.4908 %

<sup>Updated at: 2019-02-26T21:19:47.481840.</sup>

fanquake added the label Tests on Feb 26, 2019

in test/functional/rpc_misc.py:30 in 2fa85ebd1c outdated

  25 | +        self.log.info("test getmemoryinfo")
  26 | +        memory = node.getmemoryinfo()['locked']
  27 | +        assert_greater_than(memory['used'], 0)
  28 | +        assert_greater_than(memory['free'], 0)
  29 | +        assert_greater_than(memory['total'], 0)
  30 | +        assert_greater_than(memory['locked'], 0)

MarcoFalke commented at 5:27 PM on February 28, 2019:

This fails on appveyor:

                                   AssertionError: 0 <= 0

jnewbery commented at 7:06 PM on February 28, 2019:

See assert_greater_than_or_equal()

in test/functional/rpc_misc.py:6 in 2fa85ebd1c outdated

   0 | @@ -0,0 +1,48 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2019 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +"""Test RPC misc output."""
   6 | +import xml.etree.ElementTree as ET

practicalswift commented at 8:21 PM on February 28, 2019:

Worth being aware of regarding the security of xml.etree.ElementTree:

Warning The xml.etree.ElementTree module is not secure against maliciously constructed data. If you need to parse untrusted or unauthenticated data see XML vulnerabilities.

We'll only be parsing trusted data from what I can think of, so likely not a problem in the testing code :-)

adamjonas commented at 3:09 PM on March 1, 2019:

Thanks for flagging @practicalswift. I was looking for other examples of xml libraries and this precedent that I tried to follow.

modify test for memory locked in case locking pages failed at some point f13ad1cae0

adamjonas force-pushed on Mar 1, 2019

MarcoFalke merged this on Mar 1, 2019

MarcoFalke closed this on Mar 1, 2019

MarcoFalke referenced this in commit f9dbb319d2 on Mar 1, 2019

adamjonas deleted the branch on Oct 21, 2019

deadalnix referenced this in commit 8d1c05b523 on Mar 31, 2020

ftrader referenced this in commit 3796783993 on Aug 17, 2020

ogabrielides referenced this in commit f64595c398 on Sep 14, 2021

ogabrielides referenced this in commit 71f0f738ee on Sep 15, 2021

ogabrielides referenced this in commit 728bcde5bb on Sep 15, 2021

ogabrielides referenced this in commit 1b75f4f1d7 on Sep 16, 2021

PastaPastaPasta referenced this in commit 0cb3dd5b87 on Sep 16, 2021

DrahtBot locked this on Dec 16, 2021