From b1b9a7845a478163a964b6b80fe0752cf99ac998 Mon Sep 17 00:00:00 2001 From: Schuyler Martin Date: Wed, 20 Nov 2024 07:39:17 -0700 Subject: [PATCH] [Partial] 168 change the references of the old version to the new version (#247) * Adds check to bump-recipe that ensures version is provided when needed * Fixes/updates some of the bump-recipe unit tests * Fixes remaining bump-recipe unit tests from version option fall-out * Moves test_bump_recipe.py to use read-only parser when editing commands are not used in assert statements * test_bump_recipe.py now uses more consistent names across tests. * Starts work on version update capability * Temp workaround for linting error --- conda_recipe_manager/commands/bump_recipe.py | 66 ++++++++++----- tests/commands/test_bump_recipe.py | 85 ++++++++++++-------- 2 files changed, 98 insertions(+), 53 deletions(-) diff --git a/conda_recipe_manager/commands/bump_recipe.py b/conda_recipe_manager/commands/bump_recipe.py index 43f4100c..e146022b 100644 --- a/conda_recipe_manager/commands/bump_recipe.py +++ b/conda_recipe_manager/commands/bump_recipe.py @@ -6,7 +6,7 @@ import sys from pathlib import Path -from typing import cast +from typing import Optional, cast import click @@ -49,30 +49,35 @@ def _update_build_num(recipe_parser: RecipeParser, increment_build_num: bool) -> print_err("`/build` key could not be found in the recipe.") sys.exit(ExitCode.ILLEGAL_OPERATION) - # If build key is found, try to get build/number key in case of `build_num` set to false, `build/number` key will be - # added and set to zero when `build_num` is set to true, throw error and sys.exit() - - # TODO use contains_value() instead of try catch - try: + # From the previous check, we know that `/build` exists. If `/build/number` is missing, it'll be added by + # a patch-add operation and set to a default value of 0. Otherwise, we attempt to increment the build number, if + # requested. + if increment_build_num and recipe_parser.contains_value("/build/number"): build_number = recipe_parser.get_value("/build/number") - if increment_build_num: - if not isinstance(build_number, int): - print_err("Build number is not an integer.") - sys.exit(ExitCode.ILLEGAL_OPERATION) - - _exit_on_failed_patch( - recipe_parser, - cast(JsonPatchType, {"op": "replace", "path": "/build/number", "value": build_number + 1}), - ) - return - except KeyError: - if increment_build_num: - print_err("`/build/number` key could not be found in the recipe.") + + if not isinstance(build_number, int): + print_err("Build number is not an integer.") sys.exit(ExitCode.ILLEGAL_OPERATION) + _exit_on_failed_patch( + recipe_parser, + cast(JsonPatchType, {"op": "replace", "path": "/build/number", "value": build_number + 1}), + ) + return + _exit_on_failed_patch(recipe_parser, cast(JsonPatchType, {"op": "add", "path": "/build/number", "value": 0})) +def _update_version(recipe_parser: RecipeParser, target_version: str) -> None: # pylint: disable=unused-argument + """ + Attempts to update the `/package/version` field and/or the commonly used `version` JINJA variable. + + :param recipe_parser: Recipe file to update. + :param target_version: Target version to update to. + """ + # TODO branch on `/package/version` being specified without a `version` variable + + def _update_sha256(recipe_parser: RecipeParser) -> None: """ Attempts to update the SHA-256 hash(s) in the `/source` section of a recipe file, if applicable. Note that this is @@ -117,16 +122,29 @@ def _update_sha256(recipe_parser: RecipeParser) -> None: @click.command(short_help="Bumps a recipe file to a new version.") @click.argument("recipe_file_path", type=click.Path(exists=True, path_type=str)) @click.option( + "-b", "--build-num", is_flag=True, help="Bump the build number by 1.", ) -def bump_recipe(recipe_file_path: str, build_num: bool) -> None: +@click.option( + "-t", + "--target-version", + default=None, + type=str, + help="New project version to target. Required if `--build-num` is NOT specified.", +) +def bump_recipe(recipe_file_path: str, build_num: bool, target_version: Optional[str]) -> None: """ Bumps a recipe to a new version. RECIPE_FILE_PATH: Path to the target recipe file """ + + if not build_num and target_version is None: + print_err("The `--target-version` option must be set if `--build-num` is not specified.") + sys.exit(ExitCode.CLICK_USAGE) + try: contents_recipe = Path(recipe_file_path).read_text(encoding="utf-8") except IOError: @@ -141,7 +159,13 @@ def bump_recipe(recipe_file_path: str, build_num: bool) -> None: # Attempt to update fields _update_build_num(recipe_parser, build_num) - if not build_num: + + # NOTE: We check if `target_version` is specified to perform a "full bump" for type checking reasons. Also note that + # the `build_num` flag is invalidated if we are bumping to a new version. The build number must be reset to 0 in + # this case. + if target_version is not None: + # Version must be updated before hash to ensure the correct artifact is hashed. + _update_version(recipe_parser, target_version) _update_sha256(recipe_parser) Path(recipe_file_path).write_text(recipe_parser.render(), encoding="utf-8") diff --git a/tests/commands/test_bump_recipe.py b/tests/commands/test_bump_recipe.py index e8ec08b4..769a2d0e 100644 --- a/tests/commands/test_bump_recipe.py +++ b/tests/commands/test_bump_recipe.py @@ -2,7 +2,8 @@ :Description: Tests the `bump-recipe` CLI """ -from typing import Final, cast +from pathlib import Path +from typing import Final, Optional, cast from unittest.mock import patch import pytest @@ -12,7 +13,7 @@ from conda_recipe_manager.commands import bump_recipe from conda_recipe_manager.commands.utils.types import ExitCode from conda_recipe_manager.fetcher.http_artifact_fetcher import HttpArtifactFetcher -from conda_recipe_manager.parser.recipe_parser import RecipeParser +from conda_recipe_manager.parser.recipe_reader import RecipeReader from tests.file_loading import get_test_path, load_recipe from tests.http_mocking import MockHttpStreamResponse from tests.smoke_testing import assert_cli_usage @@ -77,32 +78,35 @@ def test_usage() -> None: @pytest.mark.parametrize( - "recipe_file,increment_build_num,expected_recipe_file", + "recipe_file,version,expected_recipe_file", [ - ("types-toml.yaml", True, "bump_recipe/types-toml_build_num_1.yaml"), + ("types-toml.yaml", None, "bump_recipe/types-toml_build_num_1.yaml"), # TODO: Re-enable test when version bumping is supported. - # ("types-toml.yaml", False, "bump_recipe/types-toml_version_bump.yaml"), + # ("types-toml.yaml", "0.10.8.20240310", "bump_recipe/types-toml_version_bump.yaml"), # NOTE: These have no source section, therefore all SHA-256 update attempts (and associated network requests) # should be skipped. - ("bump_recipe/build_num_1.yaml", True, "bump_recipe/build_num_2.yaml"), - ("bump_recipe/build_num_1.yaml", False, "simple-recipe.yaml"), - ("bump_recipe/build_num_42.yaml", True, "bump_recipe/build_num_43.yaml"), - ("bump_recipe/build_num_42.yaml", False, "simple-recipe.yaml"), - ("bump_recipe/build_num_-1.yaml", True, "simple-recipe.yaml"), - ("bump_recipe/build_num_-1.yaml", False, "simple-recipe.yaml"), + ("bump_recipe/build_num_1.yaml", None, "bump_recipe/build_num_2.yaml"), + ("bump_recipe/build_num_1.yaml", "0.10.8.6", "simple-recipe.yaml"), + ("bump_recipe/build_num_42.yaml", None, "bump_recipe/build_num_43.yaml"), + ("bump_recipe/build_num_42.yaml", "0.10.8.6", "simple-recipe.yaml"), + ("bump_recipe/build_num_-1.yaml", None, "simple-recipe.yaml"), + ("bump_recipe/build_num_-1.yaml", "0.10.8.6", "simple-recipe.yaml"), ], ) def test_bump_recipe_cli( fs: FakeFilesystem, recipe_file: str, - increment_build_num: bool, + version: Optional[str], expected_recipe_file: str, ) -> None: """ Test that the recipe file is successfully updated/bumped. :param fs: `pyfakefs` Fixture used to replace the file system - :param request: Pytest fixture request object. + :param recipe_file: Target recipe file to update + :param version: (Optional) version to bump to. If `None`, this indicates `bump-recipe` should be run in + increment-only mode. + :param expected_recipe_file: Expected resulting recipe file """ runner = CliRunner() fs.add_real_directory(get_test_path(), read_only=False) @@ -111,41 +115,54 @@ def test_bump_recipe_cli( expected_recipe_file_path = get_test_path() / expected_recipe_file cli_args: Final[list[str]] = ( - ["--build-num", str(recipe_file_path)] if increment_build_num else [str(recipe_file_path)] + ["--build-num", str(recipe_file_path)] if version is None else ["-t", version, str(recipe_file_path)] ) with patch("requests.get", new=mock_requests_get): result = runner.invoke(bump_recipe.bump_recipe, cli_args) - parser = load_recipe(recipe_file_path, RecipeParser) - expected_parser = load_recipe(expected_recipe_file_path, RecipeParser) + # Read the edited file and check it against the expected file. + parser = load_recipe(recipe_file_path, RecipeReader) + expected_parser = load_recipe(expected_recipe_file_path, RecipeReader) assert parser == expected_parser assert result.exit_code == ExitCode.SUCCESS -def test_bump_cli_build_number_key_missing(fs: FakeFilesystem) -> None: +def test_bump_recipe_exits_if_target_version_missing() -> None: """ - Test that a `build: number:` key is added and set to 0 when it's missing. + Ensures that the `--target-version` flag is required when `--build-num` is NOT provided. + """ + runner = CliRunner() + result = runner.invoke(bump_recipe.bump_recipe, [str(get_test_path() / "types-toml.yaml")]) + + # Ensure the expected error case was hit (as opposed to a different error case). + assert result.stdout == "The `--target-version` option must be set if `--build-num` is not specified.\n" + assert result.exit_code == ExitCode.CLICK_USAGE + + +def test_bump_recipe_increment_build_number_key_missing(fs: FakeFilesystem) -> None: + """ + Test that a `/build/number` key is added and set to 0 when it's missing. :param fs: `pyfakefs` Fixture used to replace the file system """ runner = CliRunner() fs.add_real_directory(get_test_path(), read_only=False) - recipe_file_path = get_test_path() / "bump_recipe/no_build_num.yaml" - expected_recipe_file_path = get_test_path() / "bump_recipe/build_num_added.yaml" + recipe_file_path: Final[Path] = get_test_path() / "bump_recipe/no_build_num.yaml" + expected_recipe_file_path: Final[Path] = get_test_path() / "bump_recipe/build_num_added.yaml" - result = runner.invoke(bump_recipe.bump_recipe, [str(recipe_file_path)]) + result = runner.invoke(bump_recipe.bump_recipe, ["--build-num", str(recipe_file_path)]) - parser = load_recipe(recipe_file_path, RecipeParser) - expected_parser = load_recipe(expected_recipe_file_path, RecipeParser) + parser = load_recipe(recipe_file_path, RecipeReader) + expected_parser = load_recipe(expected_recipe_file_path, RecipeReader) assert parser == expected_parser assert result.exit_code == ExitCode.SUCCESS -def test_bump_cli_build_number_not_int(fs: FakeFilesystem) -> None: +def test_bump_recipe_increment_build_number_not_int(fs: FakeFilesystem) -> None: """ Test that the command fails gracefully case when the build number is not an integer, and we are trying to increment it. @@ -156,15 +173,16 @@ def test_bump_cli_build_number_not_int(fs: FakeFilesystem) -> None: runner = CliRunner() fs.add_real_directory(get_test_path(), read_only=False) - recipe_file_path = get_test_path() / "bump_recipe/non_int_build_num.yaml" + recipe_file_path: Final[Path] = get_test_path() / "bump_recipe/non_int_build_num.yaml" result = runner.invoke(bump_recipe.bump_recipe, ["--build-num", str(recipe_file_path)]) assert result.exit_code == ExitCode.ILLEGAL_OPERATION -def test_bump_cli_increment_build_num_key_not_found(fs: FakeFilesystem) -> None: +def test_bump_recipe_increment_build_num_key_not_found(fs: FakeFilesystem) -> None: """ - Test that the command fails gracefully when the build number key is missing and we try to increment it's value. + Test that the command fixes the recipe file when the `/build/number` key is missing and we try to increment it's + value. :param fs: `pyfakefs` Fixture used to replace the file system """ @@ -172,12 +190,15 @@ def test_bump_cli_increment_build_num_key_not_found(fs: FakeFilesystem) -> None: runner = CliRunner() fs.add_real_directory(get_test_path(), read_only=False) - recipe_file_path = get_test_path() / "bump_recipe/no_build_num.yaml" + recipe_file_path: Final[Path] = get_test_path() / "bump_recipe/no_build_num.yaml" result = runner.invoke(bump_recipe.bump_recipe, ["--build-num", str(recipe_file_path)]) - assert result.exit_code == ExitCode.ILLEGAL_OPERATION + # TODO: Can't compare directly to `simple-recipe.yaml` as the added key `/build/number` is not canonically sorted to + # be in the standard position. + assert load_recipe(recipe_file_path, RecipeReader).get_value("/build/number") == 0 + assert result.exit_code == ExitCode.SUCCESS -def test_bump_cli_no_build_key_found(fs: FakeFilesystem) -> None: +def test_bump_recipe_increment_no_build_key_found(fs: FakeFilesystem) -> None: """ Test that the command fails gracefully when the build key is missing and we try to revert build number to zero. @@ -187,6 +208,6 @@ def test_bump_cli_no_build_key_found(fs: FakeFilesystem) -> None: runner = CliRunner() fs.add_real_directory(get_test_path(), read_only=False) - recipe_file_path = get_test_path() / "bump_recipe/no_build_key.yaml" - result = runner.invoke(bump_recipe.bump_recipe, [str(recipe_file_path)]) + recipe_file_path: Final[Path] = get_test_path() / "bump_recipe/no_build_key.yaml" + result = runner.invoke(bump_recipe.bump_recipe, ["--build-num", str(recipe_file_path)]) assert result.exit_code == ExitCode.ILLEGAL_OPERATION