Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: Add Python dead code linter (vulture) to Travis #14365

Merged
merged 2 commits into from Nov 7, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -8,3 +8,4 @@ export LC_ALL=C

travis_retry pip install codespell==1.13.0
travis_retry pip install flake8==3.5.0
travis_retry pip install vulture==0.29
@@ -25,7 +25,6 @@

# Reject codes that we might receive in this test
REJECT_INVALID = 16
REJECT_OBSOLETE = 17
REJECT_NONSTANDARD = 64

def cltv_invalidate(tx):
@@ -22,7 +22,6 @@

# Reject codes that we might receive in this test
REJECT_INVALID = 16
REJECT_OBSOLETE = 17
REJECT_NONSTANDARD = 64

# A canonical signature consists of:
@@ -35,7 +35,6 @@
MY_SUBVERSION = b"/python-mininode-tester:0.0.3/"
MY_RELAY = 1 # from version 70001 onwards, fRelay should be appended to version messages (BIP37)

MAX_INV_SZ = 50000
MAX_LOCATOR_SZ = 101
MAX_BLOCK_BASE_SIZE = 1000000

@@ -58,9 +57,6 @@
def sha256(s):
return hashlib.new('sha256', s).digest()

def ripemd160(s):
return hashlib.new('ripemd160', s).digest()

def hash256(s):
return sha256(sha256(s))

@@ -887,13 +883,12 @@ def __repr__(self):


class CPartialMerkleTree:
__slots__ = ("fBad", "nTransactions", "vBits", "vHash")
__slots__ = ("nTransactions", "vBits", "vHash")

def __init__(self):
self.nTransactions = 0
self.vHash = []
self.vBits = []
self.fBad = False
This conversation was marked as resolved by practicalswift

This comment has been minimized.

Copy link
@jb55

jb55 Oct 3, 2018

Contributor

even though this is unused, is it really a good idea to remove code that initializes default values?

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 4, 2018

Author Member

@jb55 Do you mean the fact that fBad was still in __slots__? I've now removed it also from __slots__. fBad is gone 100 %. Updated version looks good? :-)

This comment has been minimized.

Copy link
@jb55

jb55 Oct 4, 2018

Contributor

perhaps, 3a4449e states:

Classes use slots to ensure extraneous attributes aren't accidentally added by tests, compromising their intended effect.

this seems to be reversing the original intent of that commit. /cc @JustinTArthur ?

This comment has been minimized.

Copy link
@JustinTArthur

JustinTArthur Oct 4, 2018

Contributor

If fBad isn't going to be used, removing it won't go against the intentions of the slots work, which is just there to protect test developers from making tests that don't do what the developers think they do.

I just want to make sure the removal doesn't break the intentions of those message module classes, which provide test developers with Python equivalents of the common Core data objects—fBad is still in the Core C++ classes. If we generally only include attributes we need for existing tests, then no problem removing it and someone can re-add it later if their test incorporates it. I don't have enough history with the functional test suite to make a judgement call.

This comment has been minimized.

Copy link
@promag

promag Oct 4, 2018

Member

+1 remove fBad.

This comment has been minimized.

Copy link
@jb55

jb55 Oct 4, 2018

Contributor

+0 sounds good to me.

nit: it looks like the fBad slot removal was squashed into the wrong commit.

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 4, 2018

Author Member

@jb55 Thanks for letting me know about the squash mixup. Now fixed :-)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Oct 8, 2018

Member

I think it's fine for fBad to be removed. These classes are for sending and receiving serialized objects to bitcoind. In bitcoind, the serialization method for CPartialMerkleTree does not serialize the fBad value - it's only used internally in the class. I think it's fine to therefore not have fBad in our python CPartialMerkleTree implementation.


def deserialize(self, f):
self.nTransactions = struct.unpack("<i", f.read(4))[0]
@@ -349,7 +349,7 @@ def on_ping(self, message):
self.send_message(msg_pong(message.nonce))

def on_verack(self, message):
self.verack_received = True
pass

def on_version(self, message):
assert message.nVersion >= MIN_VERSION_SUPPORTED, "Version {} received. Test framework only supports versions greater than {}".format(message.nVersion, MIN_VERSION_SUPPORTED)
@@ -54,10 +54,9 @@ def __repr__(self):
return 'Socks5Command(%s,%s,%s,%s,%s,%s)' % (self.cmd, self.atyp, self.addr, self.port, self.username, self.password)

class Socks5Connection():
def __init__(self, serv, conn, peer):
def __init__(self, serv, conn):
self.serv = serv
self.conn = conn
self.peer = peer

def handle(self):
"""Handle socks5 request according to RFC192."""
@@ -137,9 +136,9 @@ def __init__(self, conf):

def run(self):
while self.running:
(sockconn, peer) = self.s.accept()
(sockconn, _) = self.s.accept()
if self.running:
conn = Socks5Connection(self, sockconn, peer)
conn = Socks5Connection(self, sockconn)
thread = threading.Thread(None, conn.handle)
thread.daemon = True
thread.start()
@@ -635,7 +635,7 @@ def _get_uncovered_rpc_commands(self):
with open(coverage_ref_filename, 'r', encoding="utf8") as coverage_ref_file:
all_cmds.update([line.strip() for line in coverage_ref_file.readlines()])

for root, dirs, files in os.walk(self.dir):
for root, _, files in os.walk(self.dir):
for filename in files:
if filename.startswith(coverage_file_prefix):
coverage_filenames.add(os.path.join(root, filename))
@@ -0,0 +1,19 @@
#!/bin/bash
#
# Copyright (c) 2018 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
#
# Find dead Python code.

export LC_ALL=C

if ! command -v vulture > /dev/null; then
echo "Skipping Python dead code linting since vulture is not installed. Install by running \"pip3 install vulture\""
exit 0
fi

vulture \
--min-confidence 60 \
--ignore-names "argtypes,connection_lost,connection_made,converter,data_received,daemon,errcheck,get_ecdh_key,get_privkey,is_compressed,is_fullyvalid,msg_generic,on_*,optionxform,restype,set_privkey" \
$(git ls-files -- "*.py" ":(exclude)contrib/")
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.