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

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:lint-python-dead-code changing 8 files +26 −14
  1. practicalswift commented at 10:53 am on October 2, 2018: contributor

    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

    Based on #14312 to make linter job pass.

  2. practicalswift force-pushed on Oct 2, 2018
  3. fanquake added the label Tests on Oct 2, 2018
  4. practicalswift commented at 11:23 am on October 2, 2018: contributor

    I’ve tested this linter over the set of all open mergeable pull requests.

    A total of 196 pull requests were tested:

    • One true positive was found (and reported!)
    • Zero false positives were found

    A precision score of 100 percent :-)

  5. practicalswift force-pushed on Oct 2, 2018
  6. practicalswift force-pushed on Oct 2, 2018
  7. promag commented at 1:41 pm on October 2, 2018: member

    In my system it takes around 1 second so it’s not a big deal.

    I suggest closing #14312.

  8. practicalswift commented at 1:44 pm on October 2, 2018: contributor
    @promag Thanks for reviewing. Good idea! Done.
  9. in test/functional/test_framework/test_framework.py:167 in 6d3822d5f7 outdated 
    160@@ -161,8 +161,10 @@ def main(self):
161         success = TestStatus.FAILED
162 
163         try:
164-            if self.options.usecli and not self.supports_cli:
165-                raise SkipTest("--usecli specified but test does not support using CLI")
166+            if self.options.usecli:
167+                if not self.supports_cli:
168+                    raise SkipTest("--usecli specified but test does not support using CLI")
169+                self.skip_if_no_cli()
    MarcoFalke commented at 4:58 am on October 3, 2018:
    This hunk probably needs testing. Mind to submit as separate pull request?
  10. practicalswift force-pushed on Oct 3, 2018
  11. practicalswift commented at 8:12 am on October 3, 2018: contributor
    @MarcoFalke Updated. Please re-review :-)
  12. in test/functional/test_framework/messages.py:896 in ae92b3a70f outdated 
    888@@ -893,7 +889,6 @@ def __init__(self):
889         self.nTransactions = 0
890         self.vHash = []
891         self.vBits = []
892-        self.fBad = False
    jb55 commented at 6:05 pm on October 3, 2018:
    even though this is unused, is it really a good idea to remove code that initializes default values?
    practicalswift commented at 7:54 am on October 4, 2018:
    @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? :-)
    jb55 commented at 4:28 pm on October 4, 2018:

    perhaps, 3a4449e9ad945313c6637283757de8d539cf790f 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 ?

    JustinTArthur commented at 5:10 pm on October 4, 2018:

    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.

    promag commented at 6:51 pm on October 4, 2018:
    +1 remove fBad.
    jb55 commented at 8:49 pm on October 4, 2018:

    +0 sounds good to me.

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

    practicalswift commented at 8:54 pm on October 4, 2018:
    @jb55 Thanks for letting me know about the squash mixup. Now fixed :-)
    jnewbery commented at 5:24 am on October 8, 2018:
    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.
  13. MarcoFalke referenced this in commit bfaeb84f0b on Oct 4, 2018
  14. practicalswift force-pushed on Oct 4, 2018
  15. promag commented at 6:54 pm on October 4, 2018: member
    Should explicitly set --min-confidence? Currently it’s 60.
  16. practicalswift force-pushed on Oct 4, 2018
  17. practicalswift commented at 8:56 pm on October 4, 2018: contributor
    @promag Judging from my testing of vulture over the set of the 196 mergeable open PR:s the default of 60 seems reasonable. Should we keep the default for now?
  18. promag commented at 9:07 pm on October 4, 2018: member

    My suggestion is to add --min-confidence 60.

    Also, from the vulture documentation:

    We recommend using whitelists instead of –ignore-names or –ignore-decorators whenever possible

    WDYT?

  19. practicalswift force-pushed on Oct 4, 2018
  20. practicalswift commented at 9:52 pm on October 4, 2018: contributor

    @promag I’ve not added an explicit --min-confidence 60.

    Regarding the white-list method: I’m afraid a whitelist replicating the current wildcard based method would be quite long. And perhaps burdensome to keep updated.

    The trade-off looks like this:

    0    --ignore-names "argtypes,connection_lost,connection_made,converter,data_received,\
1        daemon,errcheck,msg_generic,on_*,optionxform,restype,skip_if_no_cli"

    Versus:

    0$ vulture --min-confidence 60 --make-whitelist $(git ls-files -- "*.py" ":(exclude)contrib/") | wc -l
1110

    Despite the recommendation I think --ignore-names might be the better option for us since we need the wildcard functionality.

  21. in test/lint/lint-python-dead-code.sh:18 in 2348ebf8f4 outdated 
    13+    exit 0
