Fix all flake8 violations in the test/ directory #1021

dorothykiz1 · 2022-02-07T18:51:08Z

closes 11

Fix flake8 violations for files below;

test/conftest.py
test/test_dev.py
test/test_feed.py
.github/workflows/ci.yml
setup.cfg

datapythonista · 2022-02-08T00:18:01Z

.github/workflows/ci.yml

What is the goal of this change?

I wanted flake8 to check the files I changed but I will remove this as it's not necessary

datapythonista · 2022-02-08T00:18:59Z

test/test_repo_template/asv_test_repo/__init__.py

@@ -3,4 +3,4 @@

 from __future__ import absolute_import, division, unicode_literals, print_function

-dummy_value = {dummy_value}
+dummy_value = {dummy_value} # noqa 


For a noqa we should have the error code you want to ignore, not all them. And also a comment explaining why we want flake8 to ignore the problem here, instead of fixing the error.

Thanks, Marc, Let me add this asap to be more descriptive

datapythonista · 2022-02-08T00:19:40Z

Thanks for the PR. Added couple of comments, and you also have conflicts to fix.

…to fix_flake8_03

dorothykiz1 · 2022-02-08T18:05:03Z

@datapythonista this PR is ready for your review will await your guidance

datapythonista

Besides the comments, you will have to fix the conflicts

datapythonista · 2022-02-08T18:22:49Z

test/test_dev.py

 import re
 from os.path import abspath, dirname, join, relpath
 import shutil
 import pytest

-import six
+import six  # noqa


Why is this? In case it's really needed you need the error code to skip and the reason.

datapythonista · 2022-02-08T18:24:35Z

test/test_repo_template/asv_test_repo/__init__.py

@@ -3,4 +3,4 @@

 from __future__ import absolute_import, division, unicode_literals, print_function

-dummy_value = {dummy_value}
+dummy_value = {dummy_value}  # noqa:F821 dummy_value not defined but used


Can you create an issue for this, and link it here? It may be worth running this test file to see why this test file is not failing in the CI.

…to fix_flake8_03

dorothykiz1 · 2022-02-08T20:28:43Z

@datapythonista this PR is ready for your review again

datapythonista · 2022-02-09T00:33:51Z

test/test_dev.py

 import re
 from os.path import abspath, dirname, join, relpath
 import shutil
 import pytest

-import six
+import six  # noqa F401 'six' imported but unused


I don't see why six needs to be imported if it's not used. What's the error?

It is only the flake8 check throws an error (F401 'six' imported but unused)
but the tests pass. its the reason I had removed it completely earlier

We already discussed this case few times. What we did was:

Remove the import

If something fails in the test, we check the test failure, we try to understand why this import needs to be here, and we add a the noqa with the error code to skip, and a comment for other devs to know

If it's difficult for you to understand the error, as often requires a lot of experience, just find the error, post it here, so we can discuss and you can learn about it

Just adding the error from flake8 doesn't add much value. The first question I'll ask myself if I see this code is "why the import hasn't been removed it it's unused". This is what the code/comment should tell us. Regardless of flake8, why we are importing the module if it's not used

sure let me try this

datapythonista · 2022-02-09T00:34:44Z

test/test_repo_template/asv_test_repo/__init__.py

@@ -3,4 +3,4 @@

 from __future__ import absolute_import, division, unicode_literals, print_function

-dummy_value = {dummy_value}
+dummy_value = {dummy_value}  # noqa:F821 see #1023 Bug: dummy_value is not defined


Thanks for creating the issue. Did you check why this doesn't raise an error in the CI?

@datapythonista Am thinking it should be raised in the Ci but it is not, its only flakes 8 that is catching it.

Also what I'm thinking. But the CI is green (both in this PR and in master). So, we are missing something. Can you have a look at what's going on please. If you don't find out easily, please let me know what's your approach to find out and we can discuss about it.

Let me try to find out, had run pytest on all the files but it's still quite abstract.

FAILED test/test_update.py::test_update_simple[hg] - FileNotFoundError: [Errno 2] No such file or directory: 'hg'
ERROR test/test_repo.py::test_get_branch_commits[hg] - FileNotFoundError: [Errno 2] No such file or directory: 'hg'
ERROR test/test_repo.py::test_get_new_branch_commits[hg-all] - FileNotFoundError: [Errno 2] No such file or directory: 'hg'
ERROR test/test_repo.py::test_get_new_branch_commits[hg-new] - FileNotFoundError: [Errno 2] No such file or directory: 'hg'
ERROR test/test_repo.py::test_get_new_branch_commits[hg-no-new] - FileNotFoundError: [Errno 2] No such file or directory: 'hg'
ERROR test/test_repo.py::test_get_new_branch_commits[hg-new-branch-added-in-config] - FileNotFoundError: [Errno 2] No such file or dire..

@datapythonista kindly share your thoughts on this

when I run rgrep -I "dummy_value" *I can see the variable dummy_value is being used and its imported from the file in question.

