From 45dfc2cdbacf70161c80dba3f5c92f65f219b6cf Mon Sep 17 00:00:00 2001 From: Ryan Northey Date: Wed, 4 Aug 2021 08:59:17 +0100 Subject: [PATCH] tooling: Fixes/cleanups/tests for `rst_checks.py` Signed-off-by: Ryan Northey --- docs/root/version_history/current.rst | 5 +- tools/docs/rst_check.py | 94 +++++++++++-------- tools/docs/tests/test_rst_check.py | 130 +++++++++++++------------- 3 files changed, 124 insertions(+), 105 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index fe14c7af5c5e5..21072c5ccd193 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -39,7 +39,7 @@ Bug Fixes *Changes expected to improve the state of the world and are unlikely to have negative effects* * access log: fix ``%UPSTREAM_CLUSTER%`` when used in http upstream access logs. Previously, it was always logging as an unset value. -* access log: fix `%UPSTREAM_CLUSTER%` when used in http upstream access logs. Previously, it was always logging as an unset value. +* access log: fix ``%UPSTREAM_CLUSTER%`` when used in http upstream access logs. Previously, it was always logging as an unset value. * aws request signer: fix the AWS Request Signer extension to correctly normalize the path and query string to be signed according to AWS' guidelines, so that the hash on the server side matches. See `AWS SigV4 documentaion `_. * cluster: delete pools when they're idle to fix unbounded memory use when using PROXY protocol upstream with tcp_proxy. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.conn_pool_delete_when_idle`` runtime guard to false. * xray: fix the AWS X-Ray tracer bug where span's error, fault and throttle information was not reported properly as per the `AWS X-Ray documentation `_. Before this fix, server error was reported under 'annotations' section of the segment data. @@ -59,7 +59,7 @@ New Features * http: added :ref:`string_match ` in the header matcher. * http: added support for :ref:`max_requests_per_connection ` for both upstream and downstream connections. * jwt_authn: added support for :ref:`Jwt Cache ` and its size can be specified by :ref:`jwt_cache_size `. -* listener: new listener metric `downstream_cx_transport_socket_connect_timeout` to track transport socket timeouts. +* listener: new listener metric ``downstream_cx_transport_socket_connect_timeout`` to track transport socket timeouts. * rbac: added :ref:`destination_port_range ` for matching range of destination ports. Deprecated @@ -71,4 +71,3 @@ Deprecated * listener: :ref:`reuse_port ` has been deprecated in favor of :ref:`enable_reuse_port `. At the same time, the default has been changed from false to true. See above for more information. - diff --git a/tools/docs/rst_check.py b/tools/docs/rst_check.py index c06d264c2691a..8a5b692d036d3 100644 --- a/tools/docs/rst_check.py +++ b/tools/docs/rst_check.py @@ -1,23 +1,30 @@ +import pathlib import re import sys -from typing import Iterator +from functools import cached_property +from typing import Iterator, List, Pattern from tools.base import checker -INVALID_REFLINK = re.compile(r".* ref:.*") -REF_WITH_PUNCTUATION_REGEX = re.compile(r".*\. <[^<]*>`\s*") -RELOADABLE_FLAG_REGEX = re.compile(r".*(...)(envoy.reloadable_features.[^ ]*)\s.*") -VERSION_HISTORY_NEW_LINE_REGEX = re.compile(r"\* ([a-z \-_]+): ([a-z:`]+)") -VERSION_HISTORY_SECTION_NAME = re.compile(r"^[A-Z][A-Za-z ]*$") +INVALID_REFLINK = r".* ref:.*" +REF_WITH_PUNCTUATION_REGEX = r".*\. <[^<]*>`\s*" +VERSION_HISTORY_NEW_LINE_REGEX = r"\* ([a-z \-_]+): ([a-z:`]+)" +VERSION_HISTORY_SECTION_NAME = r"^[A-Z][A-Za-z ]*$" # Make sure backticks come in pairs. # Exceptions: reflinks (ref:`` where the backtick won't be preceded by a space # links `title `_ where the _ is checked for in the regex. BAD_TICKS_REGEX = re.compile(r".* `[^`].*`[^_]") +# TODO(phlax): +# - generalize these checks to all rst files +# - improve checks/handling of "default role"/inline literals +# (perhaps using a sphinx plugin) +# - add rstcheck and/or rstlint -class CurrentVersionFile(object): - def __init__(self, path): +class CurrentVersionFile: + + def __init__(self, path: pathlib.Path): self._path = path @property @@ -26,8 +33,20 @@ def lines(self) -> Iterator[str]: for line in f.readlines(): yield line.strip() + @cached_property + def backticks_re(self) -> Pattern[str]: + return re.compile(BAD_TICKS_REGEX) + + @cached_property + def invalid_reflink_re(self) -> Pattern[str]: + return re.compile(INVALID_REFLINK) + + @cached_property + def new_line_re(self) -> Pattern[str]: + return re.compile(VERSION_HISTORY_NEW_LINE_REGEX) + @property - def path(self) -> str: + def path(self) -> pathlib.Path: return self._path @property @@ -37,25 +56,18 @@ def prior_endswith_period(self) -> bool: # Don't punctuation-check empty lines. or not self.prior_line # The text in the :ref ends with a . - or - (self.prior_line.endswith('`') and REF_WITH_PUNCTUATION_REGEX.match(self.prior_line))) - - def check_flags(self, line: str) -> list: - # TODO(phlax): improve checking of inline literals - # make sure flags are surrounded by ``s (ie "inline literal") - flag_match = RELOADABLE_FLAG_REGEX.match(line) - return ([f"Flag {flag_match.groups()[1]} should be enclosed in double back ticks"] - if flag_match and not flag_match.groups()[0].startswith(' ``') else []) - - def check_ticks(self, line: str) -> list: - ticks_match = BAD_TICKS_REGEX.match(line) - return ([f"Backticks should come in pairs (except for links and reflinks): {line}"] - if ticks_match else []) - - def check_line(self, line: str) -> list: - errors = self.check_reflink(line) + self.check_flags(line) - if RELOADABLE_FLAG_REGEX.match(line): - errors += self.check_ticks(line) + or (self.prior_line.endswith('`') and self.punctuation_re.match(self.prior_line))) + + @cached_property + def punctuation_re(self) -> Pattern[str]: + return re.compile(REF_WITH_PUNCTUATION_REGEX) + + @cached_property + def section_name_re(self) -> Pattern[str]: + return re.compile(VERSION_HISTORY_SECTION_NAME) + + def check_line(self, line: str) -> List[str]: + errors = self.check_reflink(line) + self.check_ticks(line) if line.startswith("* "): errors += self.check_list_item(line) elif not line: @@ -66,16 +78,16 @@ def check_line(self, line: str) -> list: self.prior_line += line return errors - def check_list_item(self, line: str) -> list: + def check_list_item(self, line: str) -> List[str]: errors = [] if not self.prior_endswith_period: errors.append(f"The following release note does not end with a '.'\n {self.prior_line}") - match = VERSION_HISTORY_NEW_LINE_REGEX.match(line) + match = self.new_line_re.match(line) if not match: return errors + [ "Version history line malformed. " - f"Does not match VERSION_HISTORY_NEW_LINE_REGEX in docs_check.py\n {line}\n" + f"Does not match VERSION_HISTORY_NEW_LINE_REGEX\n {line}\n" "Please use messages in the form 'category: feature explanation.', " "starting with a lower-cased letter and ending with a period." ] @@ -97,19 +109,22 @@ def check_list_item(self, line: str) -> list: self.set_tokens(line, first_word, next_word) return errors - def check_previous_period(self) -> list: + def check_previous_period(self) -> List[str]: return ([f"The following release note does not end with a '.'\n {self.prior_line}"] if not self.prior_endswith_period else []) - def check_reflink(self, line: str) -> list: - # TODO(phlax): Check reflinks for all rst files + def check_reflink(self, line: str) -> List[str]: return ([f"Found text \" ref:\". This should probably be \" :ref:\"\n{line}"] - if INVALID_REFLINK.match(line) else []) + if self.invalid_reflink_re.match(line) else []) + + def check_ticks(self, line: str) -> List[str]: + return ([f"Backticks should come in pairs (except for links and refs): {line}"] if + (self.backticks_re.match(line)) else []) def run_checks(self) -> Iterator[str]: self.set_tokens() for line_number, line in enumerate(self.lines): - if VERSION_HISTORY_SECTION_NAME.match(line): + if self.section_name_re.match(line): if line == "Deprecated": break self.set_tokens() @@ -125,13 +140,14 @@ def set_tokens(self, line: str = "", first_word: str = "", next_word: str = "") class RSTChecker(checker.Checker): checks = ("current_version",) - def check_current_version(self): - errors = list(CurrentVersionFile("docs/root/version_history/current.rst").run_checks()) + def check_current_version(self) -> None: + errors = list( + CurrentVersionFile(pathlib.Path("docs/root/version_history/current.rst")).run_checks()) if errors: self.error("current_version", errors) -def main(*args) -> int: +def main(*args: str) -> int: return RSTChecker(*args).run() diff --git a/tools/docs/tests/test_rst_check.py b/tools/docs/tests/test_rst_check.py index 19678db367817..a2bef68a71f0b 100644 --- a/tools/docs/tests/test_rst_check.py +++ b/tools/docs/tests/test_rst_check.py @@ -13,6 +13,29 @@ def test_rst_check_current_version_constructor(): assert version_file.path == "PATH" +@pytest.mark.parametrize( + "constant", + (("backticks_re", "BAD_TICKS_REGEX"), + ("invalid_reflink_re", "INVALID_REFLINK"), + ("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): + version_file = rst_check.CurrentVersionFile("PATH") + prop, constant = constant + patched = patches( + "re", + prefix="tools.docs.rst_check") + + with patched as (m_re, ): + assert getattr(version_file, prop) == m_re.compile.return_value + + assert ( + list(m_re.compile.call_args) + == [(getattr(rst_check, constant),), {}]) + assert prop in version_file.__dict__ + + def test_rst_check_current_version_lines(patches): version_file = rst_check.CurrentVersionFile("PATH") patched = patches( @@ -50,47 +73,6 @@ def test_rst_check_current_version_prior_ends_with_period(prior): assert version_file.prior_endswith_period == expected -@pytest.mark.parametrize("matches", [True, False, "partial"]) -def test_rst_check_current_version_check_flags(patches, matches): - version_file = rst_check.CurrentVersionFile("PATH") - patched = patches( - "RELOADABLE_FLAG_REGEX", - prefix="tools.docs.rst_check") - - with patched as (m_flag, ): - if matches == "partial": - m_flag.match.return_value.groups.return_value.__getitem__.return_value.startswith.return_value = False - elif not matches: - m_flag.match.return_value = False - result = version_file.check_flags("LINE") - - assert ( - list(m_flag.match.call_args) - == [('LINE',), {}]) - - if matches: - assert ( - list(m_flag.match.return_value.groups.call_args) - == [(), {}]) - assert ( - list(m_flag.match.return_value.groups.return_value.__getitem__.return_value.startswith.call_args) - == [(' ``',), {}]) - if matches == "partial": - assert ( - result - == [f"Flag {m_flag.match.return_value.groups.return_value.__getitem__.return_value} should be enclosed in double back ticks"]) - assert ( - list(list(c) for c in m_flag.match.return_value.groups.return_value.__getitem__.call_args_list) - == [[(0,), {}], [(1,), {}]]) - else: - assert ( - list(list(c) for c in m_flag.match.return_value.groups.return_value.__getitem__.call_args_list) - == [[(0,), {}]]) - assert result == [] - else: - assert result == [] - - @pytest.mark.parametrize("line", ["", " ", "* ", "*asdf"]) @pytest.mark.parametrize("prior_period", [True, False]) @pytest.mark.parametrize("prior_line", ["", "line_content"]) @@ -98,25 +80,26 @@ def test_rst_check_current_version_check_line(patches, line, prior_period, prior version_file = rst_check.CurrentVersionFile("PATH") patched = patches( "CurrentVersionFile.check_reflink", - "CurrentVersionFile.check_flags", "CurrentVersionFile.check_list_item", "CurrentVersionFile.check_previous_period", + "CurrentVersionFile.check_ticks", prefix="tools.docs.rst_check") version_file.prior_line = prior_line - with patched as (m_ref, m_flags, m_item, m_period): + with patched as (m_ref, m_item, m_period, m_ticks): result = version_file.check_line(line) expected = m_ref.return_value.__add__.return_value + assert ( list(m_ref.call_args) == [(line,), {}]) assert ( - list(m_flags.call_args) + list(m_ticks.call_args) == [(line,), {}]) assert ( list(m_ref.return_value.__add__.call_args) - == [(m_flags.return_value,), {}]) + == [(m_ticks.return_value,), {}]) if line.startswith("* "): assert ( @@ -159,9 +142,9 @@ def test_rst_check_current_version_check_line(patches, line, prior_period, prior def test_rst_check_current_version_check_list_item(patches, matches, prior, prior_first, prior_next, first_word, next_word): version_file = rst_check.CurrentVersionFile("PATH") patched = patches( - "VERSION_HISTORY_NEW_LINE_REGEX", "CurrentVersionFile.set_tokens", ("CurrentVersionFile.prior_endswith_period", dict(new_callable=PropertyMock)), + ("CurrentVersionFile.new_line_re", dict(new_callable=PropertyMock)), prefix="tools.docs.rst_check") version_file.prior_line = "PRIOR LINE" version_file.first_word_of_prior_line = prior_first @@ -172,11 +155,11 @@ def _get_item(item): return first_word return next_word - with patched as (m_regex, m_tokens, m_prior): + with patched as (m_tokens, m_prior, m_regex): if not matches: - m_regex.match.return_value = False + m_regex.return_value.match.return_value = False else: - m_regex.match.return_value.groups.return_value.__getitem__.side_effect = _get_item + m_regex.return_value.match.return_value.groups.return_value.__getitem__.side_effect = _get_item m_prior.return_value = prior result = version_file.check_list_item("LINE") @@ -185,13 +168,13 @@ def _get_item(item): expected += ["The following release note does not end with a '.'\n PRIOR LINE"] assert ( - list(m_regex.match.call_args) + list(m_regex.return_value.match.call_args) == [('LINE',), {}]) if not matches: expected += [ f"Version history line malformed. " - f"Does not match VERSION_HISTORY_NEW_LINE_REGEX in docs_check.py\n LINE\n" + f"Does not match VERSION_HISTORY_NEW_LINE_REGEX\n LINE\n" "Please use messages in the form 'category: feature explanation.', " "starting with a lower-cased letter and ending with a period."] assert result == expected @@ -199,7 +182,7 @@ def _get_item(item): return assert ( - list(list(c) for c in m_regex.match.return_value.groups.call_args_list) + list(list(c) for c in m_regex.return_value.match.return_value.groups.call_args_list) == [[(), {}], [(), {}]]) if prior_first and prior_first > first_word: @@ -236,15 +219,15 @@ def test_rst_check_current_version_check_previous_period(patches, prior): def test_rst_check_current_version_check_reflink(patches, matches): version_file = rst_check.CurrentVersionFile("PATH") patched = patches( - "INVALID_REFLINK", + ("CurrentVersionFile.invalid_reflink_re", dict(new_callable=PropertyMock)), prefix="tools.docs.rst_check") with patched as (m_reflink, ): - m_reflink.match.return_value = matches + m_reflink.return_value.match.return_value = matches result = version_file.check_reflink("LINE") assert ( - list(m_reflink.match.call_args) + list(m_reflink.return_value.match.call_args) == [('LINE',), {}]) if matches: @@ -255,6 +238,24 @@ def test_rst_check_current_version_check_reflink(patches, matches): assert result == [] +@pytest.mark.parametrize("matches", [True, False]) +def test_rst_check_current_version_check_ticks(patches, matches): + version_file = rst_check.CurrentVersionFile("PATH") + patched = patches( + ("CurrentVersionFile.backticks_re", dict(new_callable=PropertyMock)), + prefix="tools.docs.rst_check") + + with patched as (m_re, ): + m_re.return_value.match.return_value = matches + assert ( + version_file.check_ticks("LINE") + == (["Backticks should come in pairs (except for links and refs): LINE"] + if matches else [])) + assert ( + list(m_re.return_value.match.call_args) + == [('LINE',), {}]) + + @pytest.mark.parametrize( "lines", [[], @@ -267,16 +268,16 @@ def test_rst_check_current_version_run_checks(patches, lines, errors, matches): version_file = rst_check.CurrentVersionFile("PATH") patched = patches( "enumerate", - "VERSION_HISTORY_SECTION_NAME", "CurrentVersionFile.set_tokens", "CurrentVersionFile.check_line", ("CurrentVersionFile.lines", dict(new_callable=PropertyMock)), + ("CurrentVersionFile.section_name_re", dict(new_callable=PropertyMock)), prefix="tools.docs.rst_check") - with patched as (m_enum, m_section, m_tokens, m_check, m_lines): + with patched as (m_enum, m_tokens, m_check, m_lines, m_section): m_enum.return_value = lines m_check.return_value = errors - m_section.match.return_value = matches + m_section.return_value.match.return_value = matches _result = version_file.run_checks() assert isinstance(_result, types.GeneratorType) result = list(_result) @@ -287,7 +288,7 @@ def test_rst_check_current_version_run_checks(patches, lines, errors, matches): if not lines: assert result == [] - assert not m_section.match.called + assert not m_section.return_value.match.called assert not m_check.called assert ( list(list(c) for c in m_tokens.call_args_list) @@ -309,7 +310,7 @@ def test_rst_check_current_version_run_checks(patches, lines, errors, matches): for error in errors: _errors.append((line_number, error)) assert ( - list(list(c) for c in m_section.match.call_args_list) + list(list(c) for c in m_section.return_value.match.call_args_list) == [[(line,), {}] for line in _match]) assert ( list(list(c) for c in m_tokens.call_args_list) @@ -342,19 +343,22 @@ def test_rst_checker_constructor(): @pytest.mark.parametrize("errors", [[], ["err1", "err2"]]) def test_rst_checker_check_current_version(patches, errors): checker = rst_check.RSTChecker("path1", "path2", "path3") - patched = patches( + "pathlib", "CurrentVersionFile", "RSTChecker.error", prefix="tools.docs.rst_check") - with patched as (m_version, m_error): + with patched as (m_plib, m_version, m_error): m_version.return_value.run_checks.return_value = errors checker.check_current_version() assert ( - list(m_version.call_args) + list(m_plib.Path.call_args) == [('docs/root/version_history/current.rst',), {}]) + assert ( + list(m_version.call_args) + == [(m_plib.Path.return_value,), {}]) assert ( list(m_version.return_value.run_checks.call_args) == [(), {}])