Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Enable flake8 and re-enable it everywhere#11477

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
tacaswell merged 7 commits intomatplotlib:masterfromQuLogic:flake8
Jul 8, 2018

Conversation

QuLogic
Copy link
Member

PR Summary

Due to the removal of the examples symlink in#11141, pytest was no longer seeing the examples and running pep8 on them. This adds that directory back to pytest's search.

It also adds a rebase of#10940 to enable flake8.

PR Checklist

  • [N/A] Has Pytest style unit tests
  • Code is PEP 8 compliant
  • [N/A] New features are documented, with examples if plot related
  • [N/A] Documentation is sphinx and numpydoc compliant
  • [N/A] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • [N/A] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@QuLogicQuLogic added this to thev3.0 milestoneJun 22, 2018
@jklymakjklymak added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelJun 22, 2018
@QuLogicQuLogic mentioned this pull requestJun 22, 2018
6 tasks
@QuLogic
Copy link
MemberAuthor

The meaning of the additional exceptions to flake8 options are describedhere by@cclauss.

pytest.ini Outdated
@@ -7,8 +7,9 @@ markers =
network: Mark a test that uses the network.
style: Set alternate Matplotlib style temporarily.

pep8ignore =
* E111 E114 E115 E116 E121 E122 E123 E124 E125 E126 E127 E128 E129 E131 E226 E240 E241 E242 E243 E244 E245 E246 E247 E248 E249 E265 E266 E704 W503
flake8-max-line-length = 80
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Shouldn't this be left at 79 (the default, i.e. remove this line)?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I got this from@cclauss's PR, but I think you're right, so I'm going to remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

sorry, off-by-one bug on my part.

@cclauss
Copy link

LOL...#10940 (comment)

@QuLogic
Copy link
MemberAuthor

Well, okay, now pyside(2) seems to be broken.

Also, I have no idea why, but flake8 make the tests ridiculously slow. I ran a non-parallel test locally and it took ~7 minutes, but with the--flake8 flag it's not finished for the past hour.

@timhoffm
Copy link
Member

Maybe relatedtholo/pytest-flake8#19?

@anntzer
Copy link
Contributor