def track_find_test(n):
import asv_test_repo

return asv_test_repo.dummy_value[n - 1]

track_find_test.params = [1, 2]

def time_find_test_timeout():
import asv_test_repo, time
if asv_test_repo.dummy_value[1] < 0:
time.sleep(100)

datapythonista · 2022-02-09T00:35:41Z

.github/workflows/ci.yml

@@ -16,4 +16,4 @@ jobs:
        run: flake8 . || true # preventing CI failing while flake8 issues exist

      - name: flake8 validating correct file(s)
-        run: flake8 asv/main.py asv/repo.py asv/results.py asv/runner.py asv/statistics.py asv/step_detect.py asv/util.py test/benchmark/ asv/__init__.py asv/benchmarks.py asv/config.py asv/benchmark.py
+        run: flake8 asv/main.py asv/repo.py asv/results.py asv/runner.py asv/statistics.py asv/step_detect.py asv/util.py test/benchmark/ asv/__init__.py asv/benchmarks.py asv/config.py asv/benchmark.py test/conftest.py test/test_dev.py test/test_feed.py test/test_repo_template/asv_test_repo/__init__.py 


Maybe time to split this in two lines, this is becoming unreadable.

sure,will do this

dorothykiz1 · 2022-02-10T11:45:27Z

Hello @datapythonista this PR is ready for your review again

datapythonista

Thanks @dorothykiz1, this looks great, and will be very useful.

I added couple of minor comments on things that can help, but other than that looks perfect.

datapythonista · 2022-02-10T12:16:51Z

.github/workflows/ci.yml

-
-      - name: flake8 validating correct file(s)
-        run: flake8 asv/main.py asv/repo.py asv/results.py asv/runner.py asv/statistics.py asv/step_detect.py asv/util.py test/benchmark/ asv/__init__.py asv/benchmarks.py asv/config.py asv/benchmark.py
+        run: flake8 . || true # preventing CI failing while flake8 issues exist


We don't need the || true anymore, since all the files that would make the CI fail are now excluded.

When you do that, maybe you can also simplify the name of the job now, to just flake8 or similar, since there is only one now, and it's not so complex to understand what each does.

Thanks, let me do this

datapythonista · 2022-02-10T12:17:13Z

setup.cfg

@@ -1,6 +1,14 @@
 [flake8]
-max-line-length = 99
-
+max-line-length = 99,


I don't think we need the comma at the end (and not sure if it can cause problems)

datapythonista · 2022-02-10T12:19:13Z

setup.cfg

-max-line-length = 99
-
+max-line-length = 99,
+exclude = setup.py,asv/commands,asv/extern,asv/plugins,asv/template/benchmarks,


This looks great, thanks.

Only thing I'd do, is to have one file per line. It looks better the way you did it, as it's not super long, but with one line per file PRs will be easier to review when we keep fixing the pending files. And also, conflicts will be easier to solve, if files are fixed in parallel.

Yes, I agree. Let me incorporate all the changes

dorothykiz1 · 2022-02-10T14:15:04Z

@datapythonista this PR is ready for your review

datapythonista

Thanks @dorothykiz1, couple of details, but almost there.

datapythonista · 2022-02-10T14:19:56Z

.github/workflows/ci.yml

@@ -13,7 +13,4 @@ jobs:
        run: pip install flake8

      - name: flake8 all repo only show erros


