diff options
author | David Bradford <david.bradford@mongodb.com> | 2021-04-29 14:24:06 -0400 |
---|---|---|
committer | Evergreen Agent <no-reply@evergreen.mongodb.com> | 2021-05-12 20:35:47 +0000 |
commit | 82997e89d49be1c031b99a0feb6cc30366fe7d3f (patch) | |
tree | bc5b47639e50065f049c4f1517596557f2896741 /buildscripts | |
parent | 7f55c838799f0cfe66d61aa3ff10ca71524b7f52 (diff) | |
download | mongo-82997e89d49be1c031b99a0feb6cc30366fe7d3f.tar.gz |
SERVER-56216: Add commit-queue checks for unresolved TODOs
(cherry picked from commit 272b9e0bd2425ae15766b198de0f6a1a522bf8d3)
(cherry picked from commit a8782fc7ea197dd31964be8f238c618d9a776a01)
Diffstat (limited to 'buildscripts')
-rw-r--r-- | buildscripts/tests/test_todo_check.py | 266 | ||||
-rw-r--r-- | buildscripts/todo_check.py | 263 |
2 files changed, 529 insertions, 0 deletions
diff --git a/buildscripts/tests/test_todo_check.py b/buildscripts/tests/test_todo_check.py new file mode 100644 index 00000000000..b158c3b18fb --- /dev/null +++ b/buildscripts/tests/test_todo_check.py @@ -0,0 +1,266 @@ +"""Unit tests for todo_check.py.""" + +from __future__ import absolute_import + +import os +import textwrap +import unittest +from tempfile import TemporaryDirectory + +from typing import Iterable + +import buildscripts.todo_check as under_test + +# pylint: disable=missing-docstring,invalid-name,unused-argument,no-self-use,protected-access + + +def create_file_iterator(file_contents: str) -> Iterable[str]: + return textwrap.dedent(file_contents.strip()).splitlines() + + +class TestTodo(unittest.TestCase): + def test_line_without_a_jira_issues(self): + content = "a line with some random text" + todo = under_test.Todo.from_line("file", 42, f"\n{content}\n\t\n") + + self.assertEqual(todo.file_name, "file") + self.assertEqual(todo.line_number, 42) + self.assertEqual(todo.line, content) + self.assertIsNone(todo.ticket) + + def test_line_with_a_server_ticket(self): + ticket = "SERVER-12345" + content = f"a line with {ticket} some random text" + todo = under_test.Todo.from_line("file", 42, f"\n{content}\n\t\n") + + self.assertEqual(todo.file_name, "file") + self.assertEqual(todo.line_number, 42) + self.assertEqual(todo.line, content) + self.assertEqual(todo.ticket, ticket) + + def test_line_with_a_wiredtiger_ticket(self): + ticket = "WT-5555" + content = f"a line with {ticket} some random text" + todo = under_test.Todo.from_line("file", 42, f"\n{content}\n\t\n") + + self.assertEqual(todo.file_name, "file") + self.assertEqual(todo.line_number, 42) + self.assertEqual(todo.line, content) + self.assertEqual(todo.ticket, ticket) + + +class TestTodoChecker(unittest.TestCase): + def test_todo_checker_starts_out_empty(self): + todos = under_test.TodoChecker() + + self.assertEqual(len(todos.found_todos.no_tickets), 0) + self.assertEqual(len(todos.found_todos.with_tickets), 0) + self.assertEqual(len(todos.found_todos.by_file), 0) + + def test_a_file_with_no_todos(self): + file_contents = """ + line 0 + line 1 + this is the file contents. + + """ + todos = under_test.TodoChecker() + + todos.check_file("my file", create_file_iterator(file_contents)) + + self.assertEqual(len(todos.found_todos.no_tickets), 0) + self.assertEqual(len(todos.found_todos.with_tickets), 0) + self.assertEqual(len(todos.found_todos.by_file), 0) + + def test_a_file_with_an_untagged_todo(self): + file_contents = """ + line 0 + line 1 + /* todo this needs some updating */ + this is the file contents. + """ + todos = under_test.TodoChecker() + + todos.check_file("my file", create_file_iterator(file_contents)) + + self.assertEqual(len(todos.found_todos.no_tickets), 1) + self.assertEqual(len(todos.found_todos.with_tickets), 0) + self.assertEqual(len(todos.found_todos.by_file), 1) + + todo = todos.found_todos.no_tickets[0] + self.assertEqual(todo.file_name, "my file") + self.assertEqual(todo.line_number, 3) + self.assertEqual(todo.ticket, None) + + self.assertEqual(todo, todos.found_todos.by_file["my file"][0]) + + def test_a_file_with_a_tagged_todo(self): + file_contents = """ + line 0 + line 1 + line 2 + /* TODO server-1234 this needs some updating */ + this is the file contents. + """ + todos = under_test.TodoChecker() + + todos.check_file("my file", create_file_iterator(file_contents)) + + self.assertEqual(len(todos.found_todos.no_tickets), 0) + self.assertEqual(len(todos.found_todos.with_tickets), 1) + self.assertEqual(len(todos.found_todos.by_file), 1) + + todo = todos.found_todos.with_tickets["SERVER-1234"][0] + self.assertEqual(todo.file_name, "my file") + self.assertEqual(todo.line_number, 4) + self.assertEqual(todo.ticket, "SERVER-1234") + + self.assertEqual(todo, todos.found_todos.by_file["my file"][0]) + + def test_report_on_ticket_will_return_true_if_ticket_is_found(self): + file_contents = """ + line 0 + line 1 + line 2 + /* TODO server-1234 this needs some updating */ + this is the file contents. + """ + todos = under_test.TodoChecker() + todos.check_file("my file", create_file_iterator(file_contents)) + + self.assertTrue(todos.report_on_ticket("SERVER-1234")) + + def test_report_on_ticket_will_return_false_if_ticket_is_not_found(self): + file_contents = """ + line 0 + line 1 + line 2 + /* TODO server-1234 this needs some updating */ + this is the file contents. + """ + todos = under_test.TodoChecker() + todos.check_file("my file", create_file_iterator(file_contents)) + + self.assertFalse(todos.report_on_ticket("SERVER-9876")) + + def test_report_all_tickets_will_return_true_if_any_ticket_is_found(self): + file_contents = """ + line 0 + line 1 + line 2 + /* TODO server-1234 this needs some updating */ + this is the file contents. + /* TODO server-54321 this also needs some updating */ + """ + todos = under_test.TodoChecker() + todos.check_file("my file", create_file_iterator(file_contents)) + + self.assertTrue(todos.report_on_all_tickets()) + + def test_report_all_tickets_will_return_false_if_no_ticket_is_found(self): + file_contents = """ + line 0 + line 1 + line 2 + this is the file contents. + """ + todos = under_test.TodoChecker() + todos.check_file("my file", create_file_iterator(file_contents)) + + self.assertFalse(todos.report_on_all_tickets()) + + +class TestValidateCommitQueue(unittest.TestCase): + def test_revert_commits_should_not_fail(self): + commit_message = "Reverts commit SERVER-1234" + file_contents = """ + line 0 + line 1 + line 2 + /* TODO server-1234 this needs some updating */ + this is the file contents. + /* TODO server-54321 this also needs some updating */ + """ + todos = under_test.TodoChecker() + todos.check_file("my file", create_file_iterator(file_contents)) + + self.assertFalse(todos.validate_commit_queue(commit_message)) + + def test_todos_associated_with_commit_message_should_be_found(self): + commit_message = "SERVER-1234 making a commit" + file_contents = """ + line 0 + line 1 + line 2 + /* TODO server-1234 this needs some updating */ + this is the file contents. + /* TODO server-54321 this also needs some updating */ + """ + todos = under_test.TodoChecker() + todos.check_file("my file", create_file_iterator(file_contents)) + + self.assertTrue(todos.validate_commit_queue(commit_message)) + + def test_commit_messages_with_multiple_commits_search_all_of_them(self): + commit_message = """ + Making a wiredtiger drop + + WT-1234 + WT-4321 + WT-9876 + """ + file_contents = """ + line 0 + line 1 + line 2 + /* TODO server-1234 this needs some updating */ + this is the file contents. + /* TODO WT-9876 this also needs some updating */ + """ + todos = under_test.TodoChecker() + todos.check_file("my file", create_file_iterator(file_contents)) + + self.assertTrue(todos.validate_commit_queue(commit_message)) + + def test_commit_messages_with_no_tickets_doesnt_cause_issues(self): + commit_message = "A random commit" + file_contents = """ + line 0 + line 1 + line 2 + /* TODO server-1234 this needs some updating */ + this is the file contents. + /* TODO WT-9876 this also needs some updating */ + """ + todos = under_test.TodoChecker() + todos.check_file("my file", create_file_iterator(file_contents)) + + self.assertFalse(todos.validate_commit_queue(commit_message)) + + +def write_file(path: str, contents: str): + with open(path, "w") as fh: + fh.write(contents) + + +class TestWalkFs(unittest.TestCase): + def test_walk_fs_walks_the_fs(self): + expected_files = { + "file1.txt": "The contents of file 1", + "file2.txt": "The contents of file 2", + } + with TemporaryDirectory() as tmpdir: + write_file(os.path.join(tmpdir, "file1.txt"), expected_files["file1.txt"]) + os.makedirs(os.path.join(tmpdir, "dir0", "dir1")) + write_file( + os.path.join(tmpdir, "dir0", "dir1", "file2.txt"), expected_files["file2.txt"]) + + seen_files = {} + + def visit_file(file_name, file_contents): + base_name = os.path.basename(file_name) + seen_files[base_name] = "\n".join(file_contents) + + under_test.walk_fs(tmpdir, visit_file) + + self.assertDictEqual(expected_files, seen_files) diff --git a/buildscripts/todo_check.py b/buildscripts/todo_check.py new file mode 100644 index 00000000000..314d4b2d284 --- /dev/null +++ b/buildscripts/todo_check.py @@ -0,0 +1,263 @@ +#!/usr/bin/env python3 +"""Check for TODOs in the source code.""" +import os +import re +import sys +from collections import defaultdict +from dataclasses import dataclass +from typing import Iterable, Callable, Optional, NamedTuple, Dict, List + +import click + +BASE_SEARCH_DIR = "." +ISSUE_RE = re.compile('(BUILD|SERVER|WT|PM|TOOLS|TIG|PERF|BF)-[0-9]+') + + +class Todo(NamedTuple): + """ + A TODO comment found the in the code. + + file_name: Name of file comment was found in. + line_number: Line number comment was found in. + line: Content of line comment was found in. + ticket: Jira ticket associated with comment. + """ + + file_name: str + line_number: int + line: str + ticket: Optional[str] = None + + @classmethod + def from_line(cls, file_name: str, line_number: int, line: str) -> "Todo": + """ + Create a found todo from the given line of code. + + :param file_name: File name comment was found in. + :param line_number: Line number comment was found in. + :param line: Content of line. + :return: Todo representation of found comment. + """ + issue_key = cls.get_issue_key_from_line(line) + return cls(file_name, line_number, line.strip(), issue_key) + + @staticmethod + def get_issue_key_from_line(line: str) -> Optional[str]: + """ + Check if the given line appears to reference a jira ticket. + + :param line: Content of line. + :return: Jira ticket if one was found. + """ + match = ISSUE_RE.search(line.upper()) + if match: + return match.group(0) + return None + + +@dataclass +class FoundTodos: + """ + Collection of TODO comments found in the code base. + + no_tickets: TODO comments found without any Jira ticket references. + with_tickets: Dictionary of Jira tickets references with a list of references. + by_file: All the references found mapped by the files they were found in. + """ + + no_tickets: List[Todo] + with_tickets: Dict[str, List[Todo]] + by_file: Dict[str, List[Todo]] + + +class TodoChecker: + """A tool to find and track TODO references.""" + + def __init__(self) -> None: + """Initialize a new TODO checker.""" + self.found_todos = FoundTodos(no_tickets=[], with_tickets=defaultdict(list), + by_file=defaultdict(list)) + + def check_file(self, file_name: str, file_contents: Iterable[str]) -> None: + """ + Check the given file for TODO references. + + Any TODOs will be added to `found_todos`. + + :param file_name: Name of file being checked. + :param file_contents: Iterable of the file contents. + """ + for i, line in enumerate(file_contents): + if "todo" in line.lower(): + todo = Todo.from_line(file_name, i + 1, line) + if todo.ticket is not None: + self.found_todos.with_tickets[todo.ticket].append(todo) + else: + self.found_todos.no_tickets.append(todo) + self.found_todos.by_file[file_name].append(todo) + + def check_all_files(self, base_dir: str) -> None: + """ + Check all files under the base directory for TODO references. + + :param base_dir: Base directory to start searching. + """ + walk_fs(base_dir, self.check_file) + + @staticmethod + def print_todo_list(todo_list: List[Todo]) -> None: + """Display all the TODOs in the given list.""" + last_file = None + for todo in todo_list: + if last_file != todo.file_name: + print(f"{todo.file_name}") + print(f"\t{todo.line_number}: {todo.line}") + + def report_on_ticket(self, ticket: str) -> bool: + """ + Report on any TODOs found referencing the given ticket. + + Any found references will be printed to stdout. + + :param ticket: Jira ticket to search for. + :return: True if any TODOs were found. + """ + todo_list = self.found_todos.with_tickets.get(ticket) + if todo_list: + print(f"{ticket}") + self.print_todo_list(todo_list) + return True + return False + + def report_on_all_tickets(self) -> bool: + """ + Report on all TODOs found that reference a Jira ticket. + + Any found references will be printed to stdout. + + :return: True if any TODOs were found. + """ + if not self.found_todos.with_tickets: + return False + + for ticket in self.found_todos.with_tickets.keys(): + self.report_on_ticket(ticket) + + return True + + def validate_commit_queue(self, commit_message: str) -> bool: + """ + Check that the given commit message does not reference TODO comments. + + :param commit_message: Commit message to check. + :return: True if any TODOs were found. + """ + print("*" * 80) + print("Checking for TODOs associated with Jira key in commit message.") + if "revert" in commit_message.lower(): + print("Skipping checks since it looks like this is a revert.") + # Reverts are a special case and we shouldn't fail them. + return False + + found_any = False + ticket = Todo.get_issue_key_from_line(commit_message) + while ticket: + found_any = self.report_on_ticket(ticket) or found_any + rest_index = commit_message.find(ticket) + commit_message = commit_message[rest_index + len(ticket):] + ticket = Todo.get_issue_key_from_line(commit_message) + + print(f"Checking complete - todos found: {found_any}") + print("*" * 80) + return found_any + + +def walk_fs(root: str, action: Callable[[str, Iterable[str]], None]) -> None: + """ + Walk the file system and perform the given action on each file. + + :param root: Base to start walking the filesystem. + :param action: Action to perform on each file. + """ + for base, _, files in os.walk(root): + for file_name in files: + try: + file_path = os.path.join(base, file_name) + with open(file_path) as search_file: + action(file_path, search_file) + except UnicodeDecodeError: + # If we try to read any non-text files. + continue + + +@click.command() +@click.option("--ticket", help="Only report on TODOs associated with given Jira ticket.") +@click.option("--base-dir", default=BASE_SEARCH_DIR, help="Base directory to search in.") +@click.option("--commit-message", + help="For commit-queue execution only, ensure no TODOs for this commit") +def main(ticket: Optional[str], base_dir: str, commit_message: Optional[str]): + """ + Search for and report on TODO comments in the code base. + + Based on the arguments given, there are two types of searches you can perform: + + \b + * By default, search for all TODO comments that reference any Jira ticket. + * Search for references to a specific Jira ticket with the `--ticket` option. + + \b + Examples + -------- + + \b + Search all TODO comments with Jira references: + ``` + > python buildscripts/todo_check.py + SERVER-12345 + ./src/mongo/db/file.cpp + 140: // TODO: SERVER-12345: Need to fix this. + 183: // TODO: SERVER-12345: Need to fix this as well. + SERVER-54321 + ./src/mongo/db/other_file.h + 728: // TODO: SERVER-54321 an update is needed here + ``` + + \b + Search for any TODO references to a given ticket. + ``` + > python buildscripts/todo_check.py --ticket SERVER-56197 + SERVER-56197 + ./src/mongo/db/query/query_feature_flags.idl + 33: # TODO SERVER-56197: Remove feature flag. + ``` + + \b + Exit Code + --------- + In any mode, if any TODO comments are found a non-zero exit code will be returned. + \f + :param ticket: Only report on TODOs associated with this jira ticket. + :param base_dir: Search files in this base directory. + :param commit_message: Commit message if running in the commit-queue.[ + + """ + if commit_message and ticket is not None: + raise click.UsageError("--ticket cannot be used in commit queue.") + + todo_checker = TodoChecker() + todo_checker.check_all_files(base_dir) + + if commit_message: + found_todos = todo_checker.validate_commit_queue(commit_message) + elif ticket: + found_todos = todo_checker.report_on_ticket(ticket) + else: + found_todos = todo_checker.report_on_all_tickets() + + if found_todos: + sys.exit(1) + sys.exit(0) + + +if __name__ == "__main__": + main() # pylint: disable=no-value-for-parameter |