Skip to content

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

Closed
phlax wants to merge 4 commits intoenvoyproxy:mainfrom
phlax:tooling-py-checkers
Closed

[WIP] python: Integrate linting/utils/tests#15833
phlax wants to merge 4 commits intoenvoyproxy:mainfrom
phlax:tooling-py-checkers

Conversation

@phlax
Copy link
Copy Markdown
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
Copy Markdown

repokitteh-read-only bot commented Apr 4, 2021

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
@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
phlax
phlax commented Apr 4, 2021
View reviewed changes
phlax
phlax commented Apr 4, 2021
View reviewed changes
phlax
phlax commented Apr 4, 2021
View reviewed changes
tools/base/checker.py Outdated
Copy link
Copy Markdown
Member Author

@phlax phlax Apr 4, 2021

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
phlax
phlax commented Apr 4, 2021
View reviewed changes
phlax
phlax commented Apr 4, 2021
View reviewed changes
phlax
phlax commented Apr 4, 2021
View reviewed changes
phlax
phlax commented Apr 4, 2021
View reviewed changes
@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
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Apr 4, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Apr 4, 2021

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
phlax
phlax commented Apr 4, 2021
View reviewed changes
phlax
phlax commented Apr 4, 2021
View reviewed changes
phlax
phlax commented Apr 4, 2021
View reviewed changes
phlax
phlax commented Apr 5, 2021
View reviewed changes
phlax
phlax commented Apr 5, 2021
View reviewed changes
tools/base/checker.py Outdated
Copy link
Copy Markdown
Member Author

@phlax phlax Apr 5, 2021

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
phlax
phlax commented Apr 5, 2021
View reviewed changes
phlax
phlax commented Apr 5, 2021
View reviewed changes
phlax
phlax commented Apr 5, 2021
View reviewed changes
@phlax phlax marked this pull request as ready for review April 7, 2021 13:42
@phlax
Copy link
Copy Markdown
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
Copy Markdown
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 Outdated
Copy link
Copy Markdown
Member Author

@phlax phlax Apr 7, 2021

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
Copy link
Copy Markdown
Member Author

@phlax phlax Apr 7, 2021

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
Copy link
Copy Markdown
Member Author

@phlax phlax Apr 7, 2021

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
phlax
phlax commented Apr 7, 2021
View reviewed changes
tools/base/checker.py Outdated
Copy link
Copy Markdown
Member Author

@phlax phlax Apr 7, 2021

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
Copy link
Copy Markdown
Member Author

@phlax phlax Apr 7, 2021

Choose a reason for hiding this comment

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

hint

Copy link
Copy Markdown
Member Author

@phlax phlax Apr 7, 2021

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
Copy link
Copy Markdown
Member Author

@phlax phlax Apr 7, 2021

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

@agrawroh agrawroh Awaiting requested review from agrawroh agrawroh will be requested when the pull request is marked ready for review agrawroh is a code owner

@jwendell jwendell Awaiting requested review from jwendell jwendell will be requested when the pull request is marked ready for review jwendell is a code owner

1 more reviewer

@htuch htuch htuch left review comments

Reviewers whose approvals may not affect merge requirements

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