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

[WIP] python: Integrate linting/utils/tests #15833

Closed
wants to merge 4 commits into from
Closed

[WIP] python: Integrate linting/utils/tests #15833

wants to merge 4 commits into from

Conversation

phlax
Copy link
Member

@phlax phlax commented Apr 3, 2021 •
edited
Loading

Signed-off-by: Ryan Northey ryan@synca.io

Commit Message: python: Integrate linting/utils/tests
Additional Description:

This PR:

  • consolidates linting tools and ci
  • moves all tooling configs to repo root to allow using them directly and make explicit
  • improves the previouly added Checker check runner
  • adds a generic Runner with argparse
  • adds pytest
  • adds unit tests for all of the included code (coverage == 100% for added code)
  • separates tooling tests into its own ci job
  • adds coverage for tooling tests (eg https://storage.googleapis.com/envoy-pr/15833/tooling/index.html)
  • moves the python linting job to format_pre only and includes a diff there

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@phlax phlax marked this pull request as draft April 3, 2021 12:19
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #15833 was synchronize by phlax.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Apr 4, 2021
phlax
phlax commented Apr 4, 2021
View reviewed changes
tools/dependency/pip_check.py Outdated Show resolved Hide resolved
@phlax phlax force-pushed the tooling-py-checkers branch from 56cd4db to 2e3ef71 Compare April 4, 2021 13:12
phlax
phlax commented Apr 4, 2021
View reviewed changes
tools/base/checker.py Outdated Show resolved Hide resolved
phlax
phlax commented Apr 4, 2021
View reviewed changes
tools/base/checker.py Outdated Show resolved Hide resolved
phlax
phlax commented Apr 4, 2021
View reviewed changes
tools/base/checker.py Outdated Show resolved Hide resolved
phlax
phlax commented Apr 4, 2021
View reviewed changes
tools/base/checker.py Outdated
resp = self.fork(["bazel", "query", f"'{query}'"])
return resp.stdout.decode("utf-8").split("\n")

def error(self, name: str, errors: list, log: bool = True) -> str:
Copy link
Member Author

Choose a reason for hiding this comment

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

add a comment about only calling these once per check::subcheck - ie check on a file

phlax
phlax commented Apr 4, 2021
View reviewed changes
tools/base/checker.py Outdated Show resolved Hide resolved
phlax
phlax commented Apr 4, 2021
View reviewed changes
tools/base/checker.py Outdated Show resolved Hide resolved
phlax
phlax commented Apr 4, 2021
View reviewed changes
tools/testing/all_pytests.py Outdated Show resolved Hide resolved
phlax
phlax commented Apr 4, 2021
View reviewed changes
tools/testing/python_pytest.py Outdated Show resolved Hide resolved
@phlax phlax force-pushed the tooling-py-checkers branch from 360bed2 to 7787031 Compare April 4, 2021 13:42
@phlax phlax changed the title [WIP] python: Integrate linting/utils [WIP] python: Integrate linting/utils/tests Apr 4, 2021
@phlax phlax force-pushed the tooling-py-checkers branch from b3a6edd to 4fb8512 Compare April 4, 2021 14:50
phlax
phlax commented Apr 4, 2021
View reviewed changes
tools/base/checker.py Outdated Show resolved Hide resolved
@phlax
Copy link
Member Author

phlax commented Apr 4, 2021

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15833 (comment) was created by @phlax.

see: more, trace.

phlax
phlax commented Apr 4, 2021
View reviewed changes
tools/base/pytest_checker.py Outdated Show resolved Hide resolved
phlax
phlax commented Apr 5, 2021
View reviewed changes
tools/base/checker.py Outdated
class Checker(object):
"""Runs check methods prefixed with `check_` and named in `self.checks`

check methods should return the count of errors and log warnings and errors
Copy link
Member Author

Choose a reason for hiding this comment

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

better explanation.

phlax
phlax commented Apr 5, 2021
View reviewed changes
tools/base/checker.py Outdated Show resolved Hide resolved
phlax
phlax commented Apr 5, 2021
View reviewed changes
tools/base/checker.py Outdated Show resolved Hide resolved
phlax
phlax commented Apr 5, 2021
View reviewed changes
tools/base/checker.py Outdated Show resolved Hide resolved
phlax
phlax commented Apr 5, 2021
View reviewed changes
tools/base/BUILD Outdated Show resolved Hide resolved
@phlax phlax marked this pull request as ready for review April 7, 2021 13:42
@phlax
Copy link
Member Author

phlax commented Apr 7, 2021

@htuch @mattklein123 this should be ready for review

please see the updated description for an explanation as to what the pr does

@phlax phlax changed the title [WIP] python: Integrate linting/utils/tests python: Integrate linting/utils/tests Apr 7, 2021
@phlax
Copy link
Member Author

phlax commented Apr 7, 2021

i have a few cleanups left, but just nits i think

phlax
phlax commented Apr 7, 2021
View reviewed changes
tools/dependency/pip_check.py
#
# bazel //tools/dependency:pip_check -- -h
#
# alternatively, if you have the necessary python deps available
Copy link
Member Author

Choose a reason for hiding this comment

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

no deps required

phlax
phlax commented Apr 7, 2021
View reviewed changes
tools/code_format/python_check.py Outdated
# - isort


class PythonChecker(checker.ForkingChecker):
Copy link
Member Author

Choose a reason for hiding this comment

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

add some docstrings

phlax
phlax commented Apr 7, 2021
View reviewed changes
tools/base/checker.py Outdated
return self.summary_class(self)

@property
def summary_class(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

hint

phlax
phlax commented Apr 7, 2021
View reviewed changes
tools/base/checker.py Outdated Show resolved Hide resolved
phlax
phlax commented Apr 7, 2021
View reviewed changes
tools/base/checker.py Outdated
return 1 if self.has_failed else 0

def run_checks(self) -> int:
"""Run all configured checks and return the sum of their error counts"""
Copy link
Member Author

Choose a reason for hiding this comment

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

wrong description

phlax
phlax commented Apr 7, 2021
View reviewed changes
tools/base/checker.py Outdated
"""Maximum warnings to display in summary"""
return self.checker.args.summary_warnings

def print_failed(self, problem_type):
Copy link
Member Author

Choose a reason for hiding this comment

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

hint

Copy link
Member Author

Choose a reason for hiding this comment

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

failed is not necessarily correct/ambiguous

phlax
phlax commented Apr 7, 2021
View reviewed changes
tools/base/utils.py Outdated
yield

if stdout is not None:
_stdout.seek(0)
Copy link
Member Author

Choose a reason for hiding this comment

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

unnecessary seek

htuch
htuch reviewed Apr 8, 2021
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

@phlax is there any way to split this up into multiple PRs? It's pretty huge, would prefer to eat the elephant a bite at a time.

@phlax
Copy link
Member Author

phlax commented Apr 8, 2021

@htuch ive broken out 2 prs which should hopefully smooth the way and make separate some of the issues being addressed

firstly, consolidating the configs and ci for the python linting tools - #15886

secondly, adding pytest/coverage and some initial tests - #15888

@phlax phlax changed the title python: Integrate linting/utils/tests [WIP] python: Integrate linting/utils/tests Apr 8, 2021
@phlax phlax marked this pull request as draft April 8, 2021 12:00
@phlax phlax force-pushed the tooling-py-checkers branch 5 times, most recently from 08dbe08 to d74ce0c Compare April 12, 2021 09:18
phlax added 3 commits April 16, 2021 09:09
@phlax
python: Integrate linting/utils/tests
36c5860 
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
cleanup
c307543 
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
pytests
98300f6 
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the tooling-py-checkers branch from d74ce0c to 98300f6 Compare April 16, 2021 08:10
@phlax
cleanup
5bbd174 
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax mentioned this pull request Apr 16, 2021
Merged
@phlax
Copy link
Member Author

phlax commented Apr 20, 2021

this has now landed in other prs

@phlax phlax closed this Apr 20, 2021
htuch pushed a commit that referenced this pull request Apr 22, 2021
@phlax
tooling: Add all pytests checker (#16031)
15d71b0 
another breakout from #15833

Signed-off-by: Ryan Northey <ryan@synca.io>
gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021
@phlax
tooling: Add all pytests checker (envoyproxy#16031)
efc3a07 
another breakout from envoyproxy#15833

Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Gokul Nair <gnair@twitter.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htuch htuch htuch left review comments

Assignees
No one assigned
Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@phlax @htuch