Stricter and more checks for python code by oschaaf · Pull Request #395 · envoyproxy/nighthawk

oschaaf · 2020-07-01T22:38:43Z

In #337 some review comments related
to doc comments have been postponed to a followup.

Adds more flake8 convention checks. This broadens the checks we introduced in the
benchmarking flow to cover the global code base, and exclude a few
files that we either don't own or care about.

Also updates yapf & flake8 version requirements, which enhances automatic formatting,
as well as fixes a python 3.8 compatibility bug.

Should fix #371
Should fix #400

Summary of compliance failures under the proposed checks before this change:

13    D100 Missing docstring in public module
2     D101 Missing docstring in public class
9     D102 Missing docstring in public method
46    D103 Missing docstring in public function
4     D107 Missing docstring in __init__
29    D200 One-line docstring should fit on one line with quotes
9     D202 No blank lines allowed after function docstring
32    D205 1 blank line required between summary line and description
3     D208 Docstring is over-indented
1     D209 Multi-line docstring closing quotes should be on a separate line
1     D210 No whitespaces allowed surrounding docstring text
1     D300 Use """triple double quotes"""
39    D400 First line should end with a period
16    D401 First line should be in imperative mood
1     E124 closing bracket does not match visual indentation
2     E125 continuation line with same indent as next logical line
7     E126 continuation line over-indented for hanging indent
1     E401 multiple imports on one line
2     E714 test for object identity should be 'is not'
4     F403 'from test.integration.utility import *' used; unable to detect undefined names
152   F405 'assertCounterEqual' may be undefined, or defined from star imports: test.integration.utility
2     F841 local variable 'logs' is assigned to but never used
8     W291 trailing whitespace
1     W293 blank line contains whitespace
3     W504 line break after binary operator
1     W605 invalid escape sequence '\:'
389

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-07-01T22:40:49Z

@mum4k requested a review, it would be great to have your thoughts on the broadened
checking introduced here, before following up and making the code compliant with that.

Resolves envoyproxy#371 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-07-03T22:51:55Z

This now fails check_format with:

9     D100 Missing docstring in public module
2     D101 Missing docstring in public class
9     D102 Missing docstring in public method
42    D103 Missing docstring in public function
4     D107 Missing docstring in __init__

So just the the missing docstrings are left to work in to complete this.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k

This is awesome, would you mind updating the PR description with the latest list of remaining findings?

mum4k · 2020-07-07T17:36:19Z

test/integration/asserts.py

@@ -0,0 +1,98 @@
+"""A library with assertation helpers for unit tests."""


I wonder - do we have a good reason to maintain our own library as compared to using something like:

https://docs.python.org/2/library/unittest.html

Note - it would be ok to address this in a follow-up.

This came to be out of unawareness of that library existing. I agree it would be good to move to that.
I would prefer to punt this to an issue though, to not increase size of this PR (and keep it a no-op)

mum4k · 2020-07-07T17:38:04Z

test/integration/asserts.py

+  assertLessEqual(a, max_value)
+
+
+def assertCounterEqual(counters, name, value):


Do we need assert helpers like this? Assuming we do switch to the standard unittest library, we could simply assert on:

self.assertEquals(counters[name], value)

And if the counter isn't in the dictionary, it would still "correctly" fail.

mum4k · 2020-07-07T17:38:29Z

test/integration/common.py

+      Exception (string): exception message.
+  """
+
  pass


(nit, optional) "pass" is no longer needed now that we have a docstring.

mum4k · 2020-07-07T17:40:03Z

test/integration/common.py

 class NighthawkException(Exception):
+  """Base exception class for Nighthawk's python code.
+
+  Args:


Class docstring doesn't really have Args section, since classes don't take arguments. We would typically put that in the docstring of the init method, but that one isn't needed here.

We can just say:

class NighthawkException(Exception): """Base exception class for Nighthawk's python code."""

mum4k · 2020-07-07T17:41:35Z

test/integration/integration_test_fixtures.py

 def determineIpVersionsFromEnvironment():
