diff --git a/rspec-tools/rspec_tools/cli.py b/rspec-tools/rspec_tools/cli.py index 56b9f34c95..558db8b832 100644 --- a/rspec-tools/rspec_tools/cli.py +++ b/rspec-tools/rspec_tools/cli.py @@ -1,30 +1,34 @@ import os -from typing import Optional from pathlib import Path +from typing import Optional import click -from rspec_tools.checklinks import check_html_links -from rspec_tools.errors import RuleNotFoundError, RuleValidationError -from rspec_tools.rules import RulesRepository, LanguageSpecificRule -from rspec_tools.validation.metadata import validate_rule_metadata -from rspec_tools.validation.description import validate_section_names, validate_section_levels, validate_parameters, validate_source_language -from rspec_tools.coverage import update_coverage_for_all_repos, update_coverage_for_repo, update_coverage_for_repo_version -import rspec_tools.create_rule +import rspec_tools.create_rule +import rspec_tools.modify_rule +from rspec_tools.checklinks import check_html_links +from rspec_tools.coverage import (update_coverage_for_all_repos, + update_coverage_for_repo, + update_coverage_for_repo_version) +from rspec_tools.errors import RuleValidationError from rspec_tools.notify_failure_on_slack import notify_slack +from rspec_tools.rules import LanguageSpecificRule, RulesRepository +from rspec_tools.validation.description import (validate_parameters, + validate_section_levels, + validate_section_names, + validate_source_language) +from rspec_tools.validation.metadata import validate_rule_metadata + + +def _fatal_error(message: str): + click.echo(message, err=True) + raise click.Abort(message) @click.group() @click.option('--debug/--no-debug', default=False) def cli(debug): 'Tools automating RSPEC workflows.' -@cli.command() -@click.option('--rule', help='Validate only the rule matching the provided ID.') -def validate(rule): - '''Validate rules.''' - # TODO - if rule == '42': - raise RuleNotFoundError(rule) @cli.command() @click.option('--d', required=True) @@ -32,6 +36,7 @@ def check_links(d): '''Check links in html.''' check_html_links(d) + @cli.command() @click.option('--languages', required=True) @click.option('--user', required=False) @@ -40,6 +45,7 @@ def create_rule(languages: str, user: Optional[str]): token = os.environ.get('GITHUB_TOKEN') rspec_tools.create_rule.create_new_rule(languages, token, user) + @cli.command() @click.option('--language', required=True) @click.option('--rule', required=True) @@ -49,6 +55,7 @@ def add_lang_to_rule(language: str, rule: str, user: Optional[str]): token = os.environ.get('GITHUB_TOKEN') rspec_tools.create_rule.add_language_to_rule(language, rule, token, user) + @cli.command() @click.option('--language', required=True) @click.option('--rule', required=True) @@ -57,7 +64,8 @@ def add_lang_to_rule(language: str, rule: str, user: Optional[str]): def update_quickfix_status(language: str, rule: str, status: str, user: Optional[str]): '''Update the status of quick fix for the given rule/language''' token = os.environ.get('GITHUB_TOKEN') - rspec_tools.create_rule.update_rule_quickfix_status(language, rule, status, token, user) + rspec_tools.modify_rule.update_rule_quickfix_status(language, rule, status, token, user) + @cli.command() @click.argument('rules', nargs=-1, required=True) @@ -75,7 +83,7 @@ def validate_rules_metadata(rules): error_counter += 1 if error_counter > 0: - fatal_error(f"Validation failed due to {error_counter} errors out of {len(rules)} analyzed rules") + _fatal_error(f"Validation failed due to {error_counter} errors out of {len(rules)} analyzed rules") VALIDATORS = [validate_section_names, @@ -83,7 +91,7 @@ VALIDATORS = [validate_section_names, validate_parameters, validate_source_language, ] -def validate_rule_specialization(lang_spec_rule: LanguageSpecificRule): +def _validate_rule_specialization(lang_spec_rule: LanguageSpecificRule): error_counter = 0 for validator in VALIDATORS: try: @@ -93,6 +101,7 @@ def validate_rule_specialization(lang_spec_rule: LanguageSpecificRule): error_counter += 1 return error_counter + @cli.command() @click.option('--d', required=True) @click.argument('rules', nargs=-1) @@ -105,9 +114,10 @@ def check_description(d, rules): if rules and rule.id not in rules: continue for lang_spec_rule in rule.specializations: - error_counter += validate_rule_specialization(lang_spec_rule) + error_counter += _validate_rule_specialization(lang_spec_rule) if error_counter > 0: - fatal_error(f"Validation failed due to {error_counter} errors") + _fatal_error(f"Validation failed due to {error_counter} errors") + @cli.command() @click.option('--repository', required=False) @@ -121,14 +131,12 @@ def update_coverage(repository: Optional[str], version: Optional[str]): else: update_coverage_for_repo_version(repository, version) + @cli.command() @click.option('--message', required=True) @click.option('--channel', required=True) def notify_failure_on_slack(message: str, channel: str): notify_slack(message, channel) -__all__=['cli'] -def fatal_error(message: str): - click.echo(message, err=True) - raise click.Abort(message) +__all__=['cli'] diff --git a/rspec-tools/rspec_tools/create_rule.py b/rspec-tools/rspec_tools/create_rule.py index d28c0d7227..7d74ebc877 100644 --- a/rspec-tools/rspec_tools/create_rule.py +++ b/rspec-tools/rspec_tools/create_rule.py @@ -1,132 +1,57 @@ import json -import os -import tempfile from contextlib import contextmanager from pathlib import Path from typing import Callable, Final, Iterable, Optional import click -from git import Repo -from github import Github from github.PullRequest import PullRequest from rspec_tools.errors import InvalidArgumentError +from rspec_tools.repo import RspecRepo, tmp_rspec_repo from rspec_tools.utils import (LANG_TO_SOURCE, copy_directory_content, get_label_for_language, get_labels_for_languages, is_empty_metadata, parse_and_validate_language_list, resolve_rule, - swap_metadata_files, validate_language) + swap_metadata_files) -def build_github_repository_url(token: str, user: Optional[str]): - '''Builds the rspec repository url''' - repo = os.environ.get('GITHUB_REPOSITORY', 'SonarSource/rspec') - if user: - return f'https://{user}:{token}@github.com/{repo}.git' - else: - return f'https://{token}@github.com/{repo}.git' - -def extract_repository_name(url): - url_end = url.split('/')[-2:] - return '/'.join(url_end).removesuffix('.git') - -def auto_github(token: str) -> Callable[[Optional[str]], Github]: - def ret(user: Optional[str]): - if user: - return Github(user, token) - else: - return Github(token) - return ret - -def _get_url_and_config(token: str, user: Optional[str]): - url = build_github_repository_url(token, user) - config = {} - if user: - config['user.name'] = user - config['user.email'] = f'{user}@users.noreply.github.com' - - return url, config - -def _get_valid_label_for_language(language: str): - validate_language(language) - return get_label_for_language(language) - @contextmanager def _rule_creator(token: str, user: Optional[str]): - url, config = _get_url_and_config(token, user) - with tempfile.TemporaryDirectory() as tmpdirname: - rule_creator = RuleCreator(url, tmpdirname, config) - yield rule_creator + with tmp_rspec_repo(token, user) as repo: + yield RuleCreator(repo) + def create_new_rule(languages: str, token: str, user: Optional[str]): lang_list = parse_and_validate_language_list(languages) label_list = get_labels_for_languages(lang_list) with _rule_creator(token, user) as rule_creator: - rule_number = rule_creator.reserve_rule_number() - rule_creator.create_new_rule_pull_request(auto_github(token), rule_number, lang_list, label_list, user=user) + rule_number = rule_creator.rspec_repo.reserve_rule_number() + rule_creator.create_new_rule_pull_request(token, rule_number, lang_list, label_list, user) + def add_language_to_rule(language: str, rule: str, token: str, user: Optional[str]): - label = _get_valid_label_for_language(language) + label = get_label_for_language(language) rule_number = resolve_rule(rule) with _rule_creator(token, user) as rule_creator: - rule_creator.add_language_pull_request(auto_github(token), rule_number, language, label, user=user) + rule_creator.add_language_pull_request(token, rule_number, language, label, user) -def update_rule_quickfix_status(language: str, rule: str, status: str, token: str, user: Optional[str]): - label = _get_valid_label_for_language(language) - rule_number = resolve_rule(rule) - with _rule_creator(token, user) as rule_creator: - rule_creator.update_quickfix_status_pull_request(auto_github(token), rule_number, language, status, label, user) class RuleCreator: - ''' Create a new Rule in a repository following the official Github 'rspec' repository structure.''' - MASTER_BRANCH: Final[str] = 'master' - ID_COUNTER_BRANCH: Final[str] = 'rspec-id-counter' - ID_COUNTER_FILENAME: Final[str] = 'next_rspec_id.txt' + '''Create a new Rule in a repository following the official Github 'rspec' repository structure.''' TEMPLATE_PATH: Final[Path] = Path(__file__).parent.parent.joinpath('rspec_template') - repository: Final[Repo] - origin_url: Final[str] - - def __init__(self, repository_url: str, clone_directory: str, configuration: dict[str, str]): - self.repository = Repo.clone_from(repository_url, clone_directory) - self.origin_url = repository_url - - # create local branches tracking remote ones - for branch in [self.MASTER_BRANCH, self.ID_COUNTER_BRANCH]: - self.repository.remote().fetch(branch) - self.repository.git.checkout('-B', branch, f'origin/{branch}') - - # update repository config - with self.repository.config_writer() as config_writer: - for key, value in configuration.items(): - split_key = key.split('.') - config_writer.set_value(*split_key, value) - - def reserve_rule_number(self) -> int: - '''Reserve an id on the id counter branch of the repository.''' - with self._current_git_branch(self.ID_COUNTER_BRANCH): - counter_file_path = Path(self.repository.working_dir).joinpath(self.ID_COUNTER_FILENAME) - counter = int(counter_file_path.read_text()) - counter_file_path.write_text(str(counter + 1)) - - self.repository.index.add([str(counter_file_path)]) - self.repository.index.commit('Increment RSPEC ID counter') - - origin = self.repository.remote(name='origin') - origin.push() - - click.echo(f'Reserved Rule ID S{counter}') - return counter + def __init__(self, rspec_repo: RspecRepo): + self.rspec_repo = rspec_repo + self.repo_dir = Path(self.rspec_repo.repository.working_dir) def add_language_branch(self, rule_number: int, language: str) -> str: '''Create and move files to add a new language to an existing rule.''' branch_name = f'rule/S{rule_number}-add-{language}' - with self._current_git_branch(self.MASTER_BRANCH, branch_name): - repo_dir = Path(self.repository.working_dir) - rule_dir = repo_dir.joinpath('rules', f'S{rule_number}') + with self.rspec_repo.checkout_branch(self.rspec_repo.MASTER_BRANCH, branch_name): + rule_dir = self.repo_dir / 'rules' / f'S{rule_number}' if not rule_dir.is_dir(): raise InvalidArgumentError(f"Rule \"S{rule_number}\" does not exist.") - lang_dir = rule_dir.joinpath(language) + lang_dir = rule_dir / language if lang_dir.is_dir(): lang_url = f"https://github.com/SonarSource/rspec/tree/master/rules/S{rule_number}/{language}" raise InvalidArgumentError(f"Rule \"S{rule_number}\" is already defined for language {language}. Modify the definition here: {lang_url}") @@ -135,66 +60,28 @@ class RuleCreator: swap_metadata_files(rule_dir, lang_dirs[0]) lang_dir.mkdir() - lang_specific_template = self.TEMPLATE_PATH.joinpath('multi_language', 'language_specific') + lang_specific_template = self.TEMPLATE_PATH / 'multi_language' / 'language_specific' copy_directory_content(lang_specific_template, lang_dir) self._fill_in_the_blanks_in_the_template(lang_dir, rule_number) self._fill_language_name_in_the_template(lang_dir, language) - self.repository.git.add('--all') - self.repository.index.commit(f'Add {language} to rule S{rule_number}') - self.repository.git.push('origin', branch_name) + self.rspec_repo.commit_all_and_push(f'Add {language} to rule S{rule_number}') return branch_name def create_new_rule_branch(self, rule_number: int, languages: Iterable[str]) -> str: '''Create all the files required for a new rule.''' branch_name = f'rule/add-RSPEC-S{rule_number}' - with self._current_git_branch(self.MASTER_BRANCH, branch_name): - repo_dir = Path(self.repository.working_dir) - rule_dir = repo_dir.joinpath('rules', f'S{rule_number}') + with self.rspec_repo.checkout_branch(self.rspec_repo.MASTER_BRANCH, branch_name): + rule_dir = self.repo_dir / 'rules' / f'S{rule_number}' rule_dir.mkdir() lang_count = sum(1 for _ in languages) if lang_count > 1: self._fill_multi_lang_template_files(rule_dir, rule_number, languages) else: self._fill_single_lang_template_files(rule_dir, rule_number, next(iter(languages))) + self.rspec_repo.commit_all_and_push(f'Create rule S{rule_number}') - self.repository.git.add('--all') - self.repository.index.commit(f'Create rule S{rule_number}') - self.repository.git.push('origin', branch_name) return branch_name - def update_quickfix_status_branch(self, title: str, rule_number: int, language: str, status: str) -> str: - '''Update the given rule/language quick fix metadata field.''' - branch_name = f'rule/S{rule_number}-{language}-quickfix' - with self._current_git_branch(self.MASTER_BRANCH, branch_name): - self._update_quickfix_status(rule_number, language, status) - self.repository.git.add('--all') - self.repository.index.commit(title) - self.repository.git.push('origin', branch_name) - return branch_name - - def _get_generic_quickfix_status(self, rule_number: int): - DEFAULT = 'unknown' - generic_metadata_path = Path(self.repository.working_dir, 'rules', f'S{rule_number}', 'metadata.json') - if not generic_metadata_path.is_file(): - return DEFAULT - generic_metadata = json.loads(generic_metadata_path.read_text()) - return generic_metadata.get('quickfix', DEFAULT) - - def _update_quickfix_status(self, rule_number: int, language: str, status: str): - metadata_path = Path(self.repository.working_dir, 'rules', f'S{rule_number}', language, 'metadata.json') - if not metadata_path.is_file(): - raise InvalidArgumentError(f'{metadata_path} does not exist or is not a file') - - metadata = json.loads(metadata_path.read_text()) - generic_status = self._get_generic_quickfix_status(rule_number) - if status == metadata.get('quickfix', generic_status): - raise InvalidArgumentError(f'{metadata_path} has already the same status {status}') - - metadata['quickfix'] = status - # When generating the JSON, ensure forward slashes are escaped. See RULEAPI-750. - json_string = json.dumps(metadata, indent=2).replace("/", "\\/") - metadata_path.write_text(json_string) - def _fill_in_the_blanks_in_the_template(self, rule_dir: Path, rule_number: int): for rule_item in rule_dir.glob('**/*'): if rule_item.is_file(): @@ -211,12 +98,12 @@ class RuleCreator: rule_item.write_text(final_content) def _fill_multi_lang_template_files(self, rule_dir: Path, rule_number: int, languages: Iterable[str]): - common_template = self.TEMPLATE_PATH.joinpath('multi_language', 'common') - lang_specific_template = self.TEMPLATE_PATH.joinpath('multi_language', 'language_specific') + common_template = self.TEMPLATE_PATH / 'multi_language' / 'common' + lang_specific_template = self.TEMPLATE_PATH / 'multi_language' / 'language_specific' copy_directory_content(common_template, rule_dir) for lang in languages: - lang_dir = rule_dir.joinpath(lang) + lang_dir = rule_dir / lang lang_dir.mkdir() copy_directory_content(lang_specific_template, lang_dir) self._fill_language_name_in_the_template(lang_dir, lang) @@ -224,41 +111,22 @@ class RuleCreator: self._fill_in_the_blanks_in_the_template(rule_dir, rule_number) def _fill_single_lang_template_files(self, rule_dir: Path, rule_number: int, language: str): - common_template = self.TEMPLATE_PATH.joinpath('single_language', 'common') - lang_specific_template = self.TEMPLATE_PATH.joinpath('single_language', 'language_specific') + common_template = self.TEMPLATE_PATH / 'single_language' / 'common' + lang_specific_template = self.TEMPLATE_PATH / 'single_language' / 'language_specific' copy_directory_content(common_template, rule_dir) - lang_dir = rule_dir.joinpath(language) + lang_dir = rule_dir /language lang_dir.mkdir() copy_directory_content(lang_specific_template, lang_dir) self._fill_in_the_blanks_in_the_template(rule_dir, rule_number) self._fill_language_name_in_the_template(lang_dir, language) - def _create_pull_request(self, github_api: Callable[[Optional[str]], Github], branch_name: str, title: str, body: str, labels: Iterable[str], user: Optional[str]): - repository_url = extract_repository_name(self.origin_url) - github = github_api(user) - github_repo = github.get_repo(repository_url) - pull_request = github_repo.create_pull( - title=title, - body=body, - head=branch_name, base=self.MASTER_BRANCH, - draft=True, maintainer_can_modify=True - ) - click.echo(f'Created rule Pull Request {pull_request.html_url}') - - # Note: It is not possible to get the authenticated user using get_user() from a github action. - login = user if user else github.get_user().login - pull_request.add_to_assignees(login) - pull_request.add_to_labels(*labels) - click.echo(f'Pull request assigned to {login}') - return pull_request - - def add_language_pull_request(self, github_api: Callable[[Optional[str]], Github], rule_number: int, language: str, label: str, user: Optional[str]): + def add_language_pull_request(self, token: str, rule_number: int, language: str, label: str, user: Optional[str]): branch_name = self.add_language_branch(rule_number, language) click.echo(f'Created rule branch {branch_name}') - return self._create_pull_request( - github_api, + return self.rspec_repo.create_pull_request( + token, branch_name, f'Create rule S{rule_number}', f'You can preview this rule [here](https://sonarsource.github.io/rspec/#/rspec/S{rule_number}/{language}) (updated a few minutes after each push).', @@ -266,12 +134,12 @@ class RuleCreator: user ) - def create_new_rule_pull_request(self, github_api: Callable[[Optional[str]], Github], rule_number: int, languages: Iterable[str], labels: Iterable[str], *, user: Optional[str]) -> PullRequest: + def create_new_rule_pull_request(self, token: str, rule_number: int, languages: Iterable[str], labels: Iterable[str], user: Optional[str]) -> PullRequest: branch_name = self.create_new_rule_branch(rule_number, languages) click.echo(f'Created rule branch {branch_name}') first_lang = next(iter(languages)) - return self._create_pull_request( - github_api, + return self.rspec_repo.create_pull_request( + token, branch_name, f'Create rule S{rule_number}', f'You can preview this rule [here](https://sonarsource.github.io/rspec/#/rspec/S{rule_number}/{first_lang}) (updated a few minutes after each push).', @@ -279,31 +147,3 @@ class RuleCreator: user ) - def update_quickfix_status_pull_request(self, github_api: Callable[[Optional[str]], Github], rule_number: int, language: str, status: str, label: str, user: Optional[str]): - title = f'Modify rule S{rule_number}: mark quick fix as "{status}"' - branch_name = self.update_quickfix_status_branch(title, rule_number, language, status) - click.echo(f'Created rule branch {branch_name}') - return self._create_pull_request( - github_api, - branch_name, - title, - f'''See the original rule [here](https://sonarsource.github.io/rspec/#/rspec/S{rule_number}/{language}). - -The rule won't be updated until this PR is merged, see [RULEAPI-655](https://jira.sonarsource.com/browse/RULEAPI-655)''', - [label], - user - ) - - @contextmanager - def _current_git_branch(self, base_branch: str, new_branch: Optional[str] = None): - '''Checkout a given branch before yielding, then revert to the previous branch.''' - past_branch = self.repository.active_branch - try: - self.repository.git.checkout(base_branch) - origin = self.repository.remote(name='origin') - origin.pull() - if new_branch is not None: - self.repository.git.checkout('-B', new_branch) - yield - finally: - self.repository.git.checkout(past_branch) diff --git a/rspec-tools/rspec_tools/errors.py b/rspec-tools/rspec_tools/errors.py index a84b97be92..914fe2259c 100644 --- a/rspec-tools/rspec_tools/errors.py +++ b/rspec-tools/rspec_tools/errors.py @@ -1,20 +1,15 @@ from click import ClickException -class RuleNotFoundError(ClickException): - def __init__(self, id): - super().__init__(f'No rule has ID {id}') class InvalidArgumentError(ClickException): '''Exception raised when an invalid argument is given to a CLI command.''' + def __init__(self, message): super().__init__(message) -class GitError(ClickException): - '''Exception raised when some error happened with git commands.''' - def __init__(self, message): - super().__init__(message) class RuleValidationError(ClickException): '''Exception raised when a rule did not pass validation.''' + def __init__(self, message): super().__init__(message) diff --git a/rspec-tools/rspec_tools/modify_rule.py b/rspec-tools/rspec_tools/modify_rule.py new file mode 100644 index 0000000000..ce423668c6 --- /dev/null +++ b/rspec-tools/rspec_tools/modify_rule.py @@ -0,0 +1,78 @@ +from contextlib import contextmanager +import json +from pathlib import Path +from typing import Optional + +import click +from rspec_tools.errors import InvalidArgumentError + +from rspec_tools.repo import RspecRepo, tmp_rspec_repo +from rspec_tools.utils import get_label_for_language, resolve_rule + + +@contextmanager +def _rule_editor(token: str, user: Optional[str]): + with tmp_rspec_repo(token, user) as repo: + yield RuleEditor(repo) + + +def update_rule_quickfix_status(language: str, rule: str, status: str, token: str, user: Optional[str]): + label = get_label_for_language(language) + rule_number = resolve_rule(rule) + with _rule_editor(token, user) as editor: + editor.update_quickfix_status_pull_request(token, rule_number, language, status, label, user) + + +class RuleEditor: + '''Modify an existing Rule in a repository following the official Github 'rspec' repository structure.''' + + def __init__(self, rspec_repo: RspecRepo): + self.rspec_repo = rspec_repo + self.repo_dir = Path(self.rspec_repo.repository.working_dir) + + def update_quickfix_status_branch(self, title: str, rule_number: int, language: str, status: str) -> str: + '''Update the given rule/language quick fix metadata field.''' + branch_name = f'rule/S{rule_number}-{language}-quickfix' + with self.rspec_repo.checkout_branch(self.rspec_repo.MASTER_BRANCH, branch_name): + self._update_quickfix_status(rule_number, language, status) + self.rspec_repo.commit_all_and_push(title) + + return branch_name + + def update_quickfix_status_pull_request(self, token: str, rule_number: int, language: str, status: str, label: str, user: Optional[str]): + title = f'Modify rule S{rule_number}: mark quick fix as "{status}"' + branch_name = self.update_quickfix_status_branch(title, rule_number, language, status) + click.echo(f'Created rule branch {branch_name}') + return self.rspec_repo.create_pull_request( + token, + branch_name, + title, + f'''See the original rule [here](https://sonarsource.github.io/rspec/#/rspec/S{rule_number}/{language}). + +The rule won't be updated until this PR is merged, see [RULEAPI-655](https://jira.sonarsource.com/browse/RULEAPI-655)''', + [label], + user + ) + + def _get_generic_quickfix_status(self, rule_number: int): + DEFAULT = 'unknown' + generic_metadata_path = self.repo_dir / 'rules' / f'S{rule_number}' / 'metadata.json' + if not generic_metadata_path.is_file(): + return DEFAULT + generic_metadata = json.loads(generic_metadata_path.read_text()) + return generic_metadata.get('quickfix', DEFAULT) + + def _update_quickfix_status(self, rule_number: int, language: str, status: str): + metadata_path = self.repo_dir / 'rules' / f'S{rule_number}' / language / 'metadata.json' + if not metadata_path.is_file(): + raise InvalidArgumentError(f'{metadata_path} does not exist or is not a file') + + metadata = json.loads(metadata_path.read_text()) + generic_status = self._get_generic_quickfix_status(rule_number) + if status == metadata.get('quickfix', generic_status): + raise InvalidArgumentError(f'{metadata_path} has already the same status {status}') + + metadata['quickfix'] = status + # When generating the JSON, ensure forward slashes are escaped. See RULEAPI-750. + json_string = json.dumps(metadata, indent=2).replace("/", "\\/") + metadata_path.write_text(json_string) diff --git a/rspec-tools/rspec_tools/repo.py b/rspec-tools/rspec_tools/repo.py new file mode 100644 index 0000000000..187e565f0f --- /dev/null +++ b/rspec-tools/rspec_tools/repo.py @@ -0,0 +1,130 @@ +import os +import tempfile +from contextlib import contextmanager +from pathlib import Path +from typing import Callable, Final, Iterable, Optional + +import click +from git import Repo +from github import Github + + +def _auto_github(token: str) -> Callable[[Optional[str]], Github]: + def ret(user: Optional[str]): + if user: + return Github(user, token) + else: + return Github(token) + return ret + + +class RspecRepo: + '''Provide operations on a git repository for rule specifications.''' + MASTER_BRANCH: Final[str] = 'master' + ID_COUNTER_BRANCH: Final[str] = 'rspec-id-counter' + ID_COUNTER_FILENAME: Final[str] = 'next_rspec_id.txt' + + repository: Final[Repo] + origin_url: Final[str] + + def __init__(self, origin_url: str, clone_directory: str, configuration: dict[str, str]): + self.repository = Repo.clone_from(origin_url, clone_directory) + self.origin_url = origin_url + + # Create local branches tracking remote ones + for branch in [self.MASTER_BRANCH, self.ID_COUNTER_BRANCH]: + self.repository.remote().fetch(branch) + self.repository.git.checkout('-B', branch, f'origin/{branch}') + + # Update repository config + with self.repository.config_writer() as config_writer: + for key, value in configuration.items(): + split_key = key.split('.') + config_writer.set_value(*split_key, value) + + def get_repository_name(self): + url_end = self.origin_url.split('/')[-2:] + return '/'.join(url_end).removesuffix('.git') + + @contextmanager + def checkout_branch(self, base_branch: str, new_branch: Optional[str] = None): + '''Checkout a given branch before yielding, then revert to the previous branch.''' + past_branch = self.repository.active_branch + try: + self.repository.git.checkout(base_branch) + origin = self.repository.remote(name='origin') + origin.pull() + if new_branch is not None: + self.repository.git.checkout('-B', new_branch) + yield + finally: + self.repository.git.checkout(past_branch) + + def commit_all_and_push(self, message: str): + self.repository.git.add('--all') + self.repository.index.commit(message) + self.repository.git.push('origin', self.repository.active_branch) + + def create_pull_request(self, token: str, branch_name: str, title: str, body: str, labels: Iterable[str], user: Optional[str]): + '''Create a pull request from the given branch.''' + repository_url = self.get_repository_name() + github_api = _auto_github(token) + github = github_api(user) + github_repo = github.get_repo(repository_url) + pull_request = github_repo.create_pull( + title=title, + body=body, + head=branch_name, base=self.MASTER_BRANCH, + draft=True, maintainer_can_modify=True + ) + click.echo(f'Created rule Pull Request {pull_request.html_url}') + + # Note: It is not possible to get the authenticated user using get_user() from a github action. + login = user if user else github.get_user().login + pull_request.add_to_assignees(login) + pull_request.add_to_labels(*labels) + click.echo(f'Pull request assigned to {login}') + return pull_request + + def reserve_rule_number(self) -> int: + '''Reserve an id on the id counter branch of the repository.''' + with self.checkout_branch(self.ID_COUNTER_BRANCH): + counter_file_path = Path(self.repository.working_dir, self.ID_COUNTER_FILENAME) + counter = int(counter_file_path.read_text()) + counter_file_path.write_text(str(counter + 1)) + + self.repository.index.add([str(counter_file_path)]) + self.repository.index.commit('Increment RSPEC ID counter') + + origin = self.repository.remote(name='origin') + origin.push() + + click.echo(f'Reserved Rule ID S{counter}') + return counter + + +def _build_github_repository_url(token: str, user: Optional[str]): + '''Builds the rspec repository url''' + repo = os.environ.get('GITHUB_REPOSITORY', 'SonarSource/rspec') + if user: + return f'https://{user}:{token}@github.com/{repo}.git' + else: + return f'https://{token}@github.com/{repo}.git' + + +def _get_url_and_config(token: str, user: Optional[str]): + url = _build_github_repository_url(token, user) + config = {} + if user: + config['user.name'] = user + config['user.email'] = f'{user}@users.noreply.github.com' + + return url, config + + +@contextmanager +def tmp_rspec_repo(token: str, user: Optional[str]): + '''Yield a temporary repository''' + url, config = _get_url_and_config(token, user) + with tempfile.TemporaryDirectory() as tmpdirname: + yield RspecRepo(url, tmpdirname, config) diff --git a/rspec-tools/rspec_tools/utils.py b/rspec-tools/rspec_tools/utils.py index 549e8ef642..3155314073 100644 --- a/rspec-tools/rspec_tools/utils.py +++ b/rspec-tools/rspec_tools/utils.py @@ -126,7 +126,7 @@ def parse_and_validate_language_list(languages): _validate_languages(lang_list) return lang_list -def validate_language(language): +def _validate_language(language): _validate_languages([language]) def get_labels_for_languages(lang_list): @@ -134,6 +134,7 @@ def get_labels_for_languages(lang_list): return list(set(labels)) def get_label_for_language(language: str) -> str: + _validate_language(language) return LANG_TO_LABEL[language] def resolve_rule(rule_id: str) -> int: @@ -146,12 +147,3 @@ def resolve_rule(rule_id: str) -> int: def load_json(file): with open(file) as json_file: return json.load(json_file) - -@contextlib.contextmanager -def pushd(new_dir): - previous_dir = os.getcwd() - os.chdir(new_dir) - try: - yield - finally: - os.chdir(previous_dir) diff --git a/rspec-tools/tests/conftest.py b/rspec-tools/tests/conftest.py index a2dbd1d331..e9946b1408 100644 --- a/rspec-tools/tests/conftest.py +++ b/rspec-tools/tests/conftest.py @@ -1,6 +1,11 @@ +import shutil +from contextlib import contextmanager from pathlib import Path +from unittest.mock import Mock, patch import pytest +from git import Head, Repo +from rspec_tools.repo import RspecRepo @pytest.fixture @@ -8,7 +13,69 @@ def mockrules(): '''Provides a path to test rules resources.''' return Path(__file__).parent.joinpath('resources', 'rules') + @pytest.fixture def mockinvalidrules(): '''Provides a path to test rules resources.''' return Path(__file__).parent.joinpath('resources', 'invalid-rules') + + +@pytest.fixture +def git_config(): + '''Create a mock git configuration.''' + return { + 'user.name': 'testuser', + 'user.email': 'testuser@mock.mock' + } + + +@pytest.fixture +def mock_git_rspec_repo(tmpdir, mockrules: Path): + repo_dir = tmpdir.mkdir("mock_rspec") + repo = Repo.init(str(repo_dir)) + repo.init() + rules_dir = repo_dir.join('rules') + shutil.copytree(mockrules, rules_dir) + + with repo.config_writer() as config_writer: + config_writer.set_value('user', 'name', 'originuser') + config_writer.set_value('user', 'email', 'originuser@mock.mock') + + repo.git.add('--all') + repo.index.commit('init rules') + + # Create the id counter branch. Note that it is an orphan branch. + repo.head.reference = Head(repo, f'refs/heads/{RspecRepo.ID_COUNTER_BRANCH}') + repo.git.reset('--hard') + counter_file = repo_dir.join(RspecRepo.ID_COUNTER_FILENAME) + counter_file.write('0') + repo.index.add([str(counter_file)]) + commit = repo.index.commit('init counter', parent_commits=None) + + # Checkout a specific commit so that the repo can be pushed to without + # making the index and work tree inconsistent. + repo.git.checkout(commit.hexsha) + + return repo + + +@pytest.fixture +def rspec_repo(tmpdir, mock_git_rspec_repo: Repo, git_config: dict[str, str]): + cloned_repo = tmpdir.mkdir("cloned_repo") + return RspecRepo(mock_git_rspec_repo.working_dir, str(cloned_repo), git_config) + + +@contextmanager +def mock_github(): + token = 'TOKEN' + user = 'testuser' + mock_repo = Mock() + mock_github = Mock() + mock_github.get_repo = Mock(return_value=mock_repo) + mock_github_api = Mock(return_value=mock_github) + mock_auto_github = Mock(return_value=mock_github_api) + with patch('rspec_tools.repo._auto_github', mock_auto_github): + yield (token, user, mock_repo) + mock_auto_github.assert_called_once_with(token) + mock_github_api.assert_called_once_with(user) + mock_github.get_repo.assert_called_once() diff --git a/rspec-tools/tests/test_cli.py b/rspec-tools/tests/test_cli.py index 52efc9f812..0d43ef8fd0 100644 --- a/rspec-tools/tests/test_cli.py +++ b/rspec-tools/tests/test_cli.py @@ -2,19 +2,18 @@ import os import re from pathlib import Path from typing import List -from unittest.mock import Mock, patch +from unittest.mock import patch from click.testing import CliRunner from rspec_tools.cli import cli from rspec_tools.rules import RulesRepository -import rspec_tools class TestCLIUpdateQuickfixStatus: '''Unit test for quickfix status update through Command Line Interface.''' @patch.dict(os.environ, {'GITHUB_TOKEN': 'TOKEN'}) - @patch('rspec_tools.create_rule.update_rule_quickfix_status') + @patch('rspec_tools.modify_rule.update_rule_quickfix_status') def test_basic_cli_usage(self, mock): arguments = [ 'update-quickfix-status', diff --git a/rspec-tools/tests/test_coverage.py b/rspec-tools/tests/test_coverage.py index 28d61c1db7..40912fffa6 100644 --- a/rspec-tools/tests/test_coverage.py +++ b/rspec-tools/tests/test_coverage.py @@ -1,10 +1,22 @@ -from git import Repo, Head -from unittest.mock import Mock, patch -import pytest -import json +import os +from contextlib import contextmanager +from unittest.mock import patch + +from rspec_tools.coverage import (update_coverage_for_all_repos, + update_coverage_for_repo, + update_coverage_for_repo_version) +from rspec_tools.utils import load_json + + +@contextmanager +def pushd(new_dir): + previous_dir = os.getcwd() + os.chdir(new_dir) + try: + yield + finally: + os.chdir(previous_dir) -from rspec_tools.coverage import update_coverage_for_repo_version, update_coverage_for_repo, update_coverage_for_all_repos -from rspec_tools.utils import load_json, pushd def test_update_coverage_for_repo_version(tmpdir): with pushd(tmpdir): @@ -36,6 +48,7 @@ def test_update_coverage_for_repo_version(tmpdir): update_coverage_for_repo_version(REPO, 'master') assert load_json(coverage)['JAVASCRIPT']['S100'] == REPO + ' ' + VER + def test_update_coverage_for_repo(tmpdir): with pushd(tmpdir): REPO = 'SonarJS' @@ -50,6 +63,7 @@ def test_update_coverage_for_repo(tmpdir): assert 'S1145' in cov['JAVASCRIPT'] assert cov['JAVASCRIPT']['S1145'] == {'since': REPO + ' 3.3.0.5702', 'until': REPO + ' 6.7.0.14237'} + @patch('rspec_tools.coverage.REPOS', ['SonarJS', 'sonar-xml']) def test_update_coverage_for_all_repos(tmpdir): with pushd(tmpdir): diff --git a/rspec-tools/tests/test_create_rule.py b/rspec-tools/tests/test_create_rule.py index 173d05f0ce..4319a55a75 100644 --- a/rspec-tools/tests/test_create_rule.py +++ b/rspec-tools/tests/test_create_rule.py @@ -1,215 +1,33 @@ -import json import os -import shutil from pathlib import Path -from typing import Optional from unittest.mock import Mock, patch import pytest -from git import Head, Repo +from git import Repo from rspec_tools.create_rule import (RuleCreator, add_language_to_rule, - create_new_rule, - update_rule_quickfix_status) + create_new_rule) from rspec_tools.errors import InvalidArgumentError +from rspec_tools.repo import RspecRepo from rspec_tools.utils import LANG_TO_SOURCE, is_empty_metadata +from tests.conftest import mock_github + @pytest.fixture -def git_config(): - '''Create a mock git configuration.''' - return { - 'user.name': 'testuser', - 'user.email': 'testuser@mock.mock' - } - -@pytest.fixture -def mock_rspec_repo(tmpdir, mockrules: Path): - repo_dir = tmpdir.mkdir("mock_rspec") - repo = Repo.init(str(repo_dir)) - repo.init() - rules_dir = repo_dir.join('rules') - shutil.copytree(mockrules, rules_dir) - - with repo.config_writer() as config_writer: - config_writer.set_value('user', 'name', 'originuser') - config_writer.set_value('user', 'email', 'originuser@mock.mock') - - repo.git.add('--all') - repo.index.commit('init rules') - - # Create the id counter branch. Note that it is an orphan branch. - repo.head.reference = Head(repo, f'refs/heads/{RuleCreator.ID_COUNTER_BRANCH}') - repo.git.reset('--hard') - counter_file = repo_dir.join(RuleCreator.ID_COUNTER_FILENAME) - counter_file.write('0') - repo.index.add([str(counter_file)]) - commit = repo.index.commit('init counter', parent_commits=None) - - # Checkout a specific commit so that the repo can be pushed to without - # making the index and work tree inconsistent. - repo.git.checkout(commit.hexsha) - - return repo - -@pytest.fixture -def rule_creator(tmpdir, mock_rspec_repo: Repo, git_config: dict[str, str]): - cloned_repo = tmpdir.mkdir("cloned_repo") - return RuleCreator(mock_rspec_repo.working_dir, str(cloned_repo), git_config) +def rule_creator(rspec_repo: RspecRepo): + return RuleCreator(rspec_repo) -def test_reserve_rule_number_simple(rule_creator: RuleCreator, mock_rspec_repo: Repo): - '''Test that RuleCreator.reserve_rule_id() increments the id and returns the old value.''' - assert rule_creator.reserve_rule_number() == 0 - - assert read_counter_file(mock_rspec_repo) == '1' - - -def test_reserve_rule_number_parallel_reservations(tmpdir, mock_rspec_repo: Repo, git_config): - '''Test that RuleCreator.reserve_rule_id() works when multiple reservations are done in parallel.''' - cloned_repo1 = tmpdir.mkdir("cloned_repo1") - rule_creator1 = RuleCreator(mock_rspec_repo.working_dir, str(cloned_repo1), git_config) - cloned_repo2 = tmpdir.mkdir("cloned_repo2") - rule_creator2 = RuleCreator(mock_rspec_repo.working_dir, str(cloned_repo2), git_config) - - assert rule_creator1.reserve_rule_number() == 0 - assert rule_creator2.reserve_rule_number() == 1 - assert rule_creator1.reserve_rule_number() == 2 - - assert read_counter_file(mock_rspec_repo) == '3' - - -def read_counter_file(repo): - '''Reads the counter file from the provided repository and returns its content.''' - repo.git.checkout(RuleCreator.ID_COUNTER_BRANCH) - counter_path = Path(repo.working_dir).joinpath(RuleCreator.ID_COUNTER_FILENAME) - return counter_path.read_text() - - -def test_update_quickfix_status_branch1(rule_creator: RuleCreator, mock_rspec_repo: Repo): - '''Test update_quickfix_status_branch when quickfix field is not present in language-specific metadata''' - rule_number = 100 - language = 'cfamily' - - sub_path = Path('rules', f'S{rule_number}', language, 'metadata.json') - metadata_path = Path(mock_rspec_repo.working_dir) / sub_path - mock_rspec_repo.git.checkout('master') - initial_metadata = json.loads(metadata_path.read_text()) - assert 'quickfix' not in initial_metadata # It is in the parent metadata.json file. - - branch = rule_creator.update_quickfix_status_branch('Some title', rule_number, language, 'covered') - mock_rspec_repo.git.checkout(branch) - new_metadata = json.loads(metadata_path.read_text()) - - # Verify that only the quickfix status is introduced. - assert len(initial_metadata.keys()) + 1 == len(new_metadata.keys()) - assert 'quickfix' in new_metadata - assert 'covered' == new_metadata['quickfix'] - for key in initial_metadata: - assert initial_metadata[key] == new_metadata[key] - - # Ensure only one file is modified. - modified_files = mock_rspec_repo.git.diff('master', '--name-only').strip() - assert modified_files == str(sub_path) - - -def test_update_quickfix_status_branch2(rule_creator: RuleCreator, mock_rspec_repo: Repo): - '''Test update_quickfix_status_branch when quickfix field is already present in language-specific metadata''' - rule_number = 100 - language = 'java' - - sub_path = Path('rules', f'S{rule_number}', language, 'metadata.json') - metadata_path = Path(mock_rspec_repo.working_dir) / sub_path - mock_rspec_repo.git.checkout('master') - initial_metadata = json.loads(metadata_path.read_text()) - assert 'quickfix' in initial_metadata - assert initial_metadata['quickfix'] == 'targeted' - - branch = rule_creator.update_quickfix_status_branch('Some title', rule_number, language, 'covered') - mock_rspec_repo.git.checkout(branch) - new_metadata = json.loads(metadata_path.read_text()) - - # Verify that only the quickfix status is updated. - assert len(initial_metadata.keys()) == len(new_metadata.keys()) - assert 'quickfix' in new_metadata - assert 'covered' == new_metadata['quickfix'] - for key in initial_metadata: - if key != 'quickfix': - assert initial_metadata[key] == new_metadata[key] - - # Ensure only one file is modified. - modified_files = mock_rspec_repo.git.diff('master', '--name-only').strip() - assert modified_files == str(sub_path) - - -def test_update_quickfix_status_branch3(rule_creator: RuleCreator, mock_rspec_repo: Repo): - '''Test update_quickfix_status_branch when new and old quickfix status are the same''' - rule_number = 100 - language = 'java' - - metadata_path = Path(mock_rspec_repo.working_dir, 'rules', f'S{rule_number}', language, 'metadata.json') - mock_rspec_repo.git.checkout('master') - initial_metadata = json.loads(metadata_path.read_text()) - assert 'quickfix' in initial_metadata - assert initial_metadata['quickfix'] == 'targeted' - - with pytest.raises(InvalidArgumentError): - rule_creator.update_quickfix_status_branch('Some title', rule_number, language, 'targeted') - - -def test_update_quickfix_status_branch4(rule_creator: RuleCreator, mock_rspec_repo: Repo): - '''Test update_quickfix_status_branch when the rule does not exist''' - rule_number = 404 - language = 'java' - - metadata_path = Path(mock_rspec_repo.working_dir, 'rules', f'S{rule_number}', language, 'metadata.json') - mock_rspec_repo.git.checkout('master') - assert not metadata_path.exists() - - with pytest.raises(InvalidArgumentError): - rule_creator.update_quickfix_status_branch('Some title', rule_number, language, 'targeted') - - -def test_update_quickfix_status_pull_request(rule_creator: RuleCreator): - '''Test update_quickfix_status_pull_request adds the right user and label.''' - ghMock = Mock() - ghRepoMock = Mock() - pullMock = Mock() - ghRepoMock.create_pull = Mock(return_value=pullMock) - ghMock.get_repo = Mock(return_value=ghRepoMock) - def mockGithub(user: Optional[str]): - return ghMock - - rule_creator.update_quickfix_status_pull_request(mockGithub, 100, 'cfamily', 'covered', 'label-fraicheur', 'testuser') - ghRepoMock.create_pull.assert_called_once(); - assert ghRepoMock.create_pull.call_args.kwargs['title'].startswith('Modify rule S100') - pullMock.add_to_assignees.assert_called_with('testuser') - pullMock.add_to_labels.assert_called_with('label-fraicheur') - - -@patch('rspec_tools.create_rule.RuleCreator') -def test_update_rule_quickfix_status(mockRuleCreator): - '''Test update_rule_quickfix_status uses the expected implementation.''' - mockRuleCreator.return_value = Mock() - mockRuleCreator.return_value.update_quickfix_status_pull_request = Mock() - prMock = mockRuleCreator.return_value.update_quickfix_status_pull_request - update_rule_quickfix_status('cfamily', 'S100', 'covered', 'my token', 'testuser') - prMock.assert_called_once() - assert prMock.call_args.args[1] == 100 - assert prMock.call_args.args[2] == 'cfamily' - assert prMock.call_args.args[3] == 'covered' - assert prMock.call_args.args[4] == 'cfamily' - - -def test_create_new_multi_lang_rule_branch(rule_creator: RuleCreator, mock_rspec_repo: Repo): +def test_create_new_multi_lang_rule_branch(rule_creator: RuleCreator, mock_git_rspec_repo: Repo): '''Test create_new_rule_branch for a multi-language rule.''' - rule_number = rule_creator.reserve_rule_number() + rule_number = rule_creator.rspec_repo.reserve_rule_number() languages = ['java', 'javascript'] branch = rule_creator.create_new_rule_branch(rule_number, languages) # Check that the branch was pushed successfully to the origin - mock_rspec_repo.git.checkout(branch) - rule_dir = Path(mock_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') + mock_git_rspec_repo.git.checkout(branch) + rule_dir = Path(mock_git_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') assert rule_dir.exists() common_root = rule_creator.TEMPLATE_PATH.joinpath('multi_language', 'common') @@ -230,16 +48,17 @@ def test_create_new_multi_lang_rule_branch(rule_creator: RuleCreator, mock_rspec actual_content = rule_dir.joinpath(lang, relative_path).read_text() assert actual_content == expected_content -def test_create_new_single_lang_rule_branch(rule_creator: RuleCreator, mock_rspec_repo: Repo): + +def test_create_new_single_lang_rule_branch(rule_creator: RuleCreator, mock_git_rspec_repo: Repo): '''Test create_new_rule_branch for a single-language rule.''' - rule_number = rule_creator.reserve_rule_number() + rule_number = rule_creator.rspec_repo.reserve_rule_number() languages = ['cfamily'] branch = rule_creator.create_new_rule_branch(rule_number, languages) # Check that the branch was pushed successfully to the origin - mock_rspec_repo.git.checkout(branch) - rule_dir = Path(mock_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') + mock_git_rspec_repo.git.checkout(branch) + rule_dir = Path(mock_git_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') assert rule_dir.exists() common_root = rule_creator.TEMPLATE_PATH.joinpath('single_language', 'common') @@ -261,52 +80,43 @@ def test_create_new_single_lang_rule_branch(rule_creator: RuleCreator, mock_rspe actual_content = rule_dir.joinpath(lang, relative_path).read_text() assert actual_content == expected_content -def test_create_new_rule_pr(rule_creator: RuleCreator): + +def test_create_new_rule_pull_request(rule_creator: RuleCreator): '''Test create_new_rule_branch adds the right user and labels.''' - rule_number = rule_creator.reserve_rule_number() + rule_number = rule_creator.rspec_repo.reserve_rule_number() languages = ['cfamily'] - ghMock = Mock() - ghRepoMock = Mock() - pullMock = Mock() - ghRepoMock.create_pull = Mock(return_value=pullMock) - ghMock.get_repo = Mock(return_value=ghRepoMock) - def mockGithub(user: Optional[str]): - return ghMock + with mock_github() as (token, user, mock_repo): + rule_creator.create_new_rule_pull_request(token, rule_number, languages, ['mylab', 'other-lab'], user) - rule_creator.create_new_rule_pull_request(mockGithub, rule_number, languages, ['mylab', 'other-lab'], user='testuser') + mock_repo.create_pull.assert_called_once(); + assert mock_repo.create_pull.call_args.kwargs['title'].startswith('Create rule S') + mock_repo.create_pull.return_value.add_to_assignees.assert_called_with(user); + mock_repo.create_pull.return_value.add_to_labels.assert_called_with('mylab', 'other-lab'); - ghRepoMock.create_pull.assert_called_once(); - assert ghRepoMock.create_pull.call_args.kwargs['title'].startswith('Create rule S') - pullMock.add_to_assignees.assert_called_with('testuser'); - pullMock.add_to_labels.assert_called_with('mylab', 'other-lab'); @patch('rspec_tools.create_rule.RuleCreator') def test_create_new_rule(mockRuleCreator): - mockRuleCreator.return_value = Mock() - mockRuleCreator.return_value.create_new_rule_pull_request = Mock() prMock = mockRuleCreator.return_value.create_new_rule_pull_request create_new_rule('cfamily,php', 'my token', 'testuser') prMock.assert_called_once() assert set(prMock.call_args.args[2]) == set(['cfamily', 'php']) assert set(prMock.call_args.args[3]) == set(['cfamily', 'php']) + @patch('rspec_tools.create_rule.RuleCreator') def test_create_new_rule_unsupported_language(mockRuleCreator): - mockRuleCreator.return_value = Mock() - mockRuleCreator.return_value.create_new_rule_pull_request = Mock() - prMock = mockRuleCreator.return_value.create_new_rule_pull_request with pytest.raises(InvalidArgumentError): create_new_rule('russian,php', 'my token', 'testuser') -def test_add_lang_singlelang_nonconventional_rule_create_branch(rule_creator: RuleCreator, mock_rspec_repo: Repo): +def test_add_lang_singlelang_nonconventional_rule_create_branch(rule_creator: RuleCreator, mock_git_rspec_repo: Repo): '''Test add_language_branch for a single-language rule with metadata lifted to the generic rule level.''' rule_number = 4727 language = 'php' - mock_rspec_repo.git.checkout('master') - orig_rule_dir = Path(mock_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') + mock_git_rspec_repo.git.checkout('master') + orig_rule_dir = Path(mock_git_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') assert(not is_empty_metadata(orig_rule_dir)) # nonconventional: singlelang rule with metadata on the upper level assert(is_empty_metadata(orig_rule_dir.joinpath('cobol'))) original_metadata = orig_rule_dir.joinpath('metadata.json').read_text() @@ -314,8 +124,8 @@ def test_add_lang_singlelang_nonconventional_rule_create_branch(rule_creator: Ru branch = rule_creator.add_language_branch(rule_number, language) # Check that the branch was pushed successfully to the origin - mock_rspec_repo.git.checkout(branch) - rule_dir = Path(mock_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') + mock_git_rspec_repo.git.checkout(branch) + rule_dir = Path(mock_git_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') assert rule_dir.exists() lang_dir = rule_dir.joinpath(f'{language}') assert lang_dir.exists() @@ -332,13 +142,14 @@ def test_add_lang_singlelang_nonconventional_rule_create_branch(rule_creator: Ru actual_content = rule_dir.joinpath(language, relative_path).read_text() assert actual_content == expected_content -def test_add_lang_singlelang_conventional_rule_create_branch(rule_creator: RuleCreator, mock_rspec_repo: Repo): + +def test_add_lang_singlelang_conventional_rule_create_branch(rule_creator: RuleCreator, mock_git_rspec_repo: Repo): '''Test add_language_branch for a regular single language rule.''' rule_number = 1033 language = 'php' - mock_rspec_repo.git.checkout('master') - orig_rule_dir = Path(mock_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') + mock_git_rspec_repo.git.checkout('master') + orig_rule_dir = Path(mock_git_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') assert(is_empty_metadata(orig_rule_dir)) # conventional: singlelang rule with metadata on the lang-specific level assert(not is_empty_metadata(orig_rule_dir.joinpath('cfamily'))) original_lmetadata = orig_rule_dir.joinpath('cfamily', 'metadata.json').read_text() @@ -346,8 +157,8 @@ def test_add_lang_singlelang_conventional_rule_create_branch(rule_creator: RuleC branch = rule_creator.add_language_branch(rule_number, language) # Check that the branch was pushed successfully to the origin - mock_rspec_repo.git.checkout(branch) - rule_dir = Path(mock_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') + mock_git_rspec_repo.git.checkout(branch) + rule_dir = Path(mock_git_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') assert rule_dir.exists() lang_dir = rule_dir.joinpath(f'{language}') assert lang_dir.exists() @@ -355,7 +166,8 @@ def test_add_lang_singlelang_conventional_rule_create_branch(rule_creator: RuleC assert rule_dir.joinpath('metadata.json').read_text() == original_lmetadata assert(is_empty_metadata(rule_dir.joinpath('cfamily'))) -def test_add_lang_multilang_rule_create_branch(rule_creator: RuleCreator, mock_rspec_repo: Repo): + +def test_add_lang_multilang_rule_create_branch(rule_creator: RuleCreator, mock_git_rspec_repo: Repo): '''Test add_language_branch for a multi-language rule.''' rule_number = 120 language = 'php' @@ -363,8 +175,8 @@ def test_add_lang_multilang_rule_create_branch(rule_creator: RuleCreator, mock_r branch = rule_creator.add_language_branch(rule_number, language) # Check that the branch was pushed successfully to the origin - mock_rspec_repo.git.checkout(branch) - rule_dir = Path(mock_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') + mock_git_rspec_repo.git.checkout(branch) + rule_dir = Path(mock_git_rspec_repo.working_dir).joinpath('rules', f'S{rule_number}') assert rule_dir.exists() lang_dir = rule_dir.joinpath(f'{language}') assert lang_dir.exists() @@ -378,42 +190,44 @@ def test_add_lang_multilang_rule_create_branch(rule_creator: RuleCreator, mock_r actual_content = rule_dir.joinpath(language, relative_path).read_text() assert actual_content == expected_content + @patch('rspec_tools.create_rule.RuleCreator') -def test_add_unsupported_language(mockRuleCreator): +def test_add_unsupported_language(mock): '''Test language validation.''' - mockRuleCreator.return_value = Mock() - mockRuleCreator.return_value.create_new_rule_pull_request = Mock() - prMock = mockRuleCreator.return_value.create_new_rule_pull_request with pytest.raises(InvalidArgumentError): add_language_to_rule('russian', 'S1033', 'my token', 'testuser') + mock.return_value.add_language_pull_request.assert_not_called() + + +@patch('rspec_tools.create_rule.RuleCreator') +def test_add_supported_language(mock): + '''Test language validation.''' + add_language_to_rule('cfamily', 'S1033', 'my token', 'testuser') + mock.return_value.add_language_pull_request.assert_called_once() + def test_add_language_the_rule_is_already_defined_for(rule_creator: RuleCreator): '''Test add_language_branch fails when trying to add a langage already added to the rule.''' with pytest.raises(InvalidArgumentError): rule_creator.add_language_branch(100, 'cfamily') + def test_add_language_to_nonexistent_rule(rule_creator: RuleCreator): '''Test add_language_branch correctly fails when invoked for a non-existent rule.''' with pytest.raises(InvalidArgumentError): rule_creator.add_language_branch(101, 'cfamily') -def test_add_language_new_pr(rule_creator: RuleCreator): + +def test_add_language_new_pull_request(rule_creator: RuleCreator): '''Test add_language_pull_request adds the right user and labels.''' rule_number = 120 language = 'php' - ghMock = Mock() - ghRepoMock = Mock() - pullMock = Mock() - ghRepoMock.create_pull = Mock(return_value=pullMock) - ghMock.get_repo = Mock(return_value=ghRepoMock) - def mockGithub(user: Optional[str]): - return ghMock + with mock_github() as (token, user, mock_repo): + rule_creator.add_language_pull_request(token, rule_number, language, 'mylab', user) - rule_creator.add_language_pull_request(mockGithub, rule_number, language, 'mylab', user='testuser') - - ghRepoMock.create_pull.assert_called_once(); - assert ghRepoMock.create_pull.call_args.kwargs['title'].startswith(f'Create rule S{rule_number}') - ghRepoMock.create_pull.call_args.kwargs['head'].startswith('rule/') - pullMock.add_to_assignees.assert_called_with('testuser'); - pullMock.add_to_labels.assert_called_with('mylab'); + mock_repo.create_pull.assert_called_once(); + assert mock_repo.create_pull.call_args.kwargs['title'].startswith(f'Create rule S{rule_number}') + assert mock_repo.create_pull.call_args.kwargs['head'].startswith('rule/') + mock_repo.create_pull.return_value.add_to_assignees.assert_called_with(user) + mock_repo.create_pull.return_value.add_to_labels.assert_called_with('mylab') diff --git a/rspec-tools/tests/test_modify_rule.py b/rspec-tools/tests/test_modify_rule.py new file mode 100644 index 0000000000..ebb974412a --- /dev/null +++ b/rspec-tools/tests/test_modify_rule.py @@ -0,0 +1,122 @@ +import json +from pathlib import Path +from unittest.mock import Mock, patch + +import pytest +from git import Repo +from rspec_tools.errors import InvalidArgumentError +from rspec_tools.modify_rule import RuleEditor, update_rule_quickfix_status +from rspec_tools.repo import RspecRepo + +from tests.conftest import mock_github + + +@pytest.fixture +def rule_editor(rspec_repo: RspecRepo): + return RuleEditor(rspec_repo) + + +def test_update_quickfix_status_branch1(rule_editor: RuleEditor, mock_git_rspec_repo: Repo): + '''Test update_quickfix_status_branch when quickfix field is not present in language-specific metadata''' + rule_number = 100 + language = 'cfamily' + + sub_path = Path('rules', f'S{rule_number}', language, 'metadata.json') + metadata_path = Path(mock_git_rspec_repo.working_dir) / sub_path + mock_git_rspec_repo.git.checkout('master') + initial_metadata = json.loads(metadata_path.read_text()) + assert 'quickfix' not in initial_metadata # It is in the parent metadata.json file. + + branch = rule_editor.update_quickfix_status_branch('Some title', rule_number, language, 'covered') + mock_git_rspec_repo.git.checkout(branch) + new_metadata = json.loads(metadata_path.read_text()) + + # Verify that only the quickfix status is introduced. + assert len(initial_metadata.keys()) + 1 == len(new_metadata.keys()) + assert 'quickfix' in new_metadata + assert 'covered' == new_metadata['quickfix'] + for key in initial_metadata: + assert initial_metadata[key] == new_metadata[key] + + # Ensure only one file is modified. + modified_files = mock_git_rspec_repo.git.diff('master', '--name-only').strip() + assert modified_files == str(sub_path) + + +def test_update_quickfix_status_branch2(rule_editor: RuleEditor, mock_git_rspec_repo: Repo): + '''Test update_quickfix_status_branch when quickfix field is already present in language-specific metadata''' + rule_number = 100 + language = 'java' + + sub_path = Path('rules', f'S{rule_number}', language, 'metadata.json') + metadata_path = Path(mock_git_rspec_repo.working_dir) / sub_path + mock_git_rspec_repo.git.checkout('master') + initial_metadata = json.loads(metadata_path.read_text()) + assert 'quickfix' in initial_metadata + assert initial_metadata['quickfix'] == 'targeted' + + branch = rule_editor.update_quickfix_status_branch('Some title', rule_number, language, 'covered') + mock_git_rspec_repo.git.checkout(branch) + new_metadata = json.loads(metadata_path.read_text()) + + # Verify that only the quickfix status is updated. + assert len(initial_metadata.keys()) == len(new_metadata.keys()) + assert 'quickfix' in new_metadata + assert 'covered' == new_metadata['quickfix'] + for key in initial_metadata: + if key != 'quickfix': + assert initial_metadata[key] == new_metadata[key] + + # Ensure only one file is modified. + modified_files = mock_git_rspec_repo.git.diff('master', '--name-only').strip() + assert modified_files == str(sub_path) + + +def test_update_quickfix_status_branch3(rule_editor: RuleEditor, mock_git_rspec_repo: Repo): + '''Test update_quickfix_status_branch when new and old quickfix status are the same''' + rule_number = 100 + language = 'java' + + metadata_path = Path(mock_git_rspec_repo.working_dir, 'rules', f'S{rule_number}', language, 'metadata.json') + mock_git_rspec_repo.git.checkout('master') + initial_metadata = json.loads(metadata_path.read_text()) + assert 'quickfix' in initial_metadata + assert initial_metadata['quickfix'] == 'targeted' + + with pytest.raises(InvalidArgumentError): + rule_editor.update_quickfix_status_branch('Some title', rule_number, language, 'targeted') + + +def test_update_quickfix_status_branch4(rule_editor: RuleEditor, mock_git_rspec_repo: Repo): + '''Test update_quickfix_status_branch when the rule does not exist''' + rule_number = 404 + language = 'java' + + metadata_path = Path(mock_git_rspec_repo.working_dir, 'rules', f'S{rule_number}', language, 'metadata.json') + mock_git_rspec_repo.git.checkout('master') + assert not metadata_path.exists() + + with pytest.raises(InvalidArgumentError): + rule_editor.update_quickfix_status_branch('Some title', rule_number, language, 'targeted') + + +def test_update_quickfix_status_pull_request(rule_editor: RuleEditor): + '''Test update_quickfix_status_pull_request adds the right user and label.''' + with mock_github() as (token, user, mock_repo): + rule_editor.update_quickfix_status_pull_request(token, 100, 'cfamily', 'covered', 'label-fraicheur', user) + mock_repo.create_pull.assert_called_once(); + assert mock_repo.create_pull.call_args.kwargs['title'].startswith('Modify rule S100') + mock_repo.create_pull.return_value.add_to_assignees.assert_called_with(user) + mock_repo.create_pull.return_value.add_to_labels.assert_called_with('label-fraicheur') + + +@patch('rspec_tools.modify_rule.RuleEditor') +def test_update_rule_quickfix_status(mockRuleEditor): + '''Test update_rule_quickfix_status uses the expected implementation.''' + prMock = mockRuleEditor.return_value.update_quickfix_status_pull_request + update_rule_quickfix_status('cfamily', 'S100', 'covered', 'my token', 'testuser') + prMock.assert_called_once() + assert prMock.call_args.args[1] == 100 + assert prMock.call_args.args[2] == 'cfamily' + assert prMock.call_args.args[3] == 'covered' + assert prMock.call_args.args[4] == 'cfamily' diff --git a/rspec-tools/tests/test_repo.py b/rspec-tools/tests/test_repo.py new file mode 100644 index 0000000000..5ed9c168b8 --- /dev/null +++ b/rspec-tools/tests/test_repo.py @@ -0,0 +1,33 @@ +from pathlib import Path + +import pytest +from git import Repo +from rspec_tools.repo import RspecRepo + + +def _read_counter_file(repo: Repo): + '''Reads the counter file from the provided repository and returns its content.''' + repo.git.checkout(RspecRepo.ID_COUNTER_BRANCH) + counter_path = Path(repo.working_dir, RspecRepo.ID_COUNTER_FILENAME) + return counter_path.read_text() + + +def test_reserve_rule_number_simple(rspec_repo: RspecRepo, mock_git_rspec_repo: Repo): + '''Test that RspecRepo.reserve_rule_number() increments the id and returns the old value.''' + assert rspec_repo.reserve_rule_number() == 0 + + assert _read_counter_file(mock_git_rspec_repo) == '1' + + +def test_reserve_rule_number_parallel_reservations(tmpdir, mock_git_rspec_repo: Repo, git_config): + '''Test that RspecRepo.reserve_rule_number() works when multiple reservations are done in parallel.''' + cloned_repo1 = tmpdir.mkdir("cloned_repo1") + rule_creator1 = RspecRepo(mock_git_rspec_repo.working_dir, str(cloned_repo1), git_config) + cloned_repo2 = tmpdir.mkdir("cloned_repo2") + rule_creator2 = RspecRepo(mock_git_rspec_repo.working_dir, str(cloned_repo2), git_config) + + assert rule_creator1.reserve_rule_number() == 0 + assert rule_creator2.reserve_rule_number() == 1 + assert rule_creator1.reserve_rule_number() == 2 + + assert _read_counter_file(mock_git_rspec_repo) == '3' diff --git a/rspec-tools/tests/test_rules_repository.py b/rspec-tools/tests/test_rules_repository.py index 16684fc2c7..0d675b5dd0 100644 --- a/rspec-tools/tests/test_rules_repository.py +++ b/rspec-tools/tests/test_rules_repository.py @@ -1,8 +1,5 @@ from pathlib import Path -import pytest -from pytest import TempPathFactory - from rspec_tools.rules import RulesRepository def test_list_rules(mockrules: Path): diff --git a/rspec-tools/tests/test_utils.py b/rspec-tools/tests/test_utils.py index 383866615f..01f4976953 100644 --- a/rspec-tools/tests/test_utils.py +++ b/rspec-tools/tests/test_utils.py @@ -1,6 +1,10 @@ -from rspec_tools.errors import InvalidArgumentError -from rspec_tools.utils import parse_and_validate_language_list, load_valid_languages, get_mapped_languages, get_labels_for_languages, resolve_rule, validate_language, get_label_for_language import pytest +from rspec_tools.errors import InvalidArgumentError +from rspec_tools.utils import (get_label_for_language, + get_labels_for_languages, get_mapped_languages, + load_valid_languages, + parse_and_validate_language_list, resolve_rule) + def test_fails_when_no_languages_listed(): '''Test that language validation fails on empty list.''' @@ -52,8 +56,5 @@ def test_resolve_rule(): def test_label_for_language(): assert get_label_for_language('java') == 'java' - -def test_validate_language(): - validate_language('java') with pytest.raises(InvalidArgumentError): - validate_language('russian') + get_label_for_language('russian')