[Tests] Clean and lint the python regtest suite #2452
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fuzzbawls
force-pushed
the
2021_regtest-python-linting
branch
from
4d82e59 to
646175b
Compare
Collaborator
Author
|
rebased to resolve conflicts and fix an issue introduced in #2410 |
random-zebra
suggested changes
Jul 17, 2021
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
This needs rebase.
Aside from that, noticed that the first commit introduces a bunch of changes, regarding import ordering, that are going in the wrong direction (I haven't commented them all).
According to PEP8 styling guideline, and our own comment in example_test.py:
Imports are always put at the top of the file, just after any module comments and docstrings,
and before module globals and constants.
Imports should be grouped in the following order:
1. Standard library imports.
2. Related third party imports.
3. Local application/library specific imports.
You should put a blank line between each group of imports.
fixes the following PEP error F403 # 'from foo_module import *' used; unable to detect undefined names also uses consistent import grouping/ording and standardizes a blank line between the copyright/docstring and the first import
comparison to None should be 'if cond is None:' comparison to None should be 'if cond is not None:'
no newline at end of file
too many leading '#' for block comment
local variable 'n' assigned but not used
E701 # multiple statements on one line (colon)
W605 invalid escape sequence
E703 statement ends with a semicolon
This file isn't used in our tests, so it doesn't need to pass any future lint checks. It is also sorely out of sync with the current test suite and needs updating before implementing it after the point when this test would be useful, so make it's contents a large block comment for now.
Another test that is currently unused, make it pass flak8 linting by disabling unused imports and only outputting a simple log message if run
Lint scripts now have their own home so as to not clutter the `contrib/devtools` path, in preparation for the addition of even more linters.
Add `lint-python.sh` (currently constrained to `test/functional`) Add `lint-all.sh` (catch-all script to run any lint-*.sh file) The GA script now runs `lint-all.sh` on every run, meaning that `lint-whitespace.sh` and `lint-qt.sh` are also now run on every CI run. New lint scripts after this need only to be added to the `test/lint/` path to be included in CI runs.
Two test scripts define a subclass of P2PInterface called TestNode. This commit renames those to TestP2PConn since we already have a TestNode class in the test framework. -BEGIN VERIFY SCRIPT- sed -i s/TestNode/TestP2PConn/ test/functional/p2p_feefilter.py test/functional/p2p_timeouts.py _END VERIFY SCRIPT-
Fuzzbawls
force-pushed
the
2021_regtest-python-linting
branch
from
646175b to
1a15dc4
Compare
Collaborator
Author
|
rebased and fixed the first commit to do imports in the popper order for all test suite files. |
furszy
approved these changes
Jul 18, 2021
Comment on lines
+28
to
+30
| # !TODO: backport bitcoin#12220 | ||
| #data_dir = lambda *p: os.path.join(node.datadir, 'regtest', *p) | ||
| #wallet_dir = lambda *p: data_dir('wallets', *p) |
There was a problem hiding this comment.
btc#12220 already back ported in #2423.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cleanup the python regtest suite to adhere to a number of flake8 standards and start linting the test suite in CI jobs.
This will, ultimately, lead to easier to read and more uniform test code.
Commits here have been made as atomic as possible in order to ease the review process given the relatively large number of files touched.
For reference only: this is a GA build job that shows the new
lint-python.shscript returning an intentional error https://github.com/Fuzzbawls/PIVX/actions/runs/970753309Still todo in separate PRs in the future: update/cleanup any python files to also adhere to / pass mypy inspections, enforce python 3.6 as a minimum requirement (python 3.5 was EOL'd in September 2020), and extend the python linting to python scripts outside the regtest suite.