Testing scripts & pull_request_template #12862

kellerza · 2018-03-03T08:42:31Z

Description:

Dropped -all from script/lint. @cdce8p merged Script/lint: Changed default from 'all' to 'changed' #12660 a bit quick.
Small update to PULL_REQUEST_TEMPLATE (less info should always be better)
Added a new quick script for testing before submitting a PR - scripts/lazytox.py

script/lazytox.py behaviour

Get changed filed from git diff (same as script/lint)
Run flake8 & pylint. If any issues, STOP
If any component/ changed, run gen_requirements validate. If issue, STOP
Create list of changed tests as suggested by @OttoWinter and feed them through pylint

In 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

OttoWinter

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 like script/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__.py it would be great if the script would also know to run tests/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.

kellerza · 2018-03-03T11:06:46Z

Thanks Otto,

+1 for laziness and not patient enough to wait for tox
I was thinking of sed to achieve this? Gets hairy very soon though

General Poll: ideas for names for 1?

OttoWinter · 2018-03-03T11:12:55Z

I'm not a bash pro, but I do agree that it could get very complicated very quickly. I mean we do have python available, so why not rewrite the script using subprocess, re and pathlib? Then the logic would be much easier to write+understand.

MartinHjelmare · 2018-03-03T11:25:05Z

.github/PULL_REQUEST_TEMPLATE.md

@@ -12,19 +12,18 @@

 ## 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**


Why do we have to run the scripts if we also run tox?

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

Tox covers everything right, so please make that clear in the instructions. If you run tox, you don't need to run the rest.

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.

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.

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.

@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)

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.

cdce8p · 2018-03-03T12:13:59Z

I like the idea of adding script/lint to the PR_Template. It could prevent so many flake and pylint errors. Just keep in mind that it requires test dependencies (at least pylint and flake8).

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

kellerza · 2018-03-03T13:23:58Z

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

kellerza · 2018-03-04T22:51:11Z

For now I only added script/lint to the PR template.

We can start testing script/lazytox.py and eventually this could even generate a final PR template...

MartinHjelmare · 2018-03-05T08:33:16Z

I don't think we should change the PR template. tox covers everything and is the only test runner required before PR submission.

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 tox is still good to do.

OttoWinter · 2018-03-05T08:40:57Z

script/lazytox.py

+async def git():
+    """Exec git."""
+    try:
+        with open('lazytox.log') as file:


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)?

.gitignore contains *.log. The development Docker image do not contain git and did not have luck installing it, hence the "cache"

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.

OttoWinter · 2018-03-05T08:41:38Z

script/lazytox.py

+            read_stream(proc.stderr, sys.stderr.write))
+    exit_code = await proc.wait()
+    # Convert to ASCII
+    stdout = stdout.decode('utf-8')


Shouldn't the comment then be: # Convert to UTF-8 ? 👀

OttoWinter · 2018-03-05T08:56:38Z

script/lazytox.py

+        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)))


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)))

https://docs.python.org/3/library/shlex.html#shlex.quote

OttoWinter · 2018-03-05T08:59:49Z

script/lazytox.py

+        _, log = await async_exec(
+            'git', 'diff', 'upstream/dev...', '--name-only')
+    except FileNotFoundError:
+        print("Using a cached version of changed files.")


FileNotFoundError only gets thrown if git isn't installed, right?

I don't think we really need to catch that exception, since git is pretty much a prerequisite for Home Assistant development.

If git isn'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.

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

OttoWinter · 2018-03-05T09:00:58Z

script/lazytox.py

+        _, log = await async_exec('pylint', '-f', 'parseable', *files,
+                                  mute_err=True)
+    except FileNotFoundError:
+        return []


Same here, let's not fail silently if we can't run pylint.

Error message printed by async_exec

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...

OttoWinter · 2018-03-05T09:02:09Z

script/lazytox.py

+from collections import namedtuple
+
+RE_ASCII = re.compile(r"\033\[[^m]*m")
+ERROR = namedtuple('ERROR', "file line col msg")


Python classes are usually UpperCamelCase to avoid confusion with constants.

Error = namedtuple('Error', "file line col msg")

Will change and add pylint disable directive

OttoWinter · 2018-03-05T09:04:19Z

script/lazytox.py

+        line = line.split(':')
+        if len(line) != 4:
+            continue
+        res.append(ERROR(line[0].replace('\\', '/'), line[1], "", line[2]))


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...

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

OttoWinter · 2018-03-05T09:09:18Z

script/lazytox.py

+    pyfile = re.compile(r".+\.py$")
+    pyfiles = [file for file in files if pyfile.match(file)]
+
+    print("CHANGED FILES:", ' '.join(files))


Same as with the script/lint script: For better readability it would be better to place all changed files on new lines.

OttoWinter · 2018-03-05T09:12:14Z

script/lazytox.py

+            test_files.add(err.file)
+        else:
+            tfile = err.file.replace('homeassistant', 'tests') \
+                .replace('__init__', 'test_init')


AFAIK all tests start with test_, not just __init__. For example, homeassistant/components/mqtt/discovery.py → tests/components/mqtt/test_discovery.py

Ah, my test sample had a changed test_ in! --> good catch!. Will need a re replace for this one

OttoWinter · 2018-03-05T09:14:00Z

