tests: Remove unused testing code #14312

practicalswift · 2018-09-24T19:44:24Z

Remove unused testing code.

Rationale:

Less is more :-)
Unused code is by definition "untested"
YAGNI

maflcko · 2018-09-24T20:28:09Z

test/functional/test_framework/test_framework.py

@@ -515,18 +515,6 @@ def skip_if_no_wallet(self):
        if not self.is_wallet_compiled():
            raise SkipTest("wallet has not been compiled.")

-    def skip_if_no_cli(self):


Should probably call this:

diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 9a589240a8..238b50387d 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -161,8 +161,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): success = TestStatus.FAILED try: - if self.options.usecli and not self.supports_cli: - raise SkipTest("--usecli specified but test does not support using CLI") + if self.options.usecli: + if not self.supports_cli: + raise SkipTest("--usecli specified but test does not support using CLI") + self.skip_if_no_cli() self.skip_test_if_missing_module() self.setup_chain() self.setup_network()

maflcko · 2018-09-24T20:28:32Z

test/functional/test_framework/mininode.py

@@ -313,7 +313,7 @@ def on_ping(self, message):
        self.send_message(msg_pong(message.nonce))

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


maflcko · 2018-09-24T20:28:41Z

test/functional/test_framework/messages.py

-        return self.data
-
-    def __repr__(self):
-        return "msg_generic()"


maflcko · 2018-09-24T20:29:58Z

test/functional/test_framework/messages.py

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

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


When has this last been used? Not sure if we want to remove library functions that are still used in other branches. Same for the key.py changes.

Last use was removed in b9f34e8

maflcko · 2018-09-24T20:31:01Z

test/functional/feature_cltv.py

@@ -25,7 +25,7 @@

 # Reject codes that we might receive in this test
 REJECT_INVALID = 16
-REJECT_OBSOLETE = 17
+# REJECT_OBSOLETE = 17


When was the first and last use? Might want to remove altogether after investigating that.

Last use was removed in your commit fac3e22 :-)

So please remove the constants here as well.

DrahtBot · 2018-09-24T20:38:13Z

No more conflicts as of last run.

practicalswift · 2018-09-24T20:50:42Z

@MarcoFalke Thanks for the quick review. Updated. Please re-review :-)

maflcko · 2018-09-24T20:57:29Z

test/functional/test_framework/messages.py

@@ -31,7 +31,7 @@
 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_INV_SZ = 50000


remove? (This is no longer used after the removal of the comptool)

DrahtBot · 2018-09-28T04:43:12Z

Coverage	Change (pull 14312)	Reference (master)
Lines	-0.0154 %	87.0361 %
Functions	+0.0926 %	84.1130 %
Branches	-0.0199 %	51.5451 %

promag

Concept ACK, agree with rationale.

Mind sharing how you find these unused code?

promag · 2018-10-02T10:08:55Z

test/functional/test_framework/test_framework.py

+            if self.options.usecli:
+                if not self.supports_cli:
+                    raise SkipTest("--usecli specified but test does not support using CLI")
+                self.skip_if_no_cli()


Does this change belong here?

That was suggested by @MarcoFalke in #14312 (comment) :-)

promag · 2018-10-02T10:10:10Z

test/functional/test_framework/messages.py

@@ -1136,7 +1131,6 @@ def serialize(self):
    def __repr__(self):
        return "msg_block(block=%s)" % (repr(self.block))

-


nit, unrelated.

practicalswift · 2018-10-02T11:01:09Z

@promag vulture is a great tool for finding dead Python code. See #14365 for vulture integration in Travis :-)

practicalswift · 2018-10-02T13:44:17Z

Closing this PR in favour of #14365 as suggested by @promag :-)

c82190c tests: Add Python dead code linter (vulture) (practicalswift) 590a57f tests: Remove unused testing code (practicalswift) Pull request description: Add Python dead code linter (`vulture`) to Travis. Rationale for allowing dead code only after explicit opt-in (via `--ignore-names`): * Less is more :-) * Unused code is by definition "untested" * Unused code can be an indication of bugs/logical errors. By making the contributor aware of newly introduced unused code it gives him/her an opportunity to investigate if the unused code they introduce is malignant or benign :-) * Unused code is hard to spot for humans and is thus often missed during manual review * [YAGNI](https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it) Based on #14312 to make linter job pass. Tree-SHA512: 4c581df7c34986e226e4ade479e0d3c549daf38f4a4dc4564b25564d63e773a1830ba55d1289c771b1fa325483e8855b82b56e61859fe8e4b7dfa54034b093b6

