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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove runtests.py #5274

Merged
merged 5 commits into from Jul 18, 2018

Conversation

Projects
None yet
7 participants
@elazarg
Contributor

elazarg commented Jun 25, 2018

  • Replace runtests.py with a gentle reminder to use pytest script that executes pytest+lint+self check.
  • Remove waiter.py
  • Remove selfcheck unit test
  • Run self check from travis using python3 -m mypy --config-file mypy_self_check.ini -p mypy
  • Run linter explicitly from travis using flake8 -j0
  • Update docs

This is work-in-progress, here for review and testing. I tried to implement what I understood from the comments in #5270, but I'm not sure I got it right.

TODO: fix test run in typeshed.

Concludes #1673.

@elazarg elazarg changed the title from [WIP] Remove runtests.py to Remove runtests.py Jun 26, 2018

@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Jun 26, 2018

Contributor

I'm not completely sure what is the ideal workflow. I've separated the tests from the self check, but it is one of the tests that one should run before opening a PR.

pytest && flake8 && python3 -m mypy --config-file mypy_self_check.ini -p mypy is a lengthy command to run. Maybe leave runtests.py as a shorthand?

(Or maybe you meant something entirely different? @gvanrossum @ilevkivskyi)

Contributor

elazarg commented Jun 26, 2018

I'm not completely sure what is the ideal workflow. I've separated the tests from the self check, but it is one of the tests that one should run before opening a PR.

pytest && flake8 && python3 -m mypy --config-file mypy_self_check.ini -p mypy is a lengthy command to run. Maybe leave runtests.py as a shorthand?

(Or maybe you meant something entirely different? @gvanrossum @ilevkivskyi)

@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs Jun 26, 2018

Collaborator

I personally would prefer to leave the self check under pytest, as it is a bit of a lengthy command to type out the entire command, and not very memorable. However I defer to the judgement of others, I can always use tox :)

Collaborator

ethanhs commented Jun 26, 2018

I personally would prefer to leave the self check under pytest, as it is a bit of a lengthy command to type out the entire command, and not very memorable. However I defer to the judgement of others, I can always use tox :)

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Jun 26, 2018

Collaborator

I don't have a strong preference about whether to run self check under pytest, but if it's not being run under pytest, we should definitely have a simple, consistent way of running self check locally instead of running a long command line. It would be good to have an easy way of self checking using dmypy as well.

Collaborator

JukkaL commented Jun 26, 2018

I don't have a strong preference about whether to run self check under pytest, but if it's not being run under pytest, we should definitely have a simple, consistent way of running self check locally instead of running a long command line. It would be good to have an easy way of self checking using dmypy as well.

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jun 26, 2018

Collaborator

I would rather keep all three separate: tests, self-check, and flake8 (but this is also not a strong preference). For self-check I am mostly using Ctrk + Shift + M :-) If anyone else uses PyCharm, I can share my config for mypy plugin:

Mypy command: dmypy start -- --follow-imports=error --config-file=mypy_self_check.ini ; dmypy check mypy
PATH suffix: path/to/venv/bin
Collaborator

ilevkivskyi commented Jun 26, 2018

I would rather keep all three separate: tests, self-check, and flake8 (but this is also not a strong preference). For self-check I am mostly using Ctrk + Shift + M :-) If anyone else uses PyCharm, I can share my config for mypy plugin:

Mypy command: dmypy start -- --follow-imports=error --config-file=mypy_self_check.ini ; dmypy check mypy
PATH suffix: path/to/venv/bin
@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Jun 26, 2018

Collaborator

I would actually prefer to have everything (including self-check and lint) run under pytest, so that it's easy to run all tests locally without having to remember a lot of commands.

Collaborator

JelleZijlstra commented Jun 26, 2018

I would actually prefer to have everything (including self-check and lint) run under pytest, so that it's easy to run all tests locally without having to remember a lot of commands.

@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Jun 26, 2018

Contributor

Looks like the simplest solution is to keep runtests as a simple scripts that executes pytest, self check and lint.