+  """Determine the IP version(s) for test execution from the environment.
+
+  Raises:


We should only add the Raises section for exceptions raised by the function directly. Otherwise this is a futile effort since every line and character of Python code can raise at least one exception (SyntaxError, MemoryError, RuntimeError, ...).

This functions doesn't have the raise statement, so it shouldn't need the Raises section.

determineIpVersionsFromEnvironment() has:

else: raise NighthawkException("Unknown ip version: '%s'" % versions)

So I think in this instance it's OK?

mum4k · 2020-07-07T17:43:44Z

test/integration/integration_test_fixtures.py

-    """
-    Utility for getting the http://host:port/ that can be used to query the server we started in setUp()
-    """
+    """Get the http://host:port/ that can be used to query the server we started in setUp()."""


(nit) The one-line docstring should fit on a single line (80 chars).

If it doesn't (i.e. we need to say more) we should still choose an abstract that fits into 80 charts and write the rest into the body of the docstring.

Applies in a few places.

mum4k · 2020-07-07T17:47:51Z

test/integration/utility.py

@@ -1,67 +1,20 @@
+"""Packages utility methods for tests."""


(nit, pre-existing) We may want to choose a better name, "utility" is a catch all and doesn't help the reader much. Even something like "test_utility" would be better.

mum4k · 2020-07-07T17:48:56Z

tools/format_python_tools.sh

 # Check everything, except indentation and line length for now.
-# Also, we ignore unused imports and redefinitions of unused, as those seems to raise false flags in test definitions.
-flake8 ../benchmarks/ ${EXCLUDE} --docstring-convention pep257 --ignore=E114,E111,E501,F401,F811 --count --show-source --statistics
+# We ignore unused imports and redefinitions of unused, as those seems to raise false flags in test definitions.


Is this worth investigating? Can you help by providing some details / examples?

mum4k · 2020-07-07T17:49:17Z

tools/format_python_tools.sh

-# Also, we ignore unused imports and redefinitions of unused, as those seems to raise false flags in test definitions.
-flake8 ../benchmarks/ ${EXCLUDE} --docstring-convention pep257 --ignore=E114,E111,E501,F401,F811 --count --show-source --statistics
+# We ignore unused imports and redefinitions of unused, as those seems to raise false flags in test definitions.
+# We ignore formatting isssues E124/E125/E126 because they conflict with the automtic fix format feature.


Are we using the wrong auto formatter? Which formatting issues are we ignoring here?

mum4k · 2020-07-07T17:49:47Z

tools/format_python_tools.sh

-flake8 ../benchmarks/ ${EXCLUDE} --docstring-convention pep257 --ignore=E114,E111,E501,F401,F811 --count --show-source --statistics
+# We ignore unused imports and redefinitions of unused, as those seems to raise false flags in test definitions.
+# We ignore formatting isssues E124/E125/E126 because they conflict with the automtic fix format feature.
+flake8 . ${EXCLUDE} --ignore=E114,E111,E501,F401,F811,E124,E125,E126,D --count --show-source --statistics


What is this and the following excludes excluding?

This ignores/excludes:

ignored, because of conflict with the automatic fix format script. This reformats according to https://github.com/envoyproxy/nighthawk/blob/master/tools/.style.yapf. We followed Envoy's lead
there, but maybe this is a good time to evaluate that:
E111 Indentation is not a multiple of four
E114 Indentation is not a multiple of four (comment)
E501 Line too long (82 > 79 characters)
E124 Closing bracket does not match visual indentation
E125 Continuation line with same indent as next logical line
E126 Continuation line over-indented for hanging indent

False positives because of what I think are pytest peculiarities [1]. Taking a look at this again, I do think it's worth following up, because there's some actually valid complaints about unused imports too it seems. But there's also real false positives; e.g. importing pytest is something we really must do.
F401 Module imported but unused
F811 Redefinition of unused name from line n

