[test] Remove all * imports

kallewoof commented at 1:35 am on May 9, 2017: member

Another take at #9876 (I didn’t realize it existed, but it’s closed and I really do think we should get rid of import *s if possible!).

Main difference that I can see: each commit corresponds to one file.

I attempted to pull imported imports out as much as possible, e.g. instead of importing Decimal from util.py, it is now imported directly from decimal. I may have missed some of these.

Options, please advice:

Discard this PR as redundant in favor of outcome in discussion in #9876 (__all__) – personally I don’t think this approach is ideal, as it breeds bad behavior from example (import * is considered bad behavior, even if __all__ makes it more controllable) – or in favor of reopening #9876.
Split into multiple PR’s to spread impact out over time, e.g. “p2p-*, mempool_*, …”.
Take as is or squashed in some fashion (p2p-*, mempool_*, …)

fanquake added the label Tests on May 9, 2017

practicalswift commented at 8:13 am on May 9, 2017: contributor

utACK 6e43829

Very good! This will make Python linting easier!

sdaftuar commented at 3:40 pm on May 10, 2017: member

I still question this goal, because I think having to (for example) manually add each script opcode as an import when you want to use one in a test is needlessly time-wasting for test writers, but for now I’d like to make the more important observation that merging this will likely cause substantive PRs to need to be rebased (punishment for actually including or updating a test!).

For reference, my earlier comments on this idea: #9876 (comment)

practicalswift commented at 3:58 pm on May 10, 2017: contributor

@sdaftuar Do you have any examples of substantive PRs that would need to be rebased after merging this one?

MarcoFalke commented at 4:14 pm on May 10, 2017: member

This will make Python linting easier!

Even though I can understand the desire to have a single style or converge the whole python code base to pep8, I tend to object based on the maintenance overhead. The python syntax is already rather strict and currently we should not make it more strict without clear reasoning. The thread Suhas liked to, explains why wildcards could be useful to write tests more productively and with less hassle.

Please note that we don’t have a general coding guide for python code at this point and refactoring for the sake of pep8 without previous discussion might cause more hassle than it is worth.

So Concept NACK on this pull for now, as it would conflict with the ongoing test_framework refactoring. More precisely, it would go in the wrong direction, when i see + stop_node in the diff, which is planned to be made a private method of the test framework, so should not be added to the list of imports.

On Wed, May 10, 2017 at 5:40 PM, Suhas Daftuar notifications@github.com wrote:

I still question this goal, because I think having to (for example) manually add each script opcode as an import when you want to use one in a test is needlessly time-wasting for test writers, but for now I’d like to make the more important observation that merging this will cause substantive PRs to all need to be rebased (punishment for actually including or updating a test!).

For reference, my earlier comments on this idea: #9876 (comment) https://github.com/bitcoin/bitcoin/pull/9876

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10366#issuecomment-300523187, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv1zMoNpRFfmh65687T0aL6yBNvJfks5r4dp7gaJpZM4NUs4F .

kallewoof commented at 4:20 pm on May 10, 2017: member

@sdaftuar I have to respectfully disagree. Adding opcodes as you use them makes perfect sense to me. It also gives you an overview of what opcodes are actually being checked, as an added bonus, just by looking at the imports and not the tests themselves (which can be a bit long-winded at times, unfortunately). I also have a hard time believing this PR would cause conflicts in a lot of test PRs, and I am fairly sure most of the test developers agree that * imports are just generally bad. @MarcoFalke It’s no so much about a style but about proper development. Importing * means you lose control over what is going where, just like rampant usage of using namespace xyz in C++. In core proper it can be flat out dangerous and lead to unpredictable behavior with version upgrades, which may not necessarily be the case with functional tests, but it’s still a step in the right direction.

MarcoFalke commented at 4:26 pm on May 10, 2017: member

I agree it is a step in the right direction, but right now is not a good time to do global refactors that stall progress of other pulls. I think we should wait until less refactoring happens in the core files of the test framework (such as https://github.com/bitcoin/bitcoin/pull/10359/files#diff- bf6de4c07d2e9114c8f01eea45c6cab3L11) and revisit this pull after the dust has settled.

On Wed, May 10, 2017 at 6:20 PM, kallewoof notifications@github.com wrote:

@sdaftuar https://github.com/sdaftuar I have to respectfully disagree. Adding opcodes as you use them makes perfect sense to me. It also gives you an overview of what opcodes are actually being checked, as an added bonus, just by looking at the imports and not the tests themselves (which can be a bit long-winded at times, unfortunately). I also have a hard time believing this PR would cause conflicts in a lot of test PRs, and I am fairly sure most of the test developers agree that * imports are just generally bad.

@MarcoFalke https://github.com/MarcoFalke It’s no so much about a style but about proper development. Importing * means you lose control over what is going where, just like rampant usage of using namespace xyz in C++. In core proper it can be flat out dangerous and lead to unpredictable behavior with version upgrades, which may not necessarily be the case with functional tests, but it’s still a step in the right direction.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10366#issuecomment-300535680, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmvwlNzvLJbPYa-xieJMvnPjetCp2Eks5r4ePhgaJpZM4NUs4F .

practicalswift commented at 8:36 pm on May 10, 2017: contributor

@MarcoFalke When mentioning the linting improvements I was specifically thinking about finding unused imports (see below on why that matters), not full PEP-8 compliance :-)