Contributor

elazarg commented Jun 26, 2018

Looks like the simplest solution is to keep runtests as a simple scripts that executes pytest, self check and lint.

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Jun 26, 2018

Collaborator

Looks like the simplest solution is to keep runtests as a simple scripts that executes pytest, self check and lint.

This is still worse that the legacy script since it will be harder to run just lint or self check. Whatever solution we use to replace runtests should make all of these easy (i.e. no need to remember complex command lines):

  • Run everything, including tests, lint and self check (matches what the travis build does).
  • Run only self check.
  • Run only lint.
  • Run some subset of tests (this already works).

Nice to haves, for extra credit:

  • It would be nice if the self check would run an incremental build by default.
  • Being able to run all tests without running self check or lint.
  • Provide also a way of running a dmypy self check, since these are often much faster than non-dmypy builds.
Collaborator

JukkaL commented Jun 26, 2018

Looks like the simplest solution is to keep runtests as a simple scripts that executes pytest, self check and lint.

This is still worse that the legacy script since it will be harder to run just lint or self check. Whatever solution we use to replace runtests should make all of these easy (i.e. no need to remember complex command lines):

  • Run everything, including tests, lint and self check (matches what the travis build does).
  • Run only self check.
  • Run only lint.
  • Run some subset of tests (this already works).

Nice to haves, for extra credit:

  • It would be nice if the self check would run an incremental build by default.
  • Being able to run all tests without running self check or lint.
  • Provide also a way of running a dmypy self check, since these are often much faster than non-dmypy builds.
@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Jun 26, 2018

Contributor

Ok. Here's another suggestion:

$ ./runtests.py (runs everything)

$ ./runtests.py [lint] [self] [pytest] [args...] any subset of the three. Arguments are passed to pytest only without any preprocessing (if pytest is not mentioned, reject additional arguments). Arguments are trailing only.

Passing arguments to self check will be performed manually.

Possibly use self-dmypy or something similar instead of self for dmypy self check.

Contributor

elazarg commented Jun 26, 2018

Ok. Here's another suggestion:

$ ./runtests.py (runs everything)

$ ./runtests.py [lint] [self] [pytest] [args...] any subset of the three. Arguments are passed to pytest only without any preprocessing (if pytest is not mentioned, reject additional arguments). Arguments are trailing only.

Passing arguments to self check will be performed manually.

Possibly use self-dmypy or something similar instead of self for dmypy self check.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 26, 2018

Member
Member

gvanrossum commented Jun 26, 2018

@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Jun 27, 2018

Contributor

Done. The commands are evaluated eagerly asserted to find typos.

Contributor

elazarg commented Jun 27, 2018

Done. The commands are evaluated eagerly asserted to find typos.

@ethanhs

Just a few minor points. Overall looks great!

Show outdated Hide outdated runtests.py Outdated
Show outdated Hide outdated runtests.py Outdated
Show outdated Hide outdated .travis.yml Outdated
Show outdated Hide outdated tox.ini Outdated
@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Jul 8, 2018

Contributor

@ethanhs do you approve now?

Contributor

elazarg commented Jul 8, 2018

@ethanhs do you approve now?

@ethanhs

ethanhs approved these changes Jul 8, 2018

@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs Jul 8, 2018

Collaborator

You have a merge conflict, but otherwise I approve.

Collaborator

ethanhs commented Jul 8, 2018

You have a merge conflict, but otherwise I approve.

@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Jul 9, 2018

Contributor

I have no idea what's this conflict is about. It does not show up in my local merge, and the "resolve conflicts" key is disabled.

Contributor

elazarg commented Jul 9, 2018

I have no idea what's this conflict is about. It does not show up in my local merge, and the "resolve conflicts" key is disabled.

@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Jul 9, 2018

Contributor

Moreover, the conflict said to be in a file that should be removed completely.

Contributor

elazarg commented Jul 9, 2018

Moreover, the conflict said to be in a file that should be removed completely.

