jiridanek commented on a change in pull request #1504:
URL: https://github.com/apache/qpid-dispatch/pull/1504#discussion_r795050922
##########
File path: .github/workflows/build.yaml
##########
@@ -543,6 +543,49 @@ jobs:
name: book.pdf
path: ${{github.workspace}}/docs/books/user-guide/book.pdf
+ python:
+ name: 'Python Checkers (${{ matrix.container }})'
+ runs-on: '${{ matrix.os }}'
+ strategy:
+ matrix:
+ os: [ 'ubuntu-20.04' ]
+ container: [ 'fedora' ]
+ containerTag: [ '35' ]
+
+ container:
+ image: 'library/${{ matrix.container }}:${{ matrix.containerTag }}'
+ volumes:
+ - ${{github.workspace}}:${{github.workspace}}
+
+ env:
+ DispatchBuildDir: ${{github.workspace}}/build
+ InstallPrefix: ${{github.workspace}}/install
+ DispatchCMakeExtraArgs: >
+ -GNinja
+ -DCONSOLE_INSTALL=OFF
+
+ steps:
+
+ - name: Install build dependencies
+ run: |
+ dnf install -y 'dnf-command(builddep)' && dnf builddep -y
qpid-dispatch-router && dnf install -y git tox ninja-build
Review comment:
Short and sweet. This is what I had in mind when I filled the
aspirational [DISPATCH-1928 Design for a platform-independent way of doing
CI](https://issues.apache.org/jira/browse/DISPATCH-1928). The CI script should
be short and sweet, avoiding the ever repeating lists of dependencies.
One way to get a nicer CI script is to rely on the RPM shipped with Fedora,
which lists build requirements. Not flexible enough, and works only on Fedora,
but the YAML is so much nicer for it.
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -208,10 +208,6 @@ def test_get_types(self):
out = json.loads(self.run_qdmanage("get-types"))
self.assertEqual(len(out), TOTAL_ENTITIES)
- def test_get_attributes(self):
- out = json.loads(self.run_qdmanage("get-attributes"))
- self.assertEqual(len(out), 28)
-
Review comment:
Test of the same name is defined right below. Since the latter shadows
the former, I am deleting the former. Probably a copypaste error anyways.
##########
File path: tests/system_tests_edge_router.py
##########
@@ -2540,6 +2540,7 @@ def __init__(self, receiver1_host, receiver2_host,
receiver3_host,
def on_released(self, event):
self.n_released += 1
+ self.send_test_message()
Review comment:
There were two definitions of `on_released` in the same class. This is
the case with many of the mypy warnings in this PR. Either duplicate method in
class or duplicate class in module, with one instance imported `from proton
import Timeout` and the other one defined there directly `class Timeout:`
##########
File path: tests/TCP_echo_server.py
##########
@@ -92,9 +92,8 @@ def __init__(self, prefix="ECHO_SERVER", port: Union[str,
int] = "0", echo_count
:param echo_count: exit after echoing this many bytes
:param timeout: exit after this many seconds
:param logger: Logger() object
- :return:
"""
- self.sock = None
+ self.sock: socket.socket
Review comment:
```
75: /home/jdanek/repos/qpid/qpid-dispatch/tests/TCP_echo_server.py:129:
error: "None" has no attribute "getsockname" [attr-defined]
```
##########
File path: tests/system_test.py
##########
@@ -363,7 +365,7 @@ def wait_ports(self, **retry_kwargs):
class Qdrouterd(Process):
"""Run a Qpid Dispatch Router Daemon"""
- class Config(list, Config):
+ class Config(list, Config): # type: ignore[misc] # Cannot resolve name
"Config" (possible cyclic definition) # mypy#10958
Review comment:
Known issue. We may rename the shadowed class eventually, or just leave
this suppressed.
##########
File path: tests/system_test.py
##########
@@ -126,7 +128,7 @@ def retry_delay(deadline, delay, max_delay):
TIMEOUT = float(os.environ.get("QPID_SYSTEM_TEST_TIMEOUT", 60))
-def retry(function, timeout=TIMEOUT, delay=.001, max_delay=1):
+def retry(function: Callable[[], bool], timeout: float = TIMEOUT, delay: float
= .001, max_delay: float = 1):
Review comment:
Just a note about mypy. It only checks functions that have type
annotations in their signature. Anything that is not annotated is not checked.
This is how adding annotations expands the fraction of the code that gets
analyzed. It also means that gradually adding types somewhere may reveal type
error somewhere else entirely. Therefore, when adding type annotations "on the
side" like this, one needs to tread lightly, otherwise one ends up in typing
hell, instead of just doing the originally intended task.
##########
File path: tests/run_system_tests.py
##########
@@ -42,5 +42,3 @@
all_tests.addTest(tests)
result = unittest.TextTestRunner(verbosity=2).run(all_tests)
sys.exit(not result.wasSuccessful())
-
-sys.argv = ['unittest', '-v'] + tests
Review comment:
This looks like something @alanconway left there by mistake. It is a
dead code anyways. Deleting.
##########
File path: tests/tox.ini.in
##########
@@ -28,27 +28,37 @@ skip_install = True
commands =
flake8 --count --show-source \
${CMAKE_SOURCE_DIR}/python \
- ${CMAKE_SOURCE_DIR}/console \
${CMAKE_SOURCE_DIR}/docs \
${CMAKE_SOURCE_DIR}/tests \
${CMAKE_SOURCE_DIR}/tools \
${CMAKE_SOURCE_DIR}/tools/qdstat \
${CMAKE_SOURCE_DIR}/tools/qdmanage
# TODO(pylint#5648): crash while parsing system_test.py
- pylint --rcfile ${CMAKE_BINARY_DIR}/tests/tox.ini \
+ pylint --jobs 2 --rcfile ${CMAKE_BINARY_DIR}/tests/tox.ini \
--ignore=system_test.py \
${CMAKE_SOURCE_DIR}/python \
- ${CMAKE_SOURCE_DIR}/console \
Review comment:
there is no python code there; only mypy complains about it, but I'm
removing the dir everywhere
##########
File path: tests/system_tests_sasl_plain.py
##########
@@ -676,10 +676,6 @@ def setUpClass(cls):
cls.routers[1].wait_router_connected('QDR.X')
- @staticmethod
- def ssl_file(name):
- return os.path.join(DIR, 'ssl_certs', name)
-
Review comment:
Duplicate method in class.
##########
File path: python/qpid_dispatch/management/error.py
##########
@@ -114,27 +114,27 @@ def __init__(self, description):
ManagementError.__init__(self, status, descript
return Error
-class BadRequestStatus(_error_class(BAD_REQUEST)):
+class BadRequestStatus(_error_class(BAD_REQUEST)): # type: ignore[misc] #
Unsupported dynamic base class "_error_class"
Review comment:
This is how you suppress mypy line per line. Here, the _static_ type
checker protests against us inheriting from a _dynamically_ determined class.
Makes sense and obviously needs a suppression.
##########
File path: tests/system_tests_ssl.py
##########
@@ -117,6 +117,7 @@ class RouterTestSslClient(RouterTestSslBase):
p = Popen(['openssl', 'version'], stdout=PIPE,
universal_newlines=True)
openssl_out = p.communicate()[0]
m = re.search(r'[0-9]+\.[0-9]+\.[0-9]+', openssl_out)
+ assert m is not None
Review comment:
There is the catch all Except below, so this does not really do anything
meaningful. Just to quiet mypy.
##########
File path: tests/test_command.py
##########
@@ -34,9 +34,9 @@ def mock_error(self, message):
raise ValueError(message)
-argparse.ArgumentParser.error = mock_error
+argparse.ArgumentParser.error = mock_error # type: ignore[assignment] #
Cannot assign to a method
Review comment:
argparse encourages inheriting from ArgumentParser, instead of doing
this. That would be however quite inconvenient to do (although python's
unittest.mock should be able to get me what I need). I decided to suppress,
just to be done with it. It was wrong since forever, so it might just as well
stay "wrong" (in mypy's estimation).
##########
File path: tests/system_tests_ssl.py
##########
@@ -132,13 +133,8 @@ class RouterTestSslClient(RouterTestSslBase):
OPENSSL_ALLOW_TLSV1_3 = False
# Test if OpenSSL has TLSv1_3
- OPENSSL_HAS_TLSV1_3 = False
- if OPENSSL_VER_1_1_GT:
- try:
- _ = ssl.TLSVersion.TLSv1_3
- OPENSSL_HAS_TLSV1_3 = True
- except AttributeError:
- pass
+ # (see
https://mypy.readthedocs.io/en/stable/common_issues.html#python-version-and-system-platform-checks
for mypy considerations)
+ OPENSSL_HAS_TLSV1_3 = OPENSSL_VER_1_1_GT and sys.version_info >= (3, 7)
and ssl.HAS_TLSv1_3
Review comment:
Seems to me I am rewriting these lines from @fgiorgetti every other
month. Hopefully this time I've rewritten it to unobjectionable perfection.
##########
File path: tests/http2_slow_q2_server.py
##########
@@ -88,20 +88,29 @@ def handle(sock):
sock.sendall(data_to_send)
-signal.signal(signal.SIGHUP, receive_signal)
-signal.signal(signal.SIGINT, receive_signal)
-signal.signal(signal.SIGQUIT, receive_signal)
-signal.signal(signal.SIGILL, receive_signal)
-signal.signal(signal.SIGTERM, receive_signal)
-
-sock = socket.socket()
-sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
-sock.bind(('0.0.0.0', int(os.getenv('SERVER_LISTEN_PORT'))))
-sock.listen(5)
-
-while True:
- # The accept method blocks until someone attempts to connect to our TCP
- # port: when they do, it returns a tuple: the first element is a new
- # socket object, the second element is a tuple of the address the new
- # connection is from
- handle(sock.accept()[0])
+def main():
+ signal.signal(signal.SIGHUP, receive_signal)
+ signal.signal(signal.SIGINT, receive_signal)
+ signal.signal(signal.SIGQUIT, receive_signal)
+ signal.signal(signal.SIGILL, receive_signal)
+ signal.signal(signal.SIGTERM, receive_signal)
+
+ port = os.getenv('SERVER_LISTEN_PORT')
+ if port is None:
+ raise RuntimeError("Environment variable `SERVER_LISTEN_PORT` is not
set.")
+
+ sock = socket.socket()
+ sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+ sock.bind(('0.0.0.0', int(port)))
Review comment:
`int(os.getenv('SERVER_LISTEN_PORT'))` would fail due to unsupported
None type if `SERVER_LISTEN_PORT` env variable is not set
##########
File path: tests/tox.ini.in
##########
@@ -28,27 +28,37 @@ skip_install = True
commands =
flake8 --count --show-source \
${CMAKE_SOURCE_DIR}/python \
- ${CMAKE_SOURCE_DIR}/console \
${CMAKE_SOURCE_DIR}/docs \
${CMAKE_SOURCE_DIR}/tests \
${CMAKE_SOURCE_DIR}/tools \
${CMAKE_SOURCE_DIR}/tools/qdstat \
${CMAKE_SOURCE_DIR}/tools/qdmanage
# TODO(pylint#5648): crash while parsing system_test.py
- pylint --rcfile ${CMAKE_BINARY_DIR}/tests/tox.ini \
+ pylint --jobs 2 --rcfile ${CMAKE_BINARY_DIR}/tests/tox.ini \
--ignore=system_test.py \
${CMAKE_SOURCE_DIR}/python \
- ${CMAKE_SOURCE_DIR}/console \
${CMAKE_SOURCE_DIR}/docs \
${CMAKE_SOURCE_DIR}/tests \
${CMAKE_SOURCE_DIR}/tools \
${CMAKE_SOURCE_DIR}/tools/qdstat \
${CMAKE_SOURCE_DIR}/tools/qdmanage
+ mypy --config-file ${CMAKE_BINARY_DIR}/tests/tox.ini \
+ ${CMAKE_SOURCE_DIR}/python \
+ ${CMAKE_SOURCE_DIR}/docs \
+ ${CMAKE_SOURCE_DIR}/tests \
+ ${CMAKE_SOURCE_DIR}/tools \
+ ${CMAKE_BINARY_DIR}/python/qpid_dispatch_site.py
+
+# new checker versions sometimes add new checks; pin the versions to avoid
unexpected
+# surprises to folks just trying to get their job done (c.f. DISPATCH-1305,
DISPATCH-1466, DISPATCH-1834)
deps =
- hacking>=1.1.0
- pylint
+ # TODO(DISPATCH-2321) also install python-qpid-proton here
+ flake8==3.8.4 # hacking 4.1.0 requires flake8<3.9.0 and >=3.8.0
Review comment:
hacking forces us to use older flake8; sucks
##########
File path: tests/tox.ini.in
##########
@@ -28,27 +28,37 @@ skip_install = True
commands =
flake8 --count --show-source \
${CMAKE_SOURCE_DIR}/python \
- ${CMAKE_SOURCE_DIR}/console \
${CMAKE_SOURCE_DIR}/docs \
${CMAKE_SOURCE_DIR}/tests \
${CMAKE_SOURCE_DIR}/tools \
${CMAKE_SOURCE_DIR}/tools/qdstat \
${CMAKE_SOURCE_DIR}/tools/qdmanage
# TODO(pylint#5648): crash while parsing system_test.py
- pylint --rcfile ${CMAKE_BINARY_DIR}/tests/tox.ini \
+ pylint --jobs 2 --rcfile ${CMAKE_BINARY_DIR}/tests/tox.ini \
Review comment:
thow the cores on it!
##########
File path: tests/tox.ini.in
##########
@@ -205,3 +215,67 @@ disable =
useless-else-on-loop,
useless-super-delegation,
wrong-import-position,
+
+[mypy]
+warn_redundant_casts = True
+warn_unused_ignores = True
+
+# mypy cannot handle overridden attributes
+# https://github.com/python/mypy/issues/7505
+allow_untyped_globals = True
+
+# https://mypy.readthedocs.io/en/stable/error_codes.html#displaying-error-codes
+show_error_codes = True
+
+# this would print lots and lots of errors
+# check_untyped_defs = True
+
+# ignore missing stub files for dependencies
+
+#[mypy-_ssl]
+#ignore_missing_imports = True
+
+[mypy-proton.*]
+ignore_missing_imports = True
Review comment:
disabled for now; typechecking proton here might be useful (mainly for
proton, to ensure the annotations there are meaningful)
##########
File path: python/qpid_dispatch_internal/dispatch.pyi
##########
@@ -0,0 +1,68 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License
+#
+
+"""Type stubs for objects implemented in the C extension module"""
Review comment:
The C module is invisible for mypy, so it needs a `.pyi` file that
describes the interface.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