script/lazytox.py

+        if err.file.startswith('tests/'):
+            test_files.add(err.file)
+        else:
+            tfile = err.file.replace('homeassistant', 'tests') \


Why do we only add the matching tests/ files iff there's an error in the homeassistant/ files?

😕 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

OttoWinter · 2018-03-07T09:31:30Z

script/lazytox.py

+        print("No files identified, ideally you should run tox.")
+        return
+
+    print('pytest -vv --', ' '.join(shlex.quote(fle) for fle in test_files))


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.

--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

Now printing all commands being executed using this method you suggested

+1 for colorlog 🎉

👍 ..... quickly... merge!

OttoWinter · 2018-03-07T09:35:36Z

script/lazytox.py

+
+async def flake8(files):
+    """Exec flake8."""
+    _, log = await async_exec('flake8', *files)


flake8 seems to be used with --doctests in the script/lint script, we should do the same here then.

OttoWinter

Just a few comments, most of them can safely be ignored. Already looking forward to this one very much 🎉

OttoWinter · 2018-03-09T18:20:00Z

script/lazytox.py

@@ -0,0 +1,234 @@
+#!/usr/bin/env python


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

OttoWinter · 2018-03-09T18:23:24Z

script/lazytox.py

+
+RE_ASCII = re.compile(r"\033\[[^m]*m")
+Error = namedtuple('ERROR',
+                   "file line col msg")  # pylint: disable=invalid-name


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.)

OttoWinter · 2018-03-09T18:24:58Z

script/lazytox.py

+            argsp.append("\\\n  {}".format(shlex.quote(arg)))
+        else:
+            argsp.append(shlex.quote(arg))
+    printc('cyan', ' '.join(argsp))


(You can ignore this if this is for readability). printc already does ' '.join(args)...

printc('cyan', *argsp)

OttoWinter · 2018-03-09T18:29:04Z

script/lazytox.py

+
+async def pylint(files):
+    """Exec pylint."""
+    _, log = await async_exec('pylint', '-f', 'parseable', '--persistent=n',


+1 for --persistent=n 🎉

OttoWinter · 2018-03-09T18:36:42Z

script/lazytox.py

+    print("=============================")
+
+    if code == 0:
+        printc(PASS, "Yay! This will most likely pass tox")


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 🎉")

This will probably run mostly in bash, otherwise it would have been a good idea! 👍

OttoWinter

Awesome 🎉

kellerza · 2018-03-09T20:28:12Z

happy testing ! 🎉

OttoWinter · 2018-03-10T07:05:21Z

Oh... I think you forgot to make script/lazytox.py executable (chmod +x script/lazytox.py)

kellerza · 2018-03-10T07:32:54Z

Mine is executable, but I commit from Windows, so guess that concept does not exist :-)

All my files are executable... whahahaha

Add script/pytest

e087e2f

homeassistant added the cla-signed label Mar 3, 2018

OttoWinter reviewed Mar 3, 2018

View reviewed changes

MartinHjelmare reviewed Mar 3, 2018

View reviewed changes

lazytox

fd2ec17

homeassistant added the cla-signed label Mar 4, 2018

OttoWinter reviewed Mar 5, 2018

View reviewed changes

feedback

ed8a5ff

homeassistant added the cla-signed label Mar 6, 2018

add sugar & --skiplint

e9f6df6

OttoWinter reviewed Mar 7, 2018

View reviewed changes

print all and colorlog

52185b2

OttoWinter reviewed Mar 9, 2018

View reviewed changes

feedback

6e6bceb

OttoWinter approved these changes Mar 9, 2018

View reviewed changes

kellerza merged commit 37d8cd7 into home-assistant:dev Mar 9, 2018

kellerza deleted the scripts branch March 9, 2018 20:27

OttoWinter mentioned this pull request Mar 10, 2018

Make lazytox script executable #13040

Merged

1 task

balloob mentioned this pull request Mar 30, 2018

0.66.0 #13554

Merged

home-assistant locked and limited conversation to collaborators Jul 26, 2018

Testing scripts & pull_request_template #12862

Testing scripts & pull_request_template #12862

Conversation

kellerza commented Mar 3, 2018 • edited Loading

Description:

script/lazytox.py behaviour

script/lazytox.py arguments

OttoWinter left a comment

Choose a reason for hiding this comment

kellerza commented Mar 3, 2018

OttoWinter commented Mar 3, 2018

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

cdce8p commented Mar 3, 2018 • edited Loading

kellerza commented Mar 3, 2018

kellerza commented Mar 4, 2018

MartinHjelmare commented Mar 5, 2018

OttoWinter Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OttoWinter Mar 5, 2018 • 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

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

kellerza Mar 5, 2018 • 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

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

OttoWinter 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

OttoWinter left a comment

Choose a reason for hiding this comment

kellerza commented Mar 9, 2018

OttoWinter commented Mar 10, 2018

kellerza commented Mar 10, 2018

kellerza commented Mar 3, 2018 •

edited

Loading

cdce8p commented Mar 3, 2018 •

edited

Loading

OttoWinter Mar 5, 2018 •

edited

Loading

OttoWinter Mar 5, 2018 •

edited

Loading

kellerza Mar 5, 2018 •

edited

Loading