landoscript: refactor extension_manifest_version_bump into smaller functions (bug 2025695)

Author: JohanLorenzo

landoscript/src/landoscript/actions/extension_manifest_version_bump.py

@@ -18,54 +18,64 @@
 ALLOWED_MANIFEST_FILES = ("browser/extensions/newtab/manifest.json",)
 
 
-async def run(
-    github_client: GithubClient,
-    public_artifact_dir: str,
-    branch: str,
-    info: dict,
-) -> list[LandoAction]:
-    file = info["file"]
+def _validate_file(file: str) -> None:
     if file not in ALLOWED_MANIFEST_FILES:
         raise TaskVerificationError(f"{file} is not in extension manifest version bump allowlist")
 
+
+async def _fetch_file(github_client: GithubClient, file: str, branch: str) -> str:
     try:
         log.info(f"fetching {file} from github")
         orig_files = await github_client.get_files([file], branch)
     except TransportError as e:
         raise LandoscriptError(f"couldn't retrieve {file} from github") from e
-
     orig = orig_files.get(file)
     if not orig:
         raise LandoscriptError(f"{file} does not exist!")
+    return orig
 
-    log.info(f"{file} contents:")
-    log_file_contents(orig)
 
+def _bump_manifest_version(orig: str, file: str) -> tuple[str, str] | None:
     manifest = json.loads(orig)
-    version_str = manifest["version"]
-    cur = BaseVersion.parse(version_str)
-    next_ = cur.bump("patch_number")
-
-    log.info(f"{file}: bumping {cur} -> {next_}")
-
+    cur = BaseVersion.parse(manifest["version"])
+    next_ = cur.bump("minor_number")
     modified = re.sub(
         r'("version":\s*")' + re.escape(str(cur)) + r'"',
         rf'\g<1>{next_}"',
         orig,
     )
     if orig == modified:
-        raise LandoscriptError(f"{file}: version replacement produced no change, this should be impossible")
+        log.warning(f"{file}: version replacement produced no change, skipping")
+        return None
+    return modified, str(next_)
 
-    log.info(f"{file}: new contents are:")
-    log_file_contents(modified)
 
+def _build_commit(orig: str, modified: str, file: str, next_version: str, public_artifact_dir: str) -> LandoAction:
     diff = diff_contents(orig, modified, file)
-
     with open(os.path.join(public_artifact_dir, "extension-manifest-version-bump.diff"), "w+") as f:
         f.write(diff)
-
     log.info("adding version bump commit! diff contents are:")
     log_file_contents(diff)
+    commitmsg = f"No Bug - Bump newtab manifest.json version to {next_version} a=release CLOSED TREE DONTBUILD"
+    return create_commit_action(commitmsg, diff)
 
-    commitmsg = f"No Bug - Bump newtab manifest.json version to {next_} a=release CLOSED TREE DONTBUILD"
-    return [create_commit_action(commitmsg, diff)]
+
+async def run(
+    github_client: GithubClient,
+    public_artifact_dir: str,
+    branch: str,
+    info: dict,
+) -> list[LandoAction]:
+    file = info["file"]
+    _validate_file(file)
+    orig = await _fetch_file(github_client, file, branch)
+    log.info(f"{file} contents:")
+    log_file_contents(orig)
+    result = _bump_manifest_version(orig, file)
+    if result is None:
+        return []
+    modified, next_version = result
+    log.info(f"{file}: bumping to {next_version}")
+    log.info(f"{file}: new contents are:")
+    log_file_contents(modified)
+    return [_build_commit(orig, modified, file, next_version, public_artifact_dir)]

landoscript/tests/test_extension_manifest_version_bump.py

@@ -4,7 +4,12 @@
 from pytest_scriptworker_client import get_files_payload
 from scriptworker.client import TaskVerificationError
 
-from landoscript.actions.extension_manifest_version_bump import ALLOWED_MANIFEST_FILES
+from landoscript.actions.extension_manifest_version_bump import (
+    ALLOWED_MANIFEST_FILES,
+    _build_commit,
+    _bump_manifest_version,
+    _validate_file,
+)
 from landoscript.errors import LandoscriptError
 from landoscript.script import async_main
 
@@ -30,6 +35,55 @@ def _manifest(version):
     )
 
 
+# --- Unit tests for helper functions (no mocks needed) ---
+
+
+@pytest.mark.parametrize(
+    "initial_version,expected_version",
+    [
+        pytest.param("151.0.0", "151.1.0", id="initial_after_merge_day"),
+        pytest.param("151.1.0", "151.2.0", id="normal_increment"),
+    ],
+)
+def test_bump_manifest_version(initial_version, expected_version):
+    modified, next_version = _bump_manifest_version(_manifest(initial_version), MANIFEST_FILE)
+    assert next_version == expected_version
+    assert f'"version": "{expected_version}"' in modified
+    assert f'"version": "{initial_version}"' not in modified
+
+
+def test_validate_file_allowed():
+    _validate_file(MANIFEST_FILE)
+
+
+def test_validate_file_not_in_allowlist():
+    with pytest.raises(TaskVerificationError, match="not in extension manifest version bump allowlist"):
+        _validate_file("browser/extensions/webcompat/manifest.json")
+
+
+def test_bump_manifest_version_no_change():
+    orig = _manifest("151.0.0")
+    # Simulate a regex match failure (e.g. version format in file doesn't match
+    # what BaseVersion.parse produces) by patching re.sub to return orig unchanged.
+    with pytest.MonkeyPatch().context() as mp:
+        mp.setattr("landoscript.actions.extension_manifest_version_bump.re.sub", lambda *a, **kw: orig)
+        result = _bump_manifest_version(orig, MANIFEST_FILE)
+    assert result is None
+
+
+def test_build_commit(tmp_path):
+    orig = _manifest("151.0.0")
+    modified = orig.replace('"151.0.0"', '"151.0.1"')
+    action = _build_commit(orig, modified, MANIFEST_FILE, "151.0.1", str(tmp_path))
+    assert action["action"] == "create-commit"
+    assert "151.0.1" in action["commitmsg"]
+    assert "a=release" in action["commitmsg"]
+    assert (tmp_path / "extension-manifest-version-bump.diff").exists()
+
+
+# --- Integration tests via async_main ---
+
+
 def assert_success(req, initial_version, expected_version):
     assert "json" in req.kwargs
     create_commit_actions = [a for a in req.kwargs["json"]["actions"] if a["action"] == "create-commit"]
@@ -126,7 +180,3 @@ async def test_file_not_found(aioresponses, github_installation_responses, conte
         err=LandoscriptError,
         errmsg="does not exist",
     )
-
-
-def test_allowed_manifest_files():
-    assert MANIFEST_FILE in ALLOWED_MANIFEST_FILES