If#11477 (comment) is still an issue, perhaps replace pytest-flake8 by a direct invocation of flake8? (with e.g.https://github.com/snoack/flake8-per-file-ignores for per-file ignores)
(TBH I don't really see the point of a pytest plugin that just calls another executable (if it is not active by default).)

cclauss reacted with thumbs up emojicclauss reacted with hooray emojicclauss reacted with heart emoji

@jklymak
Copy link
Member

I ran

  • pytest -v --pep8 lib/matplotlib/tests/test_axes.py: 143.09 s
  • pytest -v --flake8 lib/matplotlib/tests/test_axes.py: 131.09 s

Sure I didn't have the exceptions turned on or anything (the pep8 test failed).

@cclauss
Copy link

Try running PEP8 and flake8 WITHOUT passing thru pytest.

@QuLogic
Copy link
MemberAuthor

Well, I tried conda, virtualenv, and even downloading the docker image that Travis used, but I cannot reproduce the slow checks anymore. I guess standaloneflake8 is the only option; the only annoyance is that configuration will have to go into a new file, aspytest.ini is not a supported file.

@cclauss
Copy link

cclauss commentedJul 7, 2018•
edited
Loading

Fromhttp://flake8.pycqa.org/en/latest/user/invocation.html

$flake8 . --config=CONFIG # Path to the config file that will be the authoritative config source. This will cause Flake8 to ignore all other configuration files.

@tacaswell
Copy link
Member

Is the issue that pytest spins up a new flake8 process for each file which just takes forever on the travis vms?

@tacaswell
Copy link
Member

watching a running test it is not hung, just from 91% on it seems to be taking it's time (and then we hit the 50 minute timeout).

@cclauss
Copy link

cclauss commentedJul 7, 2018•
edited
Loading

One minute, 10 seconds to flake8 test the entire repo seems reasonable to me.

$time flake8 . --config=pytest.ini # Python 3.7 on Travis CI

real0m26.769suser1m10.204ssys0m0.953s

Copy link

@cclausscclauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Running flake8 from inside pytest has proved to be a bad idea when each file in the repo has its own style rules. Running flake8 standalone with the--config=pytest.ini seems the best approach.

@QuLogicQuLogicforce-pushed theflake8 branch 2 times, most recently from9d55403 to6fa1391CompareJuly 7, 2018 23:57
@QuLogicQuLogic changed the titleEnable flake8 and re-enable pep8 on examplesEnable flake8 and re-enable it everywhereJul 7, 2018
@QuLogic
Copy link
MemberAuthor

At some point in the near future, I would like to get rid of the** exceptions, as they cause confusion when linting a single file (per-file-ignores thinks that the exception is unnecessary and raises itsX100 error).

@jklymak
Copy link
Member

Where does the flake8 output show up in the test? I can't see any evidence that it ran, but maybe I'm looking in the wrong spot...

@QuLogic
Copy link
MemberAuthor

That's because it passed 😉 I'll add a message to say it's okay on there.

jklymak reacted with thumbs up emoji

jklymak
jklymak previously approved these changesJul 8, 2018
@cclauss
Copy link

Fixes#11550

.travis.yml Outdated
@@ -160,6 +160,9 @@ before_script: |
script: |
echo "Calling pytest with the following arguments: $PYTEST_ADDOPTS"
python -mpytest
if [[ $RUN_FLAKE8 == 1 ]]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I suspect this is going to have the same issue we discovered in the pytzectomy PR where if the tests fail but flake8 passes, travis will report success.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Probably set -e is the way to go?
Or add|| return 1 at the end of each (relevant) line.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't likeset -e; it bleeds into Travis' code and causes spurious errors, so you need to remember to turn it off. The last time that happened, we ended up just removing it instead. I split these into separate entries instead, which does fail correctly.

@@ -25,13 +25,13 @@ def __get__(self, obj, objtype=None):


class TaggedValueMeta(type):
def __init__(cls, name, bases, dict):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is used as a meta-class so I ma not sure that this change is correct (I am also not sure off the top of my head whatself is in this case and when this__init__ gets called).

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It may be that the object is a class/type here, but it's still an instance at this point, so I'm not sure calling it something other thanself is warranted.

tacaswell
tacaswell previously requested changesJul 8, 2018
Copy link
Member

@tacaswelltacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think that the way this is called in travis will mask failures in the test suite from failing the test.

Anyone can clear this if this is fixed or they are sure I am wrong.

@jklymakjklymak mentioned this pull requestJul 8, 2018
6 tasks
@cclauss
Copy link

Do the flake8 tests first and the pytests after?

@jklymak
Copy link
Member

@cclauss then it won't fail if flake8 fails?

#11597 shows that@tacaswell is indeed correct - that failing a test still returns a positive Travis check the way this is setup now....

@jklymakjklymak dismissed theirstale reviewJuly 8, 2018 16:48

Doesn't catch failing pytest tests anymore

@jklymak
Copy link
Member

#11597 shows that addingset -e causes a failure when pytest should fail. Testing if it fails if there is a flake8 error

@QuLogic
Copy link
MemberAuthor

Splitting the two commands into separate entriesshould work fine.

@tacaswelltacaswell merged commit94f41d1 intomatplotlib:masterJul 8, 2018
@tacaswell
Copy link
Member

Thanks everyone!

cclauss reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer left review comments

@timhoffmtimhoffmtimhoffm approved these changes

@cclausscclausscclauss approved these changes

@tacaswelltacaswelltacaswell left review comments

@jklymakjklymakjklymak left review comments

Assignees
No one assigned
Labels
Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.topic: testing
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.

6 participants
@QuLogic@cclauss@timhoffm@anntzer@jklymak@tacaswell
[8]ページ先頭
©2009-2025 Movatter.jp