14+fi
15+
16+vulture \
17+    --min-confidence 60 \
18+    --ignore-names "argtypes,connection_lost,connection_made,converter,data_received,daemon,errcheck,msg_generic,on_*,optionxform,restype,skip_if_no_cli" \
    MarcoFalke commented at 9:56 pm on October 4, 2018:
    why is skip_if_no_cli ignored?
    practicalswift commented at 7:14 am on October 5, 2018:
    Nice catch! Now removed!
  22. promag commented at 9:57 pm on October 4, 2018: member

    @promag I’ve not added an explicit --min-confidence 60.

    You have.

  23. practicalswift force-pushed on Oct 5, 2018
  24. practicalswift commented at 7:14 am on October 5, 2018: contributor

    @promag I’ve not added an explicit --min-confidence 60.

    You have.

    Oh, “not” should have been “now” :-D

  25. ken2812221 commented at 4:44 pm on October 6, 2018: contributor
    utACK c422cf2b50b08b489172de28f69083784c35a455
  26. jb55 commented at 11:19 am on October 8, 2018: member
    utACK c422cf2b50b08b489172de28f69083784c35a455
  27. conscott commented at 3:38 pm on October 12, 2018: contributor
    utACk c422cf2b50b08b489172de28f69083784c35a455 - vulture is great!
  28. practicalswift commented at 12:19 pm on October 16, 2018: contributor

    Added a commit which pins the vulture version to avoid the possibility of having the build broken if a new vulture version being released.

    Please re-review :-)

  29. DrahtBot added the label Needs rebase on Oct 20, 2018
  30. practicalswift force-pushed on Oct 20, 2018
  31. practicalswift commented at 0:11 am on October 21, 2018: contributor
    Rebased! :-)
  32. DrahtBot removed the label Needs rebase on Oct 21, 2018
  33. DrahtBot commented at 7:31 pm on October 27, 2018: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #14505 (Make all single parameter constructors explicit (C++11) by practicalswift)
    • #13728 (Scripts and tools: Run the CI lint stage on mac by Empact)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  34. tests: Remove unused testing code 590a57fdec
  35. in test/functional/test_framework/key.py:146 in 7c2dd72227 outdated 
    140-        return ecdh_keybuffer.raw
141-
142-    def get_ecdh_key(self, other_pubkey, kdf=lambda k: hashlib.sha256(k).digest()):
143-        # FIXME: be warned it's not clear what the kdf should be as a default
144-        r = self.get_raw_ecdh_key(other_pubkey)
145-        return kdf(r)
    MarcoFalke commented at 4:56 pm on November 7, 2018:
    This is a library (ECC secp256k1 OpenSSL wrapper), so removing the wrapper functions might break code that is not in this commit. (Maybe a different branch or repo, and it will break on merge or rebase)
    practicalswift commented at 5:10 pm on November 7, 2018:
    Good point! Updated. No longer removing these functions. Please re-review :-)
  36. practicalswift force-pushed on Nov 7, 2018
  37. tests: Add Python dead code linter (vulture) c82190cdb6
  38. practicalswift force-pushed on Nov 7, 2018
  39. MarcoFalke commented at 5:22 pm on November 7, 2018: member
    Going to merge this since it had 3 utACKs and is probably not worth to wait for all re-ACKs.
  40. MarcoFalke merged this on Nov 7, 2018
  41. MarcoFalke closed this on Nov 7, 2018
  42. MarcoFalke referenced this in commit d26d15c6c8 on Nov 7, 2018
  43. jasonbcox referenced this in commit 6ef2cd8aea on Oct 20, 2020
  44. practicalswift deleted the branch on Apr 10, 2021
  45. knst referenced this in commit edd2d3b002 on Aug 10, 2021
  46. pravblockc referenced this in commit 81b358bae3 on Aug 11, 2021
  47. pravblockc referenced this in commit 3e7efd5f32 on Aug 11, 2021
  48. pravblockc referenced this in commit 1ee408473a on Aug 11, 2021
  49. pravblockc referenced this in commit 19e8d9664e on Aug 12, 2021
  50. pravblockc referenced this in commit 12047d77d0 on Aug 12, 2021
  51. Munkybooty referenced this in commit 09d686d531 on Sep 16, 2021
  52. linuxsh2 referenced this in commit 4fdaf1ffde on Sep 16, 2021
  53. Munkybooty referenced this in commit e776359f37 on Sep 17, 2021
  54. Munkybooty referenced this in commit eb3976eaa0 on Sep 17, 2021
  55. Munkybooty referenced this in commit 854a38e0e5 on Sep 20, 2021
  56. Munkybooty referenced this in commit 8dd08373ab on Sep 21, 2021
  57. kittywhiskers referenced this in commit 315650c80e on Oct 12, 2021
  58. gades referenced this in commit 199e30639d on May 11, 2022
  59. DrahtBot locked this on Aug 16, 2022


practicalswift promag MarcoFalke jb55 JustinTArthur jnewbery ken2812221 conscott DrahtBot

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me