tests: Enable automatic detection of undefined names in Python tests scripts. Remove wildcard imports. #13054

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:imports changing 50 files +143 −94
  1. practicalswift commented at 6:10 pm on April 22, 2018: contributor

    Enable automatic detection of undefined names in Python tests scripts. Remove wildcard imports.

    Wildcard imports make it unclear which names are present in the namespace, confusing both readers and many automated tools.

    An additional benefit of not using wildcard imports in tests scripts is that readers of a test script then can infer the rough testing scope just by looking at the imports.

    Before this commit:

     0$ contrib/devtools/lint-python.sh | head -10
 1./test/functional/feature_rbf.py:8:1: F403 'from test_framework.util import *' used; unable to detect undefined names
 2./test/functional/feature_rbf.py:9:1: F403 'from test_framework.script import *' used; unable to detect undefined names
 3./test/functional/feature_rbf.py:10:1: F403 'from test_framework.mininode import *' used; unable to detect undefined names
 4./test/functional/feature_rbf.py:15:12: F405 bytes_to_hex_str may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
 5./test/functional/feature_rbf.py:17:58: F405 CScript may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
 6./test/functional/feature_rbf.py:25:13: F405 COIN may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
 7./test/functional/feature_rbf.py:26:31: F405 satoshi_round may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
 8./test/functional/feature_rbf.py:26:60: F405 COIN may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
 9./test/functional/feature_rbf.py:30:41: F405 satoshi_round may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
10./test/functional/feature_rbf.py:30:68: F405 COIN may be undefined, or defined from star imports: test_framework.mininode, test_framework.script, test_framework.util
11$

    After this commit:

    0$ contrib/devtools/lint-python.sh | head -10
