Skip to content

tooling: Fixes/cleanups/tests for rst_check.py#17589

Merged
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:tooling-rst-check-tests
Aug 5, 2021
Merged

tooling: Fixes/cleanups/tests for rst_check.py#17589
phlax merged 1 commit intoenvoyproxy:mainfrom
phlax:tooling-rst-check-tests

Conversation

@phlax
Copy link
Member

@phlax phlax commented Aug 4, 2021

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

Commit Message: tooling: Fixes/cleanups/tests for rst_checks.py
Additional Description:

Cleanups for rst_check.py

  • adds missing tests
  • removes now redundant check
  • generalizes the backtick check to all lines, not just lines that happen to contain the reloadable re
  • fixes any other backtick issues in version history file
  • brings constants into the class namespace and makes re compilation lazy
  • adds pathlib
  • general cleanups

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

@phlax
Copy link
Member Author

phlax commented Aug 4, 2021

cc @htuch @alyssawilk

@phlax phlax force-pushed the tooling-rst-check-tests branch from e219127 to 7ddb4e4 Compare August 4, 2021 08:20
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the tooling-rst-check-tests branch from 7ddb4e4 to 45dfc2c Compare August 4, 2021 08:32
@phlax
Copy link
Member Author

phlax commented Aug 4, 2021

@phlax phlax changed the title tooling: Fixes/cleanups/tests for rst_checks.py tooling: Fixes/cleanups/tests for rst_check.py Aug 4, 2021
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Nice catch, thanks for generalizing

("new_line_re", "VERSION_HISTORY_NEW_LINE_REGEX"),
("punctuation_re", "REF_WITH_PUNCTUATION_REGEX"),
("section_name_re", "VERSION_HISTORY_SECTION_NAME")))
def test_rst_check_current_version_regexes(patches, constant):
Copy link
Contributor

Choose a reason for hiding this comment

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

either in this Pr or a follow-up can you comment up these tests a bit? when I broke one the error messages were pretty confusing and the code was pretty hard to read for someone with low python skills.

Copy link
Member Author

@phlax phlax Aug 5, 2021

Choose a reason for hiding this comment

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

yep happy to add some comments in a follow up - mostly i havent because the tests fairly systematically test methods/functions and follow pytest/mock conventions

testing can be done with, eg:

bazel run //tools/docs:pytest_rst_check

if something breaks breakpoint() can be a lifesaver with the tests

Copy link
Member Author

Choose a reason for hiding this comment

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

...well that is not entirely accurate - i added the patches fixture - i should probs document how the tests work, and how to quickly debug them better more generally i think

Copy link
Member Author

Choose a reason for hiding this comment

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

actually there is this here https://github.com/envoyproxy/envoy/blob/main/tools/README.md - could probs do with some updating

Copy link
Member Author

Choose a reason for hiding this comment

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

@phlax phlax merged commit 2ca7de5 into envoyproxy:main Aug 5, 2021
baojr added a commit to baojr/envoy that referenced this pull request Aug 5, 2021
…bridge-stream

* upstream/main:
  stream info: add upstream connection id (envoyproxy#17512)
  runtime: removing require_ocsp_response_for_must_staple_certs (envoyproxy#17541)
  tooling: Fixes/cleanups/tests for `rst_checks.py` (envoyproxy#17589)
  Add a new HappyEyeballsConnectionImpl class (envoyproxy#17371)
  hcm/listener_manager: remove deprecated v2 config handling. (envoyproxy#17586)
  tools: simplify bazel deps query (envoyproxy#17599)
  tooling: Move tempdir/cleanup to base runner class (envoyproxy#17590)
  tools: fix missing format strings (envoyproxy#17600)
  tooling: Add `@envoy` prefix in `envoy_py_` macros (envoyproxy#17588)
  stream info: add attempt count for HTTP requests and TCP proxy (envoyproxy#17583)

Signed-off-by: Garrett Bourg <bourg@squareup.com>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Signed-off-by: Ryan Northey <ryan@synca.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants