From 69bfd6d9c8dd0c1e5453ac629aedca2baf6e5662 Mon Sep 17 00:00:00 2001 From: David Andersson <51036209+jdkandersson@users.noreply.github.com> Date: Tue, 4 Apr 2023 22:32:12 +1000 Subject: [PATCH] Community merge conflicts (#113) * add function that retrieves the file contents * add tests for retrieving content of a file * remove unnecessary None alternative * make repository client available to reconcile * add content from default branch to content change * add merge conflict module * add tests for content module * add module that checks actions * add check for any content conflicts * add reconcile test with merge conflicts * add content merging * separate action tests into multiple modules * add github token to e2e tests * move requirements to requirements.txt files * add support for the case where the file was not found on the branch * add tests for no base * add support for retrieving the file from a particular branch * add base branch input * add tests for that file is retrieved from the correct branch * add integration test for merge conflict * update docs * aggregate clients into a tuple * add update e2e test * move writing GitGHub output to separate module --- .github/workflows/e2e_test.yaml | 33 +- .github/workflows/integration_test.yaml | 2 +- .github/workflows/test.yaml | 2 +- README.md | 29 +- action.yaml | 6 + dev-requirements.txt | 4 + main.py | 2 + prepare_check_cleanup/migration.py | 40 +- prepare_check_cleanup/output.py | 22 + prepare_check_cleanup/reconcile.py | 142 +++++- pyproject.toml | 2 +- src/__init__.py | 90 ++-- src/action.py | 140 ++++-- src/check.py | 102 ++++ src/constants.py | 17 + src/content.py | 112 +++++ src/docs_directory.py | 2 +- src/exceptions.py | 8 + src/index.py | 9 +- src/pull_request.py | 182 +------ src/reconcile.py | 110 ++++- src/repository.py | 224 +++++++++ src/types_.py | 27 +- tests/conftest.py | 25 +- tests/factories.py | 154 +++++- .../integration/test___init__run_conflict.py | 224 +++++++++ tests/integration/test___init__run_migrate.py | 24 +- .../integration/test___init__run_reconcile.py | 96 ++-- tests/integration/test_discourse.py | 2 + tests/unit/action/__init__.py | 4 + .../test_other_actions.py} | 259 +--------- tests/unit/action/test_update_action.py | 357 ++++++++++++++ tests/unit/conftest.py | 14 +- tests/unit/test___init__.py | 142 +++--- tests/unit/test_check.py | 204 ++++++++ tests/unit/test_content.py | 136 ++++++ tests/unit/test_docs_directory.py | 5 +- tests/unit/test_index.py | 10 +- tests/unit/test_migration.py | 49 +- tests/unit/test_pull_request.py | 349 +------------ tests/unit/test_reconcile.py | 256 +++++++--- tests/unit/test_repository.py | 460 ++++++++++++++++++ tox.ini | 11 +- 43 files changed, 2895 insertions(+), 1193 deletions(-) create mode 100644 prepare_check_cleanup/output.py create mode 100644 src/check.py create mode 100644 src/constants.py create mode 100644 src/content.py create mode 100644 src/repository.py create mode 100644 tests/integration/test___init__run_conflict.py create mode 100644 tests/unit/action/__init__.py rename tests/unit/{test_action.py => action/test_other_actions.py} (72%) create mode 100644 tests/unit/action/test_update_action.py create mode 100644 tests/unit/test_check.py create mode 100644 tests/unit/test_content.py create mode 100644 tests/unit/test_repository.py diff --git a/.github/workflows/e2e_test.yaml b/.github/workflows/e2e_test.yaml index 0efe9943..d749f5d4 100644 --- a/.github/workflows/e2e_test.yaml +++ b/.github/workflows/e2e_test.yaml @@ -5,6 +5,7 @@ on: jobs: e2e-tests-reconcile: + permissions: write-all runs-on: ubuntu-22.04 steps: # Each job has to have this configuration because secrets can be passed through the output of @@ -56,6 +57,7 @@ jobs: discourse_host: discourse.charmhub.io discourse_api_username: ${{ secrets.DISCOURSE_API_USERNAME }} discourse_api_key: ${{ secrets.DISCOURSE_API_KEY }} + github_token: ${{ secrets.GITHUB_TOKEN }} dry_run: true - name: Show pages run: echo '${{ steps.e2e-test-draft.outputs.urls_with_actions }}' @@ -71,6 +73,7 @@ jobs: discourse_host: discourse.charmhub.io discourse_api_username: ${{ secrets.DISCOURSE_API_USERNAME }} discourse_api_key: ${{ secrets.DISCOURSE_API_KEY }} + github_token: ${{ secrets.GITHUB_TOKEN }} - name: Show pages run: echo '${{ steps.e2e-test-create.outputs.urls_with_actions }}' - name: Check create @@ -80,6 +83,28 @@ jobs: run: | echo "docs: ${{ steps.e2e-test-create.outputs.index_url }}" >> metadata.yaml cat metadata.yaml + - name: Prepare for update test + id: e2e-test-prepare-update + run: | + # Update the contents of one of the files, do it first to get around content conflicts + echo -n more content >> docs/doc.md + GITHUB_TOKEN='${{ secrets.GITHUB_TOKEN }}' + REPO='${{ github.repository }}' + PYTHONPATH=$(pwd) python3 prepare_check_cleanup/reconcile.py --action prepare-update --action-kwargs "{\"github_token\": \"$GITHUB_TOKEN\", \"repo\": \"$REPO\", \"filename\": \"docs/doc.md\"}" '${{ steps.e2e-test-create.outputs.urls_with_actions }}' '${{ steps.configuration.outputs.discourse }}' + - name: Update self test + id: e2e-test-update + uses: ./ + with: + discourse_host: discourse.charmhub.io + discourse_api_username: ${{ secrets.DISCOURSE_API_USERNAME }} + discourse_api_key: ${{ secrets.DISCOURSE_API_KEY }} + github_token: ${{ secrets.GITHUB_TOKEN }} + base_branch: ${{ steps.e2e-test-prepare-update.outputs.update_branch }} + - name: Show pages + run: echo '${{ steps.e2e-test-update.outputs.urls_with_actions }}' + - name: Check update + run: | + PYTHONPATH=$(pwd) python3 prepare_check_cleanup/reconcile.py --action check-update --action-kwargs '{"expected_url_results": ["success", "success", "success"]}' '${{ steps.e2e-test-create.outputs.urls_with_actions }}' '${{ steps.configuration.outputs.discourse }}' - name: Delete the alternate doc with delete_topics disabled run: rm docs/alternate_doc.md - name: Delete topics disabled self test @@ -90,6 +115,8 @@ jobs: discourse_host: discourse.charmhub.io discourse_api_username: ${{ secrets.DISCOURSE_API_USERNAME }} discourse_api_key: ${{ secrets.DISCOURSE_API_KEY }} + github_token: ${{ secrets.GITHUB_TOKEN }} + base_branch: ${{ steps.e2e-test-prepare-update.outputs.update_branch }} - name: Show pages run: echo '${{ steps.e2e-test-delete-topics.outputs.urls_with_actions }}' - name: Check delete topics disabled @@ -104,6 +131,8 @@ jobs: discourse_host: discourse.charmhub.io discourse_api_username: ${{ secrets.DISCOURSE_API_USERNAME }} discourse_api_key: ${{ secrets.DISCOURSE_API_KEY }} + github_token: ${{ secrets.GITHUB_TOKEN }} + base_branch: ${{ steps.e2e-test-prepare-update.outputs.update_branch }} - name: Show pages run: echo '${{ steps.e2e-test-delete.outputs.urls_with_actions }}' - name: Check delete topics enabled @@ -112,7 +141,9 @@ jobs: - name: Clean up if: always() run: | - PYTHONPATH=$(pwd) python3 prepare_check_cleanup/reconcile.py --action cleanup '${{ steps.e2e-test-create.outputs.urls_with_actions }}' '${{ steps.configuration.outputs.discourse }}' + GITHUB_TOKEN='${{ secrets.GITHUB_TOKEN }}' + REPO='${{ github.repository }}' + PYTHONPATH=$(pwd) python3 prepare_check_cleanup/reconcile.py --action cleanup --action-kwargs "{\"github_token\": \"$GITHUB_TOKEN\", \"repo\": \"$REPO\"}" '${{ steps.e2e-test-create.outputs.urls_with_actions }}' '${{ steps.configuration.outputs.discourse }}' e2e-tests-migration: permissions: write-all runs-on: ubuntu-22.04 diff --git a/.github/workflows/integration_test.yaml b/.github/workflows/integration_test.yaml index 5ccafd56..de14c040 100644 --- a/.github/workflows/integration_test.yaml +++ b/.github/workflows/integration_test.yaml @@ -8,4 +8,4 @@ jobs: uses: canonical/operator-workflows/.github/workflows/integration_test.yaml@main secrets: inherit with: - modules: '["discourse", "reconcile", "migrate"]' + modules: '["discourse", "reconcile", "conflict", "migrate"]' diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 77e060bc..3f914eed 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -18,5 +18,5 @@ jobs: - name: Check for any code improvements run: | PYTHON_FILES=$(find . -name '*.py') - pyupgrade --py310-plus $PYTHON_FILES + pyupgrade --py311-plus $PYTHON_FILES refurb $PYTHON_FILES diff --git a/README.md b/README.md index 28d58de3..12831e5b 100644 --- a/README.md +++ b/README.md @@ -55,6 +55,7 @@ charmhub. discourse_host: discourse.charmhub.io discourse_api_username: ${{ secrets.DISCOURSE_API_USERNAME }} discourse_api_key: ${{ secrets.DISCOURSE_API_KEY }} + github_token: ${{ secrets.GITHUB_TOKEN }} - name: Show index page run: echo '${{ steps.publishDocumentation.outputs.index_url }}' ``` @@ -101,12 +102,12 @@ charmhub. github_token: ${{ secrets.GITHUB_TOKEN }} ``` - a branch name with `upload-charm-docs/migrate` will be created and a pull - request named `[upload-charm-docs] Migrate charm docs` will be created - towards the working branch the workflow was triggered with. - In order to ensure that the branches can be created successfully, please - make sure that there are no existing branches clashing with the name above. - Please note that `dry_run` parameter has no effect on migrate mode. + a branch name with `upload-charm-docs/migrate` will be created and a pull + request named `[upload-charm-docs] Migrate charm docs` will be created + targeting the default branch of the repository. In order to ensure that the + branches can be created successfully, please make sure that there are no + existing branches clashing with the name above. Please note that the + `dry_run` input has no effect on migrate mode. The action will now compare the discourse topics with the files and directories under the `docs` directory and make any changes based on differences. @@ -117,3 +118,19 @@ Additional recommended steps: publish a new version of the charm and documentation. * Add the action in dry run mode on publishes to `edge` to see what changes to the documentation will be made once you publish to `stable`. + +## Discourse Documentation Edits + +To ensure that contributions to the documentation on discourse are not +overridden, the action compares the content that was last pushed to discourse +with the current documentation on discourse and any proposed changes. If there +are changes both on discourse and in the repository, the action will attempt to +merge the content using essentially `git merge`. If that fails, the action will +prompt you to resolve those conflicts by editing the documentation on discourse +and on the repository. Be sure to explain the reasoning for the changes on +discourse. + +Note that `git merge` will not make any changes on your branch. The content that +was last pushed to discourse is determined by getting the content from a given +file from the default branch of the repository. A different branch can be +configured using the `base_branch` input. diff --git a/action.yaml b/action.yaml index 851548e2..7d9a561b 100644 --- a/action.yaml +++ b/action.yaml @@ -40,6 +40,12 @@ inputs: Required if running in migration mode. required: false type: string + base_branch: + description: | + The name of the branch that contains the documentation after it has been uploaded to + discourse. Defaults to the default branch of the repository. + required: false + type: string outputs: urls_with_actions: description: | diff --git a/dev-requirements.txt b/dev-requirements.txt index a85126c0..176c263c 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,2 +1,6 @@ +ops +pytest-operator factory_boy>=3.2,<3.3 +pytest-asyncio>=0.21,<0.22 +pytest>=7.2,<7.3 juju==3.0.4 diff --git a/main.py b/main.py index 02bf78e6..53e37c4e 100755 --- a/main.py +++ b/main.py @@ -36,6 +36,7 @@ def _parse_env_vars() -> types_.UserInputs: delete_topics = os.getenv("INPUT_DELETE_TOPICS") == "true" dry_run = os.getenv("INPUT_DRY_RUN") == "true" github_access_token = os.getenv("INPUT_GITHUB_TOKEN") + base_branch = os.getenv("INPUT_BASE_BRANCH") return types_.UserInputs( discourse=types_.UserInputsDiscourse( @@ -47,6 +48,7 @@ def _parse_env_vars() -> types_.UserInputs: delete_pages=delete_topics, dry_run=dry_run, github_access_token=github_access_token, + base_branch=base_branch, ) diff --git a/prepare_check_cleanup/migration.py b/prepare_check_cleanup/migration.py index 6e1003f6..449e9452 100755 --- a/prepare_check_cleanup/migration.py +++ b/prepare_check_cleanup/migration.py @@ -6,7 +6,6 @@ import argparse import json import logging -import os import sys from contextlib import suppress from enum import Enum @@ -17,16 +16,16 @@ from github.PullRequest import PullRequest from github.Repository import Repository -from prepare_check_cleanup import exit_ +from prepare_check_cleanup import exit_, output +from src.constants import ( + DOCUMENTATION_FOLDER_NAME, + DOCUMENTATION_INDEX_FILENAME, + NAVIGATION_TABLE_START, +) from src.discourse import create_discourse from src.exceptions import DiscourseError -from src.index import DOCUMENTATION_FOLDER_NAME, DOCUMENTATION_INDEX_FILENAME -from src.pull_request import ( - ACTIONS_PULL_REQUEST_TITLE, - DEFAULT_BRANCH_NAME, - create_repository_client, -) -from src.reconcile import NAVIGATION_TABLE_START +from src.pull_request import DEFAULT_BRANCH_NAME +from src.repository import ACTIONS_PULL_REQUEST_TITLE, create_repository_client class Action(str, Enum): @@ -78,8 +77,7 @@ def main() -> None: case Action.CHECK_PULL_REQUEST.value: exit_.with_result(check_pull_request(**action_kwargs)) case Action.CLEANUP.value: - cleanup(**action_kwargs) - sys.exit(0) + exit_.with_result(cleanup(**action_kwargs)) case _: raise NotImplementedError(f"{args.action} has not been implemented") @@ -110,14 +108,8 @@ def prepare(index_filename: str, page_filename: str, discourse_config: dict[str, topics = {"index": index_url, "page": page_url} - github_output = os.getenv("GITHUB_OUTPUT") - assert github_output, ( - "the GITHUB_OUTPUT environment variable is empty or defined, " - "is this running in a GitHub workflow?" - ) - output_file = Path(github_output) topics_output = json.dumps(topics, separators=(",", ":")).replace('"', '\\"') - output_file.write_text(f"topics={topics_output}\nindex_url={index_url}\n", encoding="utf-8") + output.write(f"topics={topics_output}\nindex_url={index_url}\n") def _create_repository_client(github_access_token: str) -> Repository: @@ -258,14 +250,19 @@ def check_pull_request(github_access_token: str) -> bool: def cleanup( topics: dict[str, str], github_access_token: str, discourse_config: dict[str, str] -) -> None: +) -> bool: """Clean up testing artifacts on GitHub and Discourse. Args: topics: The discourse topics created for the migration. github_access_token: The secret required for interactions with GitHub. discourse_config: Details required to communicate with discourse. + + Returns: + Whether the cleanup succeeded. """ + result = True + # Delete discourse topics discourse = create_discourse(**discourse_config) try: @@ -273,6 +270,7 @@ def cleanup( discourse.delete_topic(url=topic_url) except DiscourseError as exc: logging.exception("cleanup failed for discourse, %s", exc) + result = False github_repo = _create_repository_client(github_access_token=github_access_token) # Delete the migration PR @@ -282,6 +280,7 @@ def cleanup( migration_pull_request.edit(state="closed") except GithubException as exc: logging.exception("cleanup failed for migration pull request, %s", exc) + result = False # Delete the migration branch try: migration_branch = _get_migration_branch(github_repo=github_repo) @@ -289,6 +288,9 @@ def cleanup( migration_branch.delete() except GithubException as exc: logging.exception("cleanup failed for migration branch, %s", exc) + result = False + + return result if __name__ == "__main__": diff --git a/prepare_check_cleanup/output.py b/prepare_check_cleanup/output.py new file mode 100644 index 00000000..6510822b --- /dev/null +++ b/prepare_check_cleanup/output.py @@ -0,0 +1,22 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Functions that support writing output.""" + +import os +from pathlib import Path + + +def write(text: str) -> None: + """Write text to the GitHub output file. + + Args: + text: The content to write to the GitHub output. + """ + github_output = os.getenv("GITHUB_OUTPUT") + assert github_output, ( + "the GITHUB_OUTPUT environment variable is empty or defined, " + "is this running in a GitHub workflow?" + ) + output_file = Path(github_output) + output_file.write_text(text, encoding="utf-8") diff --git a/prepare_check_cleanup/reconcile.py b/prepare_check_cleanup/reconcile.py index ec1e5131..f3a7b174 100755 --- a/prepare_check_cleanup/reconcile.py +++ b/prepare_check_cleanup/reconcile.py @@ -9,25 +9,36 @@ import logging import sys from enum import Enum +from pathlib import Path -from prepare_check_cleanup import exit_ +from github import Github +from github.GithubException import GithubException, UnknownObjectException + +from prepare_check_cleanup import exit_, output from src.discourse import Discourse, create_discourse from src.exceptions import DiscourseError +from src.pull_request import BRANCH_PREFIX + +_UPDATE_BRANCH = f"{BRANCH_PREFIX}/update-test" class Action(str, Enum): """The actions the utility can take. Attrs: - CHECK_DRAFT: Check that the draft integration test succeeded. - CHECK_CREATE: Check that the create integration test succeeded. - CHECK_DELETE_TOPICS: Check that the delete_topics integration test succeeded. - CHECK_DELETE: Check that the delete integration test succeeded. + CHECK_DRAFT: Check that the draft e2e test succeeded. + CHECK_CREATE: Check that the create e2e test succeeded. + PREPARE_UPDATE: Prepare for the update test. + CHECK_UPDATE: Check that the update e2e test succeeded. + CHECK_DELETE_TOPICS: Check that the delete_topics e2e test succeeded. + CHECK_DELETE: Check that the delete e2e test succeeded. CLEANUP: Discourse cleanup after the testing. """ CHECK_DRAFT = "check-draft" CHECK_CREATE = "check-create" + PREPARE_UPDATE = "prepare-update" + CHECK_UPDATE = "check-update" CHECK_DELETE_TOPICS = "check-delete-topics" CHECK_DELETE = "check-delete" CLEANUP = "cleanup" @@ -71,6 +82,15 @@ def main() -> None: urls_with_actions=urls_with_actions, discourse=discourse, **action_kwargs ) ) + case Action.PREPARE_UPDATE.value: + prepare_update(**action_kwargs) + sys.exit(0) + case Action.CHECK_UPDATE.value: + exit_.with_result( + check_update( + urls_with_actions=urls_with_actions, discourse=discourse, **action_kwargs + ) + ) case Action.CHECK_DELETE_TOPICS.value: exit_.with_result( check_delete_topics( @@ -84,8 +104,9 @@ def main() -> None: ) ) case Action.CLEANUP.value: - cleanup(urls_with_actions=urls_with_actions, discourse=discourse, **action_kwargs) - sys.exit(0) + exit_.with_result( + cleanup(urls_with_actions=urls_with_actions, discourse=discourse, **action_kwargs) + ) case _: raise NotImplementedError(f"{args.action} has not been implemented") @@ -161,7 +182,7 @@ def _check_url_result( if sorted(results := urls_with_actions.values()) != sorted(expected_result): logging.error( "%s check failed, the result is not as expected, " - "expected: %s, got: %s, urls_with_actions=%s", + "got: %s, expected: %s, urls_with_actions=%s", test_name, results, expected_result, @@ -235,6 +256,84 @@ def check_create( return True +def prepare_update(github_token: str, repo: str, filename: str) -> None: + """Prepare for the update action. + + Create a branch and push a file to that branch. + + Args: + github_token: Token for communication with GitHub. + repo: The name of the repository. + filename: The name of the file to push to the branch. + """ + github_client = Github(login_or_token=github_token) + github_repo = github_client.get_repo(repo) + base = github_repo.get_branch(github_repo.default_branch) + github_repo.create_git_ref(ref=f"refs/heads/{_UPDATE_BRANCH}", sha=base.commit.sha) + + # Delete the file if it already exists in the branch + with contextlib.suppress(UnknownObjectException): + contents = github_repo.get_contents(filename, ref=_UPDATE_BRANCH) + assert not isinstance(contents, list) + assert isinstance(contents.path, str) + github_repo.delete_file( + contents.path, + "remove pre-existing file for update test", + contents.sha, + branch=_UPDATE_BRANCH, + ) + + # Create the file in the branch + github_repo.create_file( + filename, + "file for update test", + Path(filename).read_text(encoding="utf-8"), + branch=_UPDATE_BRANCH, + ) + + output.write(f"update_branch={_UPDATE_BRANCH}\n") + + +def check_update( + urls_with_actions: dict[str, str], discourse: Discourse, expected_url_results: list[str] +) -> bool: + """Check that the update test succeeded. + + Success is indicated by that there are the expected number of URLs with the expected result and + that retrieving the URLs succeeds. + + Args: + urls_with_actions: The URLs that had any actions against them. + discourse: Client to the documentation server. + expected_url_results: The expected url results. + + Returns: + Whether the test succeeded. + """ + test_name = "update" + if not _check_url_count( + urls_with_actions=urls_with_actions, + expected_count=len(expected_url_results), + test_name=test_name, + ): + return False + + if not _check_url_result( + urls_with_actions=urls_with_actions, + expected_result=expected_url_results, + test_name=test_name, + ): + return False + + if not _check_url_retrieve( + urls_with_actions=urls_with_actions, discourse=discourse, test_name=test_name + ): + return False + + logging.info("%s check succeeded", test_name) + return True + + def check_delete_topics( urls_with_actions: dict[str, str], discourse: Discourse, expected_url_results: list[str] ) -> bool: @@ -328,16 +427,39 @@ def check_delete( return True -def cleanup(urls_with_actions: dict[str, str], discourse: Discourse) -> None: +def cleanup( + urls_with_actions: dict[str, str], discourse: Discourse, github_token: str, repo: str +) -> bool: """Delete all URLs. Args: urls_with_actions: The URLs that had any actions against them. discourse: Client to the documentation server. + github_token: Token for communication with GitHub. + repo: The name of the repository. + + Returns: + Whether the cleanup succeeded. """ + result = True + for url in urls_with_actions.keys(): - with contextlib.suppress(DiscourseError): + try: discourse.delete_topic(url=url) + except DiscourseError as exc: + logging.exception("cleanup failed for discourse, %s", exc) + result = False + + try: + github_client = Github(login_or_token=github_token) + github_repo = github_client.get_repo(repo) + update_branch = github_repo.get_git_ref(f"heads/{_UPDATE_BRANCH}") + update_branch.delete() + except GithubException as exc: + logging.exception("cleanup failed for GitHub, %s", exc) + result = False + + return result if __name__ == "__main__": diff --git a/pyproject.toml b/pyproject.toml index 4a7da92a..806c97b5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -18,7 +18,7 @@ show_missing = true [tool.pytest.ini_options] minversion = "6.0" log_cli_level = "INFO" -markers = ["reconcile", "migrate", "discourse"] +markers = ["reconcile", "conflict", "migrate", "discourse"] # Formatting tools configuration [tool.black] diff --git a/src/__init__.py b/src/__init__.py index 1b6cf4bc..9d823226 100644 --- a/src/__init__.py +++ b/src/__init__.py @@ -3,22 +3,26 @@ """Library for uploading docs to charmhub.""" +from itertools import tee from pathlib import Path from .action import DRY_RUN_NAVLINK_LINK, FAIL_NAVLINK_LINK from .action import run_all as run_all_actions +from .check import conflicts as check_conflicts +from .constants import DOCUMENTATION_FOLDER_NAME from .discourse import Discourse from .docs_directory import has_docs_directory from .docs_directory import read as read_docs_directory from .exceptions import InputError -from .index import DOCUMENTATION_FOLDER_NAME, contents_from_page +from .index import contents_from_page from .index import get as get_index from .metadata import get as get_metadata from .migration import run as migrate_contents from .navigation_table import from_page as navigation_table_from_page -from .pull_request import RepositoryClient, create_pull_request, create_repository_client +from .pull_request import create_pull_request from .reconcile import run as run_reconcile -from .types_ import ActionResult, Metadata, UserInputs +from .repository import create_repository_client +from .types_ import ActionResult, Clients, Metadata, UserInputs GETTING_STARTED = ( "To get started with upload-charm-docs, " @@ -27,38 +31,52 @@ def _run_reconcile( - base_path: Path, - metadata: Metadata, - discourse: Discourse, - dry_run: bool, - delete_pages: bool, + base_path: Path, metadata: Metadata, clients: Clients, user_inputs: UserInputs ) -> dict[str, str]: """Upload the documentation to charmhub. Args: base_path: The base path of the repository. metadata: Information about the charm. - discourse: A client to the documentation server. - dry_run: If enabled, only log the action that would be taken. - delete_pages: Whether to delete pages that are no longer needed. + clients: The clients to interact with things like discourse and the repository. + user_inputs: Configurable inputs for running upload-charm-docs. Returns: All the URLs that had an action with the result of that action. + Raises: + InputError: if there are any problems with executing any of the actions. + """ - index = get_index(metadata=metadata, base_path=base_path, server_client=discourse) + index = get_index(metadata=metadata, base_path=base_path, server_client=clients.discourse) path_infos = read_docs_directory(docs_path=base_path / DOCUMENTATION_FOLDER_NAME) server_content = ( index.server.content if index.server is not None and index.server.content else "" ) - table_rows = navigation_table_from_page(page=server_content, discourse=discourse) - actions = run_reconcile(path_infos=path_infos, table_rows=table_rows, discourse=discourse) + table_rows = navigation_table_from_page(page=server_content, discourse=clients.discourse) + actions = run_reconcile( + path_infos=path_infos, + table_rows=table_rows, + clients=clients, + base_path=base_path, + user_inputs=user_inputs, + ) + + # tee creates a copy of the iterator which is needed as check_conflicts consumes the iterator + # it is passed + actions, check_actions = tee(actions, 2) + problems = tuple(check_conflicts(actions=check_actions)) + if problems: + raise InputError( + "One or more of the required actions could not be executed, see the log for details" + ) + reports = run_all_actions( actions=actions, index=index, - discourse=discourse, - dry_run=dry_run, - delete_pages=delete_pages, + discourse=clients.discourse, + dry_run=user_inputs.dry_run, + delete_pages=user_inputs.delete_pages, ) return { str(report.location): report.result @@ -69,38 +87,32 @@ def _run_reconcile( } -def _run_migrate( - base_path: Path, - metadata: Metadata, - discourse: Discourse, - repository: RepositoryClient, -) -> dict[str, str]: +def _run_migrate(base_path: Path, metadata: Metadata, clients: Clients) -> dict[str, str]: """Migrate existing docs from charmhub to local repository. Args: base_path: The base path of the repository. metadata: Information about the charm. - discourse: A client to the documentation server. - repository: Repository client for managing both local and remote git repositories. + clients: The clients to interact with things like discourse and the repository. Returns: A single key-value pair dictionary containing a link to the Pull Request containing migrated documentation as key and successful action result as value. """ - index = get_index(metadata=metadata, base_path=base_path, server_client=discourse) + index = get_index(metadata=metadata, base_path=base_path, server_client=clients.discourse) server_content = ( index.server.content if index.server is not None and index.server.content else "" ) index_content = contents_from_page(server_content) - table_rows = navigation_table_from_page(page=server_content, discourse=discourse) + table_rows = navigation_table_from_page(page=server_content, discourse=clients.discourse) migrate_contents( table_rows=table_rows, index_content=index_content, - discourse=discourse, + discourse=clients.discourse, docs_path=base_path / DOCUMENTATION_FOLDER_NAME, ) - pr_link = create_pull_request(repository=repository) + pr_link = create_pull_request(repository=clients.repository) return {pr_link: ActionResult.SUCCESS} @@ -121,22 +133,14 @@ def run(base_path: Path, discourse: Discourse, user_inputs: UserInputs) -> dict[ """ metadata = get_metadata(base_path) has_docs_dir = has_docs_directory(base_path=base_path) + repository = create_repository_client( + access_token=user_inputs.github_access_token, base_path=base_path + ) + clients = Clients(discourse=discourse, repository=repository) if metadata.docs and not has_docs_dir: - repository = create_repository_client( - access_token=user_inputs.github_access_token, base_path=base_path - ) - return _run_migrate( - base_path=base_path, - metadata=metadata, - discourse=discourse, - repository=repository, - ) + return _run_migrate(base_path=base_path, metadata=metadata, clients=clients) if has_docs_dir: return _run_reconcile( - base_path=base_path, - metadata=metadata, - discourse=discourse, - dry_run=user_inputs.dry_run, - delete_pages=user_inputs.delete_pages, + base_path=base_path, metadata=metadata, clients=clients, user_inputs=user_inputs ) raise InputError(GETTING_STARTED) diff --git a/src/action.py b/src/action.py index 1a31c7ac..9d7e5a8c 100644 --- a/src/action.py +++ b/src/action.py @@ -3,15 +3,18 @@ """Module for taking the required actions to match the server state with the local state.""" -import difflib import logging import typing +from enum import Enum from . import exceptions, reconcile, types_ +from .content import diff as content_diff +from .content import merge as content_merge from .discourse import Discourse DRY_RUN_NAVLINK_LINK = "" DRY_RUN_REASON = "dry run" +BASE_MISSING_REASON = "no base for the content to be automatically merged" FAIL_NAVLINK_LINK = "" NOT_DELETE_REASON = "delete_topics is false" @@ -29,24 +32,17 @@ def _absolute_url(url: types_.Url | None, discourse: Discourse) -> types_.Url | return discourse.absolute_url(url=url) if url is not None else None -def _log_content_change(old: str, new: str) -> None: - """Log the difference between the old and new content, if any. +def _log_content_change(base: str, new: str) -> None: + """Log the difference between the base and new content, if any. Args: - old: The previous content. + base: The previous content. new: The current content. """ - old = f"{old}\n" if not old.endswith("\n") else old + old = f"{base}\n" if not base.endswith("\n") else base new = f"{new}\n" if not new.endswith("\n") else new if new != old: - logging.info( - "content change:\n%s", - "".join( - difflib.Differ().compare( - old.splitlines(keepends=True), new.splitlines(keepends=True) - ) - ), - ) + logging.info("content change:\n%s", content_diff(old, new)) def _create( @@ -117,6 +113,56 @@ def _noop(action: types_.NoopAction, discourse: Discourse) -> types_.ActionRepor ) +class UpdateCase(str, Enum): + """The possible cases for the update action. + + Attrs: + INVALID: The action is not valid. + DRY_RUN: Do not make any changes. + CONTENT_CHANGE: The content has been changed. + BASE_MISSING: The base content is not available. + DEFAULT: No other specific case applies. + """ + + INVALID = "invalid" + DRY_RUN = "dry-run" + CONTENT_CHANGE = "content-change" + BASE_MISSING = "base-missing" + DEFAULT = "default" + + +def _get_update_case(action: types_.UpdateAction, dry_run: bool) -> UpdateCase: + """Get the execution case for the update action. + + Args: + action: The update action details. + dry_run: If enabled, only log the action that would be taken. + + Returns: + The named case for the action execution. + """ + # Check that action is valid + if action.navlink_change.new.link is not None and action.content_change is None: + return UpdateCase.INVALID + if dry_run: + return UpdateCase.DRY_RUN + if ( + not dry_run + and action.navlink_change.new.link is not None + and action.content_change is not None + and action.content_change.base is not None + and action.content_change.local != action.content_change.server + ): + return UpdateCase.CONTENT_CHANGE + if ( + action.content_change is not None + and action.content_change.base is None + and action.content_change.local != action.content_change.server + ): + return UpdateCase.BASE_MISSING + return UpdateCase.DEFAULT + + def _update( action: types_.UpdateAction, discourse: Discourse, dry_run: bool ) -> types_.ActionReport: @@ -134,45 +180,45 @@ def _update( ActionError: if the content change or new content for a page is None. """ logging.info("dry run: %s, action: %s", dry_run, action) - - # Check that action is valid - if action.navlink_change.new.link is not None and action.content_change is None: - raise exceptions.ActionError( - f"internal error, content change for page is None, {action=!r}" + if action.content_change is not None: + _log_content_change( + base=action.content_change.base or action.content_change.server, + new=action.content_change.local, ) - if ( - action.navlink_change.new.link is not None - and action.content_change is not None - and action.content_change.new is None - ): - raise exceptions.ActionError(f"internal error, new content for page is None, {action=!r}") - if ( - action.content_change is not None - and action.content_change.old is not None - and action.content_change.new is not None - ): - _log_content_change(old=action.content_change.old, new=action.content_change.new) + update_case = _get_update_case(action=action, dry_run=dry_run) - if ( - not dry_run - and action.navlink_change.new.link is not None - and action.content_change is not None - and action.content_change.new is not None - and action.content_change.new != action.content_change.old - ): - try: - discourse.update_topic( - url=action.navlink_change.new.link, content=action.content_change.new + reason: str | None + match update_case: + case UpdateCase.INVALID: + raise exceptions.ActionError( + f"internal error, content change for page is None, {action=!r}" ) + case UpdateCase.DRY_RUN: + result = types_.ActionResult.SKIP + reason = DRY_RUN_REASON + case UpdateCase.CONTENT_CHANGE: + try: + content_change = typing.cast(types_.ContentChange, action.content_change) + merged_content = content_merge( + base=typing.cast(str, content_change.base), + theirs=content_change.server, + ours=content_change.local, + ) + discourse.update_topic( + url=typing.cast(str, action.navlink_change.new.link), content=merged_content + ) + result = types_.ActionResult.SUCCESS + reason = None + except (exceptions.DiscourseError, exceptions.ContentError) as exc: + result = types_.ActionResult.FAIL + reason = str(exc) + case UpdateCase.BASE_MISSING: + result = types_.ActionResult.FAIL + reason = BASE_MISSING_REASON + case _: result = types_.ActionResult.SUCCESS reason = None - except exceptions.DiscourseError as exc: - result = types_.ActionResult.FAIL - reason = str(exc) - else: - result = types_.ActionResult.SKIP if dry_run else types_.ActionResult.SUCCESS - reason = DRY_RUN_REASON if dry_run else None url = _absolute_url(action.navlink_change.new.link, discourse=discourse) table_row = types_.TableRow( @@ -343,7 +389,7 @@ def _run_index( case types_.UpdateIndexAction: try: assert isinstance(action, types_.UpdateIndexAction) # nosec - _log_content_change(old=action.content_change.old, new=action.content_change.new) + _log_content_change(base=action.content_change.old, new=action.content_change.new) discourse.update_topic(url=action.url, content=action.content_change.new) report = types_.ActionReport( table_row=None, diff --git a/src/check.py b/src/check.py new file mode 100644 index 00000000..cfd1f82d --- /dev/null +++ b/src/check.py @@ -0,0 +1,102 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Module for running checks.""" + +import logging +from collections.abc import Iterable, Iterator +from typing import NamedTuple, TypeGuard + +from .content import conflicts as content_conflicts +from .content import diff as content_diff +from .types_ import AnyAction, UpdateAction + + +class Problem(NamedTuple): + """Details about a failed check. + + Attrs: + path: Unique identifier for the file and discourse topic with the problem + description: A summary of what the problem is and how to resolve it. + + """ + + path: str + description: str + + +def _is_update_action(action: AnyAction) -> TypeGuard[UpdateAction]: + """Check whether an action is an UpdateAction. + + Args: + action: The action to check. + + Returns: + Whether the action is an UpdateAction. + """ + return isinstance(action, UpdateAction) + + +def _update_action_problem(action: UpdateAction) -> Problem | None: + """Get any problem with an update action. + + Args: + action: The action to check. + + Returns: + None if there is no problem or the problem if there is an issue with the action. + """ + if action.content_change is None: + return None + + if ( + action.content_change.base is None + and action.content_change.server == action.content_change.local + ): + return None + + if action.content_change.base is None: + diff = content_diff(action.content_change.server, action.content_change.local) + problem = Problem( + path=action.path, + description=( + "cannot execute the update action due to not finding a file on the base branch " + "and there are differences between the branch and discourse content, please ensure " + f"that there are no differences and try again. Detected differences:\n{diff}" + ), + ) + else: + action_conflicts = content_conflicts( + base=action.content_change.base, + theirs=action.content_change.server, + ours=action.content_change.local, + ) + if action_conflicts is None: + return None + problem = Problem( + path=action.path, + description=( + "cannot execute the update action due to conflicting changes on discourse, " + f"please resolve the conflicts and try again: \n{action_conflicts}" + ), + ) + + logging.error( + "there is a problem preventing the execution of an action, action: %s, problem: %s", + action, + problem, + ) + return problem + + +def conflicts(actions: Iterable[AnyAction]) -> Iterator[Problem]: + """Check whether actions have any content conflicts. + + Args: + actions: The actions to check. + + Yields: + A problem for each action with a conflict + """ + update_actions = filter(_is_update_action, actions) + yield from filter(None, (_update_action_problem(action) for action in update_actions)) diff --git a/src/constants.py b/src/constants.py new file mode 100644 index 00000000..042c795e --- /dev/null +++ b/src/constants.py @@ -0,0 +1,17 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Shared constants. + +The use of this module should be limited to cases where the constant is not better placed in +another module or to resolve circular imports. +""" + +DOCUMENTATION_FOLDER_NAME = "docs" +DOCUMENTATION_INDEX_FILENAME = "index.md" +NAVIGATION_TABLE_START = """ + +# Navigation + +| Level | Path | Navlink | +| -- | -- | -- |""" diff --git a/src/content.py b/src/content.py new file mode 100644 index 00000000..38534140 --- /dev/null +++ b/src/content.py @@ -0,0 +1,112 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Module for checking conflicts using 3-way merge and create content based on a 3 way merge.""" + +import difflib +import tempfile +from pathlib import Path + +from git.exc import GitCommandError +from git.repo import Repo + +from .exceptions import ContentError + +_BASE_BRANCH = "base" +_THEIR_BRANCH = "theirs" +_OUR_BRANCH = "ours" + + +def conflicts(base: str, theirs: str, ours: str) -> str | None: + """Check for merge conflicts based on the git merge algorithm. + + Args: + base: The starting point for both changes. + theirs: The other change. + ours: The local change. + + Returns: + The description of the merge conflicts or None if there are no conflicts. + """ + try: + merge(base=base, theirs=theirs, ours=ours) + except ContentError as exc: + return str(exc) + return None + + +def merge(base: str, theirs: str, ours: str) -> str: + """Create the merged content based on the git merge algorithm. + + Args: + base: The starting point for both changes. + theirs: The other change. + ours: The local change. + + Returns: + The merged content. + + Raises: + ContentError: if there are merge conflicts. + """ + # Handle cases that are guaranteed not to have conflicts + if theirs == base: + return ours + if ours == base: + return theirs + if theirs == ours: + return theirs + + with tempfile.TemporaryDirectory() as tmp_dir: + # Initialise repository + tmp_path = Path(tmp_dir) + repo = Repo.init(tmp_path) + writer = repo.config_writer() + writer.set_value("user", "name", "temp_user") + writer.set_value("user", "email", "temp_email") + writer.set_value("commit", "gpgsign", "false") + writer.release() + + # Create base + repo.git.checkout("-b", _BASE_BRANCH) + (content_path := tmp_path / "content.txt").write_text(base, encoding="utf-8") + repo.git.add(".") + repo.git.commit("-m", "initial commit") + + # Create their branch + repo.git.checkout("-b", _THEIR_BRANCH) + content_path.write_text(theirs, encoding="utf-8") + repo.git.add(".") + repo.git.commit("-m", "their change") + + # Create our branch + repo.git.checkout(_BASE_BRANCH) + repo.git.checkout("-b", _OUR_BRANCH) + content_path.write_text(ours, encoding="utf-8") + repo.git.add(".") + repo.git.commit("-m", "our change") + + try: + repo.git.merge(_THEIR_BRANCH) + except GitCommandError as exc: + content_conflicts = content_path.read_text(encoding="utf-8") + raise ContentError( + f"could not automatically merge, conflicts:\n{content_conflicts}" + ) from exc + + return content_path.read_text(encoding="utf-8") + + +def diff(first: str, second: str) -> str: + """Show the difference between two strings. + + Args: + first: One of the strings to compare. + second: One of the strings to compare. + + Returns: + The diff between the two strings. + """ + return "".join( + difflib.Differ().compare(first.splitlines(keepends=True), second.splitlines(keepends=True)) + ) diff --git a/src/docs_directory.py b/src/docs_directory.py index 472ff770..7ac8dc04 100644 --- a/src/docs_directory.py +++ b/src/docs_directory.py @@ -9,7 +9,7 @@ from pathlib import Path from . import types_ -from .index import DOCUMENTATION_FOLDER_NAME +from .constants import DOCUMENTATION_FOLDER_NAME def _get_directories_files(docs_path: Path) -> list[Path]: diff --git a/src/exceptions.py b/src/exceptions.py index 05dde1de..13e2a027 100644 --- a/src/exceptions.py +++ b/src/exceptions.py @@ -42,3 +42,11 @@ class MigrationError(BaseError): class RepositoryClientError(BaseError): """A problem with git repository client occurred.""" + + +class RepositoryFileNotFoundError(BaseError): + """A problem retrieving a file from a git repository occurred.""" + + +class ContentError(BaseError): + """A problem with the content occurred.""" diff --git a/src/index.py b/src/index.py index ab9835b8..3681f0e1 100644 --- a/src/index.py +++ b/src/index.py @@ -5,14 +5,15 @@ from pathlib import Path +from .constants import ( + DOCUMENTATION_FOLDER_NAME, + DOCUMENTATION_INDEX_FILENAME, + NAVIGATION_TABLE_START, +) from .discourse import Discourse from .exceptions import DiscourseError, ServerError -from .reconcile import NAVIGATION_TABLE_START from .types_ import Index, IndexFile, Metadata, Page -DOCUMENTATION_FOLDER_NAME = "docs" -DOCUMENTATION_INDEX_FILENAME = "index.md" - def _read_docs_index(base_path: Path) -> str | None: """Read the content of the index file. diff --git a/src/pull_request.py b/src/pull_request.py index e932b2f6..938645f3 100644 --- a/src/pull_request.py +++ b/src/pull_request.py @@ -4,143 +4,13 @@ """Module for handling interactions with git repository.""" import logging -import re -from pathlib import Path -from git import GitCommandError -from git.repo import Repo -from github import Github -from github.GithubException import GithubException -from github.Repository import Repository +from .exceptions import InputError +from .repository import Client as RepositoryClient -from .exceptions import InputError, RepositoryClientError -from .index import DOCUMENTATION_FOLDER_NAME - -GITHUB_HOSTNAME = "github.com" -HTTPS_URL_PATTERN = re.compile(rf"^https?:\/\/.*@?{GITHUB_HOSTNAME}\/(.+\/.+?)(.git)?$") -ACTIONS_USER_NAME = "upload-charms-docs-bot" -ACTIONS_USER_EMAIL = "upload-charms-docs-bot@users.noreply.github.com" -ACTIONS_COMMIT_MESSAGE = "migrate docs from server" -ACTIONS_PULL_REQUEST_TITLE = "[upload-charm-docs] Migrate charm docs" -ACTIONS_PULL_REQUEST_BODY = ( - "This pull request was autogenerated by upload-charm-docs to migrate " - "existing documentation from server to the git repository." -) -PR_LINK_NO_CHANGE = "" BRANCH_PREFIX = "upload-charm-docs" DEFAULT_BRANCH_NAME = f"{BRANCH_PREFIX}/migrate" - -CONFIG_USER_SECTION_NAME = "user" -CONFIG_USER_NAME = (CONFIG_USER_SECTION_NAME, "name") -CONFIG_USER_EMAIL = (CONFIG_USER_SECTION_NAME, "email") - - -class RepositoryClient: - """Wrapper for git/git-server related functionalities.""" - - def __init__(self, repository: Repo, github_repository: Repository) -> None: - """Construct. - - Args: - repository: Client for interacting with local git repository. - github_repository: Client for interacting with remote github repository. - """ - self._git_repo = repository - self._github_repo = github_repository - self._configure_git_user() - - def _configure_git_user(self) -> None: - """Configure action git profile defaults. - - Configured profile appears as the git committer. - """ - config_reader = self._git_repo.config_reader(config_level="repository") - with self._git_repo.config_writer(config_level="repository") as config_writer: - if not config_reader.has_section( - CONFIG_USER_SECTION_NAME - ) or not config_reader.get_value(*CONFIG_USER_NAME): - config_writer.set_value(*CONFIG_USER_NAME, ACTIONS_USER_NAME) - if not config_reader.has_section( - CONFIG_USER_SECTION_NAME - ) or not config_reader.get_value(*CONFIG_USER_EMAIL): - config_writer.set_value(*CONFIG_USER_EMAIL, ACTIONS_USER_EMAIL) - - def check_branch_exists(self, branch_name: str) -> bool: - """Check if branch exists on remote. - - Args: - branch_name: Branch name to check on remote. - - Raises: - RepositoryClientError: if unexpected error occurred during git operation. - - Returns: - True if branch already exists, False otherwise. - """ - try: - self._git_repo.git.fetch("origin", branch_name) - return True - except GitCommandError as exc: - if "couldn't find remote ref" in exc.stderr: - return False - raise RepositoryClientError( - f"Unexpected error checking existing branch. {exc=!r}" - ) from exc - - def create_branch(self, branch_name: str, commit_msg: str) -> None: - """Create new branch with existing changes using the default branch as the base. - - Args: - branch_name: New branch name. - commit_msg: Commit message for current changes. - - Raises: - RepositoryClientError: if unexpected error occurred during git operation. - """ - default_branch = self._github_repo.default_branch - try: - self._git_repo.git.fetch("origin", default_branch) - self._git_repo.git.checkout(default_branch, "--") - self._git_repo.git.checkout("-b", branch_name) - self._git_repo.git.add("-A", DOCUMENTATION_FOLDER_NAME) - self._git_repo.git.commit("-m", f"'{commit_msg}'") - self._git_repo.git.push("-u", "origin", branch_name) - except GitCommandError as exc: - raise RepositoryClientError(f"Unexpected error creating new branch. {exc=!r}") from exc - - def create_pull_request(self, branch_name: str) -> str: - """Create a pull request from given branch to the default branch. - - Args: - branch_name: Branch name from which the pull request will be created. - - Raises: - RepositoryClientError: if unexpected error occurred during git operation. - - Returns: - The web url to pull request page. - """ - try: - pull_request = self._github_repo.create_pull( - title=ACTIONS_PULL_REQUEST_TITLE, - body=ACTIONS_PULL_REQUEST_BODY, - base=self._github_repo.default_branch, - head=branch_name, - ) - except GithubException as exc: - raise RepositoryClientError( - f"Unexpected error creating pull request. {exc=!r}" - ) from exc - - return pull_request.html_url - - def is_dirty(self) -> bool: - """Check if repository path has any changes including new files. - - Returns: - True if any changes have occurred. - """ - return self._git_repo.is_dirty(untracked_files=True) +ACTIONS_COMMIT_MESSAGE = "migrate docs from server" def create_pull_request(repository: RepositoryClient) -> str: @@ -173,49 +43,3 @@ def create_pull_request(repository: RepositoryClient) -> str: pull_request_web_link = repository.create_pull_request(branch_name=DEFAULT_BRANCH_NAME) return pull_request_web_link - - -def _get_repository_name_from_git_url(remote_url: str) -> str: - """Get repository name from git remote URL. - - Args: - remote_url: URL of remote repository. - e.g. https://github.com/canonical/upload-charm-docs.git - - Raises: - InputError: if invalid repository url was given. - - Returns: - Git repository name. e.g. canonical/upload-charm-docs. - """ - matched_repository = HTTPS_URL_PATTERN.match(remote_url) - if not matched_repository: - raise InputError(f"Invalid remote repository url {remote_url=!r}") - return matched_repository.group(1) - - -def create_repository_client(access_token: str | None, base_path: Path) -> RepositoryClient: - """Create a Github instance to handle communication with Github server. - - Args: - access_token: Access token that has permissions to open a pull request. - base_path: Path where local .git resides in. - - Raises: - InputError: if invalid access token or invalid git remote URL is provided. - - Returns: - A Github repository instance. - """ - if not access_token: - raise InputError( - f"Invalid 'access_token' input, it must be non-empty, got {access_token=!r}" - ) - - local_repo = Repo(base_path) - logging.info("executing in git repository in the directory: %s", local_repo.working_dir) - github_client = Github(login_or_token=access_token) - remote_url = local_repo.remote().url - repository_fullname = _get_repository_name_from_git_url(remote_url=remote_url) - remote_repo = github_client.get_repo(repository_fullname) - return RepositoryClient(repository=local_repo, github_repository=remote_repo) diff --git a/src/reconcile.py b/src/reconcile.py index cc4b946e..c706de7b 100644 --- a/src/reconcile.py +++ b/src/reconcile.py @@ -5,17 +5,12 @@ import itertools import typing +from pathlib import Path from . import exceptions, types_ +from .constants import NAVIGATION_TABLE_START from .discourse import Discourse -NAVIGATION_TABLE_START = """ - -# Navigation - -| Level | Path | Navlink | -| -- | -- | -- |""" - def _local_only(path_info: types_.PathInfo) -> types_.CreateAction: """Return a create action based on information about a local documentation file. @@ -61,8 +56,36 @@ def _get_server_content(table_row: types_.TableRow, discourse: Discourse) -> str ) from exc +def _local_and_server_validation( + path_info: types_.PathInfo, + table_row: types_.TableRow, +) -> None: + """Input checks before execution. + + Args: + path_info: Information about the local documentation file. + table_row: A row from the navigation table. + + Raises: + ReconcilliationError: + If the table path or level do not match for the path info and table row. + """ + if path_info.level != table_row.level: + raise exceptions.ReconcilliationError( + f"internal error, level mismatch, {path_info=!r}, {table_row=!r}" + ) + if path_info.table_path != table_row.path: + raise exceptions.ReconcilliationError( + f"internal error, table path mismatch, {path_info=!r}, {table_row=!r}" + ) + + def _local_and_server( - path_info: types_.PathInfo, table_row: types_.TableRow, discourse: Discourse + path_info: types_.PathInfo, + table_row: types_.TableRow, + clients: types_.Clients, + base_path: Path, + user_inputs: types_.UserInputs, ) -> tuple[ types_.UpdateAction | types_.NoopAction | types_.CreateAction | types_.DeleteAction, ... ]: @@ -83,7 +106,9 @@ def _local_and_server( Args: path_info: Information about the local documentation file. table_row: A row from the navigation table. - discourse: A client to the documentation server. + clients: The clients to interact with things like discourse and the repository. + base_path: The base path of the repository. + user_inputs: Configurable inputs for running upload-charm-docs. Returns: The action to execute against the server. @@ -93,15 +118,9 @@ def _local_and_server( - If the table path or level do not match for the path info and table row. - If certain edge cases occur that are not expected, such as table_row.navlink.link for a page on the server. + - If there was a problem retrieving content from GitHub. """ - if path_info.level != table_row.level: - raise exceptions.ReconcilliationError( - f"internal error, level mismatch, {path_info=!r}, {table_row=!r}" - ) - if path_info.table_path != table_row.path: - raise exceptions.ReconcilliationError( - f"internal error, table path mismatch, {path_info=!r}, {table_row=!r}" - ) + _local_and_server_validation(path_info=path_info, table_row=table_row) # Is a directory locally and a grouping on the server if path_info.local_path.is_dir() and table_row.is_group: @@ -141,7 +160,7 @@ def _local_and_server( level=path_info.level, path=path_info.table_path, navlink=table_row.navlink, - content=discourse.retrieve_topic(url=table_row.navlink.link), + content=clients.discourse.retrieve_topic(url=table_row.navlink.link), ), types_.CreateAction( level=path_info.level, @@ -166,7 +185,7 @@ def _local_and_server( # Is a page locally and on the server local_content = path_info.local_path.read_text(encoding="utf-8").strip() - server_content = _get_server_content(table_row=table_row, discourse=discourse) + server_content = _get_server_content(table_row=table_row, discourse=clients.discourse) if server_content == local_content and table_row.navlink.title == path_info.navlink_title: return ( @@ -177,6 +196,19 @@ def _local_and_server( content=local_content, ), ) + + try: + path = str(path_info.local_path.relative_to(base_path)) + base_content = clients.repository.get_file_content( + path=path, branch=user_inputs.base_branch + ) + except exceptions.RepositoryFileNotFoundError: + base_content = None + except exceptions.RepositoryClientError as exc: + raise exceptions.ReconcilliationError( + f"Unable to retrieve content for path from branch, {path=}, " + f"branch={user_inputs.base_branch}" + ) from exc return ( types_.UpdateAction( level=path_info.level, @@ -185,7 +217,9 @@ def _local_and_server( old=table_row.navlink, new=types_.Navlink(title=path_info.navlink_title, link=table_row.navlink.link), ), - content_change=types_.ContentChange(old=server_content, new=local_content), + content_change=types_.ContentChange( + base=base_content, server=server_content, local=local_content + ), ), ) @@ -231,14 +265,20 @@ def _server_only(table_row: types_.TableRow, discourse: Discourse) -> types_.Del def _calculate_action( - path_info: types_.PathInfo | None, table_row: types_.TableRow | None, discourse: Discourse + path_info: types_.PathInfo | None, + table_row: types_.TableRow | None, + clients: types_.Clients, + base_path: Path, + user_inputs: types_.UserInputs, ) -> tuple[types_.AnyAction, ...]: """Calculate the required action for a page. Args: path_info: Information about the local documentation file. table_row: A row from the navigation table. - discourse: A client to the documentation server. + clients: The clients to interact with things like discourse and the repository. + base_path: The base path of the repository. + user_inputs: Configurable inputs for running upload-charm-docs. Returns: The action to take for the page. @@ -253,9 +293,15 @@ def _calculate_action( if path_info is not None and table_row is None: return (_local_only(path_info=path_info),) if path_info is None and table_row is not None: - return (_server_only(table_row=table_row, discourse=discourse),) + return (_server_only(table_row=table_row, discourse=clients.discourse),) if path_info is not None and table_row is not None: - return _local_and_server(path_info=path_info, table_row=table_row, discourse=discourse) + return _local_and_server( + path_info=path_info, + table_row=table_row, + clients=clients, + base_path=base_path, + user_inputs=user_inputs, + ) # Something weird has happened since all cases should already be covered raise exceptions.ReconcilliationError("internal error") # pragma: no cover @@ -264,7 +310,9 @@ def _calculate_action( def run( path_infos: typing.Iterable[types_.PathInfo], table_rows: typing.Iterable[types_.TableRow], - discourse: Discourse, + clients: types_.Clients, + base_path: Path, + user_inputs: types_.UserInputs, ) -> typing.Iterator[types_.AnyAction]: """Reconcile differences between the docs directory and documentation server. @@ -282,9 +330,11 @@ def run( effect on the navigation table that is generated and hence ordering for them doesn't matter. Args: + base_path: The base path of the repository. path_infos: Information about the local documentation files. table_rows: Rows from the navigation table. - discourse: A client to the documentation server. + clients: The clients to interact with things like discourse and the repository. + user_inputs: Configurable inputs for running upload-charm-docs. Returns: The actions required to reconcile differences between the documentation server and local @@ -303,7 +353,13 @@ def run( sorted_remaining_table_row_keys = sorted(table_row_lookup.keys() - path_info_lookup.keys()) keys = itertools.chain(sorted_path_info_keys, sorted_remaining_table_row_keys) return itertools.chain.from_iterable( - _calculate_action(path_info_lookup.get(key), table_row_lookup.get(key), discourse) + _calculate_action( + path_info_lookup.get(key), + table_row_lookup.get(key), + clients, + base_path, + user_inputs, + ) for key in keys ) diff --git a/src/repository.py b/src/repository.py new file mode 100644 index 00000000..34a4f7e9 --- /dev/null +++ b/src/repository.py @@ -0,0 +1,224 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Module for handling interactions with git repository.""" + +import base64 +import logging +import re +from pathlib import Path + +from git import GitCommandError +from git.repo import Repo +from github import Github +from github.GithubException import GithubException, UnknownObjectException +from github.Repository import Repository + +from .constants import DOCUMENTATION_FOLDER_NAME +from .exceptions import InputError, RepositoryClientError, RepositoryFileNotFoundError + +GITHUB_HOSTNAME = "github.com" +ORIGIN_NAME = "origin" +HTTPS_URL_PATTERN = re.compile(rf"^https?:\/\/.*@?{GITHUB_HOSTNAME}\/(.+\/.+?)(.git)?$") +ACTIONS_USER_NAME = "upload-charms-docs-bot" +ACTIONS_USER_EMAIL = "upload-charms-docs-bot@users.noreply.github.com" +ACTIONS_PULL_REQUEST_TITLE = "[upload-charm-docs] Migrate charm docs" +ACTIONS_PULL_REQUEST_BODY = ( + "This pull request was autogenerated by upload-charm-docs to migrate " + "existing documentation from server to the git repository." +) +PR_LINK_NO_CHANGE = "" + +CONFIG_USER_SECTION_NAME = "user" +CONFIG_USER_NAME = (CONFIG_USER_SECTION_NAME, "name") +CONFIG_USER_EMAIL = (CONFIG_USER_SECTION_NAME, "email") + + +class Client: + """Wrapper for git/git-server related functionalities.""" + + def __init__(self, repository: Repo, github_repository: Repository) -> None: + """Construct. + + Args: + repository: Client for interacting with local git repository. + github_repository: Client for interacting with remote github repository. + """ + self._git_repo = repository + self._github_repo = github_repository + self._configure_git_user() + + def _configure_git_user(self) -> None: + """Configure action git profile defaults. + + Configured profile appears as the git committer. + """ + config_reader = self._git_repo.config_reader(config_level="repository") + with self._git_repo.config_writer(config_level="repository") as config_writer: + if not config_reader.has_section( + CONFIG_USER_SECTION_NAME + ) or not config_reader.get_value(*CONFIG_USER_NAME): + config_writer.set_value(*CONFIG_USER_NAME, ACTIONS_USER_NAME) + if not config_reader.has_section( + CONFIG_USER_SECTION_NAME + ) or not config_reader.get_value(*CONFIG_USER_EMAIL): + config_writer.set_value(*CONFIG_USER_EMAIL, ACTIONS_USER_EMAIL) + + def check_branch_exists(self, branch_name: str) -> bool: + """Check if branch exists on remote. + + Args: + branch_name: Branch name to check on remote. + + Raises: + RepositoryClientError: if unexpected error occurred during git operation. + + Returns: + True if branch already exists, False otherwise. + """ + try: + self._git_repo.git.fetch(ORIGIN_NAME, branch_name) + return True + except GitCommandError as exc: + if "couldn't find remote ref" in exc.stderr: + return False + raise RepositoryClientError( + f"Unexpected error checking existing branch. {exc=!r}" + ) from exc + + def create_branch(self, branch_name: str, commit_msg: str) -> None: + """Create new branch with existing changes using the default branch as the base. + + Args: + branch_name: New branch name. + commit_msg: Commit message for current changes. + + Raises: + RepositoryClientError: if unexpected error occurred during git operation. + """ + default_branch = self._github_repo.default_branch + try: + self._git_repo.git.fetch(ORIGIN_NAME, default_branch) + self._git_repo.git.checkout(default_branch, "--") + self._git_repo.git.checkout("-b", branch_name) + self._git_repo.git.add("-A", DOCUMENTATION_FOLDER_NAME) + self._git_repo.git.commit("-m", f"'{commit_msg}'") + self._git_repo.git.push("-u", ORIGIN_NAME, branch_name) + except GitCommandError as exc: + raise RepositoryClientError(f"Unexpected error creating new branch. {exc=!r}") from exc + + def create_pull_request(self, branch_name: str) -> str: + """Create a pull request from given branch to the default branch. + + Args: + branch_name: Branch name from which the pull request will be created. + + Raises: + RepositoryClientError: if unexpected error occurred during git operation. + + Returns: + The web url to pull request page. + """ + try: + pull_request = self._github_repo.create_pull( + title=ACTIONS_PULL_REQUEST_TITLE, + body=ACTIONS_PULL_REQUEST_BODY, + base=self._github_repo.default_branch, + head=branch_name, + ) + except GithubException as exc: + raise RepositoryClientError( + f"Unexpected error creating pull request. {exc=!r}" + ) from exc + + return pull_request.html_url + + def is_dirty(self) -> bool: + """Check if repository path has any changes including new files. + + Returns: + True if any changes have occurred. + """ + return self._git_repo.is_dirty(untracked_files=True) + + def get_file_content(self, path: str, branch: str | None = None) -> str: + """Get the content of a file from the default branch. + + Args: + path: The path to the file. + branch: The branch to retrieve the file from. + + Returns: + The content of the file on the default branch. + + Raises: + RepositoryFileNotFoundError: if the file could not be retrieved from GitHub, more than + one file is returned or a non-file is returned + RepositoryClientError: if there is a problem with communicating with GitHub + """ + try: + content_file = ( + self._github_repo.get_contents(path) + if branch is None + else self._github_repo.get_contents(path, branch) + ) + except UnknownObjectException as exc: + raise RepositoryFileNotFoundError( + f"Could not retrieve the file at {path=}. {exc=!r}" + ) from exc + except GithubException as exc: + raise RepositoryClientError(f"Communication with GitHub failed. {exc=!r}") from exc + + if isinstance(content_file, list): + raise RepositoryFileNotFoundError(f"Path matched more than one file {path=}.") + + if content_file.content is None: + raise RepositoryFileNotFoundError(f"Path did not match a file {path=}.") + + return base64.b64decode(content_file.content).decode("utf-8") + + +def _get_repository_name_from_git_url(remote_url: str) -> str: + """Get repository name from git remote URL. + + Args: + remote_url: URL of remote repository. + e.g. https://github.com/canonical/upload-charm-docs.git + + Raises: + InputError: if invalid repository url was given. + + Returns: + Git repository name. e.g. canonical/upload-charm-docs. + """ + matched_repository = HTTPS_URL_PATTERN.match(remote_url) + if not matched_repository: + raise InputError(f"Invalid remote repository url {remote_url=!r}") + return matched_repository.group(1) + + +def create_repository_client(access_token: str | None, base_path: Path) -> Client: + """Create a Github instance to handle communication with Github server. + + Args: + access_token: Access token that has permissions to open a pull request. + base_path: Path where local .git resides in. + + Raises: + InputError: if invalid access token or invalid git remote URL is provided. + + Returns: + A Github repository instance. + """ + if not access_token: + raise InputError( + f"Invalid 'access_token' input, it must be non-empty, got {access_token=!r}" + ) + + local_repo = Repo(base_path) + logging.info("executing in git repository in the directory: %s", local_repo.working_dir) + github_client = Github(login_or_token=access_token) + remote_url = local_repo.remote().url + repository_fullname = _get_repository_name_from_git_url(remote_url=remote_url) + remote_repo = github_client.get_repo(repository_fullname) + return Client(repository=local_repo, github_repository=remote_repo) diff --git a/src/types_.py b/src/types_.py index 875da9d1..b43394ce 100644 --- a/src/types_.py +++ b/src/types_.py @@ -9,6 +9,9 @@ from pathlib import Path from urllib.parse import urlparse +from .discourse import Discourse +from .repository import Client as RepositoryClient + class UserInputsDiscourse(typing.NamedTuple): """Configurable user input values used to run upload-charm-docs. @@ -37,12 +40,14 @@ class UserInputs(typing.NamedTuple): migration mode. github_access_token: A Personal Access Token(PAT) or access token with repository access. Required in migration mode. + base_branch: The branch the documentation is stored on after updating discourse. """ discourse: UserInputsDiscourse dry_run: bool delete_pages: bool github_access_token: str | None + base_branch: str | None class Metadata(typing.NamedTuple): @@ -252,12 +257,14 @@ class ContentChange(typing.NamedTuple): """Represents a change to the content. Attrs: - old: The previous content. - new: The new content. + base: The content which is the base for comparison. + server: The content on the server. + local: The content on the local disk. """ - old: Content | None - new: Content | None + base: Content | None + server: Content + local: Content class IndexContentChange(typing.NamedTuple): @@ -399,3 +406,15 @@ class IndexDocumentMeta(MigrationFileMeta): """ content: str + + +class Clients(typing.NamedTuple): + """Collection of clients needed during execution. + + Attrs: + discourse: Discourse client. + repository: Client for the repository. + """ + + discourse: Discourse + repository: RepositoryClient diff --git a/tests/conftest.py b/tests/conftest.py index 93bf3118..79768706 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -37,8 +37,8 @@ def fixture_test_branch() -> str: return "test" -@pytest.fixture(name="upstream_repository") -def fixture_upstream_repository( +@pytest.fixture(name="upstream_git_repo") +def fixture_upstream_git_repo( upstream_repository_path: Path, default_branch: str, test_branch: str ) -> Repo: """Initialize upstream repository.""" @@ -64,18 +64,15 @@ def fixture_repository_path(tmp_path: Path) -> Path: return repo_path -@pytest.fixture(name="repository") -def fixture_repository( - upstream_repository: Repo, +# upstream_git_repo is required although not used +@pytest.fixture(name="git_repo") +def fixture_git_repo( + upstream_git_repo: Repo, # pylint: disable=unused-argument upstream_repository_path: Path, repository_path: Path, test_branch: str, ) -> Repo: """Create repository with mocked upstream.""" - # uptream_repository is added to create a dependency for the current fixture in order to ensure - # that the repository can be cloned after the upstream has fully initialized. - del upstream_repository - repo = Repo.clone_from(url=upstream_repository_path, to_path=repository_path) repo.git.fetch() repo.git.checkout(test_branch) @@ -119,10 +116,10 @@ def fixture_mock_github(mock_github_repo: Repository) -> Github: @pytest.fixture(name="repository_client") def fixture_repository_client( - repository: Repo, mock_github_repo: Repository + git_repo: Repo, mock_github_repo: Repository ) -> pull_request.RepositoryClient: """Get repository client.""" - return pull_request.RepositoryClient(repository=repository, github_repository=mock_github_repo) + return pull_request.RepositoryClient(repository=git_repo, github_repository=mock_github_repo) @pytest.fixture(name="patch_create_repository_client") @@ -131,12 +128,8 @@ def fixture_patch_create_repository_client( ) -> None: """Patch create_repository_client to return a mocked RepositoryClient.""" - def mock_create_repository_client(access_token: str | None, base_path: Path): + def mock_create_repository_client(**_kwargs): """Mock create_repository_client patch function.""" # noqa: DCO020 - # to accept keywords as arguments - del access_token - del base_path - return repository_client # noqa: DCO030 monkeypatch.setattr(src, "create_repository_client", mock_create_repository_client) diff --git a/tests/factories.py b/tests/factories.py index 73a87d41..a7bcc27a 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -7,6 +7,7 @@ # pylint: disable=too-few-public-methods from pathlib import Path +from typing import Generic, TypeVar import factory @@ -14,9 +15,23 @@ from . import types +T = TypeVar("T") + + +class BaseMetaFactory(Generic[T], factory.base.FactoryMetaClass): + """Used for type hints of factories.""" + + # No need for docstring because it is used for type hints + def __call__(cls, *args, **kwargs) -> T: # noqa: N805 + """Used for type hints of factories.""" # noqa: DCO020 + return super().__call__(*args, **kwargs) # noqa: DCO030 + # The attributes of these classes are generators for the attributes of the meta class -class PathInfoFactory(factory.Factory): +# mypy incorrectly believes the factories don't support metaclass +class PathInfoFactory( + factory.Factory, metaclass=BaseMetaFactory[types_.PathInfo] # type: ignore[misc] +): # Docstrings have been abbreviated for factories, checking for docstrings on model attributes # can be skipped. """Generate PathInfos.""" # noqa: DCO060 @@ -34,7 +49,9 @@ class Meta: alphabetical_rank = factory.Sequence(lambda n: n) -class ActionReportFactory(factory.Factory): +class ActionReportFactory( + factory.Factory, metaclass=BaseMetaFactory[types_.ActionReport] # type: ignore[misc] +): """Generate Action reports.""" # noqa: DCO060 class Meta: @@ -71,7 +88,123 @@ class Params: reason = None -class ContentPageFactory(factory.Factory): +class CreateActionFactory( + factory.Factory, metaclass=BaseMetaFactory[types_.CreateAction] # type: ignore[misc] +): + """Generate CreateActions.""" # noqa: DCO060 + + class Meta: + """Configuration for factory.""" # noqa: DCO060 + + model = types_.CreateAction + abstract = False + + level = factory.Sequence(lambda n: n) + path = factory.Sequence(lambda n: f"path {n}") + navlink_title = factory.Sequence(lambda n: f"title {n}") + content = factory.Sequence(lambda n: f"content {n}") + + +class NavlinkFactory( + factory.Factory, metaclass=BaseMetaFactory[types_.Navlink] # type: ignore[misc] +): + """Generate Navlink.""" # noqa: DCO060 + + class Meta: + """Configuration for factory.""" # noqa: DCO060 + + model = types_.Navlink + abstract = False + + title = factory.Sequence(lambda n: f"navlink-title-{n}") + link = factory.Sequence(lambda n: f"navlink-{n}") + + +class NoopActionFactory( + factory.Factory, metaclass=BaseMetaFactory[types_.NoopAction] # type: ignore[misc] +): + """Generate NoopActions.""" # noqa: DCO060 + + class Meta: + """Configuration for factory.""" # noqa: DCO060 + + model = types_.NoopAction + abstract = False + + level = factory.Sequence(lambda n: n) + path = factory.Sequence(lambda n: f"path {n}") + navlink = factory.SubFactory(NavlinkFactory) + content = factory.Sequence(lambda n: f"content {n}") + + +class NavlinkChangeFactory( + factory.Factory, metaclass=BaseMetaFactory[types_.NavlinkChange] # type: ignore[misc] +): + """Generate NavlinkChange.""" # noqa: DCO060 + + class Meta: + """Configuration for factory.""" # noqa: DCO060 + + model = types_.NavlinkChange + abstract = False + + old = factory.SubFactory(NavlinkFactory) + new = factory.SubFactory(NavlinkFactory) + + +class ContentChangeFactory( + factory.Factory, metaclass=BaseMetaFactory[types_.ContentChange] # type: ignore[misc] +): + """Generate ContentChange.""" # noqa: DCO060 + + class Meta: + """Configuration for factory.""" # noqa: DCO060 + + model = types_.ContentChange + abstract = False + + base = factory.Sequence(lambda n: f"base {n}") + server = factory.Sequence(lambda n: f"server {n}") + local = factory.Sequence(lambda n: f"local {n}") + + +class UpdateActionFactory( + factory.Factory, metaclass=BaseMetaFactory[types_.UpdateAction] # type: ignore[misc] +): + """Generate UpdateActions.""" # noqa: DCO060 + + class Meta: + """Configuration for factory.""" # noqa: DCO060 + + model = types_.UpdateAction + abstract = False + + level = factory.Sequence(lambda n: n) + path = factory.Sequence(lambda n: f"path {n}") + navlink_change = factory.SubFactory(NavlinkChangeFactory) + content_change = factory.SubFactory(ContentChangeFactory) + + +class DeleteActionFactory( + factory.Factory, metaclass=BaseMetaFactory[types_.DeleteAction] # type: ignore[misc] +): + """Generate DeleteActions.""" # noqa: DCO060 + + class Meta: + """Configuration for factory.""" # noqa: DCO060 + + model = types_.DeleteAction + abstract = False + + level = factory.Sequence(lambda n: n) + path = factory.Sequence(lambda n: f"path {n}") + navlink = factory.SubFactory(NavlinkFactory) + content = factory.Sequence(lambda n: f"content {n}") + + +class ContentPageFactory( + factory.Factory, metaclass=BaseMetaFactory[types.DiscoursePageMeta] # type: ignore[misc] +): """Generate discourse content page.""" # noqa: DCO060 class Meta: @@ -84,7 +217,9 @@ class Meta: content = factory.Sequence(lambda n: f"Content {n}") -class UserInputDiscourseFactory(factory.Factory): +class UserInputDiscourseFactory( + factory.Factory, metaclass=BaseMetaFactory[types_.UserInputsDiscourse] # type: ignore[misc] +): """Generate user input tuple.""" # noqa: DCO060 class Meta: @@ -99,7 +234,9 @@ class Meta: api_key = factory.Sequence(lambda n: f"discourse-test-key-{n}") -class UserInputFactory(factory.Factory): +class UserInputsFactory( + factory.Factory, metaclass=BaseMetaFactory[types_.UserInputs] # type: ignore[misc] +): """Generate user input tuple.""" # noqa: DCO060 class Meta: @@ -110,11 +247,14 @@ class Meta: discourse = factory.SubFactory(UserInputDiscourseFactory) github_access_token = factory.Sequence(lambda n: f"test-token-{n}") + base_branch = factory.Sequence(lambda n: f"base-branch-{n}") dry_run = False delete_pages = False -class TableRowFactory(factory.Factory): +class TableRowFactory( + factory.Factory, metaclass=BaseMetaFactory[types_.TableRow] # type: ignore[misc] +): """Generate table row.""" # noqa: DCO060 class Meta: @@ -142,4 +282,4 @@ class Params: level = factory.Sequence(lambda n: n) path = factory.Sequence(lambda n: f"path-{n}") - navlink = factory.Sequence(lambda n: types_.Navlink(f"navlink-title-{n}", link=f"navlink-{n}")) + navlink = factory.SubFactory(NavlinkFactory) diff --git a/tests/integration/test___init__run_conflict.py b/tests/integration/test___init__run_conflict.py new file mode 100644 index 00000000..dd0f5b5e --- /dev/null +++ b/tests/integration/test___init__run_conflict.py @@ -0,0 +1,224 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Integration tests for running the action where there is a merge conflict.""" + +# This test is fairly complex as it simulates sequential action runs +# pylint: disable=too-many-arguments,too-many-locals,too-many-statements +# The tests for reconcile are similar, although it is better to have some minor duplication than +# make the tests less clear +# pylint: disable = duplicate-code + +import logging +from base64 import b64encode +from itertools import chain +from pathlib import Path +from unittest.mock import MagicMock +from urllib.parse import urlparse + +import pytest +from github.ContentFile import ContentFile + +from src import constants, exceptions, metadata, run +from src.discourse import Discourse + +from .. import factories +from ..unit.helpers import assert_substrings_in_string, create_metadata_yaml + +pytestmark = pytest.mark.conflict + + +@pytest.mark.asyncio +@pytest.mark.usefixtures("patch_create_repository_client") +async def test_run_conflict( + discourse_api: Discourse, + caplog: pytest.LogCaptureFixture, + repository_path: Path, + mock_github_repo: MagicMock, +): + """ + arrange: given running discourse server and mocked GitHub client + act: when run is called with: + 1. docs with an index and documentation file + 2. docs with a documentation file updated and discourse updated with non-conflicting + content + 3. docs with a documentation file updated and discourse updated with conflicting content in + dry run mode + 4. docs with a documentation file updated and discourse updated with conflicting content + 5. docs with a documentation file and discourse updated to resolve conflict + assert: then: + 1. the documentation page is created + 2. the documentation page is updated + 3. the documentation page is not updated + 4. the documentation page is not updated + 5. the documentation page is updated + """ + document_name = "name 1" + caplog.set_level(logging.INFO) + create_metadata_yaml( + content=f"{metadata.METADATA_NAME_KEY}: {document_name}", path=repository_path + ) + + # 1. docs with an index and documentation file + caplog.clear() + index_url = discourse_api.create_topic( + title=f"{document_name.replace('-', ' ').title()} Documentation Overview", + content=f"{constants.NAVIGATION_TABLE_START}".strip(), + ) + create_metadata_yaml( + content=f"{metadata.METADATA_NAME_KEY}: name 1\n{metadata.METADATA_DOCS_KEY}: {index_url}", + path=repository_path, + ) + (docs_dir := repository_path / constants.DOCUMENTATION_FOLDER_NAME).mkdir() + (docs_dir / "index.md").write_text("index content 1", encoding="utf-8") + doc_table_key = "doc" + doc_title = "doc title" + (doc_file := docs_dir / f"{doc_table_key}.md").write_text( + doc_content_1 := f"# {doc_title}\nline 1\nline 2\nline 3", encoding="utf-8" + ) + + urls_with_actions = run( + base_path=repository_path, + discourse=discourse_api, + user_inputs=factories.UserInputsFactory( + dry_run=False, delete_pages=True, base_branch=None + ), + ) + + assert len(urls_with_actions) == 2 + (doc_url, _) = urls_with_actions.keys() + assert (urls := tuple(urls_with_actions)) == (doc_url, index_url) + doc_table_line_1 = f"| 1 | {doc_table_key} | [{doc_title}]({urlparse(doc_url).path}) |" + assert_substrings_in_string( + chain(urls, (doc_table_line_1, "Create", "Update", "'success'")), caplog.text + ) + index_topic = discourse_api.retrieve_topic(url=index_url) + assert doc_table_line_1 in index_topic + doc_topic = discourse_api.retrieve_topic(url=doc_url) + assert doc_topic == doc_content_1 + + # 2. docs with a documentation file updated and discourse updated with non-conflicting content + caplog.clear() + doc_file.write_text( + doc_content_2 := f"# {doc_title}\nline 1a\nline 2\nline 3", encoding="utf-8" + ) + discourse_api.update_topic(url=doc_url, content=f"# {doc_title}\nline 1\nline 2\nline 3a") + mock_content_file = MagicMock(spec=ContentFile) + mock_content_file.content = b64encode(doc_content_1.encode(encoding="utf-8")) + mock_github_repo.get_contents.return_value = mock_content_file + + urls_with_actions = run( + base_path=repository_path, + discourse=discourse_api, + user_inputs=factories.UserInputsFactory( + dry_run=False, delete_pages=True, base_branch=None + ), + ) + + assert (urls := tuple(urls_with_actions)) == (doc_url, index_url) + assert_substrings_in_string( + chain(urls, (doc_table_line_1, doc_table_line_1, "Update", "'success'")), caplog.text + ) + index_topic = discourse_api.retrieve_topic(url=index_url) + assert doc_table_line_1 in index_topic + doc_topic = discourse_api.retrieve_topic(url=doc_url) + assert doc_topic == f"# {doc_title}\nline 1a\nline 2\nline 3a" + mock_github_repo.get_contents.assert_called_once_with( + str(doc_file.relative_to(repository_path)) + ) + + # 3. docs with a documentation file updated and discourse updated with conflicting content in + # dry run mode + caplog.clear() + doc_file.write_text(f"# {doc_title}\nline 1a\nline 2a\nline 3", encoding="utf-8") + discourse_api.update_topic( + url=doc_url, content=(doc_topic_content_3 := f"# {doc_title}\nline 1a\nline 2b\nline 3a") + ) + mock_content_file.content = b64encode(doc_content_2.encode(encoding="utf-8")) + + with pytest.raises(exceptions.InputError) as exc_info: + run( + base_path=repository_path, + discourse=discourse_api, + user_inputs=factories.UserInputsFactory( + dry_run=True, delete_pages=True, base_branch=None + ), + ) + + assert_substrings_in_string( + ( + "could not automatically merge, conflicts:\\n", + "# doc title\\n", + "line 1a\\n", + "<<<<<<< HEAD\\n", + "line 2a\\n", + "line 3\\n", + "=======\\n", + "line 2b\\n", + "line 3a\\n", + ">>>>>>> theirs\\n", + ), + caplog.text, + ) + assert_substrings_in_string(("actions", "not", "executed"), str(exc_info.value)) + index_topic = discourse_api.retrieve_topic(url=index_url) + assert doc_table_line_1 in index_topic + doc_topic = discourse_api.retrieve_topic(url=doc_url) + assert doc_topic == doc_topic_content_3 + + # 4. docs with a documentation file updated and discourse updated with conflicting content + caplog.clear() + + with pytest.raises(exceptions.InputError) as exc_info: + run( + base_path=repository_path, + discourse=discourse_api, + user_inputs=factories.UserInputsFactory( + dry_run=False, delete_pages=True, base_branch=None + ), + ) + + assert_substrings_in_string( + ( + "could not automatically merge, conflicts:\\n", + "# doc title\\n", + "line 1a\\n", + "<<<<<<< HEAD\\n", + "line 2a\\n", + "line 3\\n", + "=======\\n", + "line 2b\\n", + "line 3a\\n", + ">>>>>>> theirs\\n", + ), + caplog.text, + ) + assert_substrings_in_string(("actions", "not", "executed"), str(exc_info.value)) + index_topic = discourse_api.retrieve_topic(url=index_url) + assert doc_table_line_1 in index_topic + doc_topic = discourse_api.retrieve_topic(url=doc_url) + assert doc_topic == doc_topic_content_3 + + # 5. docs with a documentation file and discourse updated to resolve conflict + caplog.clear() + doc_file.write_text( + doc_content_4 := f"# {doc_title}\nline 1a\nline 2c\nline 3a", encoding="utf-8" + ) + discourse_api.update_topic(url=doc_url, content=doc_content_4) + + urls_with_actions = run( + base_path=repository_path, + discourse=discourse_api, + user_inputs=factories.UserInputsFactory( + dry_run=False, delete_pages=True, base_branch=None + ), + ) + + assert (urls := tuple(urls_with_actions)) == (doc_url, index_url) + assert_substrings_in_string( + chain(urls, (doc_table_line_1, doc_table_line_1, "Noop", "'success'")), caplog.text + ) + index_topic = discourse_api.retrieve_topic(url=index_url) + assert doc_table_line_1 in index_topic + doc_topic = discourse_api.retrieve_topic(url=doc_url) + assert doc_topic == doc_content_4 diff --git a/tests/integration/test___init__run_migrate.py b/tests/integration/test___init__run_migrate.py index ffbf2d9f..c4c72cb0 100644 --- a/tests/integration/test___init__run_migrate.py +++ b/tests/integration/test___init__run_migrate.py @@ -14,7 +14,7 @@ from git.repo import Repo from github.PullRequest import PullRequest -from src import index, metadata, migration, pull_request, run +from src import constants, metadata, migration, pull_request, run from src.discourse import Discourse from .. import factories @@ -29,9 +29,9 @@ async def test_run_migrate( discourse_address: str, discourse_api: Discourse, caplog: pytest.LogCaptureFixture, - repository: Repo, + git_repo: Repo, repository_path: Path, - upstream_repository: Repo, + upstream_git_repo: Repo, upstream_repository_path: Path, mock_pull_request: PullRequest, ): @@ -79,12 +79,12 @@ async def test_run_migrate( | -- | -- | -- | | 1 | group-1 | [Group 1]() | | 1 | group-2 | [Group 2]() | - | 2 | group-2-content-1 | [Content Link 1]({content_page_1_url}) | - | 2 | group-2-content-2 | [Content Link 2]({content_page_2_url}) | + | 2 | group-2-content-1 | [{content_page_1.content}]({content_page_1_url}) | + | 2 | group-2-content-2 | [{content_page_2.content}]({content_page_2_url}) | | 1 | group-3 | [Group 3]() | | 2 | group-3-group-4 | [Group 4]() | - | 3 | group-3-group-4-content-3 | [Content Link 3]({content_page_3_url}) | - | 2 | group-3-content-4 | [Content Link 4]({content_page_4_url}) | + | 3 | group-3-group-4-content-3 | [{content_page_3.content}]({content_page_3_url}) | + | 2 | group-3-content-4 | [{content_page_4.content}]({content_page_4_url}) | | 1 | group-5 | [Group 5]() |""" index_url = discourse_api.create_topic( title=f"{document_name.replace('-', ' ').title()} Documentation Overview", @@ -101,11 +101,11 @@ async def test_run_migrate( urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory(), + user_inputs=factories.UserInputsFactory(), ) - upstream_repository.git.checkout(pull_request.DEFAULT_BRANCH_NAME) - upstream_doc_dir = upstream_repository_path / index.DOCUMENTATION_FOLDER_NAME + upstream_git_repo.git.checkout(pull_request.DEFAULT_BRANCH_NAME) + upstream_doc_dir = upstream_repository_path / constants.DOCUMENTATION_FOLDER_NAME assert tuple(urls_with_actions) == (mock_pull_request.html_url,) assert (group_1_path := upstream_doc_dir / "group-1").is_dir() assert (group_1_path / migration.GITKEEP_FILENAME).is_file() @@ -121,12 +121,12 @@ async def test_run_migrate( # 2. with no changes applied after migration caplog.clear() - repository.git.checkout(pull_request.DEFAULT_BRANCH_NAME) + git_repo.git.checkout(pull_request.DEFAULT_BRANCH_NAME) urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory(), + user_inputs=factories.UserInputsFactory(), ) assert_substrings_in_string( diff --git a/tests/integration/test___init__run_reconcile.py b/tests/integration/test___init__run_reconcile.py index c4862490..8e91b18b 100644 --- a/tests/integration/test___init__run_reconcile.py +++ b/tests/integration/test___init__run_reconcile.py @@ -7,13 +7,16 @@ # pylint: disable=too-many-arguments,too-many-locals,too-many-statements import logging +from base64 import b64encode from itertools import chain from pathlib import Path +from unittest.mock import MagicMock from urllib.parse import urlparse import pytest +from github.ContentFile import ContentFile -from src import exceptions, index, metadata, reconcile, run +from src import constants, exceptions, metadata, run from src.discourse import Discourse from .. import factories @@ -28,6 +31,7 @@ async def test_run( discourse_api: Discourse, caplog: pytest.LogCaptureFixture, repository_path: Path, + mock_github_repo: MagicMock, ): """ arrange: given running discourse server @@ -71,27 +75,26 @@ async def test_run( caplog.clear() index_url = discourse_api.create_topic( title=f"{document_name.replace('-', ' ').title()} Documentation Overview", - content=f"{reconcile.NAVIGATION_TABLE_START}".strip(), + content=f"{constants.NAVIGATION_TABLE_START}".strip(), ) create_metadata_yaml( content=f"{metadata.METADATA_NAME_KEY}: name 1\n{metadata.METADATA_DOCS_KEY}: {index_url}", path=repository_path, ) - (docs_dir := repository_path / index.DOCUMENTATION_FOLDER_NAME).mkdir() - (index_file := docs_dir / "index.md").write_text(index_content := "index content 1") + (docs_dir := repository_path / constants.DOCUMENTATION_FOLDER_NAME).mkdir() + (index_file := docs_dir / "index.md").write_text( + index_content := "index content 1", encoding="utf-8" + ) urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( - dry_run=True, - delete_pages=True, - ), + user_inputs=factories.UserInputsFactory(dry_run=True, delete_pages=True), ) assert tuple(urls_with_actions) == (index_url,) index_topic = discourse_api.retrieve_topic(url=index_url) - assert index_topic == f"{reconcile.NAVIGATION_TABLE_START}".strip() + assert index_topic == f"{constants.NAVIGATION_TABLE_START}".strip() assert_substrings_in_string((index_url, "Update", "'skip'"), caplog.text) # 2. docs with an index file @@ -100,26 +103,25 @@ async def test_run( urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( - dry_run=False, - delete_pages=True, - ), + user_inputs=factories.UserInputsFactory(dry_run=False, delete_pages=True), ) assert tuple(urls_with_actions) == (index_url,) index_topic = discourse_api.retrieve_topic(url=index_url) - assert index_topic == f"{index_content}{reconcile.NAVIGATION_TABLE_START}" + assert index_topic == f"{index_content}{constants.NAVIGATION_TABLE_START}" assert_substrings_in_string((index_url, "Update", "'success'"), caplog.text) # 3. docs with a documentation file added in dry run mode caplog.clear() doc_table_key = "doc" - (doc_file := docs_dir / f"{doc_table_key}.md").write_text(doc_content_1 := "doc content 1") + (doc_file := docs_dir / f"{doc_table_key}.md").write_text( + doc_content_1 := "doc content 1", encoding="utf-8" + ) urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( + user_inputs=factories.UserInputsFactory( dry_run=True, delete_pages=True, ), @@ -136,10 +138,7 @@ async def test_run( urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( - dry_run=False, - delete_pages=True, - ), + user_inputs=factories.UserInputsFactory(dry_run=False, delete_pages=True), ) assert len(urls_with_actions) == 2 @@ -156,15 +155,15 @@ async def test_run( # 5. docs with a documentation file updated in dry run mode caplog.clear() - doc_file.write_text(doc_content_2 := "doc content 2") + doc_file.write_text(doc_content_2 := "doc content 2", encoding="utf-8") + mock_content_file = MagicMock(spec=ContentFile) + mock_content_file.content = b64encode(doc_content_1.encode(encoding="utf-8")) + mock_github_repo.get_contents.return_value = mock_content_file urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( - dry_run=True, - delete_pages=True, - ), + user_inputs=factories.UserInputsFactory(dry_run=True, delete_pages=True, base_branch=None), ) assert (urls := tuple(urls_with_actions)) == (doc_url, index_url) @@ -173,6 +172,9 @@ async def test_run( assert doc_table_line_1 in index_topic doc_topic = discourse_api.retrieve_topic(url=doc_url) assert doc_topic == doc_content_1 + mock_github_repo.get_contents.assert_called_once_with( + str(doc_file.relative_to(repository_path)) + ) # 6. docs with a documentation file updated caplog.clear() @@ -180,10 +182,7 @@ async def test_run( urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( - dry_run=False, - delete_pages=True, - ), + user_inputs=factories.UserInputsFactory(dry_run=False, delete_pages=True), ) assert (urls := tuple(urls_with_actions)) == (doc_url, index_url) @@ -204,10 +203,7 @@ async def test_run( urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( - dry_run=False, - delete_pages=True, - ), + user_inputs=factories.UserInputsFactory(dry_run=False, delete_pages=True), ) assert (urls := tuple(urls_with_actions)) == (doc_url, index_url) @@ -222,16 +218,13 @@ async def test_run( caplog.clear() nested_dir_doc_table_key = "nested-dir-doc" (nested_dir_doc_file := nested_dir / "doc.md").write_text( - nested_dir_doc_content := "nested dir doc content 1" + nested_dir_doc_content := "nested dir doc content 1", encoding="utf-8" ) urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( - dry_run=False, - delete_pages=True, - ), + user_inputs=factories.UserInputsFactory(dry_run=False, delete_pages=True), ) assert len(urls_with_actions) == 3 @@ -256,10 +249,7 @@ async def test_run( urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( - dry_run=True, - delete_pages=True, - ), + user_inputs=factories.UserInputsFactory(dry_run=True, delete_pages=True), ) assert (urls := tuple(urls_with_actions)) == (doc_url, nested_dir_doc_url, index_url) @@ -272,16 +262,13 @@ async def test_run( assert nested_dir_doc_topic == nested_dir_doc_content # 10. docs with the documentation file in the nested directory removed with page deletion - # disabled + # disabled caplog.clear() urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( - dry_run=False, - delete_pages=False, - ), + user_inputs=factories.UserInputsFactory(dry_run=False, delete_pages=False), ) assert (urls := tuple(urls_with_actions)) == (doc_url, nested_dir_doc_url, index_url) @@ -300,10 +287,7 @@ async def test_run( urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( - dry_run=False, - delete_pages=True, - ), + user_inputs=factories.UserInputsFactory(dry_run=False, delete_pages=True), ) assert (urls := tuple(urls_with_actions)) == (doc_url, index_url) @@ -320,10 +304,7 @@ async def test_run( urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( - dry_run=False, - delete_pages=True, - ), + user_inputs=factories.UserInputsFactory(dry_run=False, delete_pages=True), ) assert (urls := tuple(urls_with_actions)) == (doc_url, index_url) @@ -342,10 +323,7 @@ async def test_run( urls_with_actions = run( base_path=repository_path, discourse=discourse_api, - user_inputs=factories.UserInputFactory( - dry_run=False, - delete_pages=True, - ), + user_inputs=factories.UserInputsFactory(dry_run=False, delete_pages=True), ) assert (urls := tuple(urls_with_actions)) == (index_url,) diff --git a/tests/integration/test_discourse.py b/tests/integration/test_discourse.py index f49aa9b5..3fec1f64 100644 --- a/tests/integration/test_discourse.py +++ b/tests/integration/test_discourse.py @@ -81,6 +81,7 @@ async def test_create_retrieve_update_delete_topic( slug = url_path_components[-2] topic_id = url_path_components[-1] topic = discourse_client.topic(slug=slug, topic_id=topic_id) + assert topic assert ( topic["category_id"] == discourse_category_id ), "post was not created with the correct category id" @@ -104,6 +105,7 @@ async def test_create_retrieve_update_delete_topic( discourse_api.delete_topic(url=url) topic = discourse_client.topic(slug=slug, topic_id=topic_id) + assert topic assert ( "topic deleted by author" in topic["post_stream"]["posts"][0]["cooked"] ), "topic not deleted" diff --git a/tests/unit/action/__init__.py b/tests/unit/action/__init__.py new file mode 100644 index 00000000..4dbd5891 --- /dev/null +++ b/tests/unit/action/__init__.py @@ -0,0 +1,4 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Unit tests for actions.""" diff --git a/tests/unit/test_action.py b/tests/unit/action/test_other_actions.py similarity index 72% rename from tests/unit/test_action.py rename to tests/unit/action/test_other_actions.py index 7a5c4b7f..e86b0d1b 100644 --- a/tests/unit/test_action.py +++ b/tests/unit/action/test_other_actions.py @@ -15,7 +15,7 @@ from src import action, discourse, exceptions from src import types_ as src_types -from .helpers import assert_substrings_in_string +from ..helpers import assert_substrings_in_string @pytest.mark.parametrize( @@ -213,263 +213,6 @@ def test__noop( assert returned_report.reason is None -@pytest.mark.parametrize( - "dry_run", - [ - pytest.param(True, id="dry run mode enabled"), - pytest.param(False, id="dry run mode disabled"), - ], -) -def test__update_directory(dry_run: bool, caplog: pytest.LogCaptureFixture): - """ - arrange: given update action for a directory, dry run mode and mocked discourse - act: when action is passed to _update with dry_run - assert: then no topic is updated, the action is logged and the expected report is returned. - """ - caplog.set_level(logging.INFO) - mocked_discourse = mock.MagicMock(spec=discourse.Discourse) - update_action = src_types.UpdateAction( - level=(level := 1), - path=(path := "path 1"), - navlink_change=src_types.NavlinkChange( - old=src_types.Navlink(title="title 1", link=None), - new=src_types.Navlink(title="title 2", link=None), - ), - content_change=None, - ) - - returned_report = action._update( - action=update_action, discourse=mocked_discourse, dry_run=dry_run - ) - - assert_substrings_in_string((f"action: {update_action}", f"dry run: {dry_run}"), caplog.text) - mocked_discourse.update_topic.assert_not_called() - assert returned_report.table_row is not None - assert returned_report.table_row.level == level - assert returned_report.table_row.path == path - assert returned_report.table_row.navlink == update_action.navlink_change.new - assert returned_report.location is None - assert ( - returned_report.result == src_types.ActionResult.SKIP - if dry_run - else src_types.ActionResult.SUCCESS - ) - assert returned_report.reason == (action.DRY_RUN_REASON if dry_run else None) - - -def test__update_file_dry_run(caplog: pytest.LogCaptureFixture): - """ - arrange: given update action for a file and mocked discourse - act: when action is passed to _update with dry_run True - assert: then no topic is updated, the action is logged and a skip report is returned. - """ - caplog.set_level(logging.INFO) - mocked_discourse = mock.MagicMock(spec=discourse.Discourse) - url = "url 1" - mocked_discourse.absolute_url.return_value = url - update_action = src_types.UpdateAction( - level=(level := 1), - path=(path := "path 1"), - navlink_change=src_types.NavlinkChange( - old=src_types.Navlink(title="title 1", link=(link := "link 1")), - new=src_types.Navlink(title="title 2", link=link), - ), - content_change=src_types.ContentChange( - old=(old_content := "content 1\n"), new=(new_content := "content 2\n") - ), - ) - - returned_report = action._update( - action=update_action, discourse=mocked_discourse, dry_run=True - ) - - assert_substrings_in_string( - ( - f"action: {update_action}", - f"dry run: {True}", - old_content, - new_content, - f"content change:\n- {old_content}? ^\n+ {new_content}? ^\n", - ), - caplog.text, - ) - mocked_discourse.update_topic.assert_not_called() - assert returned_report.table_row is not None - assert returned_report.table_row.level == level - assert returned_report.table_row.path == path - assert returned_report.table_row.navlink == update_action.navlink_change.new - assert returned_report.location == url - assert returned_report.result == src_types.ActionResult.SKIP - assert returned_report.reason == action.DRY_RUN_REASON - - -def test__update_file_navlink_title_change(caplog: pytest.LogCaptureFixture): - """ - arrange: given update action for a file where only the navlink title has changed and mocked - discourse - act: when action is passed to _update with dry_run False - assert: then no topic is updated, the action is logged and the expected table row is returned. - """ - caplog.set_level(logging.INFO) - mocked_discourse = mock.MagicMock(spec=discourse.Discourse) - url = "url 1" - mocked_discourse.absolute_url.return_value = url - update_action = src_types.UpdateAction( - level=(level := 1), - path=(path := "path 1"), - navlink_change=src_types.NavlinkChange( - old=src_types.Navlink(title="title 1", link=(link := "link 1")), - new=src_types.Navlink(title="title 2", link=link), - ), - content_change=src_types.ContentChange(old=(content := "content 1"), new=content), - ) - - returned_report = action._update( - action=update_action, discourse=mocked_discourse, dry_run=False - ) - - assert_substrings_in_string( - (f"action: {update_action}", f"dry run: {False}", content), - caplog.text, - ) - mocked_discourse.update_topic.assert_not_called() - assert returned_report.table_row is not None - assert returned_report.table_row.level == level - assert returned_report.table_row.path == path - assert returned_report.table_row.navlink == update_action.navlink_change.new - assert returned_report.location == url - assert returned_report.result == src_types.ActionResult.SUCCESS - assert returned_report.reason is None - - -def test__update_file_navlink_content_change_discourse_error(caplog: pytest.LogCaptureFixture): - """ - arrange: given update action for a file where content has changed and mocked discourse that - raises an error - act: when action is passed to _update with dry_run False - assert: then topic is updated, the action is logged and a fail report is returned. - """ - caplog.set_level(logging.INFO) - mocked_discourse = mock.MagicMock(spec=discourse.Discourse) - url = "url 1" - mocked_discourse.absolute_url.return_value = url - mocked_discourse.update_topic.side_effect = (error := exceptions.DiscourseError("failed")) - update_action = src_types.UpdateAction( - level=(level := 1), - path=(path := "path 1"), - navlink_change=src_types.NavlinkChange( - old=src_types.Navlink(title="title 1", link=(link := "link 1")), - new=src_types.Navlink(title="title 2", link=link), - ), - content_change=src_types.ContentChange( - old=(old_content := "content 1"), new=(new_content := "content 2") - ), - ) - - returned_report = action._update( - action=update_action, discourse=mocked_discourse, dry_run=False - ) - - assert_substrings_in_string( - ( - f"action: {update_action}", - f"dry run: {False}", - old_content, - new_content, - f"content change:\n- {old_content}\n? ^\n+ {new_content}\n? ^\n", - ), - caplog.text, - ) - assert update_action.content_change is not None - mocked_discourse.update_topic.assert_called_once_with( - url=link, content=update_action.content_change.new - ) - assert returned_report.table_row is not None - assert returned_report.table_row.level == level - assert returned_report.table_row.path == path - assert returned_report.table_row.navlink == update_action.navlink_change.new - assert returned_report.location == url - assert returned_report.result == src_types.ActionResult.FAIL - assert returned_report.reason == str(error) - - -def test__update_file_navlink_content_change(caplog: pytest.LogCaptureFixture): - """ - arrange: given update action for a file where content has changed and mocked discourse - act: when action is passed to _update with dry_run False - assert: then topic is updated, the action is logged and success report is returned. - """ - caplog.set_level(logging.INFO) - mocked_discourse = mock.MagicMock(spec=discourse.Discourse) - url = "url 1" - mocked_discourse.absolute_url.return_value = url - update_action = src_types.UpdateAction( - level=(level := 1), - path=(path := "path 1"), - navlink_change=src_types.NavlinkChange( - old=src_types.Navlink(title="title 1", link=(link := "link 1")), - new=src_types.Navlink(title="title 2", link=link), - ), - content_change=src_types.ContentChange( - old=(old_content := "content 1\n"), new=(new_content := "content 2\n") - ), - ) - - returned_report = action._update( - action=update_action, discourse=mocked_discourse, dry_run=False - ) - - assert_substrings_in_string( - ( - f"action: {update_action}", - f"dry run: {False}", - old_content, - new_content, - f"content change:\n- {old_content}? ^\n+ {new_content}? ^\n", - ), - caplog.text, - ) - assert update_action.content_change is not None - mocked_discourse.update_topic.assert_called_once_with( - url=link, content=update_action.content_change.new - ) - assert returned_report.table_row is not None - assert returned_report.table_row.level == level - assert returned_report.table_row.path == path - assert returned_report.table_row.navlink == update_action.navlink_change.new - assert returned_report.location == url - assert returned_report.result == src_types.ActionResult.SUCCESS - assert returned_report.reason is None - - -@pytest.mark.parametrize( - "content_change", - [ - pytest.param(None, id="None"), - pytest.param(src_types.ContentChange(old="content 1", new=None), id="new is None"), - ], -) -def test__update_file_navlink_content_change_error(content_change: src_types.ContentChange | None): - """ - arrange: given update action for a file where content has changed to None - act: when action is passed to _update with dry_run False - assert: ActionError is raised. - """ - mocked_discourse = mock.MagicMock(spec=discourse.Discourse) - update_action = src_types.UpdateAction( - level=1, - path="path 1", - navlink_change=src_types.NavlinkChange( - old=src_types.Navlink(title="title 1", link=(link := "link 1")), - new=src_types.Navlink(title="title 2", link=link), - ), - content_change=content_change, - ) - - with pytest.raises(exceptions.ActionError): - action._update(action=update_action, discourse=mocked_discourse, dry_run=False) - - @pytest.mark.parametrize( "dry_run, delete_pages, navlink_link, expected_result, expected_reason", [ diff --git a/tests/unit/action/test_update_action.py b/tests/unit/action/test_update_action.py new file mode 100644 index 00000000..fa95a2be --- /dev/null +++ b/tests/unit/action/test_update_action.py @@ -0,0 +1,357 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Unit tests for action.""" + +# Need access to protected functions for testing +# For some reason pylint is detecting duplicate code when there isn't any +# pylint: disable=protected-access,duplicate-code + +import logging +from unittest import mock + +import pytest + +from src import action, discourse, exceptions +from src import types_ as src_types + +from ... import factories +from ..helpers import assert_substrings_in_string + + +@pytest.mark.parametrize( + "dry_run", + [ + pytest.param(True, id="dry run mode enabled"), + pytest.param(False, id="dry run mode disabled"), + ], +) +def test__update_directory(dry_run: bool, caplog: pytest.LogCaptureFixture): + """ + arrange: given update action for a directory, dry run mode and mocked discourse + act: when action is passed to _update with dry_run + assert: then no topic is updated, the action is logged and the expected report is returned. + """ + caplog.set_level(logging.INFO) + mocked_discourse = mock.MagicMock(spec=discourse.Discourse) + update_action = src_types.UpdateAction( + level=(level := 1), + path=(path := "path 1"), + navlink_change=src_types.NavlinkChange( + old=src_types.Navlink(title="title 1", link=None), + new=src_types.Navlink(title="title 2", link=None), + ), + content_change=None, + ) + + returned_report = action._update( + action=update_action, discourse=mocked_discourse, dry_run=dry_run + ) + + assert_substrings_in_string((f"action: {update_action}", f"dry run: {dry_run}"), caplog.text) + mocked_discourse.update_topic.assert_not_called() + assert returned_report.table_row is not None + assert returned_report.table_row.level == level + assert returned_report.table_row.path == path + assert returned_report.table_row.navlink == update_action.navlink_change.new + assert returned_report.location is None + assert ( + returned_report.result == src_types.ActionResult.SKIP + if dry_run + else src_types.ActionResult.SUCCESS + ) + assert returned_report.reason == (action.DRY_RUN_REASON if dry_run else None) + + +def test__update_file_dry_run(caplog: pytest.LogCaptureFixture): + """ + arrange: given update action for a file and mocked discourse + act: when action is passed to _update with dry_run True + assert: then no topic is updated, the action is logged and a skip report is returned. + """ + caplog.set_level(logging.INFO) + mocked_discourse = mock.MagicMock(spec=discourse.Discourse) + url = "url 1" + mocked_discourse.absolute_url.return_value = url + server_content: str + update_action = src_types.UpdateAction( + level=(level := 1), + path=(path := "path 1"), + navlink_change=src_types.NavlinkChange( + old=src_types.Navlink(title="title 1", link=(link := "link 1")), + new=src_types.Navlink(title="title 2", link=link), + ), + content_change=src_types.ContentChange( + server=(server_content := "content 1\n"), + local=(local_content := "content 2\n"), + base=server_content, + ), + ) + dry_run = True + + returned_report = action._update( + action=update_action, discourse=mocked_discourse, dry_run=dry_run + ) + + assert_substrings_in_string( + ( + f"action: {update_action}", + f"dry run: {dry_run}", + server_content, + local_content, + f"content change:\n- {server_content}? ^\n+ {local_content}? ^\n", + ), + caplog.text, + ) + mocked_discourse.update_topic.assert_not_called() + assert returned_report.table_row is not None + assert returned_report.table_row.level == level + assert returned_report.table_row.path == path + assert returned_report.table_row.navlink == update_action.navlink_change.new + assert returned_report.location == url + assert returned_report.result == src_types.ActionResult.SKIP + assert returned_report.reason == action.DRY_RUN_REASON + + +def test__update_file_navlink_title_change(caplog: pytest.LogCaptureFixture): + """ + arrange: given update action for a file where only the navlink title has changed and mocked + discourse and the content missing from the default branch + act: when action is passed to _update with dry_run False + assert: then no topic is updated, the action is logged and the expected table row is returned. + """ + caplog.set_level(logging.INFO) + mocked_discourse = mock.MagicMock(spec=discourse.Discourse) + url = "url 1" + mocked_discourse.absolute_url.return_value = url + content: str + update_action = src_types.UpdateAction( + level=(level := 1), + path=(path := "path 1"), + navlink_change=src_types.NavlinkChange( + old=src_types.Navlink(title="title 1", link=(link := "link 1")), + new=src_types.Navlink(title="title 2", link=link), + ), + content_change=src_types.ContentChange( + server=(content := "content 1"), local=content, base=None + ), + ) + dry_run = False + + returned_report = action._update( + action=update_action, discourse=mocked_discourse, dry_run=dry_run + ) + + assert_substrings_in_string( + (f"action: {update_action}", f"dry run: {dry_run}", content), + caplog.text, + ) + mocked_discourse.update_topic.assert_not_called() + assert returned_report.table_row is not None + assert returned_report.table_row.level == level + assert returned_report.table_row.path == path + assert returned_report.table_row.navlink == update_action.navlink_change.new + assert returned_report.location == url + assert returned_report.result == src_types.ActionResult.SUCCESS + assert returned_report.reason is None + + +def test__update_file_navlink_content_change_discourse_error(caplog: pytest.LogCaptureFixture): + """ + arrange: given update action for a file where content has changed and mocked discourse that + raises an error + act: when action is passed to _update with dry_run False + assert: then topic is updated, the action is logged and a fail report is returned. + """ + caplog.set_level(logging.INFO) + mocked_discourse = mock.MagicMock(spec=discourse.Discourse) + url = "url 1" + mocked_discourse.absolute_url.return_value = url + mocked_discourse.update_topic.side_effect = (error := exceptions.DiscourseError("failed")) + server_content: str + update_action = src_types.UpdateAction( + level=(level := 1), + path=(path := "path 1"), + navlink_change=src_types.NavlinkChange( + old=src_types.Navlink(title="title 1", link=(link := "link 1")), + new=src_types.Navlink(title="title 2", link=link), + ), + content_change=src_types.ContentChange( + server=(server_content := "content 1"), + local=(local_content := "content 2"), + base=server_content, + ), + ) + dry_run = False + + returned_report = action._update( + action=update_action, discourse=mocked_discourse, dry_run=dry_run + ) + + assert_substrings_in_string( + ( + f"action: {update_action}", + f"dry run: {dry_run}", + server_content, + local_content, + f"content change:\n- {server_content}\n? ^\n+ {local_content}\n? ^\n", + ), + caplog.text, + ) + assert update_action.content_change is not None + mocked_discourse.update_topic.assert_called_once_with( + url=link, content=update_action.content_change.local + ) + assert returned_report.table_row is not None + assert returned_report.table_row.level == level + assert returned_report.table_row.path == path + assert returned_report.table_row.navlink == update_action.navlink_change.new + assert returned_report.location == url + assert returned_report.result == src_types.ActionResult.FAIL + assert returned_report.reason == str(error) + + +@pytest.mark.parametrize( + "content_change, expected_log_contents, expected_reason_contents", + [ + pytest.param( + factories.ContentChangeFactory(base="x", server="y", local="z"), + ("content change:\n- x\n+ z\n",), + ("merge", "conflict"), + id="merge conflict", + ), + pytest.param( + factories.ContentChangeFactory(base=None, server="y", local="z"), + (), + ("no", "base"), + id="no base", + ), + ], +) +def test__update_file_navlink_content_change_conflict( + content_change: src_types.ContentChange, + expected_log_contents: tuple[str, ...], + expected_reason_contents: tuple[str, ...], + caplog: pytest.LogCaptureFixture, +): + """ + arrange: given update action for a file where content has changed and mocked discourse + act: when action is passed to _update with dry_run False + assert: then topic is not updated, the action is logged and a fail report is returned. + """ + caplog.set_level(logging.INFO) + mocked_discourse = mock.MagicMock(spec=discourse.Discourse) + url = "url 1" + mocked_discourse.absolute_url.return_value = url + update_action = src_types.UpdateAction( + level=(level := 1), + path=(path := "path 1"), + navlink_change=src_types.NavlinkChange( + old=src_types.Navlink(title="title 1", link=(link := "link 1")), + new=src_types.Navlink(title="title 2", link=link), + ), + content_change=content_change, + ) + dry_run = False + + returned_report = action._update( + action=update_action, discourse=mocked_discourse, dry_run=dry_run + ) + + assert_substrings_in_string( + ( + f"action: {update_action}", + f"dry run: {dry_run}", + repr(content_change.base), + repr(content_change.server), + repr(content_change.local), + *expected_log_contents, + ), + caplog.text, + ) + assert update_action.content_change is not None + mocked_discourse.update_topic.assert_not_called() + assert returned_report.table_row is not None + assert returned_report.table_row.level == level + assert returned_report.table_row.path == path + assert returned_report.table_row.navlink == update_action.navlink_change.new + assert returned_report.location == url + assert returned_report.result == src_types.ActionResult.FAIL + assert returned_report.reason is not None + assert_substrings_in_string(expected_reason_contents, returned_report.reason) + + +def test__update_file_navlink_content_change(caplog: pytest.LogCaptureFixture): + """ + arrange: given update action for a file where content has changed and mocked discourse + act: when action is passed to _update with dry_run False + assert: then topic is updated with merged content, the action is logged and success report is + returned. + """ + caplog.set_level(logging.INFO) + mocked_discourse = mock.MagicMock(spec=discourse.Discourse) + url = "url 1" + mocked_discourse.absolute_url.return_value = url + server_content: str + update_action = src_types.UpdateAction( + level=(level := 1), + path=(path := "path 1"), + navlink_change=src_types.NavlinkChange( + old=src_types.Navlink(title="title 1", link=(link := "link 1")), + new=src_types.Navlink(title="title 2", link=link), + ), + content_change=src_types.ContentChange( + server=(server_content := "line 1a\nline 2\nline 3\n"), + local=(local_content := "line 1\nline 2\nline 3a\n"), + base=(base_content := "line 1\nline 2\nline 3\n"), + ), + ) + dry_run = False + + returned_report = action._update( + action=update_action, discourse=mocked_discourse, dry_run=dry_run + ) + + assert_substrings_in_string( + ( + f"action: {update_action}", + f"dry run: {dry_run}", + repr(base_content), + repr(server_content), + repr(local_content), + "content change:\n line 1\n line 2\n- line 3\n+ line 3a\n? +\n", + ), + caplog.text, + ) + assert update_action.content_change is not None + mocked_discourse.update_topic.assert_called_once_with( + url=link, content="line 1a\nline 2\nline 3a\n" + ) + assert returned_report.table_row is not None + assert returned_report.table_row.level == level + assert returned_report.table_row.path == path + assert returned_report.table_row.navlink == update_action.navlink_change.new + assert returned_report.location == url + assert returned_report.result == src_types.ActionResult.SUCCESS + assert returned_report.reason is None + + +def test__update_file_navlink_content_change_error(): + """ + arrange: given update action for a file where content change is None + act: when action is passed to _update with dry_run False + assert: ActionError is raised. + """ + mocked_discourse = mock.MagicMock(spec=discourse.Discourse) + update_action = src_types.UpdateAction( + level=1, + path="path 1", + navlink_change=src_types.NavlinkChange( + old=src_types.Navlink(title="title 1", link=(link := "link 1")), + new=src_types.Navlink(title="title 2", link=link), + ), + content_change=None, + ) + + with pytest.raises(exceptions.ActionError): + action._update(action=update_action, discourse=mocked_discourse, dry_run=False) diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 10d0b7ed..37064c7f 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -9,7 +9,7 @@ import pytest import requests -from src import index +from src import constants, repository, types_ from src.discourse import Discourse from . import helpers @@ -60,9 +60,17 @@ def fixture_topic_url(discourse_mocked_get_requests_session: Discourse) -> str: @pytest.fixture() def index_file_content(tmp_path: Path) -> str: """Create index file.""" - docs_directory = tmp_path / index.DOCUMENTATION_FOLDER_NAME + docs_directory = tmp_path / constants.DOCUMENTATION_FOLDER_NAME docs_directory.mkdir() - index_file = docs_directory / index.DOCUMENTATION_INDEX_FILENAME + index_file = docs_directory / constants.DOCUMENTATION_INDEX_FILENAME content = "content 1" index_file.write_text(content, encoding="utf-8") return content + + +@pytest.fixture() +def mocked_clients(): + """Create index file.""" + mocked_discourse = mock.MagicMock(spec=Discourse) + mocked_repository = mock.MagicMock(spec=repository.Client) + return types_.Clients(discourse=mocked_discourse, repository=mocked_repository) diff --git a/tests/unit/test___init__.py b/tests/unit/test___init__.py index 97a66152..91f20c32 100644 --- a/tests/unit/test___init__.py +++ b/tests/unit/test___init__.py @@ -18,46 +18,42 @@ GETTING_STARTED, _run_migrate, _run_reconcile, + constants, discourse, exceptions, index, metadata, pull_request, - reconcile, run, types_, ) from .. import factories -from .helpers import create_metadata_yaml +from .helpers import assert_substrings_in_string, create_metadata_yaml -def test__run_reconcile_empty_local_server(tmp_path: Path): +def test__run_reconcile_empty_local_server(tmp_path: Path, mocked_clients): """ arrange: given metadata with name but not docs and empty docs folder and mocked discourse act: when _run_reconcile is called assert: then an index page is created with empty navigation table. """ meta = types_.Metadata(name="name 1", docs=None) - mocked_discourse = mock.MagicMock(spec=discourse.Discourse) - mocked_discourse.create_topic.return_value = (url := "url 1") + mocked_clients.discourse.create_topic.return_value = (url := "url 1") + user_inputs = factories.UserInputsFactory(dry_run=False, delete_pages=True) returned_page_interactions = _run_reconcile( - base_path=tmp_path, - metadata=meta, - discourse=mocked_discourse, - dry_run=False, - delete_pages=True, + base_path=tmp_path, metadata=meta, clients=mocked_clients, user_inputs=user_inputs ) - mocked_discourse.create_topic.assert_called_once_with( + mocked_clients.discourse.create_topic.assert_called_once_with( title="Name 1 Documentation Overview", - content=f"{reconcile.NAVIGATION_TABLE_START.strip()}", + content=f"{constants.NAVIGATION_TABLE_START.strip()}", ) assert returned_page_interactions == {url: types_.ActionResult.SUCCESS} -def test__run_reconcile_local_empty_server(tmp_path: Path): +def test__run_reconcile_local_empty_server(tmp_path: Path, mocked_clients): """ arrange: given metadata with name but not docs and docs folder with a file and mocked discourse act: when _run_reconcile is called @@ -69,28 +65,24 @@ def test__run_reconcile_local_empty_server(tmp_path: Path): (docs_folder := tmp_path / "docs").mkdir() (docs_folder / "index.md").write_text(index_content := "index content") (docs_folder / "page.md").write_text(page_content := "page content") - mocked_discourse = mock.MagicMock(spec=discourse.Discourse) - mocked_discourse.create_topic.side_effect = [ + mocked_clients.discourse.create_topic.side_effect = [ (page_url := "url 1"), (index_url := "url 2"), ] + user_inputs = factories.UserInputsFactory(dry_run=False, delete_pages=True) returned_page_interactions = _run_reconcile( - base_path=tmp_path, - metadata=meta, - discourse=mocked_discourse, - dry_run=False, - delete_pages=True, + base_path=tmp_path, metadata=meta, clients=mocked_clients, user_inputs=user_inputs ) - assert mocked_discourse.create_topic.call_count == 2 - mocked_discourse.create_topic.assert_any_call( + assert mocked_clients.discourse.create_topic.call_count == 2 + mocked_clients.discourse.create_topic.assert_any_call( title=f"{name} docs: {page_content}", content=page_content ) - mocked_discourse.create_topic.assert_any_call( + mocked_clients.discourse.create_topic.assert_any_call( title="Name 1 Documentation Overview", content=( - f"{index_content}{reconcile.NAVIGATION_TABLE_START}\n" + f"{index_content}{constants.NAVIGATION_TABLE_START}\n" f"| 1 | page | [{page_content}]({page_url}) |" ), ) @@ -100,7 +92,7 @@ def test__run_reconcile_local_empty_server(tmp_path: Path): } -def test__run_reconcile_local_empty_server_dry_run(tmp_path: Path): +def test__run_reconcile_local_empty_server_dry_run(tmp_path: Path, mocked_clients): """ arrange: given metadata with name but not docs and docs folder with a file and mocked discourse act: when _run_reconcile is called with dry run mode enabled @@ -110,21 +102,17 @@ def test__run_reconcile_local_empty_server_dry_run(tmp_path: Path): (docs_folder := tmp_path / "docs").mkdir() (docs_folder / "index.md").write_text("index content") (docs_folder / "page.md").write_text("page content") - mocked_discourse = mock.MagicMock(spec=discourse.Discourse) + user_inputs = factories.UserInputsFactory(dry_run=True, delete_pages=True) returned_page_interactions = _run_reconcile( - base_path=tmp_path, - metadata=meta, - discourse=mocked_discourse, - dry_run=True, - delete_pages=True, + base_path=tmp_path, metadata=meta, clients=mocked_clients, user_inputs=user_inputs ) - mocked_discourse.create_topic.assert_not_called() + mocked_clients.discourse.create_topic.assert_not_called() assert not returned_page_interactions -def test__run_reconcile_local_empty_server_error(tmp_path: Path): +def test__run_reconcile_local_empty_server_error(tmp_path: Path, mocked_clients): """ arrange: given metadata with name but not docs and empty docs directory and mocked discourse that raises an exception @@ -132,24 +120,57 @@ def test__run_reconcile_local_empty_server_error(tmp_path: Path): assert: no pages are created. """ meta = types_.Metadata(name="name 1", docs=None) - mocked_discourse = mock.MagicMock(spec=discourse.Discourse) - mocked_discourse.create_topic.side_effect = exceptions.DiscourseError + mocked_clients.discourse.create_topic.side_effect = exceptions.DiscourseError + user_inputs = factories.UserInputsFactory(dry_run=False, delete_pages=True) returned_page_interactions = _run_reconcile( - base_path=tmp_path, - metadata=meta, - discourse=mocked_discourse, - dry_run=False, - delete_pages=True, + base_path=tmp_path, metadata=meta, clients=mocked_clients, user_inputs=user_inputs ) - mocked_discourse.create_topic.assert_called_once_with( + mocked_clients.discourse.create_topic.assert_called_once_with( title="Name 1 Documentation Overview", - content=f"{reconcile.NAVIGATION_TABLE_START.strip()}", + content=f"{constants.NAVIGATION_TABLE_START.strip()}", ) assert not returned_page_interactions +def test__run_reconcile_local_server_conflict(tmp_path: Path, mocked_clients): + """ + arrange: given metadata with name and docs and docs folder with a file and mocked discourse + with content that conflicts with the local content + act: when _run_reconcile is called + assert: InputError is raised. + """ + name = "name 1" + index_url = "index-url" + meta = types_.Metadata(name=name, docs=index_url) + (docs_folder := tmp_path / "docs").mkdir() + (docs_folder / "index.md").write_text(index_content := "index content") + main_page_content = "page content 1" + (docs_folder / "page.md").write_text(local_page_content := "page content 2") + page_url = "page-url" + server_page_content = "page content 3" + mocked_clients.discourse.retrieve_topic.side_effect = [ + ( + f"{index_content}{constants.NAVIGATION_TABLE_START}\n" + f"| 1 | page | [{local_page_content}]({page_url}) |" + ), + server_page_content, + ] + mocked_clients.repository.get_file_content.return_value = main_page_content + user_inputs = factories.UserInputsFactory(dry_run=False, delete_pages=True) + + with pytest.raises(exceptions.InputError) as exc_info: + _run_reconcile( + base_path=tmp_path, metadata=meta, clients=mocked_clients, user_inputs=user_inputs + ) + + assert_substrings_in_string(("actions", "not", "executed"), str(exc_info.value)) + assert mocked_clients.discourse.retrieve_topic.call_count == 2 + mocked_clients.discourse.retrieve_topic.assert_any_call(url=index_url) + mocked_clients.discourse.retrieve_topic.assert_any_call(url=page_url) + + def test__run_migrate_server_error_index( tmp_path: Path, repository_client: pull_request.RepositoryClient ): @@ -167,8 +188,7 @@ def test__run_migrate_server_error_index( _run_migrate( base_path=tmp_path, metadata=meta, - discourse=mocked_discourse, - repository=repository_client, + clients=types_.Clients(discourse=mocked_discourse, repository=repository_client), ) assert "Index page retrieval failed" == str(exc.value) @@ -202,14 +222,13 @@ def test__run_migrate_server_error_topic( _run_migrate( base_path=repository_path, metadata=meta, - discourse=mocked_discourse, - repository=repository_client, + clients=types_.Clients(discourse=mocked_discourse, repository=repository_client), ) def test__run_migrate( repository_path: Path, - upstream_repository: Repo, + upstream_git_repo: Repo, upstream_repository_path: Path, repository_client: pull_request.RepositoryClient, mock_pull_request: PullRequest, @@ -222,7 +241,7 @@ def test__run_migrate( index_content = """Content header. Content body.""" - index_table = f"""{index.NAVIGATION_TABLE_START} + index_table = f"""{constants.NAVIGATION_TABLE_START} | 1 | path-1 | [Tutorials](link-1) |""" index_page = f"{index_content}{index_table}" meta = types_.Metadata(name="name 1", docs="http://discourse/t/docs") @@ -235,11 +254,10 @@ def test__run_migrate( returned_migration_reports = _run_migrate( base_path=repository_path, metadata=meta, - discourse=mocked_discourse, - repository=repository_client, + clients=types_.Clients(discourse=mocked_discourse, repository=repository_client), ) - upstream_repository.git.checkout(pull_request.DEFAULT_BRANCH_NAME) + upstream_git_repo.git.checkout(pull_request.DEFAULT_BRANCH_NAME) assert returned_migration_reports == {mock_pull_request.html_url: types_.ActionResult.SUCCESS} assert ( index_file := upstream_repository_path / DOCUMENTATION_FOLDER_NAME / "index.md" @@ -251,6 +269,7 @@ def test__run_migrate( assert path_file.read_text(encoding="utf-8") == link_content +@pytest.mark.usefixtures("patch_create_repository_client") def test_run_no_docs_no_dir(repository_path: Path): """ arrange: given a path with a metadata.yaml that has no docs key and no docs directory @@ -260,17 +279,18 @@ def test_run_no_docs_no_dir(repository_path: Path): """ create_metadata_yaml(content=f"{metadata.METADATA_NAME_KEY}: name 1", path=repository_path) mocked_discourse = mock.MagicMock(spec=discourse.Discourse) - user_input = factories.UserInputFactory() + user_inputs = factories.UserInputsFactory() with pytest.raises(exceptions.InputError) as exc: # run is repeated in unit tests / integration tests _ = run( - base_path=repository_path, discourse=mocked_discourse, user_inputs=user_input + base_path=repository_path, discourse=mocked_discourse, user_inputs=user_inputs ) # pylint: disable=duplicate-code assert str(exc.value) == GETTING_STARTED +@pytest.mark.usefixtures("patch_create_repository_client") def test_run_no_docs_empty_dir(repository_path: Path): """ arrange: given a path with a metadata.yaml that has no docs key and has empty docs directory @@ -282,16 +302,16 @@ def test_run_no_docs_empty_dir(repository_path: Path): (repository_path / index.DOCUMENTATION_FOLDER_NAME).mkdir() mocked_discourse = mock.MagicMock(spec=discourse.Discourse) mocked_discourse.create_topic.return_value = (url := "url 1") - user_input = factories.UserInputFactory() + user_inputs = factories.UserInputsFactory() # run is repeated in unit tests / integration tests returned_page_interactions = run( - base_path=repository_path, discourse=mocked_discourse, user_inputs=user_input + base_path=repository_path, discourse=mocked_discourse, user_inputs=user_inputs ) # pylint: disable=duplicate-code mocked_discourse.create_topic.assert_called_once_with( title="Name 1 Documentation Overview", - content=f"{reconcile.NAVIGATION_TABLE_START.strip()}", + content=f"{constants.NAVIGATION_TABLE_START.strip()}", ) assert returned_page_interactions == {url: types_.ActionResult.SUCCESS} @@ -299,7 +319,7 @@ def test_run_no_docs_empty_dir(repository_path: Path): @pytest.mark.usefixtures("patch_create_repository_client") def test_run_no_docs_dir( repository_path: Path, - upstream_repository: Repo, + upstream_git_repo: Repo, upstream_repository_path: Path, mock_pull_request: PullRequest, ): @@ -317,21 +337,21 @@ def test_run_no_docs_dir( index_content = """Content header. Content body.\n""" - index_table = f"""{index.NAVIGATION_TABLE_START} + index_table = f"""{constants.NAVIGATION_TABLE_START} | 1 | path-1 | [empty-navlink]() | | 2 | file-1 | [file-navlink](/file-navlink) |""" index_page = f"{index_content}{index_table}" navlink_page = "file-navlink-content" mocked_discourse = mock.MagicMock(spec=discourse.Discourse) mocked_discourse.retrieve_topic.side_effect = [index_page, navlink_page] - user_input = factories.UserInputFactory() + user_inputs = factories.UserInputsFactory() # run is repeated in unit tests / integration tests returned_migration_reports = run( - base_path=repository_path, discourse=mocked_discourse, user_inputs=user_input + base_path=repository_path, discourse=mocked_discourse, user_inputs=user_inputs ) # pylint: disable=duplicate-code - upstream_repository.git.checkout(pull_request.DEFAULT_BRANCH_NAME) + upstream_git_repo.git.checkout(pull_request.DEFAULT_BRANCH_NAME) assert returned_migration_reports == {mock_pull_request.html_url: types_.ActionResult.SUCCESS} assert ( index_file := upstream_repository_path / DOCUMENTATION_FOLDER_NAME / "index.md" diff --git a/tests/unit/test_check.py b/tests/unit/test_check.py new file mode 100644 index 00000000..4b2d228c --- /dev/null +++ b/tests/unit/test_check.py @@ -0,0 +1,204 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Unit tests for check.""" + +import logging +from typing import NamedTuple, cast + +import pytest + +from src import check, types_ + +from .. import factories +from .helpers import assert_substrings_in_string + + +class ExpectedProblem(NamedTuple): + """ + Attrs: + path: The expected path. + description_contents: The expected contents of the description + + """ + + path: str + description_contents: tuple[str, ...] + + +def _test_conflicts_parameters(): + """Generate parameters for the test_conflicts test. + + Returns: + The tests. + """ + return [ + pytest.param((), (), id="empty"), + pytest.param((factories.CreateActionFactory(),), (), id="single create"), + pytest.param((factories.NoopActionFactory(),), (), id="single noop"), + pytest.param((factories.DeleteActionFactory(),), (), id="single delete"), + pytest.param( + (factories.UpdateActionFactory(content_change=None),), + (), + id="single update no content", + ), + pytest.param( + ( + factories.UpdateActionFactory( + content_change=types_.ContentChange(base=None, server="a", local="a") + ), + ), + (), + id="single update no base content same", + ), + pytest.param( + ( + action_1 := factories.UpdateActionFactory( + content_change=types_.ContentChange(base=None, server="a", local="b") + ), + ), + ( + ExpectedProblem( + path=action_1.path, + description_contents=( + "cannot", + "execute", + "branch", + "discourse", + *(cast(tuple, action_1.content_change)[1:]), + ), + ), + ), + id="single update no base content different", + ), + pytest.param( + ( + factories.UpdateActionFactory( + content_change=types_.ContentChange(base="a", server="a", local="a") + ), + ), + (), + id="single update no conflict", + ), + pytest.param( + ( + action_1 := factories.UpdateActionFactory( + content_change=types_.ContentChange(base="a", server="b", local="c") + ), + ), + ( + ExpectedProblem( + path=action_1.path, + description_contents=( + "conflict", + *(cast(tuple, action_1.content_change)[1:]), + ), + ), + ), + id="single update conflict", + ), + pytest.param( + (factories.NoopActionFactory(), factories.NoopActionFactory()), + (), + id="multiple actions no problems", + ), + pytest.param( + ( + action_1 := factories.UpdateActionFactory( + content_change=types_.ContentChange(base="a", server="b", local="c") + ), + factories.NoopActionFactory(), + ), + ( + ExpectedProblem( + path=action_1.path, + description_contents=( + "conflict", + *(cast(tuple, action_1.content_change)[1:]), + ), + ), + ), + id="multiple actions single problem first", + ), + pytest.param( + ( + factories.NoopActionFactory(), + action_2 := factories.UpdateActionFactory( + content_change=types_.ContentChange(base="x", server="y", local="z") + ), + ), + ( + ExpectedProblem( + path=action_2.path, + description_contents=( + "conflict", + *(cast(tuple, action_2.content_change)[1:]), + ), + ), + ), + id="multiple actions single problem second", + ), + pytest.param( + ( + action_1 := factories.UpdateActionFactory( + content_change=types_.ContentChange(base="a", server="b", local="c") + ), + action_2 := factories.UpdateActionFactory( + content_change=types_.ContentChange(base="x", server="y", local="z") + ), + ), + ( + ExpectedProblem( + path=action_1.path, + description_contents=( + "conflict", + *(cast(tuple, action_1.content_change)[1:]), + ), + ), + ExpectedProblem( + path=action_2.path, + description_contents=( + "conflict", + *(cast(tuple, action_2.content_change)[1:]), + ), + ), + ), + id="multiple actions multiple problems", + ), + ] + + +@pytest.mark.parametrize( + "actions, expected_problems", + _test_conflicts_parameters(), +) +def test_conflicts( + actions: tuple[types_.AnyAction, ...], + expected_problems: tuple[ExpectedProblem], + caplog: pytest.LogCaptureFixture, +): + """ + arrange: given actions + act: when conflicts is called with the actions + assert: then the expected problems are yielded. + """ + caplog.set_level(logging.INFO) + + returned_problems = tuple(check.conflicts(actions=actions)) + + assert len(returned_problems) == len(expected_problems) + for returned_problem, expected_problem in zip(returned_problems, expected_problems): + assert returned_problem.path == expected_problem.path + assert_substrings_in_string( + expected_problem.description_contents, returned_problem.description + ) + assert_substrings_in_string( + ( + "problem", + "preventing", + "execution", + "UpdateAction", + str(returned_problem), + ), + caplog.text, + ) diff --git a/tests/unit/test_content.py b/tests/unit/test_content.py new file mode 100644 index 00000000..b8063bcf --- /dev/null +++ b/tests/unit/test_content.py @@ -0,0 +1,136 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Unit tests for content.""" + +import pytest + +from src import content, exceptions + +from .helpers import assert_substrings_in_string + + +def _test_conflicts_parameters(): + """Generate parameters for the test_conflicts test. + + Returns: + The tests. + """ + return [ + pytest.param("a", "a", "a", None, id="all same"), + pytest.param("a", "a", "b", None, id="base and theirs the same"), + pytest.param("a", "b", "a", None, id="base and ours the same"), + pytest.param("a", "b", "b", None, id="theirs and ours the same"), + pytest.param("a", "b", "c", ("a", "b", "c"), id="all different conflict"), + pytest.param( + "line 1\nline 2\n line 3\n", + "line 1a\nline 2\n line 3\n", + "line 1\nline 2\n line 3a\n", + None, + id="all different no conflict", + ), + ] + + +@pytest.mark.parametrize( + "base, theirs, ours, expected_contents", + _test_conflicts_parameters(), +) +def test_conflicts(base: str, theirs: str, ours: str, expected_contents: tuple[str, ...] | None): + """ + arrange: given content for base, theirs and ours + act: when conflicts is called with the content + assert: then the expected value is returned. + """ + result = content.conflicts(base=base, theirs=theirs, ours=ours) + + if expected_contents is None: + assert result is None + else: + assert result is not None + assert_substrings_in_string( + (*expected_contents, "not", "merge", "<<<<<<< HEAD", ">>>>>>> theirs"), result + ) + + +def _test_merge_parameters(): + """Generate parameters for the test_merge test. + + Returns: + The tests. + """ + return [ + pytest.param("a", "a", "a", "a", id="all same"), + pytest.param("a", "a", "b", "b", id="base and theirs the same"), + pytest.param("a", "b", "a", "b", id="base and ours the same"), + pytest.param("a", "b", "b", "b", id="theirs and ours the same"), + pytest.param( + "line 1\nline 2\n line 3\n", + "line 1a\nline 2\n line 3\n", + "line 1\nline 2\n line 3a\n", + "line 1a\nline 2\n line 3a\n", + id="all different no conflict", + ), + ] + + +@pytest.mark.parametrize( + "base, theirs, ours, expected_contents", + _test_merge_parameters(), +) +def test_merge(base: str, theirs: str, ours: str, expected_contents: tuple[str, ...] | None): + """ + arrange: given content for base, theirs and ours + act: when merge is called with the content + assert: then the expected content is returned. + """ + returned_content = content.merge(base=base, theirs=theirs, ours=ours) + + assert returned_content == expected_contents + + +def test_merge_conflict(): + """ + arrange: given content for base, theirs and ours with conflicts + act: when merge is called with the content + assert: then ContentError is raised. + """ + base = "a" + theirs = "b" + ours = "c" + + with pytest.raises(exceptions.ContentError) as exc_info: + content.merge(base=base, theirs=theirs, ours=ours) + + assert_substrings_in_string( + (base, theirs, ours, "not", "merge", "<<<<<<< HEAD", ">>>>>>> theirs"), str(exc_info.value) + ) + + +def _test_diff_parameters(): + """Generate parameters for the test_diff test. + + Returns: + The tests. + """ + return [ + pytest.param("a", "a", " a", id="single line same"), + pytest.param("a", "x", "- a+ x", id="single line different"), + pytest.param("a\nb", "a\nb", " a\n b", id="multiple line same"), + pytest.param("a\nb", "x\ny", "- a\n- b+ x\n+ y", id="multiple line different"), + ] + + +@pytest.mark.parametrize( + "first, second, expected_diff", + _test_diff_parameters(), +) +def test_diff(first: str, second: str, expected_diff: str): + """ + arrange: given two strings + act: when diff is called with the strings + assert: then the expected diff is returned. + """ + returned_diff = content.diff(first=first, second=second) + + assert returned_diff == expected_diff diff --git a/tests/unit/test_docs_directory.py b/tests/unit/test_docs_directory.py index 3ecd6aaf..da235363 100644 --- a/tests/unit/test_docs_directory.py +++ b/tests/unit/test_docs_directory.py @@ -272,14 +272,15 @@ def test__get_path_info(tmp_path: Path): act: when _get_path_info is called with the docs director assert: then the expected local path, level, table path and navlink title is returned. """ - (path := tmp_path / "dir1").mkdir() + rel_path = "dir1" + (path := tmp_path / rel_path).mkdir() alphabetical_rank = 1 returned_path_info = docs_directory._get_path_info( path=path, alphabetical_rank=alphabetical_rank, docs_path=tmp_path ) - assert returned_path_info == (path, 1, "dir1", "Dir1", alphabetical_rank) + assert returned_path_info == (path, 1, rel_path, "Dir1", alphabetical_rank) # Pylint diesn't understand how the walrus operator works diff --git a/tests/unit/test_index.py b/tests/unit/test_index.py index a2e32c85..796e3de4 100644 --- a/tests/unit/test_index.py +++ b/tests/unit/test_index.py @@ -11,7 +11,7 @@ import pytest -from src import discourse, index, types_ +from src import constants, discourse, index, types_ from src.exceptions import DiscourseError, ServerError from .helpers import assert_substrings_in_string @@ -34,7 +34,7 @@ def test__read_docs_index_index_file_missing(tmp_path: Path): act: when _read_docs_index is called with the directory assert: then None is returned. """ - docs_directory = tmp_path / index.DOCUMENTATION_FOLDER_NAME + docs_directory = tmp_path / constants.DOCUMENTATION_FOLDER_NAME docs_directory.mkdir() returned_content = index._read_docs_index(base_path=tmp_path) @@ -123,7 +123,7 @@ def test_get_metadata_yaml_retrieve_empty(tmp_path: Path): "page, expected_content", [ pytest.param( - index.NAVIGATION_TABLE_START, + constants.NAVIGATION_TABLE_START, "", id="navigation table only", ), @@ -138,12 +138,12 @@ def test_get_metadata_yaml_retrieve_empty(tmp_path: Path): id="multiline content only", ), pytest.param( - f"{(content := 'Page content')}{index.NAVIGATION_TABLE_START}", + f"{(content := 'Page content')}{constants.NAVIGATION_TABLE_START}", content, id="page with content and navigation table", ), pytest.param( - f"{(content := 'page content')}{index.NAVIGATION_TABLE_START}\ncontent-afterwards", + f"{(content := 'page content')}{constants.NAVIGATION_TABLE_START}\ncontent-afterwards", content, id="page with content after the navigation table", ), diff --git a/tests/unit/test_migration.py b/tests/unit/test_migration.py index 99602d05..c1054550 100644 --- a/tests/unit/test_migration.py +++ b/tests/unit/test_migration.py @@ -8,6 +8,7 @@ from collections.abc import Iterable from pathlib import Path +from typing import cast from unittest import mock import pytest @@ -180,7 +181,7 @@ def test__validate_table_rows(table_rows: tuple[types_.TableRow, ...]): doc_row := factories.TableRowFactory(is_document=True, path="doc-1"), Path(), types_.DocumentMeta( - path=Path("doc-1.md"), link=doc_row.navlink.link, table_row=doc_row + path=Path("doc-1.md"), link=cast(str, doc_row.navlink.link), table_row=doc_row ), id="single doc file", ), @@ -188,7 +189,9 @@ def test__validate_table_rows(table_rows: tuple[types_.TableRow, ...]): doc_row := factories.TableRowFactory(is_document=True, path="group-1-doc-1"), Path("group-1"), types_.DocumentMeta( - path=Path("group-1/doc-1.md"), link=doc_row.navlink.link, table_row=doc_row + path=Path("group-1/doc-1.md"), + link=cast(str, doc_row.navlink.link), + table_row=doc_row, ), id="nested doc file", ), @@ -196,7 +199,9 @@ def test__validate_table_rows(table_rows: tuple[types_.TableRow, ...]): doc_row := factories.TableRowFactory(is_document=True, path="group-2-doc-1"), Path("group-1"), types_.DocumentMeta( - path=Path("group-1/group-2-doc-1.md"), link=doc_row.navlink.link, table_row=doc_row + path=Path("group-1/group-2-doc-1.md"), + link=cast(str, doc_row.navlink.link), + table_row=doc_row, ), id="typo in nested doc file path", ), @@ -265,7 +270,9 @@ def test__index_file_from_content(content: str, expected_meta: types_.IndexDocum (doc_row_1 := factories.TableRowFactory(level=1, path="doc-1", is_document=True),), ( types_.DocumentMeta( - path=Path("doc-1.md"), link=doc_row_1.navlink.link, table_row=doc_row_1 + path=Path("doc-1.md"), + link=cast(str, cast(str, doc_row_1.navlink.link)), + table_row=doc_row_1, ), ), id="single initial document", @@ -282,10 +289,14 @@ def test__index_file_from_content(content: str, expected_meta: types_.IndexDocum ), ( types_.DocumentMeta( - path=Path("doc-1.md"), link=doc_row_1.navlink.link, table_row=doc_row_1 + path=Path("doc-1.md"), + link=cast(str, doc_row_1.navlink.link), + table_row=doc_row_1, ), types_.DocumentMeta( - path=Path("doc-2.md"), link=doc_row_2.navlink.link, table_row=doc_row_2 + path=Path("doc-2.md"), + link=cast(str, doc_row_2.navlink.link), + table_row=doc_row_2, ), ), id="two documents", @@ -308,7 +319,9 @@ def test__index_file_from_content(content: str, expected_meta: types_.IndexDocum ), ( types_.DocumentMeta( - path=Path("doc-1.md"), link=doc_row_1.navlink.link, table_row=doc_row_1 + path=Path("doc-1.md"), + link=cast(str, doc_row_1.navlink.link), + table_row=doc_row_1, ), types_.GitkeepMeta(path=Path("group-1/.gitkeep"), table_row=group_row_1), ), @@ -322,7 +335,9 @@ def test__index_file_from_content(content: str, expected_meta: types_.IndexDocum ( types_.GitkeepMeta(path=Path("group-1/.gitkeep"), table_row=group_row_1), types_.DocumentMeta( - path=Path("doc-1.md"), link=doc_row_1.navlink.link, table_row=doc_row_1 + path=Path("doc-1.md"), + link=cast(str, doc_row_1.navlink.link), + table_row=doc_row_1, ), ), id="distinct group and document", @@ -335,7 +350,7 @@ def test__index_file_from_content(content: str, expected_meta: types_.IndexDocum ( types_.DocumentMeta( path=Path("group-1/doc-1.md"), - link=doc_row_1.navlink.link, + link=cast(str, doc_row_1.navlink.link), table_row=doc_row_1, ), ), @@ -372,7 +387,7 @@ def test__index_file_from_content(content: str, expected_meta: types_.IndexDocum types_.GitkeepMeta(path=Path("group-1/.gitkeep"), table_row=group_row_1), types_.DocumentMeta( path=Path("doc-1.md"), - link=doc_row_1.navlink.link, + link=cast(str, doc_row_1.navlink.link), table_row=doc_row_1, ), types_.GitkeepMeta(path=Path("group-2/.gitkeep"), table_row=group_row_2), @@ -390,7 +405,7 @@ def test__index_file_from_content(content: str, expected_meta: types_.IndexDocum ( types_.DocumentMeta( path=Path("group-1/doc-1.md"), - link=nested_doc_row_1.navlink.link, + link=cast(str, nested_doc_row_1.navlink.link), table_row=nested_doc_row_1, ), types_.GitkeepMeta(path=Path("group-2/.gitkeep"), table_row=group_row_2), @@ -410,7 +425,7 @@ def test__index_file_from_content(content: str, expected_meta: types_.IndexDocum ( types_.DocumentMeta( path=Path("group-1/doc-1.md"), - link=nested_doc_row_1.navlink.link, + link=cast(str, nested_doc_row_1.navlink.link), table_row=nested_doc_row_1, ), types_.GitkeepMeta( @@ -432,12 +447,12 @@ def test__index_file_from_content(content: str, expected_meta: types_.IndexDocum ( types_.DocumentMeta( path=Path("group-1/doc-1.md"), - link=nested_doc_row_1.navlink.link, + link=cast(str, nested_doc_row_1.navlink.link), table_row=nested_doc_row_1, ), types_.DocumentMeta( path=Path("group-1/doc-2.md"), - link=nested_doc_row_2.navlink.link, + link=cast(str, nested_doc_row_2.navlink.link), table_row=nested_doc_row_2, ), ), @@ -457,7 +472,7 @@ def test__index_file_from_content(content: str, expected_meta: types_.IndexDocum ), types_.DocumentMeta( path=Path("doc-1.md"), - link=doc_row_1.navlink.link, + link=cast(str, doc_row_1.navlink.link), table_row=doc_row_1, ), ), @@ -479,7 +494,7 @@ def test__index_file_from_content(content: str, expected_meta: types_.IndexDocum ), types_.DocumentMeta( path=Path("group-1/doc-1.md"), - link=nested_doc_row_1.navlink.link, + link=cast(str, nested_doc_row_1.navlink.link), table_row=nested_doc_row_1, ), ), @@ -498,7 +513,7 @@ def test__index_file_from_content(content: str, expected_meta: types_.IndexDocum ( types_.DocumentMeta( path=Path("group-1/group-2/doc-1.md"), - link=nested_doc_row_1.navlink.link, + link=cast(str, nested_doc_row_1.navlink.link), table_row=nested_doc_row_1, ), ), diff --git a/tests/unit/test_pull_request.py b/tests/unit/test_pull_request.py index 1c499484..c5117c56 100644 --- a/tests/unit/test_pull_request.py +++ b/tests/unit/test_pull_request.py @@ -7,255 +7,19 @@ # pylint: disable=protected-access from pathlib import Path -from unittest import mock import pytest -from git.exc import GitCommandError from git.repo import Repo -from github import Github -from github.GithubException import GithubException from github.PullRequest import PullRequest -from github.Repository import Repository from src import pull_request -from src.exceptions import InputError, RepositoryClientError -from src.index import DOCUMENTATION_FOLDER_NAME +from src.constants import DOCUMENTATION_FOLDER_NAME +from src.exceptions import InputError from src.pull_request import RepositoryClient from .helpers import assert_substrings_in_string -def test_repository_client__init__(repository: Repo, mock_github_repo: Repository): - """ - arrange: given a local git repository client and mock github repository client - act: when RepositoryClient is initialized - assert: RepositoryClient is created and git user is configured. - """ - pull_request.RepositoryClient(repository=repository, github_repository=mock_github_repo) - - config_reader = repository.config_reader() - assert ( - config_reader.get_value(*pull_request.CONFIG_USER_NAME) == pull_request.ACTIONS_USER_NAME - ) - assert ( - config_reader.get_value(*pull_request.CONFIG_USER_EMAIL) == pull_request.ACTIONS_USER_EMAIL - ) - - -def test_repository_client__init__name_email_set(repository: Repo, mock_github_repo: Repository): - """ - arrange: given a local git repository client with the user and email configuration already set - and mock github repository client - act: when RepositoryClient is initialized - assert: RepositoryClient is created and git user configuration is not overridden. - """ - user_name = "name 1" - user_email = "email 1" - with repository.config_writer(config_level="repository") as config_writer: - config_writer.set_value(*pull_request.CONFIG_USER_NAME, user_name) - config_writer.set_value(*pull_request.CONFIG_USER_EMAIL, user_email) - - repository_client = pull_request.RepositoryClient( - repository=repository, github_repository=mock_github_repo - ) - - config_reader = repository_client._git_repo.config_reader() - assert config_reader.get_value(*pull_request.CONFIG_USER_NAME) == user_name - assert config_reader.get_value(*pull_request.CONFIG_USER_EMAIL) == user_email - - -def test_repository_client_check_branch_exists_error( - monkeypatch: pytest.MonkeyPatch, repository_client: RepositoryClient -): - """ - arrange: given RepositoryClient with a mocked local git repository client that raises an - exception - act: when _check_branch_exists is called - assert: RepositoryClientError is raised from GitCommandError. - """ - err_str = "mocked error" - mock_git_repository = mock.MagicMock(spec=Repo) - mock_git_repository.git.fetch.side_effect = [GitCommandError(err_str)] - monkeypatch.setattr(repository_client, "_git_repo", mock_git_repository) - - with pytest.raises(RepositoryClientError) as exc: - repository_client.check_branch_exists("branchname-1") - - assert_substrings_in_string( - ("unexpected error checking existing branch", err_str), str(exc.value).lower() - ) - - -def test_repository_client_check_branch_not_exists(repository_client: RepositoryClient): - """ - arrange: given RepositoryClient with an upstream repository - act: when _check_branch_exists is called - assert: False is returned. - """ - assert not repository_client.check_branch_exists("no-such-branchname") - - -def test_repository_client_check_branch_exists( - repository_client: RepositoryClient, upstream_repository: Repo, upstream_repository_path: Path -): - """ - arrange: given RepositoryClient with an upstream repository with check-branch-exists branch - act: when _check_branch_exists is called - assert: True is returned. - """ - branch_name = "check-branch-exists" - head = upstream_repository.create_head(branch_name) - head.checkout() - (upstream_repository_path / "filler-file").touch() - upstream_repository.git.add(".") - upstream_repository.git.commit("-m", "test") - - assert repository_client.check_branch_exists(branch_name) - - -def test_repository_client_create_branch_error( - monkeypatch: pytest.MonkeyPatch, repository_client: RepositoryClient -): - """ - arrange: given RepositoryClient with a mocked local git repository that raises an exception - act: when _create_branch is called - assert: RepositoryClientError is raised. - """ - err_str = "mocked error" - mock_git_repository = mock.MagicMock(spec=Repo) - mock_git_repository.git.commit.side_effect = [GitCommandError(err_str)] - monkeypatch.setattr(repository_client, "_git_repo", mock_git_repository) - - with pytest.raises(RepositoryClientError) as exc: - repository_client.create_branch(branch_name="test-create-branch", commit_msg="commit-1") - - assert_substrings_in_string( - ("unexpected error creating new branch", err_str), str(exc.value).lower() - ) - - -def test_repository_client_create_branch( - repository_client: RepositoryClient, - repository_path: Path, - upstream_repository: Repo, -): - """ - arrange: given RepositoryClient and newly created files in `repo` and `repo/docs` directories - act: when _create_branch is called - assert: a new branch is successfully created upstream with only the files in the `repo/docs` - directory. - """ - root_file = repository_path / "test.txt" - root_file.write_text("content 1", encoding="utf-8") - docs_dir = Path(DOCUMENTATION_FOLDER_NAME) - (repository_path / docs_dir).mkdir() - docs_file = docs_dir / "test.txt" - (repository_path / docs_file).write_text("content 2", encoding="utf-8") - nested_docs_dir = docs_dir / "nested" - (repository_path / nested_docs_dir).mkdir() - nested_docs_file = nested_docs_dir / "test.txt" - (repository_path / nested_docs_file).write_text("content 3", encoding="utf-8") - branch_name = "test-create-branch" - - repository_client.create_branch(branch_name=branch_name, commit_msg="commit-1") - - # mypy false positive in lib due to getter/setter not being next to each other. - assert any( - branch - for branch in upstream_repository.branches # type: ignore - if branch.name == branch_name - ) - # Check files in the branch - branch_files = set( - upstream_repository.git.ls_tree("-r", branch_name, "--name-only").splitlines() - ) - assert str(root_file) not in branch_files - assert str(docs_file) in branch_files - assert str(nested_docs_file) in branch_files - - -def test_repository_client_create_branch_checkout_clash_default( - repository_client: RepositoryClient, - repository_path: Path, - upstream_repository: Repo, - default_branch: str, -): - """ - arrange: given RepositoryClient and a file with the same name as the default branch and a file - in the docs folder - act: when _create_branch is called - assert: a new branch is successfully created upstream with one or more files. - """ - root_file = repository_path / default_branch - root_file.write_text("content 1", encoding="utf-8") - branch_name = "test-create-branch" - docs_dir = Path(DOCUMENTATION_FOLDER_NAME) - (repository_path / docs_dir).mkdir() - docs_file = docs_dir / "test.txt" - (repository_path / docs_file).write_text("content 2", encoding="utf-8") - - repository_client.create_branch(branch_name=branch_name, commit_msg="commit-1") - - assert upstream_repository.git.ls_tree("-r", branch_name, "--name-only") - - -def test_repository_client_create_branch_checkout_clash_created( - repository_client: RepositoryClient, repository_path: Path, upstream_repository: Repo -): - """ - arrange: given RepositoryClient and a file with the same name as the requested branch and a - file in the docs folder - act: when _create_branch is called - assert: a new branch is successfully created upstream with one or more files. - """ - branch_name = "test-create-branch" - root_file = repository_path / branch_name - root_file.write_text("content 1", encoding="utf-8") - docs_dir = Path(DOCUMENTATION_FOLDER_NAME) - (repository_path / docs_dir).mkdir() - docs_file = docs_dir / "test.txt" - (repository_path / docs_file).write_text("content 2", encoding="utf-8") - - repository_client.create_branch(branch_name=branch_name, commit_msg="commit-1") - - assert upstream_repository.git.ls_tree("-r", branch_name, "--name-only") - - -def test_repository_client_create_pull_request_error( - monkeypatch: pytest.MonkeyPatch, repository_client: RepositoryClient -): - """ - arrange: given RepositoryClient with a mocked github repository client that raises an exception - act: when _create_pull_request is called - assert: RepositoryClientError is raised. - """ - mock_github_repository = mock.MagicMock(spec=Repository) - mock_github_repository.create_pull.side_effect = [ - GithubException(status=500, data="Internal Server Error", headers=None) - ] - monkeypatch.setattr(repository_client, "_github_repo", mock_github_repository) - - with pytest.raises(RepositoryClientError) as exc: - repository_client.create_pull_request(branch_name="branchname-1") - - assert_substrings_in_string( - ("unexpected error creating pull request", "githubexception"), str(exc.value).lower() - ) - - -def test_repository_client_create_pull_request( - repository_client: RepositoryClient, mock_pull_request: PullRequest -): - """ - arrange: given RepositoryClient with a mocked github client that returns a mocked pull request - act: when _create_pull_request is called - assert: a pull request's page link is returned. - """ - returned_url = repository_client.create_pull_request("branchname-1") - - assert returned_url == mock_pull_request.html_url - - def test_create_pull_request_no_dirty_files(repository_client: RepositoryClient): """ arrange: given RepositoryClient with no dirty files @@ -273,7 +37,7 @@ def test_create_pull_request_no_dirty_files(repository_client: RepositoryClient) def test_create_pull_request_existing_branch( repository_client: RepositoryClient, - upstream_repository: Repo, + upstream_git_repo: Repo, upstream_repository_path: Path, repository_path: Path, ): @@ -288,12 +52,12 @@ def test_create_pull_request_existing_branch( (repository_path / filler_file).write_text("filler-content") branch_name = pull_request.DEFAULT_BRANCH_NAME - head = upstream_repository.create_head(branch_name) + head = upstream_git_repo.create_head(branch_name) head.checkout() (upstream_repository_path / docs_folder).mkdir() (upstream_repository_path / filler_file).touch() - upstream_repository.git.add(".") - upstream_repository.git.commit("-m", "test") + upstream_git_repo.git.add(".") + upstream_git_repo.git.commit("-m", "test") with pytest.raises(InputError) as exc: pull_request.create_pull_request(repository=repository_client) @@ -311,7 +75,7 @@ def test_create_pull_request_existing_branch( def test_create_pull_request( repository_client: RepositoryClient, - upstream_repository: Repo, + upstream_git_repo: Repo, upstream_repository_path: Path, repository_path: Path, mock_pull_request: PullRequest, @@ -329,103 +93,6 @@ def test_create_pull_request( returned_pr_link = pull_request.create_pull_request(repository=repository_client) - upstream_repository.git.checkout(pull_request.DEFAULT_BRANCH_NAME) + upstream_git_repo.git.checkout(pull_request.DEFAULT_BRANCH_NAME) assert returned_pr_link == mock_pull_request.html_url assert (upstream_repository_path / filler_file).read_text() == filler_text - - -@pytest.mark.parametrize( - "remote_url", - [ - pytest.param("https://gitlab.com/canonical/upload-charm-docs.git", id="non-github url"), - pytest.param("http://gitlab.com/canonical/upload-charm-docs.git", id="http url"), - pytest.param("git@github.com:yanksyoon/actionrefer.git", id="ssh url"), - ], -) -def test_get_repository_name_invalid(remote_url: str): - """ - arrange: given a non-valid remote_url - act: when _get_repository_name is called - assert: InputError is raised. - """ - with pytest.raises(InputError) as exc: - pull_request._get_repository_name_from_git_url(remote_url=remote_url) - - assert_substrings_in_string( - ("invalid remote repository url",), - str(exc.value).lower(), - ) - - -@pytest.mark.parametrize( - "remote_url, expected_repository_name", - [ - pytest.param( - "https://github.com/canonical/upload-charm-docs", - "canonical/upload-charm-docs", - id="valid url", - ), - pytest.param( - "https://github.com/canonical/upload-charm-docs.git", - "canonical/upload-charm-docs", - id="valid git url", - ), - ], -) -def test_get_repository_name(remote_url: str, expected_repository_name: str): - """ - arrange: given a non-valid remote_url - act: when _get_repository_name is called - assert: GitError is raised. - """ - assert ( - pull_request._get_repository_name_from_git_url(remote_url=remote_url) - == expected_repository_name - ) - - -def test_create_repository_client_no_token( - repository_path: Path, -): - """ - arrange: given valid repository path and empty access_token - act: when create_repository_client is called - assert: InputError is raised. - """ - # the following token is for testing purposes only. - test_token = "" # nosec - - with pytest.raises(InputError) as exc: - pull_request.create_repository_client(access_token=test_token, base_path=repository_path) - - assert_substrings_in_string( - ("invalid", "access_token", "input", "it must be", "non-empty"), - str(exc.value).lower(), - ) - - -def test_create_repository_client( - monkeypatch: pytest.MonkeyPatch, - repository: Repo, - repository_path: Path, - mock_github_repo: Repository, -): - """ - arrange: given valid repository path and a valid access_token and a mocked github client - act: when create_repository_client is called - assert: RepositoryClient is returned. - """ - origin = repository.remote("origin") - repository.delete_remote(origin) - repository.create_remote("origin", "https://github.com/test-user/test-repo.git") - # the following token is for testing purposes only. - test_token = "testing-token" # nosec - mock_github_client = mock.MagicMock(spec=Github) - mock_github_client.get_repo.returns = mock_github_repo - monkeypatch.setattr(pull_request, "Github", mock_github_client) - - returned_client = pull_request.create_repository_client( - access_token=test_token, base_path=repository_path - ) - - assert isinstance(returned_client, pull_request.RepositoryClient) diff --git a/tests/unit/test_reconcile.py b/tests/unit/test_reconcile.py index dba84b29..02b85ee2 100644 --- a/tests/unit/test_reconcile.py +++ b/tests/unit/test_reconcile.py @@ -3,17 +3,19 @@ """Unit tests for reconcile module.""" -# Need access to protected functions for testing,using walrus operator causes too-many-locals +# Need access to protected functions for testing, using walrus operator causes too-many-locals # pylint: disable=protected-access,too-many-locals from pathlib import Path +from typing import cast from unittest import mock import pytest -from src import discourse, exceptions, reconcile, types_ +from src import constants, discourse, exceptions, reconcile, types_ from .. import factories +from .helpers import assert_substrings_in_string def test__local_only_file(tmp_path: Path): @@ -61,12 +63,14 @@ def test__local_only_directory(tmp_path: Path): pytest.param(1, 2, "table path 1", "table path 2", id="level and table path mismatch"), ], ) -def test__local_and_server_error( +# The arguments are needed due to parametrisation and use of fixtures +def test__local_and_server_error( # pylint: disable=too-many-arguments path_info_level: int, table_row_level: int, path_info_table_path: str, table_row_path: str, tmp_path: Path, + mocked_clients, ): """ arrange: given path info and table row where either level or table path or both do not match @@ -79,11 +83,14 @@ def test__local_and_server_error( ) navlink = types_.Navlink(title=path_info.navlink_title, link="link 1") table_row = types_.TableRow(level=table_row_level, path=table_row_path, navlink=navlink) - mock_discourse = mock.MagicMock(spec=discourse.Discourse) with pytest.raises(exceptions.ReconcilliationError): reconcile._local_and_server( - path_info=path_info, table_row=table_row, discourse=mock_discourse + path_info=path_info, + table_row=table_row, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=factories.UserInputsFactory(), ) @@ -131,7 +138,9 @@ def test__get_server_content_server_error(): pytest.param("content 1", "content 1 ", id="local trailing whitespace"), ], ) -def test__local_and_server_file_same(local_content: str, server_content: str, tmp_path: Path): +def test__local_and_server_file_same( + local_content: str, server_content: str, tmp_path: Path, mocked_clients +): """ arrange: given path info with a file and table row with no changes and discourse client that returns the same content as in the file @@ -141,13 +150,16 @@ def test__local_and_server_file_same(local_content: str, server_content: str, tm (path := tmp_path / "file1.md").touch() path.write_text(local_content, encoding="utf-8") path_info = factories.PathInfoFactory(local_path=path) - mock_discourse = mock.MagicMock(spec=discourse.Discourse) - mock_discourse.retrieve_topic.return_value = server_content + mocked_clients.discourse.retrieve_topic.return_value = server_content navlink = types_.Navlink(title=path_info.navlink_title, link=(navlink_link := "link 1")) table_row = types_.TableRow(level=path_info.level, path=path_info.table_path, navlink=navlink) (returned_action,) = reconcile._local_and_server( - path_info=path_info, table_row=table_row, discourse=mock_discourse + path_info=path_info, + table_row=table_row, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=factories.UserInputsFactory(), ) assert isinstance(returned_action, types_.NoopAction) @@ -156,26 +168,109 @@ def test__local_and_server_file_same(local_content: str, server_content: str, tm # mypy has difficulty with determining which action is returned assert returned_action.navlink == navlink # type: ignore assert returned_action.content == local_content.strip() # type: ignore - mock_discourse.retrieve_topic.assert_called_once_with(url=navlink_link) + mocked_clients.discourse.retrieve_topic.assert_called_once_with(url=navlink_link) + + +def test__local_and_server_file_content_change_repo_error(tmp_path: Path, mocked_clients): + """ + arrange: given path info with a file and table row with no changes and discourse client that + returns the different content as in the file and repository that raises an error + act: when _local_and_server is called with the path info and table row + assert: then ReconcilliationError is raised. + """ + relative_path = Path("file1.md") + (path := tmp_path / relative_path).touch() + path.write_text("content 1", encoding="utf-8") + path_info = factories.PathInfoFactory(local_path=path) + mocked_clients.discourse.retrieve_topic.return_value = "content 2" + mocked_clients.repository.get_file_content.side_effect = exceptions.RepositoryClientError + navlink = types_.Navlink(title=path_info.navlink_title, link=(navlink_link := "link 1")) + table_row = types_.TableRow(level=path_info.level, path=path_info.table_path, navlink=navlink) + user_inputs = factories.UserInputsFactory() + + with pytest.raises(exceptions.ReconcilliationError) as exc_info: + reconcile._local_and_server( + path_info=path_info, + table_row=table_row, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=user_inputs, + ) + + assert_substrings_in_string( + ("unable", "retrieve", str(relative_path), cast(str, user_inputs.base_branch)), + str(exc_info.value).lower(), + ) + mocked_clients.discourse.retrieve_topic.assert_called_once_with(url=navlink_link) + mocked_clients.repository.get_file_content.assert_called_once_with( + path=str(relative_path), branch=user_inputs.base_branch + ) + + +def test__local_and_server_file_content_change_file_not_in_repo(tmp_path: Path, mocked_clients): + """ + arrange: given path info with a file and table row with no changes and discourse client that + returns the different content as in the file and repository that cannot find the file + act: when _local_and_server is called with the path info and table row + assert: then an update action is returned with None for the base content. + """ + relative_path = Path("file1.md") + (path := tmp_path / relative_path).touch() + path.write_text(local_content := "content 1", encoding="utf-8") + path_info = factories.PathInfoFactory(local_path=path) + mocked_clients.discourse.retrieve_topic.return_value = (server_content := "content 2") + mocked_clients.repository.get_file_content.side_effect = exceptions.RepositoryFileNotFoundError + navlink = types_.Navlink(title=path_info.navlink_title, link=(navlink_link := "link 1")) + table_row = types_.TableRow(level=path_info.level, path=path_info.table_path, navlink=navlink) + user_inputs = factories.UserInputsFactory() + + (returned_action,) = reconcile._local_and_server( + path_info=path_info, + table_row=table_row, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=user_inputs, + ) + + assert isinstance(returned_action, types_.UpdateAction) + assert returned_action.level == path_info.level + assert returned_action.path == path_info.table_path + # mypy has difficulty with determining which action is returned + assert returned_action.navlink_change.old == navlink # type: ignore + assert returned_action.navlink_change.new == navlink # type: ignore + assert returned_action.content_change.server == server_content # type: ignore + assert returned_action.content_change.local == local_content # type: ignore + assert returned_action.content_change.base is None # type: ignore + mocked_clients.discourse.retrieve_topic.assert_called_once_with(url=navlink_link) + mocked_clients.repository.get_file_content.assert_called_once_with( + path=str(relative_path), branch=user_inputs.base_branch + ) -def test__local_and_server_file_content_change(tmp_path: Path): +def test__local_and_server_file_content_change(tmp_path: Path, mocked_clients): """ arrange: given path info with a file and table row with no changes and discourse client that returns the different content as in the file act: when _local_and_server is called with the path info and table row assert: then an update action is returned. """ - (path := tmp_path / "file1.md").touch() + relative_path = Path("file1.md") + (path := tmp_path / relative_path).touch() path.write_text(local_content := "content 1", encoding="utf-8") path_info = factories.PathInfoFactory(local_path=path) - mock_discourse = mock.MagicMock(spec=discourse.Discourse) - mock_discourse.retrieve_topic.return_value = (server_content := "content 2") + mocked_clients.discourse.retrieve_topic.return_value = (server_content := "content 2") + base_content = "content 3" + mocked_clients.repository.get_file_content.return_value = base_content navlink = types_.Navlink(title=path_info.navlink_title, link=(navlink_link := "link 1")) table_row = types_.TableRow(level=path_info.level, path=path_info.table_path, navlink=navlink) + user_inputs = factories.UserInputsFactory() (returned_action,) = reconcile._local_and_server( - path_info=path_info, table_row=table_row, discourse=mock_discourse + path_info=path_info, + table_row=table_row, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=user_inputs, ) assert isinstance(returned_action, types_.UpdateAction) @@ -184,28 +279,38 @@ def test__local_and_server_file_content_change(tmp_path: Path): # mypy has difficulty with determining which action is returned assert returned_action.navlink_change.old == navlink # type: ignore assert returned_action.navlink_change.new == navlink # type: ignore - assert returned_action.content_change.old == server_content # type: ignore - assert returned_action.content_change.new == local_content # type: ignore - mock_discourse.retrieve_topic.assert_called_once_with(url=navlink_link) + assert returned_action.content_change.server == server_content # type: ignore + assert returned_action.content_change.local == local_content # type: ignore + assert returned_action.content_change.base == base_content # type: ignore + mocked_clients.discourse.retrieve_topic.assert_called_once_with(url=navlink_link) + mocked_clients.repository.get_file_content.assert_called_once_with( + path=str(relative_path), branch=user_inputs.base_branch + ) -def test__local_and_server_file_navlink_title_change(tmp_path: Path): +def test__local_and_server_file_navlink_title_change(tmp_path: Path, mocked_clients): """ arrange: given path info with a file and table row with different navlink title and discourse client that returns the same content as in the file act: when _local_and_server is called with the path info and table row assert: then an update action is returned. """ - (path := tmp_path / "file1.md").touch() + relative_path = Path("file1.md") + (path := tmp_path / relative_path).touch() path.write_text(content := "content 1", encoding="utf-8") path_info = factories.PathInfoFactory(local_path=path, navlink_title="title 1") - mock_discourse = mock.MagicMock(spec=discourse.Discourse) - mock_discourse.retrieve_topic.return_value = content + mocked_clients.discourse.retrieve_topic.return_value = content + mocked_clients.repository.get_file_content.return_value = content navlink = types_.Navlink(title="title 2", link=(navlink_link := "link 1")) table_row = types_.TableRow(level=path_info.level, path=path_info.table_path, navlink=navlink) + user_inputs = factories.UserInputsFactory() (returned_action,) = reconcile._local_and_server( - path_info=path_info, table_row=table_row, discourse=mock_discourse + path_info=path_info, + table_row=table_row, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=user_inputs, ) assert isinstance(returned_action, types_.UpdateAction) @@ -216,12 +321,15 @@ def test__local_and_server_file_navlink_title_change(tmp_path: Path): assert returned_action.navlink_change.new == types_.Navlink( # type: ignore title=path_info.navlink_title, link=navlink_link ) - assert returned_action.content_change.old == content # type: ignore - assert returned_action.content_change.new == content # type: ignore - mock_discourse.retrieve_topic.assert_called_once_with(url=navlink_link) + assert returned_action.content_change.server == content # type: ignore + assert returned_action.content_change.local == content # type: ignore + mocked_clients.discourse.retrieve_topic.assert_called_once_with(url=navlink_link) + mocked_clients.repository.get_file_content.assert_called_once_with( + path=str(relative_path), branch=user_inputs.base_branch + ) -def test__local_and_server_directory_same(tmp_path: Path): +def test__local_and_server_directory_same(tmp_path: Path, mocked_clients): """ arrange: given path info with a directory and table row with no changes act: when _local_and_server is called with the path info and table row @@ -229,12 +337,15 @@ def test__local_and_server_directory_same(tmp_path: Path): """ (path := tmp_path / "dir1").mkdir() path_info = factories.PathInfoFactory(local_path=path) - mock_discourse = mock.MagicMock(spec=discourse.Discourse) navlink = types_.Navlink(title=path_info.navlink_title, link=None) table_row = types_.TableRow(level=path_info.level, path=path_info.table_path, navlink=navlink) (returned_action,) = reconcile._local_and_server( - path_info=path_info, table_row=table_row, discourse=mock_discourse + path_info=path_info, + table_row=table_row, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=factories.UserInputsFactory(), ) assert isinstance(returned_action, types_.NoopAction) @@ -243,10 +354,11 @@ def test__local_and_server_directory_same(tmp_path: Path): # mypy has difficulty with determining which action is returned assert returned_action.navlink == navlink # type: ignore assert returned_action.content is None # type: ignore - mock_discourse.retrieve_topic.assert_not_called() + mocked_clients.discourse.retrieve_topic.assert_not_called() + mocked_clients.repository.get_file_content.assert_not_called() -def test__local_and_server_directory_navlink_title_changed(tmp_path: Path): +def test__local_and_server_directory_navlink_title_changed(tmp_path: Path, mocked_clients): """ arrange: given path info with a directory and table row with different navlink title act: when _local_and_server is called with the path info and table row @@ -254,12 +366,15 @@ def test__local_and_server_directory_navlink_title_changed(tmp_path: Path): """ (path := tmp_path / "dir1").mkdir() path_info = factories.PathInfoFactory(local_path=path, navlink_title="title 1") - mock_discourse = mock.MagicMock(spec=discourse.Discourse) navlink = types_.Navlink(title="title 2", link=None) table_row = types_.TableRow(level=path_info.level, path=path_info.table_path, navlink=navlink) (returned_action,) = reconcile._local_and_server( - path_info=path_info, table_row=table_row, discourse=mock_discourse + path_info=path_info, + table_row=table_row, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=factories.UserInputsFactory(), ) assert isinstance(returned_action, types_.UpdateAction) @@ -271,10 +386,10 @@ def test__local_and_server_directory_navlink_title_changed(tmp_path: Path): title=path_info.navlink_title, link=None ) assert returned_action.content_change is None # type: ignore - mock_discourse.retrieve_topic.assert_not_called() + mocked_clients.discourse.retrieve_topic.assert_not_called() -def test__local_and_server_directory_to_file(tmp_path: Path): +def test__local_and_server_directory_to_file(tmp_path: Path, mocked_clients): """ arrange: given path info with a file and table row with a group act: when _local_and_server is called with the path info and table row @@ -283,12 +398,15 @@ def test__local_and_server_directory_to_file(tmp_path: Path): (path := tmp_path / "file1.md").touch() path.write_text(content := "content 1", encoding="utf-8") path_info = factories.PathInfoFactory(local_path=path) - mock_discourse = mock.MagicMock(spec=discourse.Discourse) navlink = types_.Navlink(title=path_info.navlink_title, link=None) table_row = types_.TableRow(level=path_info.level, path=path_info.table_path, navlink=navlink) (returned_action,) = reconcile._local_and_server( - path_info=path_info, table_row=table_row, discourse=mock_discourse + path_info=path_info, + table_row=table_row, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=factories.UserInputsFactory(), ) assert isinstance(returned_action, types_.CreateAction) @@ -297,10 +415,10 @@ def test__local_and_server_directory_to_file(tmp_path: Path): # mypy has difficulty with determining which action is returned assert returned_action.navlink_title == path_info.navlink_title # type: ignore assert returned_action.content == content # type: ignore - mock_discourse.retrieve_topic.assert_not_called() + mocked_clients.discourse.retrieve_topic.assert_not_called() -def test__local_and_server_file_to_directory(tmp_path: Path): +def test__local_and_server_file_to_directory(tmp_path: Path, mocked_clients): """ arrange: given path info with a directory and table row with a file act: when _local_and_server is called with the path info and table row @@ -308,13 +426,16 @@ def test__local_and_server_file_to_directory(tmp_path: Path): """ (path := tmp_path / "dir1").mkdir() path_info = factories.PathInfoFactory(local_path=path) - mock_discourse = mock.MagicMock(spec=discourse.Discourse) - mock_discourse.retrieve_topic.return_value = (content := "content 1") + mocked_clients.discourse.retrieve_topic.return_value = (content := "content 1") navlink = types_.Navlink(title=path_info.navlink_title, link=(navlink_link := "link 1")) table_row = types_.TableRow(level=path_info.level, path=path_info.table_path, navlink=navlink) returned_actions = reconcile._local_and_server( - path_info=path_info, table_row=table_row, discourse=mock_discourse + path_info=path_info, + table_row=table_row, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=factories.UserInputsFactory(), ) assert len(returned_actions) == 2 @@ -330,7 +451,7 @@ def test__local_and_server_file_to_directory(tmp_path: Path): # mypy has difficulty with determining which action is returned assert returned_actions[1].navlink_title == path_info.navlink_title # type: ignore assert returned_actions[1].content is None # type: ignore - mock_discourse.retrieve_topic.assert_called_once_with(url=navlink_link) + mocked_clients.discourse.retrieve_topic.assert_called_once_with(url=navlink_link) def test__server_only_file(): @@ -391,16 +512,20 @@ def test__server_only_directory(): mock_discourse.retrieve_topic.assert_not_called() -def test__calculate_action_error(): +def test__calculate_action_error(tmp_path: Path, mocked_clients): """ arrange: given path info and table row that are None act: when _calculate_action is called with the path info and table row assert: then ReconcilliationError is raised. """ - mock_discourse = mock.MagicMock(spec=discourse.Discourse) - with pytest.raises(exceptions.ReconcilliationError): - reconcile._calculate_action(path_info=None, table_row=None, discourse=mock_discourse) + reconcile._calculate_action( + path_info=None, + table_row=None, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=factories.UserInputsFactory(), + ) def path_info_mkdir(path_info: types_.PathInfo, base_dir: Path) -> types_.PathInfo: @@ -452,18 +577,22 @@ def test__calculate_action( table_row: types_.TableRow | None, expected_action_type: type[types_.AnyAction], tmp_path: Path, + mocked_clients, ): """ arrange: given path info and table row for a directory and grouping act: when _calculate_action is called with the path info and table row assert: then the expected action type is returned. """ - mock_discourse = mock.MagicMock(spec=discourse.Discourse) if path_info is not None: path_info = path_info_mkdir(path_info=path_info, base_dir=tmp_path) (returned_action,) = reconcile._calculate_action( - path_info=path_info, table_row=table_row, discourse=mock_discourse + path_info=path_info, + table_row=table_row, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=factories.UserInputsFactory(), ) assert isinstance(returned_action, expected_action_type) @@ -566,23 +695,30 @@ def test__calculate_action( ], ) # pylint: enable=undefined-variable,unused-variable -def test_run( +# The arguments are needed due to parametrisation and use of fixtures +def test_run( # pylint: disable=too-many-arguments path_infos: tuple[types_.PathInfo, ...], table_rows: tuple[types_.TableRow, ...], expected_action_types: tuple[type[types_.AnyAction], ...], expected_level_paths: tuple[tuple[types_.Level, types_.TablePath], ...], tmp_path: Path, + mocked_clients, ): """ arrange: given path infos and table rows act: when run is called with the path infos and table rows assert: then the expected actions are returned in the expected order. """ - mock_discourse = mock.MagicMock(spec=discourse.Discourse) path_infos = tuple(path_info_mkdir(path_info, base_dir=tmp_path) for path_info in path_infos) returned_actions = list( - reconcile.run(path_infos=path_infos, table_rows=table_rows, discourse=mock_discourse) + reconcile.run( + path_infos=path_infos, + table_rows=table_rows, + clients=mocked_clients, + base_path=tmp_path, + user_inputs=factories.UserInputsFactory(), + ) ) assert ( @@ -610,7 +746,7 @@ def test_run( ), (), types_.CreateIndexAction( - title=local_title, content=f"{reconcile.NAVIGATION_TABLE_START.strip()}" + title=local_title, content=f"{constants.NAVIGATION_TABLE_START.strip()}" ), id="empty local only empty rows", ), @@ -624,7 +760,7 @@ def test_run( ), (), types_.CreateIndexAction( - title=local_title, content=f"{local_content}{reconcile.NAVIGATION_TABLE_START}" + title=local_title, content=f"{local_content}{constants.NAVIGATION_TABLE_START}" ), id="local only empty rows", ), @@ -640,7 +776,7 @@ def test_run( types_.CreateIndexAction( title=local_title, content=( - f"{local_content}{reconcile.NAVIGATION_TABLE_START}\n" + f"{local_content}{constants.NAVIGATION_TABLE_START}\n" f"{table_row.to_markdown()}" ), ), @@ -661,7 +797,7 @@ def test_run( types_.CreateIndexAction( title=local_title, content=( - f"{local_content}{reconcile.NAVIGATION_TABLE_START}\n" + f"{local_content}{constants.NAVIGATION_TABLE_START}\n" f"{table_row_1.to_markdown()}\n{table_row_2.to_markdown()}" ), ), @@ -673,7 +809,7 @@ def test_run( server=types_.Page( url=(url := "url 1"), content=( - server_content := f"{local_content}{reconcile.NAVIGATION_TABLE_START}" + server_content := f"{local_content}{constants.NAVIGATION_TABLE_START}" ), ), name="name 1", @@ -688,7 +824,7 @@ def test_run( server=types_.Page( url=(url := "url 1"), content=( - server_content := f" {local_content}{reconcile.NAVIGATION_TABLE_START}" + server_content := f" {local_content}{constants.NAVIGATION_TABLE_START}" ), ), name="name 1", @@ -704,7 +840,7 @@ def test_run( url=(url := "url 1"), content=( server_content := f"{local_content.strip()}" - f"{reconcile.NAVIGATION_TABLE_START}" + f"{constants.NAVIGATION_TABLE_START}" ), ), name="name 1", @@ -723,7 +859,7 @@ def test_run( types_.UpdateIndexAction( content_change=types_.IndexContentChange( old=server_content, - new=f"{local_content}{reconcile.NAVIGATION_TABLE_START}", + new=f"{local_content}{constants.NAVIGATION_TABLE_START}", ), url=url, ), diff --git a/tests/unit/test_repository.py b/tests/unit/test_repository.py new file mode 100644 index 00000000..1ea0e306 --- /dev/null +++ b/tests/unit/test_repository.py @@ -0,0 +1,460 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Unit tests for git.""" + +# Need access to protected functions for testing +# pylint: disable=protected-access + +import base64 +import secrets +from pathlib import Path +from unittest import mock + +import pytest +from git.exc import GitCommandError +from git.repo import Repo +from github import Github +from github.ContentFile import ContentFile +from github.GithubException import GithubException, UnknownObjectException +from github.PullRequest import PullRequest +from github.Repository import Repository + +from src import repository +from src.constants import DOCUMENTATION_FOLDER_NAME +from src.exceptions import InputError, RepositoryClientError, RepositoryFileNotFoundError +from src.repository import Client + +from .helpers import assert_substrings_in_string + + +def test__init__(git_repo: Repo, mock_github_repo: Repository): + """ + arrange: given a local git repository client and mock github repository client + act: when Client is initialized + assert: Client is created and git user is configured. + """ + repository.Client(repository=git_repo, github_repository=mock_github_repo) + + config_reader = git_repo.config_reader() + assert config_reader.get_value(*repository.CONFIG_USER_NAME) == repository.ACTIONS_USER_NAME + assert config_reader.get_value(*repository.CONFIG_USER_EMAIL) == repository.ACTIONS_USER_EMAIL + + +def test__init__name_email_set(git_repo: Repo, mock_github_repo: Repository): + """ + arrange: given a local git repository client with the user and email configuration already set + and mock github repository client + act: when Client is initialized + assert: Client is created and git user configuration is not overridden. + """ + user_name = "name 1" + user_email = "email 1" + with git_repo.config_writer(config_level="repository") as config_writer: + config_writer.set_value(*repository.CONFIG_USER_NAME, user_name) + config_writer.set_value(*repository.CONFIG_USER_EMAIL, user_email) + + repository_client = repository.Client(repository=git_repo, github_repository=mock_github_repo) + + config_reader = repository_client._git_repo.config_reader() + assert config_reader.get_value(*repository.CONFIG_USER_NAME) == user_name + assert config_reader.get_value(*repository.CONFIG_USER_EMAIL) == user_email + + +def test_check_branch_exists_error(monkeypatch: pytest.MonkeyPatch, repository_client: Client): + """ + arrange: given Client with a mocked local git repository client that raises an + exception + act: when _check_branch_exists is called + assert: RepositoryClientError is raised from GitCommandError. + """ + err_str = "mocked error" + mock_git_repository = mock.MagicMock(spec=Repo) + mock_git_repository.git.fetch.side_effect = [GitCommandError(err_str)] + monkeypatch.setattr(repository_client, "_git_repo", mock_git_repository) + + with pytest.raises(RepositoryClientError) as exc: + repository_client.check_branch_exists("branchname-1") + + assert_substrings_in_string( + ("unexpected error checking existing branch", err_str), str(exc.value).lower() + ) + + +def test_check_branch_not_exists(repository_client: Client): + """ + arrange: given Client with an upstream repository + act: when _check_branch_exists is called + assert: False is returned. + """ + assert not repository_client.check_branch_exists("no-such-branchname") + + +def test_check_branch_exists( + repository_client: Client, upstream_git_repo: Repo, upstream_repository_path: Path +): + """ + arrange: given Client with an upstream repository with check-branch-exists branch + act: when _check_branch_exists is called + assert: True is returned. + """ + branch_name = "check-branch-exists" + head = upstream_git_repo.create_head(branch_name) + head.checkout() + (upstream_repository_path / "filler-file").touch() + upstream_git_repo.git.add(".") + upstream_git_repo.git.commit("-m", "test") + + assert repository_client.check_branch_exists(branch_name) + + +def test_create_branch_error(monkeypatch: pytest.MonkeyPatch, repository_client: Client): + """ + arrange: given Client with a mocked local git repository that raises an exception + act: when _create_branch is called + assert: RepositoryClientError is raised. + """ + err_str = "mocked error" + mock_git_repository = mock.MagicMock(spec=Repo) + mock_git_repository.git.commit.side_effect = [GitCommandError(err_str)] + monkeypatch.setattr(repository_client, "_git_repo", mock_git_repository) + + with pytest.raises(RepositoryClientError) as exc: + repository_client.create_branch(branch_name="test-create-branch", commit_msg="commit-1") + + assert_substrings_in_string( + ("unexpected error creating new branch", err_str), str(exc.value).lower() + ) + + +def test_create_branch( + repository_client: Client, + repository_path: Path, + upstream_git_repo: Repo, +): + """ + arrange: given Client and newly created files in `repo` and `repo/docs` directories + act: when _create_branch is called + assert: a new branch is successfully created upstream with only the files in the `repo/docs` + directory. + """ + root_file = repository_path / "test.txt" + root_file.write_text("content 1", encoding="utf-8") + docs_dir = Path(DOCUMENTATION_FOLDER_NAME) + (repository_path / docs_dir).mkdir() + docs_file = docs_dir / "test.txt" + (repository_path / docs_file).write_text("content 2", encoding="utf-8") + nested_docs_dir = docs_dir / "nested" + (repository_path / nested_docs_dir).mkdir() + nested_docs_file = nested_docs_dir / "test.txt" + (repository_path / nested_docs_file).write_text("content 3", encoding="utf-8") + branch_name = "test-create-branch" + + repository_client.create_branch(branch_name=branch_name, commit_msg="commit-1") + + # mypy false positive in lib due to getter/setter not being next to each other. + assert any( + branch + for branch in upstream_git_repo.branches # type: ignore + if branch.name == branch_name + ) + # Check files in the branch + branch_files = set( + upstream_git_repo.git.ls_tree("-r", branch_name, "--name-only").splitlines() + ) + assert str(root_file) not in branch_files + assert str(docs_file) in branch_files + assert str(nested_docs_file) in branch_files + + +def test_create_branch_checkout_clash_default( + repository_client: Client, + repository_path: Path, + upstream_git_repo: Repo, + default_branch: str, +): + """ + arrange: given Client and a file with the same name as the default branch and a file + in the docs folder + act: when _create_branch is called + assert: a new branch is successfully created upstream with one or more files. + """ + root_file = repository_path / default_branch + root_file.write_text("content 1", encoding="utf-8") + branch_name = "test-create-branch" + docs_dir = Path(DOCUMENTATION_FOLDER_NAME) + (repository_path / docs_dir).mkdir() + docs_file = docs_dir / "test.txt" + (repository_path / docs_file).write_text("content 2", encoding="utf-8") + + repository_client.create_branch(branch_name=branch_name, commit_msg="commit-1") + + assert upstream_git_repo.git.ls_tree("-r", branch_name, "--name-only") + + +def test_create_branch_checkout_clash_created( + repository_client: Client, repository_path: Path, upstream_git_repo: Repo +): + """ + arrange: given Client and a file with the same name as the requested branch and a + file in the docs folder + act: when _create_branch is called + assert: a new branch is successfully created upstream with one or more files. + """ + branch_name = "test-create-branch" + root_file = repository_path / branch_name + root_file.write_text("content 1", encoding="utf-8") + docs_dir = Path(DOCUMENTATION_FOLDER_NAME) + (repository_path / docs_dir).mkdir() + docs_file = docs_dir / "test.txt" + (repository_path / docs_file).write_text("content 2", encoding="utf-8") + + repository_client.create_branch(branch_name=branch_name, commit_msg="commit-1") + + assert upstream_git_repo.git.ls_tree("-r", branch_name, "--name-only") + + +def test_create_pull_request_error(monkeypatch: pytest.MonkeyPatch, repository_client: Client): + """ + arrange: given Client with a mocked github repository client that raises an exception + act: when _create_pull_request is called + assert: RepositoryClientError is raised. + """ + mock_github_repository = mock.MagicMock(spec=Repository) + mock_github_repository.create_pull.side_effect = [ + GithubException(status=500, data="Internal Server Error", headers=None) + ] + monkeypatch.setattr(repository_client, "_github_repo", mock_github_repository) + + with pytest.raises(RepositoryClientError) as exc: + repository_client.create_pull_request(branch_name="branchname-1") + + assert_substrings_in_string( + ("unexpected error creating pull request", "githubexception"), str(exc.value).lower() + ) + + +def test_create_pull_request(repository_client: Client, mock_pull_request: PullRequest): + """ + arrange: given Client with a mocked github client that returns a mocked pull request + act: when _create_pull_request is called + assert: a pull request's page link is returned. + """ + returned_url = repository_client.create_pull_request("branchname-1") + + assert returned_url == mock_pull_request.html_url + + +def test_get_file_content_github_error(monkeypatch: pytest.MonkeyPatch, repository_client: Client): + """ + arrange: given Client with a mocked github repository client that raises an exception + act: when get_file_content is called + assert: RepositoryClientError is raised. + """ + mock_github_repository = mock.MagicMock(spec=Repository) + mock_github_repository.get_contents.side_effect = [ + GithubException(status=401, data="unauthorized", headers=None) + ] + monkeypatch.setattr(repository_client, "_github_repo", mock_github_repository) + + with pytest.raises(RepositoryClientError) as exc: + repository_client.get_file_content(path="path 1") + + assert_substrings_in_string(("unauthorized", "401"), str(exc.value).lower()) + + +def test_get_file_content_unknown_object_error( + monkeypatch: pytest.MonkeyPatch, repository_client: Client +): + """ + arrange: given Client with a mocked github repository client that raises an exception + act: when get_file_content is called + assert: RepositoryFileNotFoundError is raised. + """ + mock_github_repository = mock.MagicMock(spec=Repository) + mock_github_repository.get_contents.side_effect = [ + UnknownObjectException(status=404, data="File not found error", headers=None) + ] + monkeypatch.setattr(repository_client, "_github_repo", mock_github_repository) + path = "path 1" + + with pytest.raises(RepositoryFileNotFoundError) as exc: + repository_client.get_file_content(path=path) + + assert_substrings_in_string(("not", "retrieve", "file", path), str(exc.value).lower()) + + +def test_get_file_content_list(monkeypatch: pytest.MonkeyPatch, repository_client: Client): + """ + arrange: given Client with a mocked github repository client that returns a list + act: when get_file_content is called + assert: RepositoryFileNotFoundError is raised. + """ + mock_github_repository = mock.MagicMock(spec=Repository) + mock_github_repository.get_contents.return_value = [] + monkeypatch.setattr(repository_client, "_github_repo", mock_github_repository) + path = "path 1" + + with pytest.raises(RepositoryFileNotFoundError) as exc: + repository_client.get_file_content(path=path) + + assert_substrings_in_string(("path", "matched", "more", "file", path), str(exc.value).lower()) + + +def test_get_file_content_content_none(monkeypatch: pytest.MonkeyPatch, repository_client: Client): + """ + arrange: given Client with a mocked github repository client that returns None + content + act: when get_file_content is called + assert: RepositoryFileNotFoundError is raised. + """ + mock_github_repository = mock.MagicMock(spec=Repository) + mock_content_file = mock.MagicMock(spec=ContentFile) + mock_content_file.content = None + mock_github_repository.get_contents.return_value = mock_content_file + monkeypatch.setattr(repository_client, "_github_repo", mock_github_repository) + path = "path 1" + + with pytest.raises(RepositoryFileNotFoundError) as exc: + repository_client.get_file_content(path=path) + + assert_substrings_in_string(("path", "not", "file", path), str(exc.value).lower()) + + +def test_get_file_content(monkeypatch: pytest.MonkeyPatch, repository_client: Client): + """ + arrange: given path, Client with a mocked github repository client that returns content + act: when get_file_content is called with the path + assert: then the content is returned. + """ + mock_github_repository = mock.MagicMock(spec=Repository) + mock_content_file = mock.MagicMock(spec=ContentFile) + content = "content 1" + mock_content_file.content = base64.b64encode(content.encode(encoding="utf-8")) + mock_github_repository.get_contents.return_value = mock_content_file + monkeypatch.setattr(repository_client, "_github_repo", mock_github_repository) + path = "path 1" + + returned_content = repository_client.get_file_content(path=path) + + assert returned_content == content + mock_github_repository.get_contents.assert_called_once_with(path) + + +def test_get_file_content_branch(monkeypatch: pytest.MonkeyPatch, repository_client: Client): + """ + arrange: given path and branch, client with a mocked github repository client that returns + content + act: when get_file_content is called with the path and branch + assert: then the content is returned. + """ + mock_github_repository = mock.MagicMock(spec=Repository) + mock_content_file = mock.MagicMock(spec=ContentFile) + content = "content 1" + mock_content_file.content = base64.b64encode(content.encode(encoding="utf-8")) + mock_github_repository.get_contents.return_value = mock_content_file + monkeypatch.setattr(repository_client, "_github_repo", mock_github_repository) + path = "path 1" + branch = "branch 1" + + returned_content = repository_client.get_file_content(path=path, branch=branch) + + assert returned_content == content + mock_github_repository.get_contents.assert_called_once_with(path, branch) + + +@pytest.mark.parametrize( + "remote_url", + [ + pytest.param("https://gitlab.com/canonical/upload-charm-docs.git", id="non-github url"), + pytest.param("http://gitlab.com/canonical/upload-charm-docs.git", id="http url"), + pytest.param("git@github.com:yanksyoon/actionrefer.git", id="ssh url"), + ], +) +def test_get_repository_name_invalid(remote_url: str): + """ + arrange: given a non-valid remote_url + act: when _get_repository_name is called + assert: InputError is raised. + """ + with pytest.raises(InputError) as exc: + repository._get_repository_name_from_git_url(remote_url=remote_url) + + assert_substrings_in_string( + ("invalid remote repository url",), + str(exc.value).lower(), + ) + + +@pytest.mark.parametrize( + "remote_url, expected_repository_name", + [ + pytest.param( + "https://github.com/canonical/upload-charm-docs", + "canonical/upload-charm-docs", + id="valid url", + ), + pytest.param( + "https://github.com/canonical/upload-charm-docs.git", + "canonical/upload-charm-docs", + id="valid git url", + ), + ], +) +def test_get_repository_name(remote_url: str, expected_repository_name: str): + """ + arrange: given a non-valid remote_url + act: when _get_repository_name is called + assert: GitError is raised. + """ + assert ( + repository._get_repository_name_from_git_url(remote_url=remote_url) + == expected_repository_name + ) + + +def test_create_repository_client_no_token( + repository_path: Path, +): + """ + arrange: given valid repository path and empty access_token + act: when create_repository_client is called + assert: InputError is raised. + """ + # the following token is deliberately empty for this test + test_token = "" # nosec + + with pytest.raises(InputError) as exc: + repository.create_repository_client(access_token=test_token, base_path=repository_path) + + assert_substrings_in_string( + ("invalid", "access_token", "input", "it must be", "non-empty"), + str(exc.value).lower(), + ) + + +def test_create_repository_client( + monkeypatch: pytest.MonkeyPatch, + git_repo: Repo, + repository_path: Path, + mock_github_repo: Repository, +): + """ + arrange: given valid repository path and a valid access_token and a mocked github client + act: when create_repository_client is called + assert: RepositoryClient is returned. + """ + # The origin is initialised to be local, need to update it to be remote + origin = git_repo.remote("origin") + git_repo.delete_remote(origin) + git_repo.create_remote("origin", "https://github.com/test-user/test-repo.git") + test_token = secrets.token_hex(16) + mock_github_client = mock.MagicMock(spec=Github) + mock_github_client.get_repo.returns = mock_github_repo + monkeypatch.setattr(repository, "Github", mock_github_client) + + returned_client = repository.create_repository_client( + access_token=test_token, base_path=repository_path + ) + + assert isinstance(returned_client, repository.Client) diff --git a/tox.ini b/tox.ini index e434321d..72e7be6d 100644 --- a/tox.ini +++ b/tox.ini @@ -75,10 +75,9 @@ commands = [testenv:unit] description = Run unit tests deps = - pytest - coverage[toml] -r{toxinidir}/requirements.txt -r{toxinidir}/dev-requirements.txt + coverage[toml] commands = coverage run --source={[vars]src_path} \ -m pytest --ignore={[vars]tst_path}integration -v --tb native -s {posargs} @@ -87,19 +86,15 @@ commands = [testenv:coverage-report] description = Create test coverage report deps = - pytest - coverage[toml] -r{toxinidir}/requirements.txt + -r{toxinidir}/dev-requirements.txt + coverage[toml] commands = coverage report [testenv:integration] description = Run integration tests deps = - pytest - ops - pytest-operator - pytest-asyncio -r{toxinidir}/requirements.txt -r{toxinidir}/dev-requirements.txt commands =