1$
  2. practicalswift force-pushed on Apr 22, 2018
  3. practicalswift force-pushed on Apr 22, 2018
  4. practicalswift force-pushed on Apr 22, 2018
  5. jnewbery commented at 10:23 pm on April 22, 2018: member

    This is the fourth attempt at this (see #9876, #10366 and #11274).

    I’m a big concept ACK. Wildcard imports are bad for the reasons you’ve listed. However, this has been NACKed by other contributors and none of us have yet been successful in getting it merged.

  6. fanquake added the label Tests on Apr 23, 2018
  7. practicalswift commented at 6:57 am on April 23, 2018: contributor
    @jnewbery Thanks for the big concept ACK! One difference with this PR compared to the previous ones is that this makes sure no wildcard imports are re-introduced in the future thanks to the Travis check. So this should hopefully be the last time we have to think about wildcard imports :-)
  8. MarcoFalke commented at 3:45 pm on April 24, 2018: member
    Just as a note: As a work around when writing tests, I replace the import list with a wildcard-star and then run some de-wildcard python script before submitting the pull request.
  9. practicalswift force-pushed on Apr 24, 2018
  10. practicalswift force-pushed on Apr 24, 2018
  11. practicalswift commented at 4:27 pm on April 24, 2018: contributor
    @MarcoFalke That’s a convenient workflow! What tool do you use? dewildcard?
  12. practicalswift commented at 7:20 am on April 26, 2018: contributor
    @ryanofsky @JeremyRubin @jtimon @kallewoof Would you mind reviewing? :-)
  13. JeremyRubin commented at 1:55 am on April 27, 2018: contributor
    @practicalswift I think my comment #9876 (comment) still stands, but I have no strong opinion on this.
  14. practicalswift force-pushed on May 4, 2018
  15. practicalswift commented at 2:12 pm on May 4, 2018: contributor
    Rebased!
  16. practicalswift force-pushed on May 5, 2018
  17. practicalswift force-pushed on May 5, 2018
  18. kallewoof commented at 10:28 am on May 7, 2018: member
    Concept ACK, obviously.
  19. practicalswift force-pushed on May 11, 2018
  20. practicalswift commented at 6:55 am on May 11, 2018: contributor
    Rebased!
  21. sipa commented at 7:18 pm on May 11, 2018: member
    This seems like a sensible policy, though I leave the decision up to people who more often work on the Python code.
  22. practicalswift force-pushed on May 12, 2018
  23. practicalswift commented at 7:20 pm on May 12, 2018: contributor
    Rebased! :-)
  24. practicalswift force-pushed on May 14, 2018
  25. practicalswift force-pushed on May 14, 2018
  26. laanwj commented at 12:41 pm on May 14, 2018: member
    I don’t care enough to NACK this, though it annoys me slightly if things that are rejected come up again and again.
  27. practicalswift commented at 12:45 pm on May 14, 2018: contributor

    @laanwj One significant difference with this PR compared to the previous ones is that this makes sure no wildcard imports are ever re-introduced in the future thanks to the Travis check.

    So this should hopefully be the last time any of us has to think about wildcard imports again :-)

  28. sdaftuar commented at 9:58 pm on May 18, 2018: member

    though it annoys me slightly if things that are rejected come up again and again.

    I feel the same way, and I’m still opposed to this idea – I don’t think we should be increasing the effort required for getting tests written and merged.

    My impression lately (which is admittedly completely unscientific so maybe others have a different impression) is that PRs that include tests seem to have a higher burden than those that don’t, because it adds more surface area for review and potential for rebase (and it’s easy to nit someone’s test code). I think this leads to too few tests in our project’s PRs, as it is – I don’t think we should make this any worse by throwing up more hurdles.

    But I don’t know, there’s lots of PRs like this one that I think are a waste of our time, but since this particular topic keeps coming up, if people want to spend their time on these kinds of things, I won’t stand in the way…

  29. kallewoof commented at 1:14 am on May 19, 2018: member

    I feel the same way, and I’m still opposed to this idea – I don’t think we should be increasing the effort required for getting tests written and merged.

    It is not an increase in effort to throw an error. That’s like saying our compilers should throw less errors cause the programmers get grumpy. Do you want more programmers who need a more gentle compiler?

    To reiterate the comparison, if a contributor gives up because of automated errors they will probably give up halfway through review anyway. Why should we care so much about the imaginary lazy developer?

  30. practicalswift commented at 7:38 am on May 19, 2018: contributor

    I see two possible states of the world:

    • State 1 – Travis does not check wildcard imports: A Python PR introducing wildcard imports is opened. During the review process human reviewers (such as perhaps me, @kallewoof, @jnewbery, @JeremyRubin and @jtimon - who have historically voiced their concern about wildcard imports) will point out that wildcard imports are problematic and discouraged. The human reviewer will explain that wildcard imports (from <module> import *) should be avoided, as they make it unclear which names are present in the namespace, confusing both readers and many automated tools. After some discussion the PR author will switch to using explicit imports. The normal review process takes place focusing on non-trivial/non-mechanical stuff.

    • State 2 – Travis does check wildcard imports: A Python PR introducing wildcard imports is opened. Travis complains within literally one minute. The PR author switches to using explicit imports before the ordinary review process starts . The normal review process takes place focusing on non-trivial/non-mechanical stuff.

    Energy used by the different participants:

    • Energy used by PR author assuming state 1: Discussing review (D). Adjusting code (A).
    • Energy used by PR author assuming state 2: Adjusting code (A).
    • Energy used by n human reviewers assuming state 1: Reviewing (n * R), where n > 0.
    • Energy used by n human reviewers assuming state 2: None (0).

    Net effects:

    • Energy used assuming state 1: D + A + n * R
    • Energy used assuming state 2: A

    Conclusion: It seems like all participants – PR author and human reviewers – are better off in state 2. What are compelling arguments for choosing state 1?

    Please let me know if any of the assumptions stated above is incorrect.

  31. ken2812221 commented at 8:57 am on May 19, 2018: contributor
    Concept ACK
  32. sdaftuar commented at 1:15 pm on May 19, 2018: member

    During the review process human reviewers (such as perhaps me, @kallewoof, @jnewbery, @JeremyRubin and @jtimon - who have historically voiced their concern about wildcard imports) will point out that wildcard imports are problematic and discouraged.

    I will refrain from debating particular style points (so whatever you guys want to do, go nuts). But as a matter of process, I don’t think style nits for things that are not part of the project’s style guide are appropriate PR comments. If you’d like to enforce a project-wide policy, I think you should start a discussion about that in a PR that would update the style guide. Otherwise we should expect people with different personal style preferences to object for things that seem arbitrary.

    Do we even have python style guidelines anywhere in our tree, apart from this linter in contrib/? If we’re going to impose rules, we should document them so contributors know what is expected.

    EDIT: I found this in test/functional/README.md:

    Style guidelines

    • Where possible, try to adhere to PEP-8 guidelines
    • Use a python linter like flake8 before submitting PRs to catch common style nits (eg trailing whitespace, unused imports, etc)
    • Avoid wildcard imports where possible
    • Use a module-level docstring to describe what the test is testing, and how it is testing it.
    • When subclassing the BitcoinTestFramwork, place overrides for the set_test_params(), add_options() and setup_xxxx() methods at the top of the subclass, then locally-defined helper methods, then the run_test() method.

    I think this is a bit unspecific if the project’s rules now are that a PR will not be accepted unless the linter passes; someone should update this to reflect the project’s new philosophy on python style.

  33. jamesob commented at 3:15 pm on May 19, 2018: member

    Concept ACK.

    Agree with @sdaftuar’s point that it isn’t fair to make style comments on PRs where the cited rule isn’t present in a linter or styleguide. IMO “no wildcard imports” is inherent in “follow PEP8.”

    Allowing wildcard imports degrades development experience in Python significantly because it reduces the effectiveness of in-editor linters like flake8. Once you allow wildcard imports, you can no longer detect legitimately undefined symbols. I think a change like this will make test development a bit more pleasant.

  34. practicalswift force-pushed on May 29, 2018
  35. practicalswift commented at 10:05 pm on May 29, 2018: contributor
    Rebased!
  36. Empact commented at 7:54 am on June 8, 2018: member
    utACK 6185a96 this seems like a good improvement and low-risk to me
  37. ken2812221 commented at 12:22 pm on June 12, 2018: contributor
    utACK 6185a96
  38. practicalswift force-pushed on Jun 30, 2018
  39. practicalswift force-pushed on Jun 30, 2018
  40. practicalswift commented at 8:39 am on June 30, 2018: contributor
    Rebased!
  41. in test/functional/wallet_txn_doublespend.py:6 in f36fcc4d1e outdated 
    2@@ -3,6 +3,7 @@