doc conventions
D = doc comment related checks (we do those below, checking both p257 AND google conventions as both seem to have different strengths in their checks; and so far I haven't experienced any conflicts)

[1]

./test/integration/test_integration_zipkin.py:3:1: F401 'pytest' imported but unused import pytest ^ ./test/integration/test_integration_zipkin.py:7:1: F401 'integration_test_fixtures.http_test_server_fixture' imported but unused from integration_test_fixtures import (http_test_server_fixture, server_config) ^ ./test/integration/test_integration_zipkin.py:7:1: F401 'integration_test_fixtures.server_config' imported but unused from integration_test_fixtures import (http_test_server_fixture, server_config) ^ ./test/integration/test_integration_zipkin.py:11:1: F811 redefinition of unused 'http_test_server_fixture' from line 7 def test_tracing_zipkin(http_test_server_fixture):

Thank you for the explanations and detailed list. I think it is generally fine for us to remain aligned with Envoy. I don't see anything that would be very concerning in the list above. This PR will significantly help on future reviews of Python code, thank you for these improvements.

Would you mind copying the explanation of the exceptions from this discussion thread into an inline comment next to the exceptions for posterity?

Done: 1e162bc

…llowup

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-07-08T10:46:23Z

This is awesome, would you mind updating the PR description with the latest list of remaining findings?

I updated the description -- but I kept the list, mentioning that these where the issues before the changes in here.
(that might be convenient as a "lookup table" to get descriptions from numbers while we're working on this)

Fixes envoyproxy#400 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-07-14T10:03:36Z

@mum4k I added be8f7ce to update the flake8/yapf versions. This should fix the python 3.8 compatibility issue that recently came up, as well as enhances automatic formatting.
In the next commit, def550c, I ran ci/do_ci.sh fix_format to get the code to comply. Also had to rename a variable called l to avoid violating a new check.

Let me know if you're OK with having these changes in here, we can also split that out to a follow up (but having them in flight separately at the same time will be challenging because of lots of merge conflict opportunities).

mum4k · 2020-07-14T23:03:00Z

Thank you @oschaaf, it is fine to have them here. I will review them in the next few days.

…llowup

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-07-15T14:41:38Z

@mum4k I filed #407 as a sink for leftovers in here.

mum4k

Waiting for CI.

oschaaf · 2020-07-16T09:24:08Z

(merged this, as this was approved & passing CI)

Add proposed constraints

acfb6d8

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf requested a review from mum4k July 1, 2020 22:39

oschaaf changed the title ~~Stricter checks for python code~~ Stricter and more checks for python code Jul 1, 2020

oschaaf added 3 commits July 3, 2020 22:30

Eliminate import * & move asserts to better named namespace/file

6dac34a

Resolves envoyproxy#371 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Fix W605 invalid escape sequence '\:'

737d905

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Fix all flake8 compliance issues modulo missing docstrings.

ec9c22a

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added tech-debt enhancement New feature or request labels Jul 3, 2020

oschaaf added 2 commits July 4, 2020 11:42

Doc asserts.py

49c9731

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Fix missing/malformed doc strings

65bb746

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added P1 waiting-for-review A PR waiting for a review. labels Jul 6, 2020

oschaaf marked this pull request as ready for review July 6, 2020 11:55

mum4k requested changes Jul 7, 2020

View reviewed changes

mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Jul 7, 2020

oschaaf added 2 commits July 8, 2020 12:12

Merge remote-tracking branch 'upstream/master' into python-linting-fo…

79ea695

…llowup

Partial review feedback - not done yet

ed4efac

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added 2 commits July 14, 2020 11:49

Bump flake8 & yapf version requirements.

be8f7ce

Fixes envoyproxy#400 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Reformat with new yapf version. Fix ambiguous var name 'l'.

def550c

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf mentioned this pull request Jul 14, 2020

Run ci/do_ci.sh fix_format cmd got error ModuleNotFoundError: No module named 'pip' #400

Closed

oschaaf added 2 commits July 15, 2020 16:23

Merge remote-tracking branch 'upstream/master' into python-linting-fo…

da19d23

…llowup

Review feedback: list exclusion details

1e162bc

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf mentioned this pull request Jul 15, 2020

Python enhancement follow ups #407

Open

mum4k approved these changes Jul 15, 2020

View reviewed changes

mum4k added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Jul 15, 2020

oschaaf merged commit 44ff257 into envoyproxy:master Jul 16, 2020

		@@ -0,0 +1,98 @@
		"""A library with assertation helpers for unit tests."""

		assertLessEqual(a, max_value)


		def assertCounterEqual(counters, name, value):

Conversation

oschaaf commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oschaaf commented Jul 1, 2020

Uh oh!

oschaaf commented Jul 3, 2020

Uh oh!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschaaf commented Jul 8, 2020

Uh oh!

oschaaf commented Jul 14, 2020

Uh oh!

mum4k commented Jul 14, 2020

Uh oh!

oschaaf commented Jul 15, 2020

Uh oh!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

oschaaf commented Jul 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

oschaaf commented Jul 1, 2020 •

edited

Loading