…Travis c82190c tests: Add Python dead code linter (vulture) (practicalswift) 590a57f tests: Remove unused testing code (practicalswift) Pull request description: Add Python dead code linter (`vulture`) to Travis. Rationale for allowing dead code only after explicit opt-in (via `--ignore-names`): * Less is more :-) * Unused code is by definition "untested" * Unused code can be an indication of bugs/logical errors. By making the contributor aware of newly introduced unused code it gives him/her an opportunity to investigate if the unused code they introduce is malignant or benign :-) * Unused code is hard to spot for humans and is thus often missed during manual review * [YAGNI](https://en.wikipedia.org/wiki/You_aren%27t_gonna_need_it) Based on bitcoin#14312 to make linter job pass. Tree-SHA512: 4c581df7c34986e226e4ade479e0d3c549daf38f4a4dc4564b25564d63e773a1830ba55d1289c771b1fa325483e8855b82b56e61859fe8e4b7dfa54034b093b6

maflcko reviewed Sep 24, 2018

View reviewed changes

test/functional/test_framework/messages.py Outdated

return self.data

def __repr__(self):

return "msg_generic()"

Copy link

Member

maflcko Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

maflcko reviewed Sep 24, 2018

View reviewed changes

DrahtBot mentioned this pull request Sep 24, 2018

Tests: enforce critical class instance attributes in functional tests, fix segwit test specificity #14305

Merged

practicalswift force-pushed the remove-dead-testing-code branch 3 times, most recently from ecb0f6b to 065bedc Compare September 24, 2018 20:49

maflcko reviewed Sep 24, 2018

View reviewed changes

practicalswift force-pushed the remove-dead-testing-code branch from 065bedc to 9153b77 Compare September 24, 2018 20:59

DrahtBot mentioned this pull request Sep 24, 2018

Bugfix: Only run bitcoin-tx tests when bitcoin-tx is enabled #12246

Merged

fanquake added the Tests label Sep 25, 2018

DrahtBot added the Needs rebase label Sep 27, 2018

practicalswift force-pushed the remove-dead-testing-code branch from 9153b77 to 908c888 Compare September 27, 2018 20:06

DrahtBot removed the Needs rebase label Sep 27, 2018

promag reviewed Oct 2, 2018

View reviewed changes

tests: Remove unused testing code

e17da14

practicalswift force-pushed the remove-dead-testing-code branch from 908c888 to e17da14 Compare October 2, 2018 10:28

practicalswift mentioned this pull request Oct 2, 2018

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

Merged

practicalswift closed this Oct 2, 2018

practicalswift deleted the remove-dead-testing-code branch April 10, 2021 19:36

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Remove unused testing code #14312

tests: Remove unused testing code #14312

practicalswift commented Sep 24, 2018 •

edited

Loading

maflcko Sep 24, 2018

maflcko Sep 24, 2018

maflcko Sep 24, 2018

maflcko Sep 24, 2018

practicalswift Sep 24, 2018

maflcko Sep 24, 2018

maflcko Sep 24, 2018

practicalswift Sep 24, 2018

maflcko Sep 24, 2018

DrahtBot commented Sep 24, 2018 •

edited

Loading

practicalswift commented Sep 24, 2018

maflcko Sep 24, 2018

practicalswift Sep 24, 2018

DrahtBot commented Sep 28, 2018

promag left a comment

promag Oct 2, 2018

practicalswift Oct 2, 2018

promag Oct 2, 2018

practicalswift commented Oct 2, 2018

practicalswift commented Oct 2, 2018

		@@ -1136,7 +1131,6 @@ def serialize(self):
		def __repr__(self):
		return "msg_block(block=%s)" % (repr(self.block))

tests: Remove unused testing code #14312

tests: Remove unused testing code #14312

Conversation

practicalswift commented Sep 24, 2018 • edited Loading

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

DrahtBot commented Sep 24, 2018 • edited Loading

practicalswift commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented Sep 28, 2018

promag left a 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

practicalswift commented Oct 2, 2018

practicalswift commented Oct 2, 2018

practicalswift commented Sep 24, 2018 •

edited

Loading

DrahtBot commented Sep 24, 2018 •

edited

Loading