Lint additional four files in the tests folder #1708
Merged
Conversation
This file contains 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
Apply all 4 linters on tests/utils.py, so we can lint it in future and not rename and exclude it. It's expected that most if not all code in tests/utils.py will be useful even when we remove the tests file on the old code. Keep in mind when reviewing that type annotations where already added in theupdateframework@e2deff3 Black and isort changes where automatically made. The only manual changes are: - pylint disables - function docstrings - initializations of attributes of "TestServerProcess" __init__() - additional asserts of the attributes types Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Apply all 4 linters on tests/test_utils.py, so we can lint it in the future and not rename and exclude it. As we are going to use part or most of tests/utils.py after we remove the test files testing the old code, then we would need to keep the test file testing utils.py - test_utils.py Black and isort changes where automatically made. The only manual changes are: - pylint disable once in can_connect - remove TestServerProcess.tearDown and instead clean it manually in the tests, so we don't have to define server_process_handler as a class variable and assert it's type in every test - move can_connect outside TestServerProcess as it doesn't use self anymore - add type annotations - function docstrings Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
MVrachev
force-pushed
the
prepare-for-lint
branch
from
1e2cfc4
to
bcb64c4
Compare
Pull Request Test Coverage Report for Build 1548823628
💛 - Coveralls |
lukpueh
approved these changes
Dec 6, 2021
tests/simple_server.py
Outdated
Comment on lines
52
to
57
| handler: Type[ | ||
| Union[SimpleHTTPRequestHandler, QuietHTTPRequestHandler] | ||
| ] = QuietHTTPRequestHandler | ||
|
|
||
| if use_quiet_http_request_handler: | ||
| handler = QuietHTTPRequestHandler | ||
| else: | ||
| handler = SimpleHTTPRequestHandler | ||
| if len(sys.argv) > 2: | ||
| handler = SimpleHTTPRequestHandler |
There was a problem hiding this comment.
For the purpose of this script the type annotation really seems like an overkill that impairs readability. Would it be problematic if we just did:
if len(sys.argv) > 2 and sys.argv[2]:
handler = QuietHTTPRequestHandler
else:
handler = SimpleHTTPRequestHandlerThere was a problem hiding this comment.
That way I receive the following two errors by mypy:
tests/simple_server.py:61: error: Cannot assign multiple types to name "handler" without an explicit "Type[...]" annotation [misc]
tests/simple_server.py:61: error: Incompatible types in assignment (expression has type "Type[SimpleHTTPRequestHandler]", variable has type "Type[QuietHTTPRequestHandler]") [assignment]
I wanted to simplify it somehow but didn't come up with an idea of how to do it without a mypy error...
That's how I end up with this ugly line...
There was a problem hiding this comment.
I guess you could "declare" handler first. Looks pretty unpythonic to me, but a bit nicer then the long line above.
handler: Type[Union[SimpleHTTPRequestHandler, QuietHTTPRequestHandler]]
if len(sys.argv) > 2 and sys.argv[2]:
handler = QuietHTTPRequestHandler
else:
handler = SimpleHTTPRequestHandlerBut I'm fine with leaving it as is.
There was a problem hiding this comment.
You are right that's better even if it's not the most pythonic way it's still more understandable.
Updated the code with that suggestion.
| + "!" | ||
| ) | ||
|
|
||
| if line.startswith(expected_msg): |
There was a problem hiding this comment.
Out of curiosity, did black make this elif an if? Or did pylint flag it?
There was a problem hiding this comment.
It was pylint who flag it with:
R1720: Unnecessary "elif" after "raise" (no-else-raise)
There was a problem hiding this comment.
👍 Oh right, I remember that one.
|
I added a new commit linting |
MVrachev
changed the title
Apply all 4 linters on tests/simple_server.py, so we can lint it in the future and not rename and exclude it. As we are going to use part or most of tests/utils.py after we remove the test files testing the old code, then we would need to keep the simple_server.py file. It's currently used in tests/updater_ng.py testing the new updater implementation. Black and isort changes where automatically made. The only manual changes are: - pylint disable once in can_connect - add type annotations - simplifications around setting up the handler variable - function docstrings Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
All changes are automatically generated by black. The other linters didn't find any errors in the file. It's expected that most if not all code in aggregate_tests.py will be useful even when we remove the tests file on the old code. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
MVrachev
force-pushed
the
prepare-for-lint
branch
from
5eb575c
to
d748bd3
Compare
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Related to #1582
Description of the changes being introduced by the pull request:
When preparing the pr which was going to resolve #1582 I had to rename the test files
related to the old codebase. That way I was able to exclude the old test files when linting
the
tests/folder.While I did that I realized I have to decide whether we want to lint and consequently rename
utils.py,test_utils.py,simple_server.py, andaggregate_tests.pyafter we remove the files testing the old code.I decided that it's worth linting them for the following reasons:
utils.pyprovides utilities that are used by our new code such as theTestServerProcessclasswhich handles server handling, logging, and the
run_sub_tests_with_datasetdecorator whichwe used in multiple files.
test_utils.pywill be needed even after we remove the old test files as we want to have teststhat verify that
TestServerProcessfromutils.pyis working as expectedsimple_server.pyis the only helper for server startup used in the new code base andmore precisely to start the server in
test_updater_ng.pyaggregate_tests.pywill be needed to aggregate the test filesThat's why I decided to run the linters upon those files.
I tried to summarize the manual changes I had to make in the commit messages
as they weren't many.
Most of the changes are automatic and are result from
blackandisort.Please verify and check that the pull request fulfills the following
requirements: