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

practicalswift · 2018-04-22T18:10:39Z

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:

$ contrib/devtools/lint-python.sh | head -10
./test/functional/feature_rbf.py:8:1: F403 'from test_framework.util import *' used; unable to detect undefined names
./test/functional/feature_rbf.py:9:1: F403 'from test_framework.script import *' used; unable to detect undefined names
./test/functional/feature_rbf.py:10:1: F403 'from test_framework.mininode import *' used; unable to detect undefined names
./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
./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
./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
./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
./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
./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
./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
$

After this commit:

$ contrib/devtools/lint-python.sh | head -10
$

jnewbery · 2018-04-22T22:23:27Z

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.

practicalswift · 2018-04-23T06:57:49Z

@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 :-)

maflcko · 2018-04-24T15:45:06Z

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.

practicalswift · 2018-04-24T16:27:15Z

@MarcoFalke That's a convenient workflow! What tool do you use? dewildcard?

practicalswift · 2018-04-26T07:20:08Z

@ryanofsky @JeremyRubin @jtimon @kallewoof Would you mind reviewing? :-)

JeremyRubin · 2018-04-27T01:55:44Z

@practicalswift I think my comment #9876 (comment) still stands, but I have no strong opinion on this.

practicalswift · 2018-05-04T14:12:51Z

Rebased!

kallewoof · 2018-05-07T10:28:43Z

Concept ACK, obviously.

practicalswift · 2018-05-11T06:55:30Z

Rebased!

sipa · 2018-05-11T19:18:00Z

This seems like a sensible policy, though I leave the decision up to people who more often work on the Python code.

practicalswift · 2018-05-12T19:20:10Z

Rebased! :-)

laanwj · 2018-05-14T12:41:07Z

I don't care enough to NACK this, though it annoys me slightly if things that are rejected come up again and again.

practicalswift · 2018-05-14T12:45:17Z

@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 :-)

sdaftuar · 2018-05-18T21:58:30Z

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...

kallewoof · 2018-05-19T01:14:48Z

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?

practicalswift · 2018-05-19T07:38:42Z

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.

ken2812221 · 2018-05-19T08:57:53Z

Concept ACK

conscott

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:

F999 <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.

ken2812221 · 2018-07-16T12:49:42Z

utACK ea71fac1f228ec5525084b82c9c2c08a3517c692

practicalswift · 2018-08-13T11:56:02Z

Rebased! Please re-review :-)

maflcko · 2018-08-13T12:24:25Z

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)

laanwj · 2018-08-13T12:31:41Z

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.

maflcko · 2018-08-13T12:43:20Z

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.

kallewoof · 2018-08-13T13:04:56Z

@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

[tests] Cleanup wildcard imports in functional tests #11274 - closed as a duplicate of [test] Remove all * imports #10366
[test] Remove all * imports #10366 - closed (unexpectedly) by someone other than the author
Remove wildcard imports from qa #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.

laanwj · 2018-08-13T13:08:27Z

Yes, I agree in this case.
It was probably best to merge this.

…ts are disallowed f618c58 Docs: Update python docs to reflect that wildcard imports are disallowed (Ben Woosley) Pull request description: These have been disallowed via flake8 since: #13054 Tree-SHA512: f41651fd883e3786a7e87c4aa4c54749308e576287f2f88da3f1d8d0f59e14519d99061f1efd05b8745f495f87a14286cea576f1507d10ccd226f8cf2f2b3cc8

…d imports are disallowed f618c58 Docs: Update python docs to reflect that wildcard imports are disallowed (Ben Woosley) Pull request description: These have been disallowed via flake8 since: bitcoin#13054 Tree-SHA512: f41651fd883e3786a7e87c4aa4c54749308e576287f2f88da3f1d8d0f59e14519d99061f1efd05b8745f495f87a14286cea576f1507d10ccd226f8cf2f2b3cc8

