Testing scripts & pull_request_template#12862
Conversation
OttoWinter
left a comment
OttoWinter
left a comment
There was a problem hiding this comment.
I like this change 😅 With this running pytest on changed files would be easier & quicker. I have two questions:
- Almost always these two scripts would be executed together/after each other; so, as a person that wants to maximize laziness, I don't really like having to type
script/lint && script/pytest... Maybe have something likescript/test(not an ideal name) - Quite a few commits just affect the core code and not the tests. With the pytest script we'd only run changed test files though; ideally i'd like the script to also run the tests matching my changed core files. For example, if I edit
homeassistant/components/mqtt/__init__.pyit would be great if the script would also know to runtests/components/mqtt/test_init.py. I think the test files already match the core directory structure quite well, so we could maybe implement some matching there. If we miss a test file with that matching, it wouldnt be that big of a problem because then tox would pick it up.
|
Thanks Otto,
General Poll: ideas for names for 1? |
|
.github/PULL_REQUEST_TEMPLATE.md
Outdated
|
|
||
| ## Checklist: | ||
| - [ ] The code change is tested and works locally. | ||
| - [ ] Local tests pass with `script/lint`, `script/pytest` & `tox`. **Your PR cannot be merged unless tests pass** |
There was a problem hiding this comment.
Why do we have to run the scripts if we also run tox?
There was a problem hiding this comment.
The scripts do not cover everything, with Otto’s suggestion we would be much closer to remove ‘tox’ here
What about scripts/lazytox?
@OttoWinter maybe not that complex with sed, let me give it one go and then we decide
There was a problem hiding this comment.
Tox covers everything right, so please make that clear in the instructions. If you run tox, you don't need to run the rest.
There was a problem hiding this comment.
The problem is that if we are going to say run this, or maybe that, we are confusing people. The nice about tox that it takes care of everything but yes, it's slow.
There was a problem hiding this comment.
I mean from what I've seen in the past few weeks, only very few PRs from newcomers actually have tox run successfully - line too long and other lint errors are picked up way to often by Travis/hound. Now I don't know if this is because a) people don't follow the checklists (probably this is the bigger reason) or b) tox just takes ages and users abort it because they think it's unresponsive.
Just throwing this out there, but maybe this could actually even increase PR quality, while yes sadly also causes a bit of confusion.
There was a problem hiding this comment.
IMHO, newcomers relying on Travis is fine (in particular if we could get the tests stable) but I think Hound is a net loss, it makes PRs permanently unreadable.
There was a problem hiding this comment.
@amelchio not sure I agree.
With Travis the experience is bad. Submit the code you worked so hard on and 20min later you get told you missed a space... Repeat process several times, frustration growing with each repition. This is the reason for Hound, a quick feedback loop.
Ideally you want to make feedback even quicker, with the proper tools, like script/lint today and maybe lazytox.py tomorrow (it will have the added benefit of not giving people idle time to complain about the code formatting rules)
There was a problem hiding this comment.
So spend the waiting time on installing the tools locally.
Local tools is the proper way and Travis is mostly a check that people did not cheat. If someone wants to rely on that check, fine – but it will be slow for them.
Having Hound give fast feedback is counterproductive to getting local tools installed and it often messes up the PR so badly that I do not want to read it.
|
I like the idea of adding Regarding the pylint tests: I'm not sure if it would be worth it. As @MartinHjelmare pointed out already, tox runs all tests whereas with pytest you kind of need to know what you are doing. Therefore it might be an idea to leave tox in there and add it only as an option to experienced developers, those who are lazy 😉 @OttoWinter |
|
My attention span ends by the time tox starts its tests (I dont suffer from ADHD),- we just have too many dependencies :-) Lets leave script/lint as is then and only add that to the PR template I do think there is value in pylint on dirty files though |
|
For now I only added We can start testing |
|
I don't think we should change the PR template. Instead let's add the new scripts to the developer docs, and encourage our developers to use these scripts during PR development. When a PR is ready for submission, a final check with |
script/lazytox.py
Outdated
| async def git(): | ||
| """Exec git.""" | ||
| try: | ||
| with open('lazytox.log') as file: |
There was a problem hiding this comment.
I expect logfiles only to be written to and never to be used for any sort of storage. Also, won't this file be picked up by git (.gitignore)?
There was a problem hiding this comment.
.gitignore contains *.log. The development Docker image do not contain git and did not have luck installing it, hence the "cache"
There was a problem hiding this comment.
Re installing git in Dockerfile.dev:
RUN apt-get update && apt-get install -y --no-install-recommends git && rm -rf /var/lib/apt/lists/*should work.
| read_stream(proc.stderr, sys.stderr.write)) | ||
| exit_code = await proc.wait() | ||
| # Convert to ASCII | ||
| stdout = stdout.decode('utf-8') |
There was a problem hiding this comment.
Shouldn't the comment then be: # Convert to UTF-8 ? 👀
script/lazytox.py
Outdated
| proc = await asyncio.create_subprocess_exec(*args, **kwargs) | ||
| except FileNotFoundError as err: | ||
| print('ERROR: You need to install {}. Could not execute: {}'.format( | ||
| args[0], ' '.join(args))) |
There was a problem hiding this comment.
Just a suggestion, but I think we should print the actual command we're trying to execute. For example:
print('ERROR: You need to install {}. Could not execute: {}'.format(
args[0], ' '.join(shlex.quote(arg) for arg in args)))
script/lazytox.py
Outdated
| _, log = await async_exec( | ||
| 'git', 'diff', 'upstream/dev...', '--name-only') | ||
| except FileNotFoundError: | ||
| print("Using a cached version of changed files.") |
There was a problem hiding this comment.
FileNotFoundError only gets thrown if git isn't installed, right?
- I don't think we really need to catch that exception, since
gitis pretty much a prerequisite for Home Assistant development. - If
gitisn't installed for some reason, I don't think we would even have cached files at hand... I think it would be better to not fail silently then.
There was a problem hiding this comment.
I use a mix of msys environment with git installed in Windows. All dependencies in the Docker container, so my workflow is slightly different. Ideally I should look at getting git in the Docker container
script/lazytox.py
Outdated
| _, log = await async_exec('pylint', '-f', 'parseable', *files, | ||
| mute_err=True) | ||
| except FileNotFoundError: | ||
| return [] |
There was a problem hiding this comment.
Same here, let's not fail silently if we can't run pylint.
There was a problem hiding this comment.
Error message printed by async_exec
There was a problem hiding this comment.
True, it's semi-silent right now. I think we should fail immediately, without even trying to continue running. It's not difficult to install pylint (I think) and I'm sure some users will not spot the warning message.
Anyway, I don't see no positive aspect of trying to continue running without pylint...
script/lazytox.py
Outdated
| from collections import namedtuple | ||
|
|
||
| RE_ASCII = re.compile(r"\033\[[^m]*m") | ||
| ERROR = namedtuple('ERROR', "file line col msg") |
There was a problem hiding this comment.
Python classes are usually UpperCamelCase to avoid confusion with constants.
Error = namedtuple('Error', "file line col msg")There was a problem hiding this comment.
Will change and add pylint disable directive
script/lazytox.py
Outdated
| line = line.split(':') | ||
| if len(line) != 4: | ||
| continue | ||
| res.append(ERROR(line[0].replace('\\', '/'), line[1], "", line[2])) |
There was a problem hiding this comment.
line[0].replace('\\', '/')I think this is for Windows, right? Shouldn't we print out fully valid Windows paths then? Also, you don't replace with flake8while you do here...
There was a problem hiding this comment.
The outputs from pylint and flake8 are not the same. flake8 was /. pylint a single \. Probably best to do both to be safe. / is perfectly valid on Windows if you want to avoid all kinds of escaping
script/lazytox.py
Outdated
| pyfile = re.compile(r".+\.py$") | ||
| pyfiles = [file for file in files if pyfile.match(file)] | ||
|
|
||
| print("CHANGED FILES:", ' '.join(files)) |
There was a problem hiding this comment.
Same as with the script/lint script: For better readability it would be better to place all changed files on new lines.
script/lazytox.py
Outdated
| test_files.add(err.file) | ||
| else: | ||
| tfile = err.file.replace('homeassistant', 'tests') \ | ||
| .replace('__init__', 'test_init') |
There was a problem hiding this comment.
AFAIK all tests start with test_, not just __init__. For example, homeassistant/components/mqtt/discovery.py → tests/components/mqtt/test_discovery.py
There was a problem hiding this comment.
Ah, my test sample had a changed test_ in! --> good catch!. Will need a re replace for this one
script/lazytox.py
Outdated
| if err.file.startswith('tests/'): | ||
| test_files.add(err.file) | ||
| else: | ||
| tfile = err.file.replace('homeassistant', 'tests') \ |
There was a problem hiding this comment.
Why do we only add the matching tests/ files iff there's an error in the homeassistant/ files?
There was a problem hiding this comment.
😕 thats not right yes. I need to iterate over the git file list and not the pylint&flake8 results! A bit of a shortcircuit here
script/lazytox.py
Outdated
| print("No files identified, ideally you should run tox.") | ||
| return | ||
|
|
||
| print('pytest -vv --', ' '.join(shlex.quote(fle) for fle in test_files)) |
There was a problem hiding this comment.
- Let's print the command we're actually executing (
pytest -vv --force-sugar --) - I think here there can be again a considerable amount of files, so splitting lines would be nice, though it would conflict a bit with 1.
print("pytest -vv --force-sugar -- \\")
print(' \\\n'.join(' {}'.format(shlex.quote(f)) for f in test_files))Would have the added benefit that you can easily copy-paste the command and run it on your own.
There was a problem hiding this comment.
--force-sugar is not needed when you are in a terminal.
To rerun these tests, the best option is really just to execute it again with script/lazytox.py --skiplint
There was a problem hiding this comment.
Now printing all commands being executed using this method you suggested
There was a problem hiding this comment.
👍 ..... quickly... merge!
script/lazytox.py
Outdated
|
|
||
| async def flake8(files): | ||
| """Exec flake8.""" | ||
| _, log = await async_exec('flake8', *files) |
There was a problem hiding this comment.
flake8 seems to be used with --doctests in the script/lint script, we should do the same here then.
OttoWinter
left a comment
OttoWinter
left a comment
There was a problem hiding this comment.
Just a few comments, most of them can safely be ignored. Already looking forward to this one very much 🎉
script/lazytox.py
Outdated
| @@ -0,0 +1,234 @@ | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
Though I hate it when people still use python 2, there are quite a few around (including me for certain outdated scientific libraries 😭...). For a subset of those people which python will point to a python 2 installation. I guess it's better to do this instead:
#!/usr/bin/env python3
script/lazytox.py
Outdated
|
|
||
| RE_ASCII = re.compile(r"\033\[[^m]*m") | ||
| Error = namedtuple('ERROR', | ||
| "file line col msg") # pylint: disable=invalid-name |
There was a problem hiding this comment.
I think if you'd write this the pylint error would go away (I think) and you'd have the actual typename you're using for the namedtuple:
Error = namedtuple('Error', ['file', 'line', 'col', 'msg'])(You could also take a look at using the attrs library that was recently added as a global Home Assistant requirement.)
script/lazytox.py
Outdated
| argsp.append("\\\n {}".format(shlex.quote(arg))) | ||
| else: | ||
| argsp.append(shlex.quote(arg)) | ||
| printc('cyan', ' '.join(argsp)) |
There was a problem hiding this comment.
(You can ignore this if this is for readability). printc already does ' '.join(args)...
printc('cyan', *argsp)|
|
||
| async def pylint(files): | ||
| """Exec pylint.""" | ||
| _, log = await async_exec('pylint', '-f', 'parseable', '--persistent=n', |
| print("=============================") | ||
|
|
||
| if code == 0: | ||
| printc(PASS, "Yay! This will most likely pass tox") |
There was a problem hiding this comment.
Suggestion for making the logs more fun 😉 (feel free to ignore this, might even cause issues with other terminals)
printc(PASS, "Yay! This will most likely pass tox 🎉")
There was a problem hiding this comment.
This will probably run mostly in bash, otherwise it would have been a good idea! 👍
|
happy testing ! 🎉 |
|
Oh... I think you forgot to make |
|
Mine is executable, but I commit from Windows, so guess that concept does not exist :-) All my files are executable... whahahaha |
7 participants
Description:
-allfromscript/lint. @cdce8p merged Script/lint: Changed default from 'all' to 'changed' #12660 a bit quick.scripts/lazytox.pyscript/lazytox.py behaviour
script/lint)gen_requirements validate. If issue, STOPIn the end this could spit out the complete PR template based on changes.
script/lazytox.py arguments
--skiplint: skipe flake8&pylint tests-- file1 file2: force certain files to be used in the diff calc -- forcing lint & pytests on certain files