Skip to content

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Sep 24, 2018

Remove unused testing code.

Rationale:

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

Copy link
Member

Choose a reason for hiding this comment

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

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()

Copy link
Member

Choose a reason for hiding this comment

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

ack

Copy link
Member

Choose a reason for hiding this comment

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

ack

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Last use was removed in b9f34e8

Copy link
Member

Choose a reason for hiding this comment

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

ack

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Last use was removed in your commit fac3e22 :-)

Copy link
Member

Choose a reason for hiding this comment

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

So please remove the constants here as well.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 24, 2018

No more conflicts as of last run.

@practicalswift
Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@DrahtBot
Copy link
Contributor

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

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK, agree with rationale.

Mind sharing how you find these unused code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change belong here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

nit, unrelated.

@practicalswift
Copy link
Contributor Author

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

@practicalswift
Copy link
Contributor Author

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

maflcko pushed a commit that referenced this pull request Nov 7, 2018
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
@practicalswift practicalswift deleted the remove-dead-testing-code branch April 10, 2021 19:36
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Aug 11, 2021
…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
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Aug 12, 2021
…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
gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 11, 2022
…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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants