diff --git a/.github/workflows/code-health.yml b/.github/workflows/code-health.yml index 835da97bed..cb577266da 100644 --- a/.github/workflows/code-health.yml +++ b/.github/workflows/code-health.yml @@ -1,8 +1,15 @@ name: C++ Code Health +# Stub: the linter (devtools/code_health) and its self-tests live on the fork's agents branch. +# This checks that branch out next to the code and runs it against the code checkout. on: push: branches: [main] pull_request: + workflow_dispatch: + inputs: + base-ref: + description: Revision to compare with (e.g. origin/main); empty = whole tree + required: false permissions: contents: read @@ -14,22 +21,33 @@ jobs: - uses: actions/checkout@v7 with: fetch-depth: 0 + - uses: actions/checkout@v7 + with: + repository: dayfine/Arduino-Source + ref: agents + path: .agents-tools - uses: actions/setup-python@v6 with: python-version: '3.11' cache: pip - cache-dependency-path: devtools/code_health/requirements.txt - - run: python -m pip install -r devtools/code_health/requirements.txt + cache-dependency-path: .agents-tools/devtools/code_health/requirements.txt + - run: python -m pip install -r .agents-tools/devtools/code_health/requirements.txt - name: Linter self-tests + working-directory: .agents-tools + env: + CODE_HEALTH_REPO: ${{ github.workspace }} run: python -m unittest discover -s devtools/code_health/tests -v - name: Check changed C++ files + working-directory: .agents-tools env: PR_BASE: ${{ github.event.pull_request.base.sha }} + INPUT_BASE: ${{ inputs.base-ref }} PUSH_BASE: ${{ github.event.before }} + CODE: ${{ github.workspace }} shell: bash run: | - base="${PR_BASE:-$PUSH_BASE}" + base="${PR_BASE:-${INPUT_BASE:-$PUSH_BASE}}" if [[ -z "$base" || "$base" == 0000000000000000000000000000000000000000 ]]; then - base=$(git hash-object -w -t tree --stdin /DownloadedResources/`. Missing -fixture/model resources are not passes. - -For a configured Linux build: - -```sh -.venv-code-health/bin/python devtools/verify.py --build-dir SerialPrograms/bin --binary SerialPrograms/bin/SerialPrograms --base-ref origin/main -``` - -On macOS use `--binary SerialPrograms/bin/SerialPrograms.app/Contents/MacOS/SerialPrograms`; -on Windows use `--binary SerialPrograms/bin/RelWithDebInfo/SerialPrograms.exe`. -This runs linter self-tests, the incremental check, CMake build, then native tests; -it stops at the first nonzero exit and propagates it. Native tests run from -the build directory. On macOS it uses a separate `agent-native-tests` profile; -Qt is offscreen on all platforms. Use this checkout's build/binary. - -## Agent work loop and reuse - -Feature branch -> one concern -> test/verify -> read exit status -> review diff -and CI -> draft PR on dayfine/Arduino-Source. No upstream PR publication by -agents. Refresh canonical rules from origin/agents at task start and before -PR mutations; never merge that notes branch into code branches. Shared -infrastructure needs portable regression coverage; gameplay changes additionally -need appropriate live run evidence. - -`metrics.py` and `policy.py` accept source/metrics as values and know nothing -about this repository's Git layout. `git_files.py` and `__main__.py` are the -integration layer; extract a separate package only when another consumer needs it. diff --git a/devtools/code_health/__main__.py b/devtools/code_health/__main__.py deleted file mode 100644 index 0536c28f01..0000000000 --- a/devtools/code_health/__main__.py +++ /dev/null @@ -1,70 +0,0 @@ -"""Lint changed C++: python -m devtools.code_health [--base-ref REV] [--github].""" -import argparse -import os -import subprocess -import sys -from pathlib import Path - -from . import layering, style -from .classes import measure_classes -from .findings import annotations -from .git_files import changed_files, default_base, read_revision -from .metrics import measure -from .policy import advisories, evaluate - - -def analyze(path, source): - return measure(path, source) + measure_classes(path, source) - - -def run(root, base_ref=None, github=False): - if base_ref is None: - base_ref, how = default_base(root) - print(f'Base: {base_ref} ({how}).') - changes = changed_files(root, base_ref) - failures, notes = [], [] - advisory = 0 - for current, original in changes: - source = (root / current).read_text(encoding='utf-8-sig') - previous_source = read_revision(root, base_ref, original) if original else None - metrics = analyze(current, source) - previous = analyze(current, previous_source) if original else [] - failures.extend(layering.check(current, source, previous_source)) - failures.extend(evaluate(metrics, previous)) - notes.extend(advisories(metrics, previous)) - style_errors, style_notes = style.check(current, source, previous_source) - failures.extend(style_errors) - notes.extend(style_notes) - advisory += sum(m.check == 'file_lines' and 500 < m.value <= 1000 for m in metrics) - print(f'Scanned {len(changes)} changed C++ files against {base_ref}.') - print(f'Advisory: {advisory} changed files exceed 500 lines; prefer functions <=25 lines.') - for note in notes: - print(f'Advisory: {note}') - for failure in failures: - print(failure, file=sys.stderr) - if github: - for line in annotations('error', failures) + annotations('warning', notes): - print(line) - print(f'Code health: {len(failures)} failures.') - return bool(failures) - - -def main(): - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument('--root', type=Path, default=Path.cwd()) - parser.add_argument('--base-ref', help='Compare changed files with this revision (default: ' - 'merge-base of HEAD and upstream/main, else origin/main)') - parser.add_argument('--github', action='store_true', - help='Also print findings as GitHub Actions annotations ' - '(default when GITHUB_ACTIONS=true)') - args = parser.parse_args() - github = args.github or os.environ.get('GITHUB_ACTIONS') == 'true' - try: - return run(args.root.resolve(), args.base_ref, github) - except (OSError, ValueError, subprocess.CalledProcessError) as error: - print(f'Code-health setup/analysis error: {error}', file=sys.stderr) - return 2 - - -if __name__ == '__main__': - sys.exit(main()) diff --git a/devtools/code_health/classes.py b/devtools/code_health/classes.py deleted file mode 100644 index 50acb4d591..0000000000 --- a/devtools/code_health/classes.py +++ /dev/null @@ -1,335 +0,0 @@ -"""Class/struct shape checks from a lexical scan (no compiler or include paths needed). - -Members are recognized from lizard's C++ tokens: access sections, declarations ended -by ';' and inline bodies. Unbalanced braces from #if branches make a class unmeasured -rather than failing the run. -""" -import re -from dataclasses import dataclass - -from lizard_languages import CLikeReader - -from .config import CLASS_LIMITS -from .metrics import Metric - - -MACRO = re.compile(r'^[A-Z][A-Z0-9_]*$') -IDENTIFIER = re.compile(r'^[A-Za-z_]\w*$') -ACCESS = {'public', 'private', 'protected'} -SIGNALS = {'signals', 'Q_SIGNALS'} -SLOTS = {'slots', 'Q_SLOTS'} -TYPE_KEYWORDS = {'class', 'struct', 'union'} -# Words that may precede '(' in a declaration without naming a function. -NOT_NAMES = {'decltype', 'alignas', 'alignof', 'sizeof', 'noexcept', 'throw', 'requires', - '__attribute__', '__declspec', 'void', 'int', 'bool', 'char', 'auto'} -SKIP_DECLARATIONS = {'friend', 'using', 'typedef', 'static_assert', 'enum'} - - -@dataclass(frozen=True) -class Member: - access: str - kind: str # method, ctor, dtor, operator, data, type - pure: bool = False - deleted: bool = False - virtual: bool = False - static: bool = False - const: bool = False # const/static method, or const data: cannot mutate the object - option: bool = False # a config option or table cell field (type named *Option or *Cell) - - -@dataclass(frozen=True) -class ClassInfo: - name: str - line: int - has_bases: bool - members: tuple - internal: bool = False # nested in a class, or in a detail or anonymous namespace - - -def tokens(source): - result, line = [], 1 - for text in CLikeReader.generate_tokens(source): - if not (text.isspace() or text.startswith(('//', '/*', '#'))): - result.append((text, line)) - line += text.count('\n') - return result - - -def brace_pairs(toks): - pairs, stack = {}, [] - for index, (text, _) in enumerate(toks): - if text == '{': - stack.append(index) - elif text == '}' and stack: - pairs[stack.pop()] = index - return pairs - - -def skip_group(toks, index, open_text, close_text): - """Index just past the group opened at index; len(toks) if unbalanced.""" - depth = 0 - for k in range(index, len(toks)): - if toks[k][0] == open_text: - depth += 1 - elif toks[k][0] == close_text: - depth -= 1 - if depth == 0: - return k + 1 - return len(toks) - - -def class_head(toks, index): - """For a class/struct/union keyword, return (name, has_bases, '{' index) or None.""" - if index > 0 and toks[index - 1][0] in ('enum', '<', ',', 'friend', '(', 'typename'): - return None - angle, bases, names, k = 0, False, [], index + 1 - while k < len(toks): - text = toks[k][0] - if text == '{' and angle == 0: - break - if text == '(' and k > 0 and (toks[k - 1][0] in NOT_NAMES or MACRO.match(toks[k - 1][0])): - k = skip_group(toks, k, '(', ')') - continue - if text == '[': - k = skip_group(toks, k, '[', ']') - continue - if text in (';', '(', ')', '=', '}', '{') and angle == 0: - return None - if text == '<': - angle += 1 - elif text == '>': - angle -= 1 - if angle < 0: - return None - elif text == ',' and angle == 0 and not bases: - return None - elif text == ':' and angle == 0: - bases = True - elif not bases and angle == 0 and IDENTIFIER.match(text) and text != 'final': - names.append(text) - k += 1 - else: - return None - if len(names) > 1: - names = [n for n in names if not MACRO.match(n)] or names - return (names[-1] if names else None), bases, k - - -def strip_template(decl): - if decl and decl[0] == 'template' and len(decl) > 1 and decl[1] == '<': - depth = 0 - for k in range(1, len(decl)): - depth += {'<': 1, '>': -1}.get(decl[k], 0) - if depth == 0: - return decl[k + 1:] - return decl - - -def call_paren(decl): - """Index of the '(' that opens a function's parameters, or None for data.""" - angle = 0 - for k, text in enumerate(decl): - if text == 'operator': - return decl.index('(', k + 2) if '(' in decl[k + 2:] else None - if text == '=' and angle == 0: - return None - if text == '<' and k > 0 and IDENTIFIER.match(decl[k - 1]): - angle += 1 - elif text == '>' and angle > 0: - angle -= 1 - elif text == '(' and angle == 0 and k > 0: - if decl[k - 1] in NOT_NAMES or not IDENTIFIER.match(decl[k - 1]): - return None - if k + 1 < len(decl) and decl[k + 1] in ('*', '&', '^'): - return None - return k - return None - - -def is_option(decl): - """Option and cell fields are the framework's reflected settings, public by convention.""" - angle, words = 0, [] - for text in decl: - if text in ('=', '{}', ':', '['): - break - angle += {'<': 1, '>': -1}.get(text, 0) - if angle == 0 and IDENTIFIER.match(text): - words.append(text) - return any(w.endswith(('Option', 'Options', 'Cell')) for w in words[:-1]) - - -def _close(tail): - depth = 0 - for k, text in enumerate(tail): - depth += {'(': 1, ')': -1}.get(text, 0) - if depth == 0: - return k + 1 - return len(tail) - - -def classify(decl, access, class_name): - decl = strip_template(decl) - if not decl or decl[0] in SKIP_DECLARATIONS: - return None - if decl[0] in TYPE_KEYWORDS: - declarator = '{}' in decl and IDENTIFIER.match(decl[-1] if decl[-1] != '{}' else '') - return Member(access, 'data') if declarator else Member(access, 'type') - paren = call_paren(decl) - if paren is None: - # A const value (not a pointer to const) cannot be modified through the public field. - read_only = 'const' in decl and '*' not in decl and '&' not in decl - return Member(access, 'data', static='static' in decl, option=is_option(decl), - const=read_only) - name = decl[paren - 1] - if 'operator' in decl[:paren]: - kind = 'operator' - elif paren >= 2 and decl[paren - 2] == '~': - kind = 'dtor' - elif name == class_name: - kind = 'ctor' - else: - kind = 'method' - tail = decl[paren:] - return Member(access, kind, - pure=len(tail) >= 2 and tail[-2:] == ['=', '0'], - deleted='delete' in tail and '=' in tail, - virtual='virtual' in decl or 'override' in tail, - static='static' in decl[:paren], - const='static' in decl[:paren] or 'const' in tail[_close(tail):]) - - -def members(toks, start, end, pairs, keyword, class_name): - access = 'private' if keyword == 'class' else 'public' - found, decl, paren_depth, k = [], [], 0, start - while k < end: - text = toks[k][0] - following = toks[k + 1][0] if k + 1 < end else '' - if not decl: - if text in ACCESS and following == ':': - access, k = text, k + 2 - continue - if text in ACCESS and following in SLOTS: - access, k = text, k + 3 - continue - if text in SIGNALS and following == ':': - access, k = 'signals', k + 2 - continue - if text == ';' or (MACRO.match(text) and following != '::' and text != class_name): - k = skip_group(toks, k + 1, '(', ')') if following == '(' else k + 1 - continue - if text == '(': - paren_depth += 1 - elif text == ')': - paren_depth -= 1 - if text == '{': - close = pairs.get(k) - if close is None or close > end: - return None - is_function = paren_depth == 0 and call_paren(strip_template(decl)) is not None - in_initializers = is_function and ':' in decl and decl[-1] not in (')', '{}') - if is_function and not in_initializers and decl[-1] != '=': - member = classify(decl, access, class_name) - if member: - found.append(member) - decl, k = [], close + 1 - if k < end and toks[k][0] == ';': - k += 1 - continue - decl.append('{}') - k = close + 1 - continue - if text == ';' and paren_depth == 0: - member = classify(decl, access, class_name) - if member: - found.append(member) - decl, k = [], k + 1 - continue - decl.append(text) - k += 1 - return found - - -def scope_label(toks, index, heads): - """Name a '{' scope for qualified class names: classes and namespaces only.""" - if index in heads: - return heads[index][0] - names = [] - for k in range(index - 1, -1, -1): - text = toks[k][0] - if text == 'namespace': - return '::'.join(reversed(names)) or None - if not (IDENTIFIER.match(text) or text == '::'): - return None - if text != '::': - names.append(text) - return None - - -def is_internal_scope(toks, index, heads): - """A '{' that opens a class, an anonymous namespace or a detail namespace.""" - if index in heads: - return True - if index > 0 and toks[index - 1][0] == 'namespace': - return True - label = scope_label(toks, index, heads) or '' - return label == 'detail' or label.endswith('::detail') - - -def find_classes(source): - toks = tokens(source) - pairs = brace_pairs(toks) - heads = {} - for index, (text, line) in enumerate(toks): - if text in TYPE_KEYWORDS: - head = class_head(toks, index) - if head and head[0] and head[2] in pairs: - heads[head[2]] = (head[0], head[1], line, text) - result, stack, internal = [], [], [] - for index, (text, _) in enumerate(toks): - if text == '{': - label = scope_label(toks, index, heads) - if index in heads: - name, bases, line, keyword = heads[index] - body = members(toks, index + 1, pairs[index], pairs, keyword, name) - if body is not None: - qualified = '::'.join([s for s in stack if s] + [name]) - result.append(ClassInfo(qualified, line, bases, tuple(body), any(internal))) - stack.append(label) - internal.append(is_internal_scope(toks, index, heads)) - elif text == '}' and stack: - stack.pop() - internal.pop() - return result - - -def is_single_method_interface(info): - """Nested and detail-namespace classes are implementation details (e.g. type erasure).""" - if info.internal: - return False - methods = [m for m in info.members if m.kind == 'method'] - others = [m for m in info.members - if m.kind != 'method' and not (m.kind == 'dtor' and m.virtual)] - return not info.has_bases and len(methods) == 1 and methods[0].pure and not others - - -def is_test_file(path): - return '/Tests/' in path or path.endswith(('_Tests.cpp', '_Test.cpp', 'Tests.cpp')) - - -def measure_classes(path, source): - """Public data is allowed in value types (only const methods) and in test fakes.""" - result = [] - for info in find_classes(source.replace('\r\n', '\n').replace('\r', '\n')): - methods = [m for m in info.members if m.kind == 'method' and not m.deleted] - public_methods = sum(m.access == 'public' for m in methods) - mutators = [m for m in methods if not m.const] - public_data = sum(m.access == 'public' and m.kind == 'data' and not (m.static or m.option or m.const) - for m in info.members) if mutators and not is_test_file(path) else 0 - values = ( - ('public_methods', public_methods), - ('public_data', public_data), - ('single_method_interface', int(is_single_method_interface(info))), - ) - for check, value in values: - result.append(Metric(path, info.name, check, value, CLASS_LIMITS[check], info.line)) - return result diff --git a/devtools/code_health/config.py b/devtools/code_health/config.py deleted file mode 100644 index e0989c452e..0000000000 --- a/devtools/code_health/config.py +++ /dev/null @@ -1,30 +0,0 @@ -"""Limits for class shapes and the repository layout used by the layering check.""" - -CLASS_LIMITS = { - 'public_methods': 7, - 'public_data': 0, - 'single_method_interface': 0, -} - -# Advisory floors: new or changed values above these (and within the limit) are reported. -# Changed files above their floor are always reported. -ADVISORY = {'file_lines': 500, 'function_lines': 25, 'public_methods': 5} - -HINTS = { - 'public_methods': 'split the class or move helpers into free functions', - 'public_data': 'make data private, or split it into a plain struct without methods', - 'single_method_interface': 'prefer a callable (MoveOnlyFunction / std::function) ' - 'over a single-method interface', -} - -# Layering: shared code may not include game modules. Directory names are relative to -# SOURCE_ROOT, which is also the include root; Common/ lives at the repository root. -SOURCE_ROOT = 'SerialPrograms/Source/' -SHARED_DIRS = ('Common/', SOURCE_ROOT + 'CommonFramework/', SOURCE_ROOT + 'CommonTools/') -# Game-specific programs, inference and data. -GAME_MODULES = ('Pokemon', 'PokemonBDSP', 'VideoGames') -# Frameworks, consoles, controllers and tools that games build on. NintendoSwitch is -# the console layer; shared code including it is not a game dependency. -PLATFORM_MODULES = ('CommandLine', 'CommonFramework', 'CommonTools', 'ComputerPrograms', - 'ControllerInput', 'Controllers', 'GameConsole', 'Integrations', - 'Kernels', 'ML', 'NintendoSwitch', 'Tests') diff --git a/devtools/code_health/findings.py b/devtools/code_health/findings.py deleted file mode 100644 index 39ee3f9cdb..0000000000 --- a/devtools/code_health/findings.py +++ /dev/null @@ -1,39 +0,0 @@ -"""Located findings and their GitHub Actions workflow-command form.""" -from dataclasses import dataclass - -# GitHub shows at most 10 of each level per step in the diff; more is noise in the log. -ANNOTATION_CAP = 50 -TITLES = {'error': 'Code health', 'warning': 'Code health (advisory)'} - - -@dataclass(frozen=True) -class Finding: - path: str - line: int - message: str - - def __str__(self): - return f'{self.path}:{self.line}: {self.message}' - - -def escape_data(text): - return text.replace('%', '%25').replace('\r', '%0D').replace('\n', '%0A') - - -def escape_property(text): - return escape_data(str(text)).replace(':', '%3A').replace(',', '%2C') - - -def annotation(level, finding): - properties = ','.join(f'{key}={escape_property(value)}' for key, value in - (('file', finding.path), ('line', finding.line), ('title', TITLES[level]))) - return f'::{level} {properties}::{escape_data(finding.message)}' - - -def annotations(level, findings, cap=ANNOTATION_CAP): - """Workflow commands for at most `cap` findings, then a plain note about the rest.""" - lines = [annotation(level, finding) for finding in findings[:cap]] - if len(findings) > cap: - lines.append(f'Annotated {cap} of {len(findings)} {TITLES[level].lower()} findings; ' - 'see the log above for the rest.') - return lines diff --git a/devtools/code_health/git_files.py b/devtools/code_health/git_files.py deleted file mode 100644 index 89615e908e..0000000000 --- a/devtools/code_health/git_files.py +++ /dev/null @@ -1,49 +0,0 @@ -"""Git adapter: changed first-party C++ and corresponding base-revision content.""" -import subprocess -from pathlib import PurePosixPath - - -EXTENSIONS = {'.h', '.hpp', '.c', '.cpp', '.tpp'} - - -def git(root, *args): - return subprocess.check_output(['git', '-C', str(root), *args]) - - -def included(path): - parts = PurePosixPath(path).parts - return (len(parts) > 1 and parts[0] in ('Common', 'SerialPrograms') - and PurePosixPath(path).suffix in EXTENSIONS - and not any(part.startswith('3rdParty') for part in parts)) - - -def changed_files(root, base_ref): - fields = iter(git(root, 'diff', '--name-status', '-z', '--find-renames', base_ref, '--').decode().split('\0')) - changes = [] - for status in fields: - if not status: - continue - original = next(fields) - current = next(fields) if status[0] in 'RC' else original - if status[0] != 'D' and included(current): - previous = original if status[0] not in 'AC' and included(original) else None - changes.append((current, previous)) - return changes - - -UPSTREAMS = ('upstream/main', 'origin/main') - - -def default_base(root): - """(merge-base of HEAD with upstream/main, else origin/main; description).""" - for ref in UPSTREAMS: - exists = subprocess.run(['git', '-C', str(root), 'rev-parse', '--verify', '-q', ref + '^{commit}'], - capture_output=True) - if exists.returncode == 0: - base = git(root, 'merge-base', 'HEAD', ref).decode().strip() - return base, f'merge-base of HEAD and {ref}' - raise ValueError('No --base-ref given and neither upstream/main nor origin/main exists') - - -def read_revision(root, revision, path): - return git(root, 'show', revision + ':' + path).decode('utf-8-sig') diff --git a/devtools/code_health/layering.py b/devtools/code_health/layering.py deleted file mode 100644 index c96f0afe36..0000000000 --- a/devtools/code_health/layering.py +++ /dev/null @@ -1,43 +0,0 @@ -"""Shared code (Common, CommonFramework, CommonTools) may not include game modules.""" -import posixpath -import re - -from .config import GAME_MODULES, SHARED_DIRS, SOURCE_ROOT -from .findings import Finding - - -INCLUDE = re.compile(r'^[ \t]*#[ \t]*include[ \t]*[<"]([^>"]+)[>"]', re.M) - - -def game_module(path, include): - """The game module an include resolves to, trying the include root then the file's dir.""" - candidates = (include, posixpath.normpath(posixpath.join(posixpath.dirname(path), include))) - for candidate in candidates: - candidate = candidate.lstrip('./') if candidate.startswith('../') else candidate - if candidate.startswith(SOURCE_ROOT): - candidate = candidate[len(SOURCE_ROOT):] - top = candidate.split('/', 1) - if len(top) == 2 and top[0] in GAME_MODULES: - return top[0] - return None - - -def game_includes(path, source): - if not path.startswith(SHARED_DIRS) or source is None: - return {} - source = source.replace('\r\n', '\n') - found = {} - for match in INCLUDE.finditer(source): - module = game_module(path, match.group(1)) - if module: - found.setdefault(match.group(1), (source.count('\n', 0, match.start()) + 1, module)) - return found - - -def check(path, source, previous_source): - """Fail includes of game modules that this change adds to shared code.""" - before = game_includes(path, previous_source) - return [Finding(path, line, f'layering: shared code includes game module {module} ("{include}"); ' - 'move the dependency into the game or invert it through an interface/callback') - for include, (line, module) in sorted(game_includes(path, source).items()) - if include not in before] diff --git a/devtools/code_health/metrics.py b/devtools/code_health/metrics.py deleted file mode 100644 index 81bed74623..0000000000 --- a/devtools/code_health/metrics.py +++ /dev/null @@ -1,62 +0,0 @@ -"""C++ measurements, independent of repository layout and baseline policy.""" -from contextlib import redirect_stderr -from dataclasses import dataclass -from io import StringIO - -import lizard - - -@dataclass(frozen=True) -class Metric: - path: str - symbol: str - check: str - value: int - limit: int - line: int = 1 - # What the base revision is matched on, if not `symbol`. Functions match on - # their qualified name without parameters: changing a parameter's type - # (std::function to MoveOnlyFunction, #90) must not turn an old long - # function into a new one held to the limit. - name: str = None - - @property - def key(self): - return (self.path, self.name if self.name is not None else self.symbol, self.check) - - -class BraceDepth: - """Observe parsed function boundaries after each token; ignore enclosing scopes.""" - def __call__(self, tokens, reader): - for token in tokens: - before = reader.context.current_function - yield token - function = reader.context.current_function - if token == '{': - function.brace_depth = getattr(function, 'brace_depth', 0) + 1 - function.brace_peak = max(getattr(function, 'brace_peak', 0), - function.brace_depth) - elif token == '}': - before.brace_depth = getattr(before, 'brace_depth', 0) - 1 - - -def measure(path, source): - source = source.replace('\r\n', '\n').replace('\r', '\n') - lines = source.splitlines() - result = [Metric(path, '', 'file_lines', len(lines), 1000)] - analyzer = lizard.FileAnalyzer(lizard.get_extensions([BraceDepth()])) - errors = StringIO() - with redirect_stderr(errors): - analysis = analyzer.analyze_source_code(path, source) - if errors.getvalue(): - raise ValueError(f'Parser could not analyze {path}: {errors.getvalue().strip()}') - for function in analysis.function_list: - values = ( - ('function_lines', function.end_line - function.start_line + 1, 50), - ('nesting', max(0, getattr(function, 'brace_peak', 1) - 1), 4), - ('parameters', function.parameter_count, 7), - ) - for check, value, limit in values: - result.append(Metric(path, function.long_name, check, value, limit, - function.start_line, function.name)) - return result diff --git a/devtools/code_health/policy.py b/devtools/code_health/policy.py deleted file mode 100644 index 05eb4df3dd..0000000000 --- a/devtools/code_health/policy.py +++ /dev/null @@ -1,38 +0,0 @@ -"""Compare changed C++ with its base revision; no stored debt registry.""" -from .config import ADVISORY, HINTS -from .findings import Finding - - -def base_values(previous): - old = {} - for metric in previous: - old[metric.key] = max(old.get(metric.key, 0), metric.value) - return old - - -def evaluate(metrics, previous): - old = base_values(previous) - failures = [] - for metric in metrics: - ceiling = max(metric.limit, old.get(metric.key, metric.limit)) - if metric.value > ceiling: - failures.append(Finding(metric.path, metric.line, f'{metric.symbol or ""}: ' - f'{metric.check}={metric.value}, limit={metric.limit}, allowed {ceiling}' - + (f'; {HINTS[metric.check]}' if metric.check in HINTS else ''))) - return failures - - -def advisories(metrics, previous): - """New or changed values between an advisory floor and the limit; legacy excess is not repeated. - - Every changed file above its floor is reported: the file changed even if its length did not.""" - old = base_values(previous) - notes = [] - for metric in metrics: - floor = ADVISORY.get(metric.check) - if floor is None or (metric.check != 'file_lines' and old.get(metric.key) == metric.value): - continue - if floor < metric.value <= metric.limit: - notes.append(Finding(metric.path, metric.line, f'{metric.symbol or ""}: ' - f'{metric.check}={metric.value} (advisory above {floor}, limit {metric.limit})')) - return notes diff --git a/devtools/code_health/requirements.txt b/devtools/code_health/requirements.txt deleted file mode 100644 index 2b8cebbabd..0000000000 --- a/devtools/code_health/requirements.txt +++ /dev/null @@ -1,3 +0,0 @@ -lizard==1.17.31 -pathspec==0.12.1 -Pygments==2.19.2 diff --git a/devtools/code_health/style.py b/devtools/code_health/style.py deleted file mode 100644 index c403503b7a..0000000000 --- a/devtools/code_health/style.py +++ /dev/null @@ -1,511 +0,0 @@ -"""Lexical C++ style checks on the lines a change adds. - -Upstream bans and formatting rules (AGENTS.md / CONTRIBUTING.md) and the Abseil Tips of -the Week adopted in the fork's C++ style notes. Only added or changed lines are checked, -so legacy code in a touched file is not reported. Comments, string and character -literals and preprocessor lines are blanked before matching. This is a lexer with a -brace-scope stack, not a parser: rules that need types stay in review. -""" -import difflib -import re -from dataclasses import dataclass - -from .findings import Finding - - -HEADERS = ('.h', '.hpp', '.tpp') - - -@dataclass(frozen=True) -class Statement: - text: str # code with whitespace collapsed, attributes removed - line: int # line of its first character - end: str # ';', '{' or '}' - scope: tuple # enclosing brace kinds: 'namespace', 'class', 'enum', 'other' - follows: str = '' # kind of the brace closed just before it: `struct{...} name;` - - @property - def namespace_scope(self): - return all(kind == 'namespace' for kind in self.scope) - - @property - def declaration_scope(self): - """Namespace or class scope: declarations, not statements in a body.""" - return all(kind in ('namespace', 'class') for kind in self.scope) - - -# ---- Lexing ---------------------------------------------------------------------- - -RAW_STRING = re.compile(r'(?:u8|u|U|L)?R"([^()\\\s]{0,16})\(') - - -DIGIT_SEPARATOR = re.compile(r"(? 0 and (source[i - 1].isalnum() or source[i - 1] == '_') - - -def blank(text): - return re.sub(r'[^\n]', ' ', text) - - -def strip_code(source): - """Lines of `source` with comments, literal contents and preprocessor lines blanked. - - Columns and line numbers are kept; quotes stay so `extern "C"` remains recognizable.""" - out, i, n = [], 0, len(source) - while i < n: - c = source[i] - if source.startswith('//', i): - end = source.find('\n', i) - end = n if end < 0 else end - out.append(blank(source[i:end])) - i = end - elif source.startswith('/*', i): - end = source.find('*/', i + 2) - end = n if end < 0 else end + 2 - out.append(blank(source[i:end])) - i = end - elif c in 'uULR' and RAW_STRING.match(source, i) and not identifier_before(source, i): - raw = RAW_STRING.match(source, i) - close = ')' + raw.group(1) + '"' - end = source.find(close, raw.end()) - end = n if end < 0 else end + len(close) - out.append('"' + blank(source[i + 1:end - 1]) + '"') - i = end - elif c == '\'' and DIGIT_SEPARATOR.search(source, max(0, i - 40), i): - out.append(c) # 1'000 - i += 1 - elif c in '"\'': - j = i + 1 - while j < n and source[j] != c and source[j] != '\n': - j += 2 if source[j] == '\\' else 1 - j = min(j + 1, n) - out.append(c + blank(source[i + 1:j - 1]) + source[j - 1:j] if j - 1 > i else c) - i = j - else: - out.append(c) - i += 1 - lines = ''.join(out).split('\n') - continued = False - for index, line in enumerate(lines): - if continued or line.lstrip().startswith('#'): - continued = line.rstrip().endswith('\\') - lines[index] = blank(line) - return lines - - -def brace_kind(text): - if re.search(r'\bnamespace\b[\w:\s]*$', text) or re.fullmatch(r'extern\s*"\s*"', text): - return 'namespace' - if re.match(r'(template\s*<.*>\s*)?(class|struct|union)\b', text): - return 'class' - if re.match(r'enum\b', text): - return 'enum' - return 'other' - - -def statements(code_lines): - """Split blanked code at ';' (outside parentheses), '{' and '}' with their scopes.""" - result, stack = [], [] - text, start, parens, closed = [], None, 0, '' - - def emit(end): - joined = re.sub(r'\[\[.*?\]\]', ' ', ' '.join(''.join(text).split())).strip() - if joined or end != ';': - result.append(Statement(joined, start or line_number, end, - tuple(k for k, _ in stack), closed)) - return joined - - for line_number, line in enumerate(code_lines, 1): - for c in line: - if c == '{': - kind = brace_kind(emit('{')) - stack.append((kind, parens)) - text, start, parens, closed = [], None, 0, '' - elif c == '}': - emit('}') - closed, parens = stack.pop() if stack else ('', 0) - text, start = [], None - elif c == ';' and parens <= 0: - emit(';') - text, start, closed = [], None, '' - else: - parens += (c == '(') - (c == ')') - if start is None and not c.isspace(): - start = line_number - text.append(c) - text.append(' ') - return result - - -def added_lines(source_lines, previous_lines): - """1-based numbers of lines in `source_lines` that are new relative to `previous_lines`.""" - if previous_lines is None: - return set(range(1, len(source_lines) + 1)) - added = set() - matcher = difflib.SequenceMatcher(None, previous_lines, source_lines, autojunk=False) - for tag, _, _, j1, j2 in matcher.get_opcodes(): - if tag in ('replace', 'insert'): - added.update(range(j1 + 1, j2 + 1)) - return added - - -# ---- Signatures -------------------------------------------------------------------- - -NOT_FUNCTIONS = {'if', 'for', 'while', 'switch', 'catch', 'return', 'sizeof', 'alignof', - 'decltype', 'static_assert', 'noexcept', 'alignas', 'requires', 'throw', - 'operator', 'new', 'delete', 'typeid', 'co_return', 'co_await'} - - -def matching(text, open_index, opening='(', closing=')'): - depth = 0 - for k in range(open_index, len(text)): - depth += (text[k] == opening) - (text[k] == closing) - if depth == 0: - return k - return -1 - - -def split_top_level(text): - parts, depth, current = [], 0, [] - for c in text: - if c in '(<[{': - depth += 1 - elif c in ')>]}': - depth -= 1 - if c == ',' and depth == 0: - parts.append(''.join(current).strip()) - current = [] - else: - current.append(c) - tail = ''.join(current).strip() - return parts + [tail] if tail or parts else parts - - -@dataclass(frozen=True) -class Signature: - name: str - params: tuple - tail: str - - -def signature(statement): - """The function a declaration or definition header names, or None.""" - text = statement.text - if statement.end == '}' or not statement.declaration_scope: - return None - if re.match(r'(using|typedef|friend|return|namespace|class|struct|enum|union)\b', text): - return None - for match in re.finditer(r'(~?[A-Za-z_]\w*(?:\s*::\s*~?[A-Za-z_]\w*)*)\s*\(', text): - name = match.group(1).replace(' ', '') - if name.split('::')[-1] in NOT_FUNCTIONS or '=' in text[:match.start()]: - continue - close = matching(text, match.end() - 1) - if close < 0: - return None - tail = text[close + 1:].strip() - if re.search(r'[(=](?!\s*(0|default|delete)\s*$)', tail) and not tail.startswith('->'): - return None - return Signature(name, tuple(split_top_level(text[match.end():close])), tail) - return None - - -def without_templates(text): - while True: - reduced = re.sub(r'<[^<>]*>', '', text) - if reduced == text: - return text - text = reduced - - -def default_split(param): - depth = 0 - for k, c in enumerate(param): - depth += (c in '(<[{') - (c in ')>]}') - if c == '=' and depth == 0 and param[k - 1:k] not in ('=', '!', '<', '>') \ - and param[k + 1:k + 2] != '=': - return param[:k].strip(), param[k + 1:].strip() - return param.strip(), None - - -BOOL_PARAM = re.compile(r'(const\s+)?bool(\s+[A-Za-z_]\w*)?') - - -def bool_params(sig): - return [(p, default) for p, default in map(default_split, sig.params) - if BOOL_PARAM.fullmatch(p)] - - -def top_level_const(param): - declared, _ = default_split(param) - plain = without_templates(declared) - if '&' in plain or '*' in plain or '(' in plain or '...' in plain: - return False - return bool(re.fullmatch(r'const\s+[\w:]+(\s+[\w:]+)*', plain) - or re.fullmatch(r'[\w:]+\s+const(\s+\w+)?', plain)) - - -# ---- Checks ------------------------------------------------------------------------ - -def rule(message, *cites): - return f'style: {message} ({", ".join(cites)})' if cites else f'style: {message}' - - -BANS = ( - (re.compile(r'\bstd::filesystem\b'), ('Common/Cpp/Filesystem/',), - 'std::filesystem is banned; use the Common/Cpp/Filesystem wrappers (Path, Filesystem::, FileIO)', - 'upstream AGENTS.md "Filesystem"'), - (re.compile(r'\bstd::j?thread\b(?!\s*::)'), ('Common/Cpp/Concurrency/',), - 'std::thread is banned; run background work on the pools in GlobalThreadPools.h', - 'upstream AGENTS.md "Concurrency"'), - (re.compile(r'\bstd::(?:(?:recursive_|timed_|recursive_timed_|shared_|shared_timed_)?mutex' - r'|condition_variable(?:_any)?)\b'), ('Common/Cpp/Concurrency/',), - 'std::mutex / std::condition_variable are banned; use PokemonAutomation::Mutex / ConditionVariable', - 'upstream AGENTS.md "Concurrency"', 'TotW #197'), - (re.compile(r'\bstd::async\b'), (), - 'std::async is banned (it may spawn a thread); use the thread pools', - 'upstream AGENTS.md "Concurrency"'), -) -BIND = re.compile(r'\bstd::bind\s*\(') -NEW = re.compile(r'(?]\s*&\s*' + re.escape(name) + r'\b(?!\s*[(.\[])', text)) - - -def line_checks(path, raw, code, added, statements_): - """Bans, expressions and formatting that one line shows.""" - errors = [] - allocator = 'alloc' in path.lower() or bool(QT_INCLUDE.search(raw)) - bodies = [s.line for s in statements_ if s.end == '{' and signature(s)] - for number in sorted(added): - line = code[number - 1] - for pattern, exempt, message, *cites in BANS: - if pattern.search(line) and not path.startswith(exempt or ('\0',)): - errors.append(Finding(path, number, rule(message, *cites))) - if BIND.search(line): - errors.append(Finding(path, number, rule('use a lambda, not std::bind', 'TotW #108'))) - if not allocator and (NEW.search(line) or DELETE.search(line)): - errors.append(Finding(path, number, rule( - 'no new/delete outside allocators; use std::make_unique / std::make_shared', - 'TotW #126'))) - moved = RETURN_MOVE.search(line) - if moved and not moved.group(1).startswith('m_') and not lvalue_reference( - moved.group(1), '\n'.join(code[function_start(bodies, number) - 1:number])): - errors.append(Finding(path, number, rule( - f"don't std::move a returned local ('{moved.group(1)}'); return it by name", - 'TotW #120'))) - errors.extend(Finding(path, number, rule(message, 'upstream AGENTS.md "Control flow formatting"')) - for message in brace_problems(line, code[number] if number < len(code) else '')) - return errors - - -def brace_problems(line, next_line): - problems = [] - if re.search(r'\}[ \t]+else\b|\belse[ \t]+\{', line): - problems.append('write }else{ and }else if (...){ with no spaces around else') - if re.search(r'\bnamespace\b[\w:\s]*?[ \t]+\{', line): - problems.append('write namespace X{ with no space before {') - control = CONTROL.search(line) - if control: - close = matching(line, control.end() - 1) - rest = line[close + 1:].strip() if close >= 0 else None - if rest is None or (control.group(1) == 'while' and rest == ';'): - pass - elif re.match(r'\s+\{', line[close + 1:]): - problems.append(f'no space between ) and {{: {control.group(1)} (...){{') - elif rest == '' and next_line.strip().startswith('{'): - problems.append(f'put the opening brace on the {control.group(1)} line') - elif rest and (not rest.startswith('{') or rest.rstrip('{').strip() or rest.count('}')): - if not (rest.startswith('{') and rest[1:].strip() == ''): - problems.append(f'{control.group(1)} body goes on its own lines, braced') - other_else = re.search(r'\belse\b(?!\s*(if\b|\{|$))', line) - if other_else: - problems.append('else body goes on its own lines, braced') - return problems - - -def is_forward_declaration(statement): - return statement.end == ';' and re.fullmatch( - r'(template\s*<.*>\s*)?(class|struct|union)\s+[A-Za-z_][\w:]*', statement.text) - - -def namespace_variable(statement): - """The name of a non-const variable this namespace-scope statement declares, or None.""" - text = statement.text - if not statement.namespace_scope or statement.end == '}' or statement.follows: - return None - if statement.end == '{' and not text.endswith('='): - if '(' in text or not re.fullmatch(r'[\w:<>,\s\*&]+\s[\*&]*[A-Za-z_]\w*', text): - return None - head = default_split(text.rstrip('=').strip() if statement.end == '{' else text)[0] - if re.match(r'(using|typedef|namespace|template|class|struct|union|enum|friend|static_assert' - r'|return|extern\s*")\b', head) or '(' in without_templates(head): - return None - match = re.fullmatch(r'((?:[\w:]+(?:<.*>)?[\s\*&]+)+?)[\*&\s]*([A-Za-z_]\w*)\s*(\[[^\]]*\])?', head) - if not match: - return None - words = set(re.findall(r'\w+', without_templates(match.group(1)))) - if words & {'const', 'constexpr'} or match.group(2) in ('const', 'constexpr'): - return None - if words <= {'static', 'inline', 'extern', 'thread_local', 'constinit', 'volatile'}: - return None - return match.group(2) - - -def statement_checks(path, statements_, added, previous_signatures, new_file): - errors, warnings = [], [] - header = path.endswith(HEADERS) - # A primary template declared before its specializations is not a forward declaration. - defined = {m.group(1) for s in statements_ if s.end == '{' - for m in [re.search(r'\b(?:class|struct|union)\s+(\w+)', s.text)] if m} - for statement in statements_: - if statement.line not in added: - continue - text = statement.text - if header and statement.namespace_scope and re.match(r'using\s+namespace\b', text): - errors.append(Finding(path, statement.line, rule( - 'no using-directive at namespace scope in a header', 'TotW #153'))) - variable = namespace_variable(statement) - if variable: - what = 'non-const extern' if re.match(r'(static\s+|inline\s+)*extern\b', text) \ - else 'non-const namespace-scope variable' - errors.append(Finding(path, statement.line, rule( - f"{what} '{variable}'; make it const/constexpr or pass options down in a struct", - 'TotW #103'))) - if header and statement.namespace_scope and is_forward_declaration(statement) \ - and text.split()[-1] not in defined: - finding = Finding(path, statement.line, rule( - f"forward declaration '{text}'; include the header that defines it", - 'fork AGENTS.md "Forward declarations"')) - (errors if new_file else warnings).append(finding) - sig = signature(statement) - if sig is None: - continue - if header and statement.end == ';': - for param in sig.params: - if top_level_const(param): - errors.append(Finding(path, statement.line, rule( - f"{sig.name}: drop top-level const from by-value parameter '{param}' " - 'in the declaration', 'TotW #109'))) - bools = bool_params(sig) - defaulted = [p for p, default in bools if default is not None] - if (len(bools) >= 2 or defaulted) and not re.search(r'\b(override|final)\b', sig.tail) \ - and (sig.name, len(bools), len(defaulted)) not in previous_signatures: - problem = 'a defaulted bool parameter' if defaulted else f'{len(bools)} bool parameters' - errors.append(Finding(path, statement.line, rule( - f'{sig.name}: {problem}; use an enum class or an options struct', 'TotW #94'))) - return errors, warnings - - -def bool_signatures(statements_): - result = set() - for statement in statements_: - sig = signature(statement) - if sig: - bools = bool_params(sig) - result.add((sig.name, len(bools), sum(d is not None for _, d in bools))) - return result - - -# ---- Function comments (advisory) ---------------------------------------------------- - -STOP_WORDS = {'the', 'and', 'for', 'from', 'with', 'get', 'set', 'run', 'try', 'all'} - - -def name_tokens(name): - return [t for t in name.split('::')[-1].lower().split('_') if len(t) >= 3 and t not in STOP_WORDS] - - -def prose_words(text): - """(first sentence, rest) of a comment as sets of lowercase words, without code - references (`x`, a_b, f(), A::B).""" - text = re.sub(r'^\s*//+', ' ', text, flags=re.M) - text = re.sub(r'`[^`]*`|\b\w+\(\)|\b\w*_\w*\b|\b\w+::\w+', ' ', text) - parts = re.split(r'(?<=[a-z0-9)])\.(?:\s|$)', text.strip(), maxsplit=1) + [''] - return tuple(set(re.findall(r'[a-z0-9]+', part.lower())) for part in parts[:2]) - - -def mentioned(token, words): - """Whether a word shares the token's stem: berry/berries, add/adds, save/saving.""" - stem = token[:-1] if len(token) >= 4 else token - return any(w.startswith(stem) or (len(w) >= 4 and token.startswith(w)) for w in words) - - -def comment_above(raw_lines, line): - """(first line, text) of the // comment block directly above `line`, or None.""" - k = line - 1 - while k >= 1 and raw_lines[k - 1].strip().startswith('//'): - k -= 1 - if k == line - 1: - return None - return k + 1, '\n'.join(raw_lines[k:line - 1]) - - -def describes(name, words, share=1.0): - tokens = name_tokens(name) - return bool(tokens) and sum(mentioned(t, words) for t in tokens) >= share * len(tokens) - - -def comment_checks(path, raw_lines, statements_, added): - """A comment whose first sentence describes a neighbouring function and none of its own, - when its own function is described later in the block or the neighbour has no comment: - a stale comment left above another, or a comment that slid one function down.""" - functions = [(s.line, signature(s)) for s in statements_] - functions = [(line, sig.name) for line, sig in functions if sig] - warnings = [] - for index, (line, name) in enumerate(functions): - block = comment_above(raw_lines, line) - if block is None or not added & set(range(block[0], line + 1)): - continue - first, rest = prose_words(block[1]) - if any(mentioned(t, first) for t in name_tokens(name)): - continue - for other_line, other in functions[max(0, index - 1):index] + functions[index + 1:index + 2]: - if other == name or len(name_tokens(other)) < 2 or not describes(other, first): - continue - if describes(name, rest, 0.5) or comment_above(raw_lines, other_line) is None: - warnings.append(Finding(path, block[0], rule( - f'this comment sits above {name} but its first sentence describes {other}; ' - 'function comments go directly above their function', - 'CONTRIBUTING', 'Google "Function Comments"'))) - break - return warnings - - -# ---- Entry point ------------------------------------------------------------------- - -def check(path, source, previous_source): - """(errors, warnings) for the style rules on lines this change adds.""" - raw = source.replace('\r\n', '\n').replace('\r', '\n') - raw_lines = raw.split('\n') - previous_lines = None - previous_statements = [] - if previous_source is not None: - previous = previous_source.replace('\r\n', '\n').replace('\r', '\n') - previous_lines = previous.split('\n') - previous_statements = statements(strip_code(previous)) - added = added_lines(raw_lines, previous_lines) - if not added: - return [], [] - code = strip_code(raw) - statements_ = statements(code) - errors = line_checks(path, raw, code, added, statements_) - more_errors, warnings = statement_checks(path, statements_, added, - bool_signatures(previous_statements), - previous_source is None) - warnings += comment_checks(path, raw_lines, statements_, added) - return sorted(errors + more_errors, key=lambda f: f.line), sorted(warnings, key=lambda f: f.line) diff --git a/devtools/code_health/tests/test_classes.py b/devtools/code_health/tests/test_classes.py deleted file mode 100644 index 94402641a8..0000000000 --- a/devtools/code_health/tests/test_classes.py +++ /dev/null @@ -1,118 +0,0 @@ -import unittest - -from devtools.code_health.classes import find_classes, measure_classes -from devtools.code_health.metrics import Metric -from devtools.code_health.policy import advisories, evaluate - - -def values(source, symbol=None): - result = {} - for metric in measure_classes('Common/x.h', source): - if symbol is None or metric.symbol == symbol: - result[metric.check] = metric.value - return result - - -def methods(count, extra=''): - return 'class A{\npublic:\n' + extra + ''.join(f' void m{i}();\n' for i in range(count)) + '};\n' - - -class ClassShapeTests(unittest.TestCase): - def test_single_method_interface(self): - for source in ('struct L{ virtual void on(int x) = 0; };', - 'class L{ public: virtual ~L() = default; virtual void on() const = 0; };', - 'class L{\npublic:\n virtual ~L(){}\n virtual void on() = 0;\n};'): - with self.subTest(source=source): - self.assertEqual(1, values(source)['single_method_interface']) - for source in ('struct L{ virtual void on() = 0; virtual void off() = 0; };', - 'struct L : Base{ virtual void on() = 0; };', - 'struct L{ virtual void on(); };', - 'struct L{ virtual void on() = 0; int state; };'): - with self.subTest(source=source): - self.assertEqual(0, values(source)['single_method_interface']) - - def test_single_method_interface_skips_implementation_details(self): - nested = ('class F{\nprivate:\n class Callable{\n public:\n virtual ~Callable() = default;\n' - ' virtual void call() = 0;\n };\n Callable* m_impl;\n};') - self.assertEqual(0, values(nested, 'F::Callable')['single_method_interface']) - for source in ('namespace{ struct L{ virtual void on() = 0; }; }', - 'namespace a{ namespace detail{ struct L{ virtual void on() = 0; }; } }', - 'namespace a::detail{ struct L{ virtual void on() = 0; }; }'): - with self.subTest(source=source): - self.assertEqual(0, values(source)['single_method_interface']) - for source in ('namespace a{ struct L{ virtual void on() = 0; }; }', - 'namespace a::b{ struct L{ virtual void on() = 0; }; }'): - with self.subTest(source=source): - self.assertEqual(1, values(source)['single_method_interface']) - - def test_public_method_count_excludes_special_members(self): - extra = (' A();\n A(const A&) = delete;\n ~A();\n A& operator=(A&&);\n' - ' operator bool() const;\n void gone() = delete;\nprivate:\n void hidden();\npublic:\n') - self.assertEqual(7, values(methods(7, extra))['public_methods']) - qt = 'class W : public QWidget{\n Q_OBJECT\npublic:\n void a();\nsignals:\n void b();\npublic slots:\n void c();\n};' - self.assertEqual(2, values(qt)['public_methods']) - - def test_public_method_limit_and_advisory(self): - for count, failing, advisory in ((5, False, False), (6, False, True), (7, False, True), (8, True, False)): - with self.subTest(count=count): - metrics = measure_classes('Common/x.h', methods(count)) - self.assertEqual(failing, bool(evaluate(metrics, []))) - self.assertEqual(advisory, bool(advisories(metrics, []))) - - def test_existing_large_class_may_change_without_growing(self): - before = measure_classes('Common/x.h', methods(9)) - self.assertEqual([], evaluate(measure_classes('Common/x.h', methods(9, ' A();\n')), before)) - self.assertEqual([], advisories(measure_classes('Common/x.h', methods(9)), before)) - self.assertEqual([], advisories(measure_classes('Common/x.h', methods(8)), before)) - self.assertTrue(evaluate(measure_classes('Common/x.h', methods(10)), before)) - - def test_public_data_only_fails_with_methods(self): - self.assertEqual(0, values('struct P{ int a; std::string b = "{"; };')['public_data']) - self.assertEqual(1, values('struct P{ int a; void set(int x); };')['public_data']) - self.assertEqual(0, values('class P{ int a; public: void set(int x); };')['public_data']) - exempt = ('struct P{ void f(); static int count; static constexpr int K = 3;\n' - ' BooleanCheckBoxOption ENABLE; EnumDropdownCell mode; Q q; };') - self.assertEqual(1, values(exempt)['public_data']) - self.assertEqual(1, values('struct P{ void f(); std::function callback; };')['public_data']) - read_only = 'class D{ public: const Type type; const char* name; virtual void run(); };' - self.assertEqual(1, values(read_only)['public_data']) - - def test_value_types_and_test_fakes_may_expose_data(self): - value = ('struct TilePos{ int x = 0; int y = 0; TilePos next(Direction d) const;\n' - ' static TilePos origin(); bool operator==(const TilePos&) const = default; };') - self.assertEqual(0, values(value)['public_data']) - fake = 'struct Fake{ std::vector calls; void hold(int x){ calls.push_back(x); } };' - self.assertEqual(1, values(fake)['public_data']) - tests = measure_classes('SerialPrograms/Source/Pokemon/X_Tests.cpp', fake) - self.assertEqual([0], [m.value for m in tests if m.check == 'public_data']) - - def test_constructor_initializers_and_inline_bodies(self): - source = ('struct A{\n A(int a) : m_a(a), m_b{2} { if (a){ m_a = 1; } }\n' - ' void f(int x = {}) { for (;;){} }\n void (*fp)(int);\n' - 'private:\n int m_a; int m_b;\n};') - self.assertEqual({'public_methods': 1, 'public_data': 1, 'single_method_interface': 0}, - values(source)) - - def test_qualified_nested_names_and_skipped_declarations(self): - source = ('namespace PA{ namespace Game{\ntemplate void g();\n' - 'class X; enum class E{ A, B }; struct Y{ struct Z{ int a; }; friend class X; };\n}}') - self.assertEqual(['PA::Game::Y', 'PA::Game::Y::Z'], [c.name for c in find_classes(source)]) - - def test_crlf_and_unbalanced_preprocessor_branches_do_not_raise(self): - source = '#if X\nclass A : public B{\n#else\nclass A{\n#endif\npublic:\n void f();\n};\n' - measure_classes('Common/x.h', source) - lf = methods(8) - self.assertEqual(measure_classes('Common/x.h', lf), - measure_classes('Common/x.h', lf.replace('\n', '\r\n'))) - - def test_failure_suggests_a_callable(self): - failures = evaluate(measure_classes('Common/x.h', 'struct L{ virtual void on() = 0; };'), []) - self.assertEqual(1, len(failures)) - self.assertIn('MoveOnlyFunction', str(failures[0])) - - def test_metrics_are_plain_values(self): - self.assertIsInstance(measure_classes('Common/x.h', methods(1))[0], Metric) - - -if __name__ == '__main__': - unittest.main() diff --git a/devtools/code_health/tests/test_cli.py b/devtools/code_health/tests/test_cli.py deleted file mode 100644 index fffa637444..0000000000 --- a/devtools/code_health/tests/test_cli.py +++ /dev/null @@ -1,199 +0,0 @@ -"""Exercise changed-file discovery and real base comparisons in temporary Git repos.""" -import os -import subprocess -import sys -import tempfile -import unittest -from pathlib import Path - -from devtools.code_health.git_files import default_base, included - - -REPO = Path(__file__).resolve().parents[3] -# These fixtures' findings must not become annotations on the CI run's own PR. -ENV = {key: value for key, value in os.environ.items() if key != 'GITHUB_ACTIONS'} - - -class CliTests(unittest.TestCase): - def setUp(self): - self.temp = tempfile.TemporaryDirectory() - self.addCleanup(self.temp.cleanup) - self.root = Path(self.temp.name) - self.git('init', '-q') - self.git('config', 'user.email', 'test@example.invalid') - self.git('config', 'user.name', 'Test') - (self.root / 'Common').mkdir() - - def git(self, *args): - return subprocess.check_output(['git', '-C', str(self.root), *args]) - - def source(self, lines, path='Common/example.cpp'): - file = self.root / path - file.parent.mkdir(parents=True, exist_ok=True) - file.write_text('void example(){\n' + ' ;\n' * (lines - 2) + '}\n') - self.git('add', '.') - - def cli(self, *args): - return subprocess.run([sys.executable, '-m', 'devtools.code_health', - '--root', str(self.root), '--base-ref', 'HEAD', *args], - cwd=REPO, capture_output=True, text=True, env=ENV) - - def commit(self): - self.git('add', '.') - self.git('commit', '-qm', 'base revision') - - def test_untouched_debt_is_not_scanned(self): - self.source(60) - self.commit() - result = self.cli() - self.assertEqual(0, result.returncode, result.stderr) - self.assertIn('Scanned 0 changed C++ files', result.stdout) - - def test_existing_debt_can_improve_but_not_worsen(self): - self.source(60) - self.commit() - self.source(59) - self.assertEqual(0, self.cli().returncode) - self.source(61) - result = self.cli() - self.assertEqual(1, result.returncode, result.stderr) - self.assertIn('function_lines=61', result.stderr) - self.assertIn('allowed 60', result.stderr) - - def test_new_file_must_meet_limits(self): - self.source(3) - self.commit() - self.source(51, 'Common/new.hpp') - self.assertEqual(1, self.cli().returncode) - - def test_unchanged_bad_function_in_changed_file_passes(self): - self.source(60) - self.commit() - with (self.root / 'Common/example.cpp').open('a') as file: - file.write('\nint small(){ return 0; }\n') - self.assertEqual(0, self.cli().returncode) - - def test_rename_compares_with_original_path(self): - self.source(60) - self.commit() - self.git('mv', 'Common/example.cpp', 'Common/renamed.cpp') - result = self.cli() - self.assertEqual(0, result.returncode, result.stderr) - self.assertIn('Scanned 1 changed C++ files', result.stdout) - - def test_deletions_untracked_and_third_party_are_excluded(self): - self.source(60) - self.commit() - self.git('rm', 'Common/example.cpp') - (self.root / 'Common').mkdir(exist_ok=True) - (self.root / 'Common/untracked.cpp').write_text('// excess\n' * 1001) - self.source(60, 'SerialPrograms/3rdParty-vendor/bad.cpp') - # source() stages everything; deliberately return the extra file to untracked. - self.git('rm', '--cached', 'Common/untracked.cpp') - result = self.cli() - self.assertEqual(0, result.returncode, result.stderr) - self.assertIn('Scanned 0 changed C++ files', result.stdout) - - def test_new_file_with_spaces_is_checked(self): - self.source(3) - self.commit() - self.source(51, 'Common/new file.cpp') - self.assertEqual(1, self.cli().returncode) - - def test_invalid_base_fails_closed(self): - self.source(3) - self.commit() - self.assertEqual(2, self.cli('--base-ref', 'missing-ref').returncode) - - def test_cpp_extension_and_directory_scope(self): - for extension in ('.h', '.hpp', '.c', '.cpp', '.tpp'): - self.assertTrue(included('Common/sub/file' + extension)) - for path in ('3rdParty/a.cpp', 'Common/3rdParty2/a.cpp', 'other/a.cpp', - 'Common/a.py', 'SerialPrograms/3rdParty/a.h'): - self.assertFalse(included(path), path) - - - def test_default_base_is_merge_base_with_upstream_then_origin(self): - self.source(3) - self.commit() - fork_point = self.git('rev-parse', 'HEAD').decode().strip() - with self.assertRaises(ValueError): - default_base(self.root) - self.git('update-ref', 'refs/remotes/origin/main', fork_point) - self.source(60) - self.commit() - self.assertEqual((fork_point, 'merge-base of HEAD and origin/main'), default_base(self.root)) - # A stale fork main: upstream moved on from an older commit; prefer its merge-base. - self.git('update-ref', 'refs/remotes/origin/main', 'HEAD') - self.git('update-ref', 'refs/remotes/upstream/main', fork_point) - self.assertEqual((fork_point, 'merge-base of HEAD and upstream/main'), default_base(self.root)) - result = subprocess.run([sys.executable, '-m', 'devtools.code_health', '--root', str(self.root)], - cwd=REPO, capture_output=True, text=True, env=ENV) - self.assertEqual(1, result.returncode, result.stderr) - self.assertIn(f'Base: {fork_point} (merge-base of HEAD and upstream/main).', result.stdout) - - - def test_class_shape_applies_to_new_classes_only(self): - interface = 'struct Listener{ virtual void on_event() = 0; };\n' - (self.root / 'Common/old.h').write_text(interface) - self.commit() - with (self.root / 'Common/old.h').open('a') as file: - file.write('int helper();\n') - self.git('add', '.') - self.assertEqual(0, self.cli().returncode) - (self.root / 'Common/new.h').write_text(interface.replace('Listener', 'Fresh')) - self.git('add', '.') - result = self.cli() - self.assertEqual(1, result.returncode, result.stderr) - self.assertIn('Fresh: single_method_interface=1', result.stderr) - self.assertIn('std::function', result.stderr) - - - def test_shared_code_may_not_add_game_includes(self): - shared = self.root / 'SerialPrograms/Source/CommonTools/Tool.cpp' - shared.parent.mkdir(parents=True) - shared.write_text('#include "Pokemon/Pokemon_Strings.h"\n') - self.commit() - shared.write_text('#include "Pokemon/Pokemon_Strings.h"\n#include "NintendoSwitch/X.h"\n') - self.git('add', '.') - self.assertEqual(0, self.cli().returncode) - shared.write_text('#include "VideoGames/PokemonSV/PokemonSV_Settings.h"\n') - self.git('add', '.') - result = self.cli() - self.assertEqual(1, result.returncode, result.stderr) - self.assertIn('Tool.cpp:1: layering', result.stderr) - - def test_style_checks_report_only_added_lines(self): - file = self.root / 'Common/style.cpp' - file.write_text('void f(){\n if (x) return;\n}\n') - self.commit() - file.write_text('void f(){\n if (x) return;\n g();\n}\n') - self.git('add', '.') - self.assertEqual(0, self.cli().returncode) - file.write_text('void f(){\n if (x) return;\n std::mutex m;\n}\n') - self.git('add', '.') - result = self.cli() - self.assertEqual(1, result.returncode, result.stderr) - self.assertIn('style.cpp:3: style: std::mutex', result.stderr) - self.assertNotIn('style.cpp:2:', result.stderr) - - def test_github_annotates_failures_and_advisories(self): - self.source(3) - self.commit() - self.source(30) - self.assertNotIn('::warning', self.cli().stdout) - result = self.cli('--github') - self.assertEqual(0, result.returncode, result.stderr) - self.assertIn('::warning file=Common/example.cpp,line=1,title=Code health (advisory)::' - 'example(): function_lines=30', result.stdout) - self.assertNotIn('::error', result.stdout) - self.source(51) - result = self.cli('--github') - self.assertEqual(1, result.returncode, result.stderr) - self.assertIn('::error file=Common/example.cpp,line=1,title=Code health::' - 'example(): function_lines=51', result.stdout) - self.assertIn('Code health: 1 failures.', result.stdout) - - -if __name__ == '__main__': - unittest.main() diff --git a/devtools/code_health/tests/test_findings.py b/devtools/code_health/tests/test_findings.py deleted file mode 100644 index 31f188282c..0000000000 --- a/devtools/code_health/tests/test_findings.py +++ /dev/null @@ -1,61 +0,0 @@ -import unittest - -from devtools.code_health.findings import Finding, annotation, annotations -from devtools.code_health.metrics import Metric -from devtools.code_health.policy import advisories - - -class AnnotationTests(unittest.TestCase): - def test_error_and_warning_carry_file_and_line(self): - finding = Finding('Common/a.cpp', 12, 'f(): function_lines=60') - self.assertEqual('Common/a.cpp:12: f(): function_lines=60', str(finding)) - self.assertEqual('::error file=Common/a.cpp,line=12,title=Code health::f(): function_lines=60', - annotation('error', finding)) - self.assertEqual('::warning file=Common/a.cpp,line=12,title=Code health (advisory)::' - 'f(): function_lines=60', annotation('warning', finding)) - - def test_message_and_properties_are_escaped(self): - finding = Finding('Common/a,b:c%.cpp', 3, '100% done\r\nnext: a,b') - self.assertEqual('::error file=Common/a%2Cb%3Ac%25.cpp,line=3,title=Code health::' - '100%25 done%0D%0Anext: a,b', annotation('error', finding)) - - def test_cap_annotates_first_findings_and_counts_the_rest(self): - findings = [Finding('Common/a.cpp', line, 'x') for line in range(1, 8)] - lines = annotations('warning', findings, cap=5) - self.assertEqual(6, len(lines)) - self.assertTrue(lines[4].startswith('::warning file=Common/a.cpp,line=5,')) - self.assertEqual('Annotated 5 of 7 code health (advisory) findings; ' - 'see the log above for the rest.', lines[5]) - self.assertEqual(3, len(annotations('error', findings[:3], cap=5))) - - -class AdvisoryLocationTests(unittest.TestCase): - PATH = 'Common/a.cpp' - - def locations(self, metrics, previous=()): - return [(f.path, f.line, f.message.split(' (')[0]) for f in advisories(metrics, list(previous))] - - def test_long_changed_file_is_reported_at_line_one(self): - self.assertEqual([(self.PATH, 1, ': file_lines=600')], - self.locations([Metric(self.PATH, '', 'file_lines', 600, 1000)])) - same_length = Metric(self.PATH, '', 'file_lines', 600, 1000) - self.assertEqual(1, len(self.locations([same_length], [same_length]))) - self.assertEqual([], self.locations([Metric(self.PATH, '', 'file_lines', 500, 1000)])) - - def test_function_between_25_and_50_lines_is_reported_at_its_start(self): - metrics = [Metric(self.PATH, 'f()', 'function_lines', 26, 50, 40), - Metric(self.PATH, 'g()', 'function_lines', 25, 50, 90), - Metric(self.PATH, 'h()', 'function_lines', 51, 50, 120)] - self.assertEqual([(self.PATH, 40, 'f(): function_lines=26')], self.locations(metrics)) - self.assertEqual([], self.locations(metrics[:1], metrics[:1])) - - def test_class_with_six_or_seven_public_methods_is_reported_at_the_class(self): - metrics = [Metric(self.PATH, 'A', 'public_methods', 6, 7, 7), - Metric(self.PATH, 'B', 'public_methods', 7, 7, 30), - Metric(self.PATH, 'C', 'public_methods', 5, 7, 50)] - self.assertEqual([(self.PATH, 7, 'A: public_methods=6'), (self.PATH, 30, 'B: public_methods=7')], - self.locations(metrics)) - - -if __name__ == '__main__': - unittest.main() diff --git a/devtools/code_health/tests/test_layering.py b/devtools/code_health/tests/test_layering.py deleted file mode 100644 index fbc7708cee..0000000000 --- a/devtools/code_health/tests/test_layering.py +++ /dev/null @@ -1,48 +0,0 @@ -import unittest -from pathlib import Path - -from devtools.code_health.config import GAME_MODULES, PLATFORM_MODULES, SOURCE_ROOT -from devtools.code_health.layering import check - - -REPO = Path(__file__).resolve().parents[3] -SHARED = SOURCE_ROOT + 'CommonTools/Async/Example.cpp' - - -class LayeringTests(unittest.TestCase): - def test_added_game_includes_fail_in_shared_code(self): - for include in ('Pokemon/Inference/Pokemon_NameReader.h', - 'VideoGames/PokemonSV/PokemonSV_Settings.h', - 'PokemonBDSP/PokemonBDSP_Settings.h', - '../../VideoGames/PokemonFRLG/PokemonFRLG_Settings.h'): - with self.subTest(include=include): - failures = check(SHARED, f'#include "{include}"\n', '') - self.assertEqual(1, len(failures)) - self.assertIn(f'{SHARED}:1: layering', str(failures[0])) - - def test_platform_includes_and_game_code_pass(self): - self.assertEqual([], check(SHARED, '#include "NintendoSwitch/Controllers/X.h"\n' - '#include "Controllers/Controller.h"\n#include \n', '')) - game = SOURCE_ROOT + 'VideoGames/PokemonSV/Programs/X.cpp' - self.assertEqual([], check(game, '#include "Pokemon/Pokemon_Strings.h"\n', '')) - - def test_existing_and_commented_includes_pass(self): - existing = '#include "Pokemon/Inference/Pokemon_NameReader.h"\r\n' - self.assertEqual([], check(SHARED, existing + 'int x;\r\n', existing)) - self.assertEqual([], check(SHARED, '//#include "Pokemon/X.h"\n', '')) - - def test_root_common_and_relative_paths(self): - source = '#include "../SerialPrograms/Source/VideoGames/PokemonLA/X.h"\n' - self.assertEqual(1, len(check('Common/Cpp/X.cpp', source, None))) - - def test_every_top_level_source_directory_is_classified(self): - root = REPO / SOURCE_ROOT - if not root.is_dir(): - self.skipTest('SerialPrograms/Source is not checked out') - directories = {p.name for p in root.iterdir() if p.is_dir()} - self.assertEqual(set(), directories - set(GAME_MODULES) - set(PLATFORM_MODULES)) - self.assertEqual(set(), set(GAME_MODULES) & set(PLATFORM_MODULES)) - - -if __name__ == '__main__': - unittest.main() diff --git a/devtools/code_health/tests/test_metrics.py b/devtools/code_health/tests/test_metrics.py deleted file mode 100644 index bee89c5333..0000000000 --- a/devtools/code_health/tests/test_metrics.py +++ /dev/null @@ -1,52 +0,0 @@ -import unittest - -from devtools.code_health.metrics import measure - - -class MetricsTests(unittest.TestCase): - def test_small_function_passes(self): - metrics = measure('Common/small.cpp', 'int small(int x){ return x; }\n') - self.assertEqual([], [m for m in metrics if m.value > m.limit]) - - def test_generated_threshold_boundaries(self): - cases = ( - ('file_lines', 1000, lambda n: '// line\n' * n), - ('function_lines', 50, lambda n: 'void f(){\n' + ' ;\n' * (n - 2) + '}\n'), - ('nesting', 4, lambda n: 'void f(){' + '{' * n + ';' + '}' * n + '}\n'), - ('parameters', 7, lambda n: 'void f(' + ', '.join(f'int p{i}' for i in range(n)) + '){}\n'), - ) - for check, limit, generate in cases: - for size in (limit, limit + 1): - with self.subTest(check=check, size=size): - metrics = [m for m in measure('generated.cpp', generate(size)) if m.check == check] - self.assertEqual([size], [m.value for m in metrics]) - self.assertEqual(size > limit, any(m.value > m.limit for m in metrics)) - - def test_strings_comments_and_raw_literals_do_not_add_nesting(self): - source = r'''void strings(){ - // {{{{{{{{ - const char* a = "{{{{{{{{"; - const char* b = R"tag({{{{{{{{)tag"; - /* {{{{{{{{ */ -}''' - nesting = [m.value for m in measure('literals.cpp', source) - if m.check == 'nesting'] - self.assertEqual([0], nesting) - - def test_qualified_functions_and_lambdas_are_measured(self): - source = 'namespace n{ struct A{ void f(){ auto g = [](){ if (true){} }; } }; }' - metrics = measure('methods.hpp', source) - self.assertTrue(any(m.symbol.endswith('A::f()') for m in metrics), metrics) - self.assertEqual([2], [m.value for m in metrics if m.check == 'nesting']) - - def test_crlf_macros_have_identical_metrics_to_lf(self): - for count in (1, 4, 16, 64): - parameters = ', '.join(f'p{i}' for i in range(count)) - source = '#define VALUES(' + parameters + ') \\\n use(' + parameters + ')\n' - source += 'void f(int a,\n int b){ return; }\n' - self.assertEqual(measure('macro.h', source), - measure('macro.h', source.replace('\n', '\r\n'))) - - -if __name__ == '__main__': - unittest.main() diff --git a/devtools/code_health/tests/test_policy.py b/devtools/code_health/tests/test_policy.py deleted file mode 100644 index 147e8324bf..0000000000 --- a/devtools/code_health/tests/test_policy.py +++ /dev/null @@ -1,37 +0,0 @@ -import unittest - -from devtools.code_health.metrics import Metric, measure -from devtools.code_health.policy import evaluate - - -def function(value, symbol='test()'): - return Metric('Common/test.cpp', symbol, 'function_lines', value, 50) - - -class RatchetTests(unittest.TestCase): - def test_existing_debt_passes_but_growth_fails(self): - self.assertEqual([], evaluate([function(70)], [function(70)])) - self.assertTrue(evaluate([function(71)], [function(70)])) - - def test_improvement_is_allowed(self): - self.assertEqual([], evaluate([function(60)], [function(70)])) - - def test_new_function_must_meet_limit(self): - self.assertTrue(evaluate([function(51, 'new()')], [function(70)])) - - def test_unchanged_bad_function_does_not_block_other_changes(self): - self.assertEqual([], evaluate([function(70), function(25, 'new()')], [function(70)])) - - def test_changed_parameter_types_keep_the_base(self): - old = measure('Common/test.cpp', 'void A::f(std::function f){\n' + 'x();\n' * 60 + '}\n') - new = measure('Common/test.cpp', 'void A::f(MoveOnlyFunction f){\n' + 'x();\n' * 60 + '}\n') - self.assertEqual([], evaluate(new, old)) - grown = measure('Common/test.cpp', 'void A::f(MoveOnlyFunction f){\n' + 'x();\n' * 61 + '}\n') - self.assertTrue(evaluate(grown, old)) - - def test_growth_below_limit_is_allowed(self): - self.assertEqual([], evaluate([function(50)], [function(25)])) - - -if __name__ == '__main__': - unittest.main() diff --git a/devtools/code_health/tests/test_style.py b/devtools/code_health/tests/test_style.py deleted file mode 100644 index 810524b7f5..0000000000 --- a/devtools/code_health/tests/test_style.py +++ /dev/null @@ -1,226 +0,0 @@ -import unittest - -from devtools.code_health.style import added_lines, check, strip_code - - -CPP = 'SerialPrograms/Source/VideoGames/PokemonFRLG/Programs/Example.cpp' -HEADER = 'SerialPrograms/Source/VideoGames/PokemonFRLG/Programs/Example.h' - - -def wrap(body): - return 'namespace PokemonAutomation{\nnamespace NintendoSwitch{\n' + body + '\n}\n}\n' - - -def in_function(body): - return wrap('void example(){\n' + body + '\n}') - - -class StyleCase(unittest.TestCase): - def errors(self, source, path=CPP, previous=None): - return [f.message for f in check(path, source, previous)[0]] - - def warnings(self, source, path=CPP, previous=None): - return [f.message for f in check(path, source, previous)[1]] - - def assertFlags(self, fragment, source, path=CPP, count=1): - messages = [m for m in self.errors(source, path) if fragment in m] - self.assertEqual(count, len(messages), self.errors(source, path)) - - def assertClean(self, source, path=CPP): - self.assertEqual([], self.errors(source, path)) - self.assertEqual([], self.warnings(source, path)) - - -class LexingTests(unittest.TestCase): - def test_comments_strings_and_preprocessor_are_blanked(self): - code = strip_code('int a; // new\n/* std::bind(\n */ s = "delete";\n#define X new int\n' - 'r = R"x(std::mutex)x"; c = \'{\'; n = 1\'000;') - self.assertEqual(['int a;', '', ' s = " ";', '', - 'r = " "; c = \' \'; n = 1\'000;'], - [line.rstrip() for line in code]) - - def test_only_inserted_and_replaced_lines_are_added(self): - self.assertEqual({2, 4}, added_lines(['a', 'B', 'c', 'd'], ['a', 'b', 'c'])) - self.assertEqual({1, 2}, added_lines(['a', 'b'], None)) - - -class BanTests(StyleCase): - def test_banned_std_facilities(self): - for line, fragment in (('std::filesystem::path p;', 'std::filesystem is banned'), - ('std::thread worker(run);', 'std::thread is banned'), - ('std::jthread worker(run);', 'std::thread is banned'), - ('std::lock_guard lg(m_lock);', 'std::mutex'), - ('std::shared_mutex m;', 'std::mutex'), - ('std::condition_variable cv;', 'std::mutex'), - ('auto f = std::async(run);', 'std::async is banned')): - with self.subTest(line=line): - self.assertFlags(fragment, in_function(line)) - - def test_allowed_neighbours_and_wrapper_directories(self): - self.assertClean(in_function('std::this_thread::sleep_for(10ms);\n' - 'size_t n = std::thread::hardware_concurrency();\n' - 'Mutex m;\n// std::mutex in a comment\nlog("std::async");')) - self.assertEqual([], self.errors(in_function('std::mutex m;'), 'Common/Cpp/Concurrency/Mutex.h')) - self.assertEqual([], self.errors(in_function('std::filesystem::path p;'), - 'Common/Cpp/Filesystem/Path.cpp')) - - -class ExpressionTests(StyleCase): - def test_std_bind(self): - self.assertFlags('TotW #108', in_function('auto f = std::bind(&run, 1);')) - self.assertClean(in_function('auto f = std::bind_front(&run, 1);\nauto g = [&]{ run(1); };')) - - def test_new_and_delete(self): - self.assertFlags('TotW #126', in_function('Foo* p = new Foo();')) - self.assertFlags('TotW #126', in_function('delete[] p;')) - self.assertClean(wrap('struct A{\n A(const A&) = delete;\n void* operator new(size_t);\n' - ' void operator delete(void*);\n};\n' - 'void f(){\n auto p = std::make_unique();\n}')) - - def test_new_allowed_in_allocators_and_qt_files(self): - self.assertEqual([], self.errors(in_function('T* p = new T();'), 'Common/Cpp/AlignedAllocator.h')) - self.assertEqual([], self.errors('#include \n' + in_function('new QLabel(this);'))) - - def test_return_std_move_of_a_local(self): - self.assertFlags('TotW #120', wrap('Foo f(){\n Foo result;\n return std::move(result);\n}')) - self.assertFlags('TotW #120', wrap('Foo f(Foo&& value){\n return std::move(value);\n}')) - - def test_return_std_move_of_members_and_lvalue_references(self): - self.assertClean(wrap('Foo A::f(){\n return std::move(m_value);\n}\n' - 'Foo g(Foo& value){\n return std::move(value);\n}\n' - 'Foo h(Map& map){\n return std::move(map.begin()->second);\n}')) - - -class FormattingTests(StyleCase): - def test_brace_spacing(self): - for line, fragment in (('if (x) {', 'no space between ) and {'), - ('for (int i = 0; i < n; i++) {', 'no space between ) and {'), - ('}else {', 'no spaces around else'), - ('} else{', 'no spaces around else')): - with self.subTest(line=line): - self.assertFlags(fragment, in_function(line + '\n run();\n}')) - self.assertFlags('namespace X{', 'namespace PokemonAutomation {\n}\n') - self.assertFlags('namespace X{', 'namespace{\n}\nnamespace A::B {\n}\n') - - def test_single_line_bodies_and_next_line_braces(self): - self.assertFlags('if body goes on its own lines', in_function('if (x) return;')) - self.assertFlags('if body goes on its own lines', in_function('if (x){ return; }')) - self.assertFlags('else body goes on its own lines', in_function('if (x){\n a();\n}else b();')) - self.assertFlags('put the opening brace on the while line', - in_function('while (x)\n{\n run();\n}')) - - def test_repo_formatting_passes(self): - self.assertClean(in_function( - 'if (x){\n a();\n}else if (y){\n b();\n}else{\n c();\n}\n' - 'do{\n a();\n}while (x);\n' - 'if (std::any_of(v.begin(), v.end(), [](int i){ return i > 0; })){\n a();\n}\n' - 'if (x &&\n y){\n a();\n}\n' - 'switch (x){\ncase 1:\n break;\n}')) - - -class DeclarationTests(StyleCase): - def test_mutable_namespace_scope_variables(self): - self.assertFlags("non-const namespace-scope variable 'counter'", wrap('int counter = 0;')) - self.assertFlags("'TABLE'", wrap('static std::map TABLE = {\n {1, 2},\n};')) - self.assertFlags("'BOX'", wrap('ImageFloatBox BOX{0.1, 0.2, 0.3, 0.4};')) - self.assertFlags("non-const extern 'g_stop'", wrap('extern bool g_stop;'), HEADER) - - def test_constants_functions_and_locals_pass(self): - self.assertClean(wrap( - 'const int LIMIT = 3;\nconstexpr double SCALE = 1.5;\nextern const ImageFloatBox AREA;\n' - 'static const std::map TABLE = {\n {1, 2},\n};\n' - 'bool operator==(const A& lhs, const A& rhs);\nusing Alias = int;\n' - 'struct Point{\n int x = 0;\n};\n' - 'void f(){\n static int calls = 0;\n int local = 1;\n}')) - self.assertEqual([], self.errors('typedef struct{\n int x;\n} PABB_PACK pabb_Message;\n')) - - def test_using_directive_in_header(self): - self.assertFlags('TotW #153', wrap('using namespace Pokemon;'), HEADER) - self.assertEqual([], self.errors(wrap('using namespace Pokemon;'))) - self.assertEqual([], self.errors(wrap('inline void f(){\n using namespace std::chrono_literals;\n}'), - HEADER)) - - def test_forward_declarations(self): - source = wrap('class ImageViewRGB32;\nstruct Stats;\nclass Reader{\n friend class Tester;\n' - ' struct Stats;\n};') - self.assertEqual(2, sum('forward declaration' in m for m in self.errors(source, HEADER))) - self.assertEqual([], self.errors(source)) # .cpp files may declare local helpers - previous = wrap('') - changed = self.warnings(wrap('class ImageViewRGB32;'), HEADER, previous) - self.assertEqual(1, sum('forward declaration' in m for m in changed)) - self.assertEqual([], self.errors(wrap('class ImageViewRGB32;'), HEADER, previous)) - - def test_primary_template_before_its_specialization_is_not_a_forward_declaration(self): - self.assertClean(wrap('template \nclass MoveOnlyFunction;\n' - 'template \nclass MoveOnlyFunction{\n};')) - - def test_top_level_const_by_value_parameters_in_header_declarations(self): - self.assertFlags("'const int count'", wrap('void f(const int count, const std::string& name);'), HEADER) - self.assertFlags("'const size_t index = 0'", wrap('struct A{\n A(const size_t index = 0);\n};'), HEADER) - self.assertClean(wrap('void f(const std::string& a, const char* b, const std::vector c_ref,\n' - ' int d, std::function&& e);')) - self.assertEqual([], self.errors(wrap('inline void f(const int x){\n g(x);\n}'), HEADER)) - self.assertEqual([], self.errors(wrap('void f(const int count);'))) # .cpp declaration - - def test_bool_parameters(self): - self.assertFlags('a defaulted bool parameter', - wrap('void checkpoint_reattempt_loop(Env& env, bool save = true);'), HEADER) - self.assertFlags('2 bool parameters', - wrap('void exit_wild_battle(Context& c, bool stop_on_move_learn, bool prevent_evolution){\n}')) - self.assertClean(wrap('void f(Context& c, bool enabled);\nvoid g(Mode mode);\n' - 'struct A : B{\n virtual void h(bool a, bool b) override;\n};\n' - 'void call(){\n run(true, false);\n auto l = [](bool a, bool b){ return a; };\n}')) - - def test_existing_bool_signature_is_not_reported_when_its_line_changes(self): - previous = wrap('void f(bool a, bool b);') - self.assertEqual([], self.errors(wrap('void f(bool a, bool b); '), HEADER, previous)) - self.assertEqual(1, len(self.errors(wrap('void f(bool a, bool b, bool c);'), HEADER, previous))) - - -class CommentTests(StyleCase): - def test_comment_above_the_wrong_function_is_advisory(self): - source = wrap('// Save the game from the overworld with retries.\n' - '// Adds `text` to the overlay log, split at spaces.\n' - 'void add_wrapped_log(VideoOverlay& overlay, const std::string& text);\n' - 'bool save_game(Env& env);') - self.assertEqual([], self.errors(source, HEADER)) - warnings = self.warnings(source, HEADER) - self.assertEqual(1, len(warnings)) - self.assertIn('sits above add_wrapped_log but its first sentence describes save_game', warnings[0]) - - def test_comment_that_slid_onto_an_uncommented_neighbour_is_advisory(self): - source = wrap('uint16_t calorie_number() const;\n' - '// How many calorie number filters this detector uses.\n' - 'static void warm_ocr();') - self.assertEqual(1, len(self.warnings(source, HEADER))) - - def test_comment_on_a_commented_neighbour_mentioning_its_words_passes(self): - self.assertClean(wrap('// The last few screens, oldest first.\n' - 'std::string recent_screens() const;\n' - '// Throws with the recent screens appended.\n' - 'void fail(const std::string& message) const;'), HEADER) - - def test_comments_that_name_their_function_or_refer_to_others_pass(self): - self.assertClean(wrap( - '// Opens the save screen and reads the location (see `save_game()`).\n' - 'std::string read_location_from_save_screen(Context& c);\n' - 'bool save_game(Env& env);\n' - '// Remove all messages added by `add_log()`.\n' - 'void clear_log();\n' - 'void add_log(std::string message);')) - - -class AddedLinesTests(StyleCase): - def test_unchanged_legacy_lines_pass(self): - previous = in_function('if (x) return;\nstd::mutex m;') - current = in_function('if (x) return;\nstd::mutex m;\nrun();') - self.assertEqual([], self.errors(current, CPP, previous)) - self.assertEqual(1, len(self.errors(in_function('if (y) return;\nstd::mutex m;'), CPP, previous))) - - def test_crlf_and_findings_carry_lines(self): - findings = check(CPP, in_function('std::thread t(run);').replace('\n', '\r\n'), None)[0] - self.assertEqual([4], [f.line for f in findings]) - - -if __name__ == '__main__': - unittest.main() diff --git a/devtools/code_health/tests/test_verify.py b/devtools/code_health/tests/test_verify.py deleted file mode 100644 index 81f24766a8..0000000000 --- a/devtools/code_health/tests/test_verify.py +++ /dev/null @@ -1,30 +0,0 @@ -import unittest -from types import SimpleNamespace - -from devtools.verify import commands, verify - - -class VerifyTests(unittest.TestCase): - def test_failure_stops_before_later_steps(self): - called = [] - def run(command, **kwargs): - called.append(command) - return SimpleNamespace(returncode=7) - self.assertEqual(7, verify([['first'], ['must-not-run']], run=run)) - self.assertEqual([['first']], called) - - def test_non_mac_does_not_claim_named_profile_support(self): - steps = commands('/build', '/binary', 'origin/main', 4, platform='linux') - self.assertNotIn('--profile', steps[-1]) - - def test_all_steps_run_and_native_tests_use_separate_profile(self): - steps = commands('/build', '/binary', 'origin/main', 4, platform='darwin') - self.assertIn('--command-line-test-mode', steps[-1]) - self.assertIn('agent-native-tests', steps[-1]) - called = [] - def run(command, **kwargs): - called.append((command, kwargs['cwd'])) - return SimpleNamespace(returncode=0) - self.assertEqual(0, verify(steps, run=run, native_cwd='/native-resources')) - self.assertEqual(steps, [command for command, cwd in called]) - self.assertEqual('/native-resources', called[-1][1]) diff --git a/devtools/verify.py b/devtools/verify.py deleted file mode 100644 index e1187e9100..0000000000 --- a/devtools/verify.py +++ /dev/null @@ -1,54 +0,0 @@ -#!/usr/bin/env python3 -"""Run harness checks, build a configured CMake tree, then native C++ tests.""" -import argparse -import os -import subprocess -import sys -from pathlib import Path - - -ROOT = Path(__file__).resolve().parents[1] - - -def commands(build_dir, binary, base_ref, jobs, platform=sys.platform): - profile = ['--profile', 'agent-native-tests'] if platform == 'darwin' else [] - return [ - [sys.executable, '-m', 'unittest', 'discover', '-s', 'devtools/code_health/tests', '-v'], - [sys.executable, '-m', 'devtools.code_health', '--base-ref', base_ref], - ['cmake', '--build', str(build_dir), '--config', 'RelWithDebInfo', '--parallel', str(jobs)], - [str(binary), *profile, '--command-line-test-mode'], - ] - - -def verify(steps, run=subprocess.run, env=None, native_cwd=None): - for index, command in enumerate(steps): - print('Running:', ' '.join(command), flush=True) - directory = native_cwd if native_cwd and index == len(steps) - 1 else ROOT - result = run(command, cwd=directory, env=env) - if result.returncode: - return result.returncode - return 0 - - -def main(): - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument('--build-dir', type=Path, required=True) - parser.add_argument('--binary', type=Path, required=True) - parser.add_argument('--base-ref', default='origin/main') - parser.add_argument('--jobs', type=int, default=4) - args = parser.parse_args() - build_dir = args.build_dir.resolve() - if not (build_dir / 'CMakeCache.txt').is_file(): - parser.error('Configure this checkout with CMake first; see devtools/code_health/README.md') - env = os.environ.copy() - env['QT_QPA_PLATFORM'] = 'offscreen' - try: - return verify(commands(build_dir, args.binary.resolve(), args.base_ref, args.jobs), - env=env, native_cwd=build_dir) - except OSError as error: - print(f'Verification could not run: {error}', file=sys.stderr) - return 2 - - -if __name__ == '__main__': - sys.exit(main())