This is the name I was referring to, it can be just left as just flake8 (it's not true anymore after you remove the || true that we only show errors, we also fail the CI if they exist).

datapythonista · 2022-02-10T14:20:24Z

.github/workflows/ci.yml

-
-      - name: flake8 validating correct file(s)
-        run: flake8 asv/main.py asv/repo.py asv/results.py asv/runner.py asv/statistics.py asv/step_detect.py asv/util.py test/benchmark/ asv/__init__.py asv/benchmarks.py asv/config.py asv/benchmark.py
+        run: flake8 .  # preventing CI failing while flake8 issues exist


You can remove the comment too, as we're not preventing the CI from failing anymore now.

datapythonista · 2022-02-10T14:21:26Z

setup.cfg

+        asv/machine.py,
+        asv/graph.py,
+        asv/.pytest_cache/d/asv-wheels/cache/tmp


This seems like a temporary file, and I don't think it'll exist in the CI. Can you remove it, and see if the build is still green? I think it should.

Thanks, @datapythonista however these temp files had some flake 8 errors ,that's why I had included their folder,kindly advise

./.pytest_cache/d/asv-wheels/cache/tmp/asv_dummy_test_package_2-0.3.9/asv_dummy_test_package_2/init.py:1:22: W292 no newline at end of file
./.pytest_cache/d/asv-wheels/cache/tmp/asv_dummy_test_package_1-0.14/asv_dummy_test_package_1/init.py:1:21: W292 no newline at end of file
./.pytest_cache/d/asv-wheels/cache/tmp/asv_dummy_test_package_2-0.3.7/asv_dummy_test_package_2/init.py:1:22: W292 no newline at end of file

Is this happening in the CI or locally? I think the CI shouldn't fail, but maybe a good idea anyway to ignore anything in .pytest_cache directories, since those are temporary. Not sure exactly how to ignore all directories with that name, but I'm sure you'll find out.

datapythonista · 2022-02-10T14:21:49Z

setup.cfg

+        benchmarks/step_detect.py,
+        benchmarks/__init__.py,
+        .pytest_cache/d/asv-wheels/cache/tmp,


Another temporary file, same as the previous.

datapythonista · 2022-02-10T14:23:07Z

setup.cfg

+        benchmarks/__init__.py,
+        .pytest_cache/d/asv-wheels/cache/tmp,
+        docs/source


I think there is just one Python file in this directory, no big deal, but probably worth having the file here, and not the whole dir (so people who wants to work in any of these know that it's just one file, and doesn't feel intimidated thinking it's lots of things in that dir).

Great, i can add this .this makes sense let me pick the exact files.

dorothykiz1 · 2022-02-10T15:41:30Z

@datapythonista this PR is ready for your review

datapythonista · 2022-02-10T16:18:02Z

@datapythonista this PR is ready for your review

Did you push your changes? I don't think there is any update since the last review.

datapythonista

Thanks @dorothykiz1 looks perfect, I'll merge on green

dorothykiz1 · 2022-02-10T16:41:17Z

Sure,Thank you @datapythonista

datapythonista · 2022-02-10T17:02:38Z

Thanks @dorothykiz1, really nice

dorothykiz1 added 2 commits February 7, 2022 21:45

fix all flake8 violations in benchmark.py

0001e13

fix all flake8 violations in benchmark.py

c82fa9f

datapythonista reviewed Feb 8, 2022

View reviewed changes

dorothykiz1 and others added 7 commits February 8, 2022 17:47

fix flake8 violations

5e27a91

fix flake8 violations

6d1b6cf

fix flake8 violations

4fee6a5

add files to the CI

ece4185

Merge branch 'master' into fix_flake8_03

6331b2b

fix flake8 violations

f81aaab

Merge branch 'fix_flake8_03' of https://github.com/dorothykiz1/asv in…

67e6e74

…to fix_flake8_03

datapythonista reviewed Feb 8, 2022

View reviewed changes

dorothykiz1 and others added 3 commits February 8, 2022 21:30

Merge branch 'master' into fix_flake8_03

f4d6382

fix flake8 violations

99e25ee

Merge branch 'fix_flake8_03' of https://github.com/dorothykiz1/asv in…

aedfb10

…to fix_flake8_03

datapythonista reviewed Feb 9, 2022

View reviewed changes

dorothykiz1 added 3 commits February 9, 2022 15:47

Fix Flake8 errors

c96c715

fix all pending flake8 errors

6f3037d

fix all pending flake8 errors

bde1791

datapythonista reviewed Feb 10, 2022

View reviewed changes

dorothykiz1 added 2 commits February 10, 2022 15:50

fix config and CI files

e4b22a6

fix config and CI files

ecd2e92

datapythonista reviewed Feb 10, 2022

View reviewed changes

modify setup.cfg

4886a06

datapythonista approved these changes Feb 10, 2022

View reviewed changes

datapythonista merged commit 13eb401 into airspeed-velocity:master Feb 10, 2022

		@@ -13,7 +13,4 @@ jobs:
		run: pip install flake8

		- name: flake8 all repo only show erros

Fix all flake8 violations in the test/ directory #1021

Fix all flake8 violations in the test/ directory #1021

Conversation

dorothykiz1 commented Feb 7, 2022 • edited Loading

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

datapythonista commented Feb 8, 2022

dorothykiz1 commented Feb 8, 2022

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothykiz1 commented Feb 8, 2022

Choose a reason for hiding this comment

dorothykiz1 Feb 9, 2022 • edited Loading

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

dorothykiz1 Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothykiz1 Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dorothykiz1 commented Feb 10, 2022

datapythonista 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

dorothykiz1 Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

dorothykiz1 commented Feb 10, 2022

datapythonista 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

dorothykiz1 Feb 10, 2022 • edited Loading

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

dorothykiz1 commented Feb 10, 2022

datapythonista commented Feb 10, 2022

datapythonista left a comment

Choose a reason for hiding this comment

dorothykiz1 commented Feb 10, 2022

datapythonista commented Feb 10, 2022

dorothykiz1 commented Feb 7, 2022 •

edited

Loading

dorothykiz1 Feb 9, 2022 •

edited

Loading

dorothykiz1 Feb 9, 2022 •

edited

Loading

dorothykiz1 Feb 9, 2022 •

edited

Loading

dorothykiz1 Feb 10, 2022 •

edited

Loading

dorothykiz1 Feb 10, 2022 •

edited

Loading