Resolve flake8 violations in tests/ #428

cjmayo · 2020-05-28T18:54:40Z

Run black and fix the remaining violations.

tests/test_clamav.py:58:89: E501 line too long (90 > 88 characters) tests/test_containers.py:38:9: F841 local variable 'dummy' is assigned to but never used tests/test_dummy.py:35:9: F841 local variable 'dummy' is assigned to but never used tests/test_ftpparse.py:94:89: E501 line too long (96 > 88 characters) tests/test_url.py:128:89: E501 line too long (130 > 88 characters) tests/test_strformat.py:62:9: E741 ambiguous variable name 'l' tests/test_strformat.py:136:9: E731 do not assign a lambda expression, use a def tests/checker/ftpserver.py:94:9: E722 do not use bare 'except' tests/checker/httpserver.py:55:39: E231 missing whitespace after ',' tests/checker/httpserver.py:224:9: E722 do not use bare 'except' tests/checker/telnetserver.py:84:9: E722 do not use bare 'except' tests/checker/__init__.py:71:89: E501 line too long (119 > 88 characters) tests/checker/__init__.py:292:13: E741 ambiguous variable name 'l' tests/checker/test_http_misc.py:30:1: W293 blank line contains whitespace tests/checker/test_https.py:21:1: F401 'tests.need_network' imported but unused tests/checker/test_news.py:35:1: E302 expected 2 blank lines, found 1

anarcat · 2020-05-29T01:01:37Z

maybe this should also enforce black or at least pep8 in CI since we seem to have completely converted the codebase...

mgedmin

What do you know, I did get to the end! And before breakfast!

Most of the changes are great, some need little tweaks.

mgedmin · 2020-05-29T07:53:18Z

tests/cache/test_urlqueue.py

-            aggregate=Aggregate(
-                result_cache=self.result_cache,
-            ),
+            aggregate=Aggregate(result_cache=self.result_cache,),


Black is wonderful, but it does make bad decisions sometimes.

Suggested change

aggregate=Aggregate(result_cache=self.result_cache,),

aggregate=Aggregate(result_cache=self.result_cache),

mgedmin · 2020-05-29T07:53:39Z

tests/cache/test_urlqueue.py

-            aggregate=Aggregate(
-                result_cache=self.result_cache,
-            ),
+            aggregate=Aggregate(result_cache=self.result_cache,),


There are many unnecessary trailing commas like this.

Well, "many" ~= 6 or 7, in this file, all about Aggregate(...,), so should be easy to search-and-replace fix.

git grep ',)' confirms that there are Aggregates here and one dict literal in

tests/checker/test_httpbin.py: entry = dict(user=user, password=password, pattern=re.compile(r".*"),)

that have the bad trailing comma right in front of the closing parenthesis. All other trailing commas like this are in 1-tuples, where they belong.

Another manual check to add to the process.

mgedmin · 2020-05-29T07:59:58Z

tests/checker/__init__.py

+                [
+                    "url %s" % x.strip()
+                    for x in re.split(
+                        r"^url .*?",


One advantage to black making the code readable: I can actually read it now, and see that this is equivalent to re.split("^url ", ...). The non-greedy .*? never needs to match anything and so always matches the empty string.

Suggested change

r"^url .*?",

r"^url ",

mgedmin · 2020-05-29T08:00:56Z

tests/checker/__init__.py

            self.result.append("baseurl %s" % url_data.base_ref)
-        if self.has_part('info'):
+        if self.has_part("info"):


I hate the noise Black creates by changing the quote characters on all the strings. And they make the diff so huge. I'm sure I'll give up somewhere in the middle.

i also find this change annoying, because i usually type single quotes because they are faster (one keystroke instead of shift+keystroke) on my keyboard.

the black docs have a section on this:

https://black.readthedocs.io/en/stable/the_black_code_style.html#strings

their argument ias that double-quotes are less ambiguous and that you can always type single quotes and let black fix the formatting if you don't like to type double-quotes, which is reasonable.

That said, there's an escape hatch in black for this:

If you are adopting Black in a large project with pre-existing string conventions (like the popular “single quotes for data, double quotes for human-readable strings”), you can pass --skip-string-normalization on the command line. This is meant as an adoption helper, avoid using this for new projects.

Do we want to do that?

Yes please, anything for a smaller diff.

(I won't mind if there's a subsequent PR that does only quote changes, separately, if anyone wants to do that. Personally, I'm not yet convinced by Black's arguments about quotes.)

mgedmin · 2020-05-29T08:02:55Z

tests/checker/__init__.py

-        if hasattr(self, 'port'):
-            d['port'] = self.port
+        if hasattr(self, "port"):
+            d["port"] = self.port
        # all result files are encoded in utf-8
        with codecs.open(resultfile, "r", "utf-8") as f:


Noticed in passing, not part of this PR, but: with the Python 2 drop we can replace codecs.open(..., "UTF-8") with just open(..., encoding="UTF-8").

mgedmin · 2020-05-29T08:11:55Z

tests/checker/test_loginurl.py

+                "user": user,
+                "password": password,
+                "pattern": re.compile("^http://localhost.*"),
+            }


Suggested change

}

},

mgedmin · 2020-05-29T08:13:00Z

tests/checker/test_news.py