Explicit imports in tests make it easier to get a quick feeling for what a specific test case covers. And perhaps more importantly, by using explicit imports while at the same time removing unused imports it makes it very clear what is not covered by a specific test case (not imported ⇔ not tested).

Strong concept ACK!

MarcoFalke commented at 8:53 pm on May 10, 2017: member

The tests in /functional/ are not supposed to test the test framework, but the bitcoind/qt api and internals, so test coverage can not be inferred from which utility methods are (or are not) imported from the test framework.

by using explicit imports while at the same time removing unused imports

As mentioned earlier, removing unused imports is a cleanup that should happen every couple of months, not on a regular basis… Another reason against explicit imports is that people will start opening daily pulls to sort all of those alphabetically (see #10302)

On Wed, May 10, 2017 at 10:36 PM, practicalswift notifications@github.com wrote:

@MarcoFalke When mentioning the linting improvements I was specifically thinking about finding unused imports (see below on why that matters), not full PEP-8 compliance :-)

Explicit imports in tests make it easier to get a quick feeling for what a specific test case covers. And perhaps more importantly, by using explicit imports while at the same time removing unused imports it makes it very clear what is not covered by a specific test case (not imported ⇔ not tested).

Strong concept ACK!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

practicalswift commented at 8:59 pm on May 10, 2017: contributor

@MarcoFalke Let’s agree that sorting imports alphabetically daily is a waste of time :-) That kind of PR:s can perhaps be avoided by adding a clear policy in developer-notes.md.

ryanofsky commented at 11:17 pm on May 10, 2017: member

I think this is a good change because import * can make unfamiliar python code really hard to read (you have no idea where identifiers are coming from).

I also think @sdaftuar has a good point about script opcodes, and if there could be a solution for referencing opcodes that doesn’t rely on import * before making this change, that would also make this PR smaller and a little easier to review. Maybe the solution could be defining an Op class and replacing OP_RETURN with Op.RETURN, etc. Or just accepting strings like "OP_RETURN".

kallewoof commented at 1:20 am on May 11, 2017: member

@ryanofsky There is the

0import test_framework.script as script
1[...]
2        p2sh_program = CScript([script.OP_TRUE])
3[...]

approach. A bit verbose perhaps, but better than import *.

kallewoof commented at 1:59 am on May 11, 2017: member

@MarcoFalke

I agree it is a step in the right direction, but right now is not a good time to do global refactors that stall progress of other pulls. I think we should wait until less refactoring happens in the core files of the test framework (such as https://github.com/bitcoin/bitcoin/pull/10359/files#diff- bf6de4c07d2e9114c8f01eea45c6cab3L11) and revisit this pull after the dust has settled.

I don’t see what would be stalled by replacing the import statements with explicit ones, aside from the cases of new imports. I’m perfectly content waiting on #10359 and other PR’s that are considered high priority though, but I am also fairly certain @jnewbery (author of #10359) is all for this PR being merged, considering he tried the same thing in #9876.

practicalswift commented at 12:40 pm on July 17, 2017: contributor

Needs rebase :-)

[test] test_framework/blockstore.py: replace * imports 0fdde242bf

[test] test_framework/blocktools.py: replace * imports 940af7a987

[test] test_framework/comptool.py: replace * imports e83dece365

kallewoof force-pushed on Jul 18, 2017

kallewoof commented at 4:55 am on July 18, 2017: member

Rebased!

kallewoof force-pushed on Jul 18, 2017

[test] abandonconflict.py: remove * imports d33f97beac

[test] bip9-softforks.py: remove * imports 42f736bb43

[test] bip65-cltv-p2p.py: remove * imports e9be2bddca

[test] bip65-cltv.py: remove * imports e7c2d56d96

[test] bip68-112-113-p2p.py: remove * imports 56cc340318

[test] bip68-sequence.py: remove * imports 2cf1ec7ee6

[test] bipdersig-p2p.py: remove * imports 681cfdbdd7