@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Jul 9, 2018

Contributor

screen shot 2018-07-08 at 22 07 18

Contributor

elazarg commented Jul 9, 2018

screen shot 2018-07-08 at 22 07 18

@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs Jul 9, 2018

Collaborator

It is listed when I merge master as CONFLICT (modify/delete): waiter.py deleted in HEAD.... I believe you should be able to git rm waiter.py then commit the merge.

Collaborator

ethanhs commented Jul 9, 2018

It is listed when I merge master as CONFLICT (modify/delete): waiter.py deleted in HEAD.... I believe you should be able to git rm waiter.py then commit the merge.

@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Jul 9, 2018

Contributor

But it does not exist.

~/workspace/mypy$ git status
On branch pytest-complete
Your branch is up to date with 'origin/pytest-complete'.

nothing to commit, working tree clean
~/workspace/mypy$ git rm waiter.py
fatal: pathspec 'waiter.py' did not match any files
Contributor

elazarg commented Jul 9, 2018

But it does not exist.

~/workspace/mypy$ git status
On branch pytest-complete
Your branch is up to date with 'origin/pytest-complete'.

nothing to commit, working tree clean
~/workspace/mypy$ git rm waiter.py
fatal: pathspec 'waiter.py' did not match any files
@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs Jul 9, 2018

Collaborator

Ah, if it doesn't exist on disk you need git rm --cached waiter.py. This is probably one of the more illogical git commands...

Collaborator

ethanhs commented Jul 9, 2018

Ah, if it doesn't exist on disk you need git rm --cached waiter.py. This is probably one of the more illogical git commands...

@JelleZijlstra

This comment has been minimized.

Show comment
Hide comment
@JelleZijlstra

JelleZijlstra Jul 9, 2018

Collaborator

The conflict is presumably that somebody else modified waiter.py on master, and you deleted it. The way to fix it would be to run git merge origin/master (I think, I don't do manual merges that often), and then when there's a conflict, git rm waiter.py as Ethan says.

Collaborator

JelleZijlstra commented Jul 9, 2018

The conflict is presumably that somebody else modified waiter.py on master, and you deleted it. The way to fix it would be to run git merge origin/master (I think, I don't do manual merges that often), and then when there's a conflict, git rm waiter.py as Ethan says.

@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Jul 9, 2018

Contributor
~/workspace/mypy$ git rm --cached waiter.py
fatal: pathspec 'waiter.py' did not match any files
Contributor

elazarg commented Jul 9, 2018

~/workspace/mypy$ git rm --cached waiter.py
fatal: pathspec 'waiter.py' did not match any files
@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Jul 9, 2018

Contributor

Sorry guys I'm so amazingly stupid. I've merged upstream/master but forgot git fetch upstream.

Contributor

elazarg commented Jul 9, 2018

Sorry guys I'm so amazingly stupid. I've merged upstream/master but forgot git fetch upstream.

@Michael0x2a

This comment has been minimized.

Show comment
Hide comment
@Michael0x2a

Michael0x2a Jul 18, 2018

Collaborator

@ethanhs -- just to check, we can merge this now, right?

Collaborator

Michael0x2a commented Jul 18, 2018

@ethanhs -- just to check, we can merge this now, right?

@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs Jul 18, 2018

Collaborator

@Michael0x2a yes, this should be good to go.

Collaborator

ethanhs commented Jul 18, 2018

@Michael0x2a yes, this should be good to go.

@ilevkivskyi ilevkivskyi merged commit 3e53036 into python:master Jul 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Jul 18, 2018

Collaborator

Thanks for all the work on tests @elazarg!

Collaborator

ilevkivskyi commented Jul 18, 2018

Thanks for all the work on tests @elazarg!

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jul 18, 2018

Member
Member

gvanrossum commented Jul 18, 2018

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Jul 19, 2018

Collaborator

This is great! Thanks Elazar!

Collaborator

JukkaL commented Jul 19, 2018

This is great! Thanks Elazar!

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