3 # Distributed under the MIT software license, see the accompanying
4 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
5 """Test the wallet accounts properly when there is a double-spend conflict."""
6+
    Empact commented at 8:30 pm on July 1, 2018:
    nit: unrelated whitespace change
  42. in test/functional/wallet_basic.py:6 in f36fcc4d1e outdated 
    2@@ -3,6 +3,7 @@
3 # Distributed under the MIT software license, see the accompanying
4 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
5 """Test the wallet."""
6+
    Empact commented at 8:32 pm on July 1, 2018:
    nit: unrelated whitespace change
  43. in test/functional/rpc_fundrawtransaction.py:20 in f36fcc4d1e outdated 
    16@@ -17,7 +17,6 @@
17     find_vout_for_address,
18 )
19 
20-
    Empact commented at 8:32 pm on July 1, 2018:
    nit: unrelated whitespace change
  44. Empact commented at 8:35 pm on July 1, 2018: member
    Could avoid touching three of these files to keep the history clean.
  45. practicalswift force-pushed on Jul 6, 2018
  46. practicalswift commented at 4:40 pm on July 6, 2018: contributor
    @Empact Thanks for reviewing. Feedback addressed! Please re-review.
  47. in test/functional/p2p_segwit.py:85 in ae2483d1fc outdated 
    81@@ -82,6 +82,7 @@
