Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Testing scripts & pull_request_template #12862

Merged
merged 6 commits into from
Mar 9, 2018
Merged

Testing scripts & pull_request_template #12862

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e087e2f
Add script/pytest
kellerza Mar 2, 2018
fd2ec17
lazytox
kellerza Mar 4, 2018
ed8a5ff
feedback
kellerza Mar 6, 2018
e9f6df6
add sugar & --skiplint
kellerza Mar 6, 2018
52185b2
print all and colorlog
kellerza Mar 7, 2018
6e6bceb
feedback
kellerza Mar 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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**
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


If user exposed functionality or configuration variables are added/changed:
- [ ] Documentation added/updated in [home-assistant.github.io](https://github.com/home-assistant/home-assistant.github.io)

If the code communicates with devices, web services, or third-party tools:
- [ ] Local tests with `tox` run successfully. **Your PR cannot be merged unless tests pass**
- [ ] New dependencies have been added to the `REQUIREMENTS` variable ([example][ex-requir]).
- [ ] New dependencies are only imported inside functions that use them ([example][ex-import]).
- [ ] New dependencies have been added to `requirements_all.txt` by running `script/gen_requirements_all.py`.
- [ ] New files were added to `.coveragerc`.

If the code does not interact with devices:
- [ ] Local tests with `tox` run successfully. **Your PR cannot be merged unless tests pass**
- [ ] Tests have been added to verify that the new code works.

[ex-requir]: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/keyboard.py#L14
Expand Down
38 changes: 17 additions & 21 deletions script/lint
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,21 @@

cd "$(dirname "$0")/.."

if [ "$1" = "--all" ]; then
tox -e lint
else
export files="`git diff upstream/dev... --name-only | grep -e '\.py$'`"
echo "================================================="
echo "FILES CHANGED (git diff upstream/dev... --name-only)"
echo "================================================="
if [ -z "$files" ] ; then
echo "No python file changed"
exit
fi
printf "%s\n" $files
echo "================"
echo "LINT with flake8"
echo "================"
flake8 --doctests $files
echo "================"
echo "LINT with pylint"
echo "================"
pylint $files
echo
export files="`git diff upstream/dev... --name-only | grep -e '\.py$'`"
echo "================================================="
echo "FILES CHANGED (git diff upstream/dev... --name-only)"
echo "================================================="
if [ -z "$files" ] ; then
echo "No python file changed. Rather use: tox -e lint"
exit
fi
printf "%s\n" $files
echo "================"
echo "LINT with flake8"
echo "================"
flake8 --doctests $files
echo "================"
echo "LINT with pylint"
echo "================"
pylint $files
echo
15 changes: 15 additions & 0 deletions script/pytest
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#!/bin/sh
# Execute pylint on changed tests.

cd "$(dirname "$0")/.."

export files="`git diff upstream/dev --name-only | grep -e 'test_.\+\.py$'`"
if [ -z "$files" ] ; then
echo "No test files changed. Rather use pytest or tox directly."
exit
fi
export files="`printf "%s " $files`"
echo "======"
echo "pytest -vv -- $files"
echo "======"
pytest -vv -- $files