-NNTP_INFO = "200 news.uni-stuttgart.de InterNetNews NNRP server " \
-            "INN 2.5.2 ready (no posting)"
+NNTP_INFO = (
+    "200 news.uni-stuttgart.de InterNetNews NNRP server " "INN 2.5.2 ready (no posting)"


Suggested change

"200 news.uni-stuttgart.de InterNetNews NNRP server " "INN 2.5.2 ready (no posting)"

"200 news.uni-stuttgart.de InterNetNews NNRP server INN 2.5.2 ready (no posting)"

If this is not a Black bug, it should be!

psf/black#26

Fixed 21 days ago.

Last release in 2019.

sigh

mgedmin · 2020-05-29T08:21:10Z

tests/test_clamav.py

+            ["stream: ClamAV-Test-File(2d1206194bd704385e37000be6113f73:781) FOUND\n"],
+            [
+              "stream: Clamav.Test.File-6(aa15bcf478d165efd2065190eb473bcb:544) FOUND\n"
+            ],


Ugh, this is why I don't like automatic formatters. They get things right in 99% of the cases, and then throw out something like this.

Suggested change

["stream: ClamAV-Test-File(2d1206194bd704385e37000be6113f73:781) FOUND\n"],

[

"stream: Clamav.Test.File-6(aa15bcf478d165efd2065190eb473bcb:544) FOUND\n"

],

[

"stream: ClamAV-Test-File(2d1206194bd704385e37000be6113f73:781) FOUND\n"

],

[

"stream: Clamav.Test.File-6(aa15bcf478d165efd2065190eb473bcb:544) FOUND\n"

],

mgedmin · 2020-05-29T08:27:07Z

tests/test_po.py

-            self.assertFalse(b"\xc2\xb7" in line,
-                 "Broken GTranslator copy/paste in %r:\n%r" % (f, line))
+            self.assertFalse(
+                b"\xc2\xb7" in line,


Unrelated to this PR, but perhaps we should be checking for Unicode \N{MIDDLE DOT} in decoded text, not the UTF-8 encoding of it in byte strings.

mgedmin · 2020-05-29T08:29:40Z

tests/test_url.py

        self.urlnormtest(url, url)
-        url = "http://redirect.alexa.com/redirect?"\
-              "http://www.offeroptimizer.com"
+        url = "http://redirect.alexa.com/redirect?" "http://www.offeroptimizer.com"


Suggested change

url = "http://redirect.alexa.com/redirect?" "http://www.offeroptimizer.com"

url = "http://redirect.alexa.com/redirect?http://www.offeroptimizer.com"

git grep '" "' tells me I missed another one like this, in this file:

url = "http://server/..%5c..%5c..%5c..%5c..%5c..%5c..%5c.." "%5ccskin.zip"

mgedmin · 2020-05-29T08:38:04Z

maybe this should also enforce black

Let's not, at least not until Black gets the known bugs fixed and released.

or at least pep8 in CI since we seem to have completely converted the codebase...

tox -e flake8 complains a lot about code in linkcheck/.

I've also noticed that tox -e flake8 complains about code in build/, which it should not do: #429

cjmayo · 2020-05-29T18:43:16Z

Added commits in response to the review comments.

cjmayo force-pushed the flake8-tests branch from 4f55939 to 059334e Compare May 28, 2020 19:16

cjmayo marked this pull request as draft May 28, 2020 19:26

cjmayo added 2 commits May 28, 2020 20:29

Run black on tests/

165c51a

cjmayo force-pushed the flake8-tests branch from 059334e to 8703991 Compare May 28, 2020 19:29

cjmayo marked this pull request as ready for review May 28, 2020 19:39

mgedmin approved these changes May 29, 2020

View reviewed changes

cjmayo added 5 commits May 29, 2020 19:40

Remove unnecessary commas before closing brackets in tests/

be53c4a

Remove unnecessary character match in regexp in TestLogger.normalize()

a534be0

Add trailing comma to single dict list in TestLoginUrl.visit_loginurl()

5ee8d8e

Tidy TestClamav.testInfected() acceptable_responses

c71cfcb

Convert space-separated strings in tests/

5df8aa0

cjmayo merged commit 152dbeb into linkchecker:master May 29, 2020

cjmayo deleted the flake8-tests branch September 14, 2020 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve flake8 violations in tests/ #428

Resolve flake8 violations in tests/ #428

cjmayo commented May 28, 2020

anarcat commented May 29, 2020

mgedmin left a comment

mgedmin May 29, 2020

cjmayo May 29, 2020

mgedmin May 29, 2020

mgedmin May 29, 2020

mgedmin May 29, 2020

cjmayo May 29, 2020

mgedmin May 29, 2020

cjmayo May 29, 2020

mgedmin May 29, 2020

anarcat May 29, 2020

mgedmin May 29, 2020 •

edited

mgedmin May 29, 2020

mgedmin May 29, 2020

cjmayo May 29, 2020

mgedmin May 29, 2020

mgedmin May 29, 2020

cjmayo May 29, 2020

mgedmin May 29, 2020

cjmayo May 29, 2020

mgedmin May 29, 2020

mgedmin May 29, 2020

mgedmin May 29, 2020

cjmayo May 29, 2020

mgedmin commented May 29, 2020 •

edited

cjmayo commented May 29, 2020

	aggregate=Aggregate(result_cache=self.result_cache,),
	aggregate=Aggregate(result_cache=self.result_cache),

	"200 news.uni-stuttgart.de InterNetNews NNRP server " "INN 2.5.2 ready (no posting)"
	"200 news.uni-stuttgart.de InterNetNews NNRP server INN 2.5.2 ready (no posting)"

	url = "http://redirect.alexa.com/redirect?" "http://www.offeroptimizer.com"
	url = "http://redirect.alexa.com/redirect?http://www.offeroptimizer.com"

Resolve flake8 violations in tests/ #428

Resolve flake8 violations in tests/ #428

Conversation

cjmayo commented May 28, 2020

anarcat commented May 29, 2020

mgedmin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgedmin May 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgedmin commented May 29, 2020 • edited

cjmayo commented May 29, 2020

mgedmin May 29, 2020 •

edited

mgedmin commented May 29, 2020 •

edited