82     sync_mempools,
83 )
84 
85+
    Empact commented at 7:03 pm on July 6, 2018:
    One more
  48. practicalswift force-pushed on Jul 6, 2018
  49. practicalswift commented at 10:11 pm on July 6, 2018: contributor
    @Empact Please re-review :-)
  50. DrahtBot added the label Needs rebase on Jul 9, 2018
  51. practicalswift force-pushed on Jul 10, 2018
  52. DrahtBot removed the label Needs rebase on Jul 11, 2018
  53. practicalswift force-pushed on Jul 11, 2018
  54. practicalswift commented at 8:35 pm on July 11, 2018: contributor
    Rebased!
  55. DrahtBot added the label Needs rebase on Jul 13, 2018
  56. practicalswift force-pushed on Jul 15, 2018
  57. practicalswift commented at 11:07 pm on July 15, 2018: contributor
    Rebased! :-)
  58. DrahtBot removed the label Needs rebase on Jul 16, 2018
  59. conscott commented at 12:24 pm on July 16, 2018: contributor

    Test ACK ea71fac1f228ec5525084b82c9c2c08a3517c692

    Not to get pedantic on this debate, but I totally agree with @jamesob when he says

    Allowing wildcard imports degrades development experience in Python significantly because it reduces the effectiveness of in-editor linters like flake8. Once you allow wildcard imports, you can no longer detect legitimately undefined symbols. I think a change like this will make test development a bit more pleasant.

    I would suspect most other Python programmers also have these “in-editor linters” setup and have encountered this problem as well. This change will significantly reduce the amount of “lint barf” seen when saving test files with wildcards, and allow uncovering real undefined variables.

    If I see the false-positives like:

    0F999 <variable> may be undefined, or defined from star imports: test_framework.blocktools, test_framework.util

    I am likely to ignore everything, whereas fixing the wildcard imports lets me see through the noise.

  60. ken2812221 commented at 12:49 pm on July 16, 2018: contributor
    utACK ea71fac1f228ec5525084b82c9c2c08a3517c692
  61. DrahtBot added the label Needs rebase on Aug 9, 2018
  62. practicalswift force-pushed on Aug 10, 2018
  63. DrahtBot removed the label Needs rebase on Aug 10, 2018
  64. practicalswift force-pushed on Aug 13, 2018
  65. practicalswift commented at 11:56 am on August 13, 2018: contributor
    Rebased! Please re-review :-)
  66. tests: Use explicit imports 68400d8b96
  67. practicalswift force-pushed on Aug 13, 2018
  68. MarcoFalke commented at 12:24 pm on August 13, 2018: member
    This has been brought up too many times and already cost too much discussion. There seems to be some disagreement, but also a lot of Concept ACKs from contributors to tests. I am just going to bite the bullet and merge this now. (Doing before the 0.17.0 branch off to avoid any backport conflicts of future test backports)
  69. MarcoFalke merged this on Aug 13, 2018
  70. MarcoFalke closed this on Aug 13, 2018
  71. ken2812221 referenced this in commit bffb35f876 on Aug 13, 2018
  72. laanwj commented at 12:31 pm on August 13, 2018: member

    On one hand I’m annoyed that apparently, insisting on doing something often enough gets it merged because the people that disagree will stop bothering to complain.

    One the other hand, fine, this specific case is not a big deal.

  73. MarcoFalke commented at 12:43 pm on August 13, 2018: member

    It wasn’t that only a small group of people insisted on doing it, but really a different person each time (see #9876 by @jnewbery, #10366 @kallewoof and #11274 @mess110). Also this one had many Concept ACKs from different contributors (not going to list them, just scroll up a bit).

    So I feel like there is a threshold where something controversial can be merged if it has enough support. Another way of thinking of it, which I sometimes use to decide if a pull request should be merged: “Imagine this already was merged, but someone proposed to revert the change. Would it be worthwhile to merge?”

    Back to this topic, personally I am a huge fan of the __all__ import: https://docs.python.org/3.5/tutorial/modules.html#importing-from-a-package. I’d be in favour to take a step back and replace some of the explicit imports with an __all__ import, where appropriate.

  74. kallewoof commented at 1:04 pm on August 13, 2018: member

    @laanwj

    On one hand I’m annoyed that apparently, insisting on doing something often enough gets it merged because the people that disagree will stop bothering to complain.

    Note that the previous PRs related to this were

    • #11274 - closed as a duplicate of #10366
    • #10366 - closed (unexpectedly) by someone other than the author
    • #9876 - closed for.. some reason. It looks like the author intended to break it into pieces before merging.

    Reading through the comments, there was no one directly disagreeing with doing this. @sdaftuar had some concerns that I believe were addressed, but mostly this was a “lets not do too big changes cause we don’t wanna make big changes” which is a valid reason most of the time, but IMO not here.

    I.e. I don’t think anyone is insisting on something repeatedly in spite of disagreement. We’re all just trying to make this repo as good as it can get with what skills we have.

  75. laanwj commented at 1:08 pm on August 13, 2018: member
    Yes, I agree in this case. It was probably best to merge this.
  76. MarcoFalke referenced this in commit ab46fe6ec1 on Jan 25, 2019
  77. practicalswift deleted the branch on Apr 10, 2021
  78. PastaPastaPasta referenced this in commit d70438987c on Jun 26, 2021
  79. PastaPastaPasta referenced this in commit 5364e8b0ec on Jun 26, 2021
  80. PastaPastaPasta referenced this in commit 25cf48b6f8 on Jun 27, 2021
  81. PastaPastaPasta referenced this in commit 00fed7aa95 on Jun 28, 2021
  82. UdjinM6 referenced this in commit e40d918371 on Jul 4, 2021
  83. UdjinM6 referenced this in commit ba7899bbd1 on Jul 6, 2021
  84. UdjinM6 referenced this in commit 9f4f52ae4e on Jul 6, 2021
  85. gades referenced this in commit 578ea70eec on May 4, 2022
  86. DrahtBot locked this on Aug 18, 2022


practicalswift jnewbery MarcoFalke JeremyRubin kallewoof sipa laanwj sdaftuar ken2812221 jamesob Empact conscott

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-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me