[test] bipdersig.py: remove * imports 4e57daba45

kallewoof force-pushed on Jul 18, 2017

[test] bumpfee.py: remove * imports f8aa39221a

[test] decodescript.py: remove * imports 69f6c77860

[test] disablewallet.py: remove * imports 65c5305b2a

kallewoof force-pushed on Jul 18, 2017

practicalswift commented at 7:56 am on July 18, 2017: contributor

Thanks for rebasing.

It appears as if test/functional/merkle_blocks.py needs to import assert_raises_jsonrpc from test_framework.util :-)

The use of assert_raises_jsonrpc was introduced in @jnewbery:s commit 6294f3283a5b6919795621dc067ec80c0cd2a334.

[test] forknotify.py: remove * imports db2c5b9bc8

[test] fundrawtransaction.py: remove * imports 735118eb3c

[test] getblocktemplate_longpoll.py: remove * imports 2dc0b69618

[test] mining.py: remove * imports ee8bbbb277

[test] httpbasics.py: remove * imports 34a07fdedd

[test] importmulti.py: remove * imports 55287a17b7

[test] importprunedfunds.py: remove * imports 62430ca6a8

[test] invalidateblock.py: remove * imports 36cd61e156

[test] invalidblockrequest.py: remove * imports acb71d4a47

[test] invalidtxrequest.py: remove * imports d3be24d493

[test] keypool.py: remove * imports 5533d2c920

[test] listtransactions.py: remove * imports 2acf11c1aa

[test] maxuploadtarget.py: remove * imports 5e22da0711

[test] mempool_limit.py: remove * imports a36b001367

[test] mempool_packages.py: remove * imports b0179137a5

[test] mempool_persist.py: remove * imports d4cd9207cb

[test] mempool_reorg.py: remove * imports 7ae971b245

[test] mempool_resurrect_test.py: remove * imports dbefb190b1

[test] mempool_spendcoinbase.py: remove * imports 60066f7ced

[test] merkle_blocks.py: remove * imports 6205bf70c5

[test] nulldummy.py: remove * imports e905694d40

[test] p2p-acceptblock.py: remove * imports 48f242707b

[test] p2p-compactblocks.py: remove * imports 024fdb8388

[test] p2p-feefilter.py: remove * imports 6d58b448ba

[test] p2p-fullblocktest.py: remove * imports 098f0b9098

[test] p2p-leaktests.py: remove * imports 09e3182dd9

[test] p2p-mempool.py: remove * imports f4fc67a71c

[test] p2p-segwit.py: remove * imports 7538a1c03c

[test] p2p-timeouts.py: remove * imports 9d048a05cd

[test] p2p-versionbits-warning.py: remove * imports a88e77c95a

[test] prioritise_transaction.py: remove * imports 4d12fe7c89

[test] rawtransactions.py: remove * imports 7b0a6ba61a

[test] receivedby.py: remove * imports 6f5008229b

[test] replace-by-fee.py: remove * imports 9158b544c9

[test] rest.py: remove * imports 01e91098ee

[test] rpcbind_test.py: remove * imports dc8fae851b

[test] segwit.py: remove * imports 7ca8e960cd

[test] sendheaders.py: remove * imports 92cf440f8a

[test] pruning.py: remove * imports 9f732de38d

[test] signrawtransactions.py: remove * imports 05fb6342fd

[test] smartfees.py: remove * imports f0271da3f9

[test] txn_clone.py: remove * imports 148f6c03f7

[test] txn_doublespend.py: remove * imports 929a7d2172

[test] wallet.py: remove * imports 89dfb5e470

[test] walletbackup.py: remove * imports 306b4bef8d

kallewoof force-pushed on Jul 18, 2017

MarcoFalke added the label Brainstorming on Jul 18, 2017

MarcoFalke added this to the milestone Future on Jul 18, 2017

jtimon commented at 8:57 am on September 23, 2017: contributor

Concept ACK, needs rebase

EDIT: Although I think it’s fine to have small exceptions like for the example of the opcodes (these examples can be separated to their own files).

kallewoof commented at 5:50 am on September 25, 2017: member

@jtimon That might make sense, yeah. (As for rebasing, I will hold off on that until I know we are ready to move forward with this.)

laanwj commented at 6:53 pm on March 7, 2018: member

Closing this, this seems to be somewhat controversial, and that’s not desirable for something that makes changes all over the test framework and causes lots of necessary rebases of other commits.

laanwj closed this on Mar 7, 2018

MarcoFalke removed this from the milestone Future on Aug 12, 2018

kallewoof deleted the branch on Aug 13, 2018

MarcoFalke locked this on Sep 8, 2021

[test] Remove all * imports #10366