…ames in Python tests scripts. Remove wildcard imports. 68400d8 tests: Use explicit imports (practicalswift) Pull request description: 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: ``` $ contrib/devtools/lint-python.sh | head -10 ./test/functional/feature_rbf.py:8:1: F403 'from test_framework.util import *' used; unable to detect undefined names ./test/functional/feature_rbf.py:9:1: F403 'from test_framework.script import *' used; unable to detect undefined names ./test/functional/feature_rbf.py:10:1: F403 'from test_framework.mininode import *' used; unable to detect undefined names ./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 ./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 ./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 ./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 ./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 ./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 ./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 $ ``` After this commit: ``` $ contrib/devtools/lint-python.sh | head -10 $ ``` Tree-SHA512: 3f826d39cffb6438388e5efcb20a9622ff8238247e882d68f7b38609877421b2a8e10e9229575f8eb6a8fa42dec4256986692e92922c86171f750a0e887438d9

practicalswift force-pushed the imports branch 3 times, most recently from b5986af to b403bc6 Compare April 22, 2018 18:43

fanquake added the Tests label Apr 23, 2018

practicalswift force-pushed the imports branch 2 times, most recently from 8f13454 to b3e92de Compare April 24, 2018 16:16

practicalswift force-pushed the imports branch from b3e92de to 67ab9c0 Compare May 4, 2018 14:12

practicalswift force-pushed the imports branch 2 times, most recently from 50865e9 to 60c99c2 Compare May 5, 2018 13:18

practicalswift force-pushed the imports branch from 60c99c2 to 1d648eb Compare May 11, 2018 06:54

practicalswift force-pushed the imports branch from 1d648eb to 4b7d66d Compare May 12, 2018 19:19

practicalswift force-pushed the imports branch 2 times, most recently from 5138f07 to 065c061 Compare May 14, 2018 08:19

This was referenced Jul 16, 2018

Tests: Cleanup create_transaction implementations #13669

Merged

test: allow arguments to be forwarded to flake8 in lint-python.sh #13649

Merged

conscott reviewed Jul 16, 2018

View reviewed changes

DrahtBot added the Needs rebase label Aug 9, 2018

practicalswift force-pushed the imports branch from ea71fac to e020677 Compare August 10, 2018 11:53

DrahtBot removed the Needs rebase label Aug 10, 2018

practicalswift force-pushed the imports branch from e020677 to ac615af Compare August 13, 2018 11:55

tests: Use explicit imports

68400d8

practicalswift force-pushed the imports branch from ac615af to 68400d8 Compare August 13, 2018 12:14

maflcko merged commit 68400d8 into bitcoin:master Aug 13, 2018

Bushstar mentioned this pull request Aug 13, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#358

Merged

Empact mentioned this pull request Jan 25, 2019

Docs: Update python docs to reflect that wildcard imports are disallowed #15249

Merged

practicalswift deleted the imports branch April 10, 2021 19:35

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

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

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

Uh oh!

Conversation

practicalswift commented Apr 22, 2018

Uh oh!

jnewbery commented Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Apr 24, 2018

Uh oh!

practicalswift commented Apr 24, 2018

Uh oh!

practicalswift commented Apr 26, 2018

Uh oh!

JeremyRubin commented Apr 27, 2018

Uh oh!

practicalswift commented May 4, 2018

Uh oh!

kallewoof commented May 7, 2018

Uh oh!

practicalswift commented May 11, 2018

Uh oh!

sipa commented May 11, 2018

Uh oh!

practicalswift commented May 12, 2018

Uh oh!

laanwj commented May 14, 2018

Uh oh!

practicalswift commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdaftuar commented May 18, 2018

Uh oh!

kallewoof commented May 19, 2018

Uh oh!

practicalswift commented May 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken2812221 commented May 19, 2018

Uh oh!

conscott left a comment

Choose a reason for hiding this comment

Uh oh!

ken2812221 commented Jul 16, 2018

Uh oh!

practicalswift commented Aug 13, 2018

Uh oh!

maflcko commented Aug 13, 2018

Uh oh!

laanwj commented Aug 13, 2018

Uh oh!

maflcko commented Aug 13, 2018

Uh oh!

kallewoof commented Aug 13, 2018

Uh oh!

laanwj commented Aug 13, 2018

Uh oh!

Uh oh!

jnewbery commented Apr 22, 2018 •

edited

Loading

practicalswift commented Apr 23, 2018 •

edited

Loading

practicalswift commented May 14, 2018 •

edited

Loading

practicalswift commented May 19, 2018 •

edited

Loading