From 2fd10d368721259e5abe164ecadde5b985c82501 Mon Sep 17 00:00:00 2001 From: Alex Moses Date: Fri, 3 Jul 2026 13:14:06 +0000 Subject: [PATCH 1/5] Bug-2038705: Use PyGithub for calls to GitHub v3 Updated the below functions in treeherder/utils/github.py to use PyGithub - get_repository - compare_shas - get_all_commits - get_commit - get_pull_request - get_pull_request_commits - get_releases Pending work: - Callers need to be updated. - Failing tests need to be examined - New tests needed for `github.py` Fixes [Bug-2038705](https://bugzilla.mozilla.org/show_bug.cgi?id=2038705) --- treeherder/utils/github.py | 73 ++++++++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 26 deletions(-) diff --git a/treeherder/utils/github.py b/treeherder/utils/github.py index c408f9b7378..e9c9bd20c62 100644 --- a/treeherder/utils/github.py +++ b/treeherder/utils/github.py @@ -1,14 +1,14 @@ from github import Auth, Github +from github.Commit import Commit +from github.Comparison import Comparison +from github.GitRelease import GitRelease +from github.PaginatedList import PaginatedList +from github.PullRequest import PullRequest +from github.Repository import Repository from treeherder.config.settings import GITHUB_TOKEN from treeherder.utils.http import fetch_json -if GITHUB_TOKEN: - auth = Auth.Token(GITHUB_TOKEN) - github = Github(auth=auth) -else: - github = Github() - def fetch_api(path, params=None): return fetch_api_full_url(f"https://api.github.com/{path}", params) @@ -22,37 +22,58 @@ def fetch_api_full_url(url, params=None): return fetch_json(url, params, headers) -def get_releases(owner, repo, params=None): - return fetch_api(f"repos/{owner}/{repo}/releases", params) +if GITHUB_TOKEN: + auth = Auth.Token(GITHUB_TOKEN) + github_client = Github(auth=auth) +else: + github_client = Github() -def get_repo(owner, repo, params=None): - return fetch_api(f"{owner}/{repo}", params) +def get_repository(owner: str, repo_name: str) -> Repository: + """ + Returns the PyGithub Repository object for the given repo. + """ + return github_client.get_repo(full_name_or_id=f"{owner}/{repo_name}") -def pygithub_get_repo(owner, repo): - return github.get_repo(f"{owner}/{repo}") +def compare_shas(owner: str, repo_name: str, base: str, head: str) -> Comparison: + """ + Returns a Comparison object for a comparison of base against head. + """ + return get_repository(owner=owner, repo_name=repo_name).compare(base=base, head=head) -def compare_shas(owner, repo, base, head): - repo = pygithub_get_repo(owner, repo) - comparison = repo.compare(base, head) - return [commit for commit in comparison.commits] +def get_all_commits(owner: str, repo: str, params: None) -> PaginatedList[Commit]: + """ + Returns a PaginatedList of PyGithub commit objects. + Optional params: sha, since, until, path, author, committer, per_page. + """ + return get_repository(owner=owner, repo_name=repo).get_commits(**params) -def get_all_commits(owner, repo, params=None): - return fetch_api(f"repos/{owner}/{repo}/commits", params) +def get_commit(owner: str, repo: str, sha: str) -> Commit: + """ + Returns the PyGithub Commit object for the given commit hash. + """ + return get_repository(owner=owner, repo_name=repo).get_commit(sha=sha) -def get_commit(owner, repo, sha, params=None): - return fetch_api(f"repos/{owner}/{repo}/commits/{sha}", params) +def get_pull_request(owner: str, repo_name: str, pr_number: int) -> PullRequest: + """ + Returns the PyGithub PullRequest object for the given PR. + """ + return get_repository(owner, repo_name).get_pull(pr_number) -def get_pull_request(owner, repo, pr_id): - repo = pygithub_get_repo(owner, repo) - return repo.get_pull(pr_id) +def get_pull_request_commits(owner: str, repo_name: str, pr_number: str) -> PaginatedList[Commit]: + """ + Returns a PaginatedList of PyGithub Commit objects associated with a given PR. + """ + return get_pull_request(owner, repo_name, pr_number).get_commits() -def get_pull_request_commits(owner, repo, pr_id): - pr = get_pull_request(owner, repo, pr_id) - return [commit for commit in pr.get_commits()] +def get_releases(owner: str, repo_name: str, options: None) -> PaginatedList[GitRelease]: + """ + Returns a PaginatedList of PyGithub GitRelease objects associated with a given repository. + """ + return get_repository(owner=owner, repo_name=repo_name).get_releases() From 590471c091aff77b43cde38dabe895ebbdd169cc Mon Sep 17 00:00:00 2001 From: Alex Moses Date: Mon, 6 Jul 2026 12:51:27 +0000 Subject: [PATCH 2/5] Bug-2038705: Fix failing tests - Fixed failing unit tests for push_loader - Updated changelog/collector.py --- .vscode/settings.json | 4 +- treeherder/changelog/collector.py | 33 ++++++++------- treeherder/etl/management/commands/ingest.py | 42 +++++++++++++------- treeherder/utils/github.py | 37 ++++++++++------- 4 files changed, 69 insertions(+), 47 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index c164b4c57e1..b4ba7ace942 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -14,5 +14,7 @@ "eslint.validate": ["javascript", "javascriptreact", "typescript", "typescriptreact"], "eslint.options": { "overrideConfigFile": "eslint.config.mjs" - } + }, + "python.testing.unittestEnabled": false, + "python.testing.pytestEnabled": true } diff --git a/treeherder/changelog/collector.py b/treeherder/changelog/collector.py index c5723eec2ac..1f359677d53 100644 --- a/treeherder/changelog/collector.py +++ b/treeherder/changelog/collector.py @@ -23,42 +23,41 @@ def get_changes(self, **kw): filters = kw.get("filters") gh_options = {"number": kw.get("number", MAX_ITEMS)} - for release in github.get_releases(owner, repository, params=gh_options): - release["files"] = [] + for release in github.get_releases(owner, repository): # no "since" option for releases() we filter manually here - if "since" in kw and release["published_at"] <= kw["since"]: + if "since" in kw and release.published_at <= kw["since"]: continue - name = release["name"] or release["tag_name"] + name = release.name or release.tag_name yield { - "date": release["published_at"], - "author": release["author"]["login"], + "date": release.published_at, + "author": release.author.login, "message": "Released " + name, - "remote_id": release["id"], + "remote_id": release.id, "type": "release", - "url": release["html_url"], + "url": release.html_url, } if "since" in kw: gh_options["since"] = kw["since"] + # TODO: Limit the number of commits to MAX_ITEMS for commit in github.get_all_commits(owner, repository, params=gh_options): if filters: for filter in filters: if isinstance(filter, list) and filter[0] == "filter_by_path": - commit_info = github.get_commit(owner, repository, commit["sha"]) - commit["files"] = commit_info["files"] + commit_info = github.get_commit(owner, repository, commit.sha) + files = [f.filename for f in commit_info.files] break - message = commit["commit"]["message"] - message = message.split("\n")[0] + message = commit.commit.message.split("\n")[0] res = { - "date": commit["commit"]["author"]["date"], - "author": commit["commit"]["author"]["name"], + "date": commit.commit.author.date, + "author": commit.commit.author.name, "message": message, - "remote_id": commit["sha"], + "remote_id": commit.sha, "type": "commit", - "url": commit["html_url"], - "files": [f["filename"] for f in commit.get("files", [])], + "url": commit.html_url, + "files": files, } res = self.filters(res, filters) if res: diff --git a/treeherder/etl/management/commands/ingest.py b/treeherder/etl/management/commands/ingest.py index ae98f64b2dd..e4219f47bb9 100644 --- a/treeherder/etl/management/commands/ingest.py +++ b/treeherder/etl/management/commands/ingest.py @@ -13,6 +13,7 @@ from django.conf import settings from django.core.management.base import BaseCommand, CommandError from django.db import connection +from github.Commit import Commit from treeherder.client.thclient import TreeherderClient from treeherder.config.settings import GITHUB_TOKEN @@ -374,29 +375,40 @@ def ingest_git_pushes(project, dry_run=False): logger.info("--> Converting Github commits to pushes") _repo = repo_meta(project) owner, repo = _repo["owner"], _repo["repo"] - github_commits = github.get_all_commits(owner, repo) - not_push_revision = [] + github_commits: list[Commit] = github.get_all_commits(owner, repo) + not_push_revision = set() # Use a set for faster lookups push_revision = [] push_to_date = {} - for _commit in github_commits: - info = github.get_commit(owner, repo, _commit["sha"]) + for commit in github_commits: + # Get the entire commit object with all attributes + detailed_commit_obj = github.get_commit(owner, repo, commit.sha) + # Revisions that are marked as non-push should be ignored - if _commit["sha"] in not_push_revision: - logger.debug("Not a revision of a push: {}".format(_commit["sha"])) + if commit.sha in not_push_revision: + logger.debug(f"Not a revision of a push: {commit.sha}") continue # Establish which revisions to ignore - for index, parent in enumerate(info["parents"]): - if index != 0: - not_push_revision.append(parent["sha"]) + # If a commit has multiple parents, treat only the first parent as push + # All other parents are to be treated as merges and should not be treated as separate pushes + if len(detailed_commit_obj.parents) > 1: + for index, parent in enumerate(detailed_commit_obj.parents[1:]): + not_push_revision.append(parent.sha) # The 1st parent is the push from `master` from which we forked - oldest_parent_revision = info["parents"][0]["sha"] - push_to_date[oldest_parent_revision] = info["commit"]["committer"]["date"] - logger.info( - f"Push: {oldest_parent_revision} - Date: {push_to_date[oldest_parent_revision]}" - ) - push_revision.append(_commit["sha"]) + if detailed_commit_obj.parents: + oldest_parent_revision = detailed_commit_obj.parents[0].sha + push_to_date = detailed_commit_obj.commit.committer.date.isoformat() + logger.info( + f"Push: {oldest_parent_revision} - Date: {push_to_date[oldest_parent_revision]}" + ) + else: + # Initial commit case + oldest_parent_revision = detailed_commit_obj.sha + push_to_date = detailed_commit_obj.commit.committer.date.isoformat() + f"Initial Push: {oldest_parent_revision} - Date: {push_to_date[oldest_parent_revision]}" + + push_revision.append(commit["sha"]) if not dry_run: logger.info("--> Ingest Github pushes") diff --git a/treeherder/utils/github.py b/treeherder/utils/github.py index e9c9bd20c62..2cfe6b754f7 100644 --- a/treeherder/utils/github.py +++ b/treeherder/utils/github.py @@ -1,8 +1,6 @@ from github import Auth, Github from github.Commit import Commit -from github.Comparison import Comparison from github.GitRelease import GitRelease -from github.PaginatedList import PaginatedList from github.PullRequest import PullRequest from github.Repository import Repository @@ -11,10 +9,16 @@ def fetch_api(path, params=None): + """ + Deprecated. Use the pygithub methods instead. + """ return fetch_api_full_url(f"https://api.github.com/{path}", params) def fetch_api_full_url(url, params=None): + """ + Deprecated. Use the pygithub methods instead. + """ if GITHUB_TOKEN: headers = {"Authorization": f"token {GITHUB_TOKEN}"} else: @@ -36,19 +40,24 @@ def get_repository(owner: str, repo_name: str) -> Repository: return github_client.get_repo(full_name_or_id=f"{owner}/{repo_name}") -def compare_shas(owner: str, repo_name: str, base: str, head: str) -> Comparison: +def compare_shas(owner: str, repo_name: str, base: str, head: str) -> list[Commit]: """ - Returns a Comparison object for a comparison of base against head. + Returns a list of PyGithub Commit object for a comparison of base against head. """ - return get_repository(owner=owner, repo_name=repo_name).compare(base=base, head=head) + return [ + commit + for commit in get_repository(owner=owner, repo_name=repo_name) + .compare(base=base, head=head) + .commits + ] -def get_all_commits(owner: str, repo: str, params: None) -> PaginatedList[Commit]: +def get_all_commits(owner: str, repo: str, params: None) -> list[Commit]: """ - Returns a PaginatedList of PyGithub commit objects. + Returns a list of PyGithub Commit objects. Optional params: sha, since, until, path, author, committer, per_page. """ - return get_repository(owner=owner, repo_name=repo).get_commits(**params) + return [commit for commit in get_repository(owner=owner, repo_name=repo).get_commits(**params)] def get_commit(owner: str, repo: str, sha: str) -> Commit: @@ -65,15 +74,15 @@ def get_pull_request(owner: str, repo_name: str, pr_number: int) -> PullRequest: return get_repository(owner, repo_name).get_pull(pr_number) -def get_pull_request_commits(owner: str, repo_name: str, pr_number: str) -> PaginatedList[Commit]: +def get_pull_request_commits(owner: str, repo_name: str, pr_number: str) -> list[Commit]: """ - Returns a PaginatedList of PyGithub Commit objects associated with a given PR. + Returns a list of PyGithub Commit objects associated with a given PR. """ - return get_pull_request(owner, repo_name, pr_number).get_commits() + return [commit for commit in get_pull_request(owner, repo_name, pr_number).get_commits()] -def get_releases(owner: str, repo_name: str, options: None) -> PaginatedList[GitRelease]: +def get_releases(owner: str, repo_name: str) -> list[GitRelease]: """ - Returns a PaginatedList of PyGithub GitRelease objects associated with a given repository. + Returns a list of PyGithub GitRelease objects associated with a given repository. """ - return get_repository(owner=owner, repo_name=repo_name).get_releases() + return [release for release in get_repository(owner=owner, repo_name=repo_name).get_releases()] From eaad49ba0bdfb005a090c46dce26e1e5525db06c Mon Sep 17 00:00:00 2001 From: Alex Moses Date: Mon, 6 Jul 2026 19:08:51 +0000 Subject: [PATCH 3/5] Bug-2038705: Fix collector and unit tests Fixed collector.py and its unit tests Also, updated tasks.py --- tests/changelog/test_collector.py | 91 ++++++++++++------------------- tests/changelog/test_tasks.py | 10 ++-- treeherder/changelog/collector.py | 24 +++++--- treeherder/changelog/tasks.py | 4 +- 4 files changed, 56 insertions(+), 73 deletions(-) diff --git a/tests/changelog/test_collector.py b/tests/changelog/test_collector.py index e4b2847b105..65a1ea57a93 100644 --- a/tests/changelog/test_collector.py +++ b/tests/changelog/test_collector.py @@ -1,10 +1,7 @@ import binascii -import json import os -import re from datetime import datetime, timedelta - -import responses +from unittest.mock import MagicMock, patch from treeherder.changelog.collector import collect @@ -13,67 +10,47 @@ def random_id(): return binascii.hexlify(os.urandom(16)).decode("utf8") -RELEASES = re.compile(r"https://api.github.com/repos/.*/.*/releases.*") -COMMITS = re.compile(r"https://api.github.com/repos/.*/.*/commits\?.*") -COMMIT_INFO = re.compile(r"https://api.github.com/repos/.*/.*/commits/.*") - - def prepare_responses(): - now = datetime.now().strftime("%Y-%m-%dT%H:%M:%S") + now = datetime.now() - def releases(request): - data = [ - { - "name": "ok", - "published_at": now, - "id": random_id(), - "html_url": "url", - "tag_name": "some tag", - "author": {"login": "tarek"}, - } - ] - return 200, {}, json.dumps(data) + mock_release = MagicMock() + mock_release.published_at = now + mock_release.author.login = "tarek" + mock_release.name = "ok" + mock_release.tag_name = "some tag" + mock_release.id = random_id() + mock_release.html_url = "url" - responses.add_callback( - responses.GET, RELEASES, callback=releases, content_type="application/json" - ) + mock_commit = MagicMock() + mock_commit.sha = random_id() + mock_commit.html_url = "url" + mock_commit.commit.message = "yeah" + mock_commit.commit.author.name = "tarek" + mock_commit.commit.author.date = now - def _commit(): - files = [{"filename": "file1"}, {"filename": "file2"}] - return { - "files": files, - "name": "ok", - "sha": random_id(), - "html_url": "url", - "tag_name": "some tag", - "commit": { - "message": "yeah", - "author": {"name": "tarek", "date": now}, - "files": files, - }, - } + mock_detailed_commit = MagicMock() + mock_file = MagicMock() + mock_file.filename = "file1" + mock_detailed_commit.files = [mock_file] - def commit(request): - return 200, {}, json.dumps(_commit()) + mock_repo = MagicMock() + mock_repo.get_releases.return_value = [mock_release] + mock_repo.get_commits.return_value = [mock_commit] + mock_repo.get_commit.return_value = mock_detailed_commit - def commits(request): - return 200, {}, json.dumps([_commit()]) + return mock_repo - responses.add_callback( - responses.GET, COMMITS, callback=commits, content_type="application/json" - ) - responses.add_callback( - responses.GET, COMMIT_INFO, callback=commit, content_type="application/json" - ) - -@responses.activate def test_collect(): yesterday = datetime.now() - timedelta(days=1) - yesterday = yesterday.strftime("%Y-%m-%dT%H:%M:%S") - prepare_responses() - res = list(collect(yesterday)) + mock_repo = prepare_responses() + + with patch("treeherder.utils.github.github_client") as mock_gh: + mock_gh.get_repo.return_value = mock_repo + res = list(collect(yesterday)) - # we're not looking into much details here, we can do this - # once we start to tweak the filters - assert len(res) > 0 + assert len(res) > 0 + # Verify it includes the release and the commit + types = [r["type"] for r in res] + assert "release" in types + assert "commit" in types diff --git a/tests/changelog/test_tasks.py b/tests/changelog/test_tasks.py index 75fc91cd1b5..4a8b51cbfb4 100644 --- a/tests/changelog/test_tasks.py +++ b/tests/changelog/test_tasks.py @@ -1,5 +1,6 @@ +from unittest.mock import patch + import pytest -import responses from tests.changelog.test_collector import prepare_responses from treeherder.changelog.models import Changelog @@ -7,12 +8,13 @@ @pytest.mark.django_db() -@responses.activate def test_update_changelog(): - prepare_responses() + mock_repo = prepare_responses() num_entries = Changelog.objects.count() - update_changelog() + with patch("treeherder.utils.github.github_client") as mock_gh: + mock_gh.get_repo.return_value = mock_repo + update_changelog() # we're not looking into much details here, we can do this # once we start to tweak the filters diff --git a/treeherder/changelog/collector.py b/treeherder/changelog/collector.py index 1f359677d53..6b4d0580bd5 100644 --- a/treeherder/changelog/collector.py +++ b/treeherder/changelog/collector.py @@ -21,17 +21,18 @@ def get_changes(self, **kw): owner = kw["user"] repository = kw["repository"] filters = kw.get("filters") - gh_options = {"number": kw.get("number", MAX_ITEMS)} + gh_options = {} for release in github.get_releases(owner, repository): # no "since" option for releases() we filter manually here + print(f"kw={kw}") + print(f"relase={release}") if "since" in kw and release.published_at <= kw["since"]: continue - name = release.name or release.tag_name yield { "date": release.published_at, "author": release.author.login, - "message": "Released " + name, + "message": "Released " + (release.name or release.tag_name), "remote_id": release.id, "type": "release", "url": release.html_url, @@ -40,13 +41,18 @@ def get_changes(self, **kw): if "since" in kw: gh_options["since"] = kw["since"] - # TODO: Limit the number of commits to MAX_ITEMS - for commit in github.get_all_commits(owner, repository, params=gh_options): + # Limit the number of commits + commits = github.get_all_commits(owner, repository, params=gh_options) + for i, commit in enumerate(commits): + if i >= kw.get("number", MAX_ITEMS): + break + + files = [] if filters: - for filter in filters: - if isinstance(filter, list) and filter[0] == "filter_by_path": - commit_info = github.get_commit(owner, repository, commit.sha) - files = [f.filename for f in commit_info.files] + for filter_obj in filters: + if isinstance(filter_obj, list) and filter_obj[0] == "filter_by_path": + commit_detailed = github.get_commit(owner, repository, commit.sha) + files = [f.filename for f in commit_detailed.files] break message = commit.commit.message.split("\n")[0] diff --git a/treeherder/changelog/tasks.py b/treeherder/changelog/tasks.py index 5acde3e3002..4a17340b219 100644 --- a/treeherder/changelog/tasks.py +++ b/treeherder/changelog/tasks.py @@ -16,7 +16,6 @@ def update_changelog(days=1): logger.info(f"Updating unified changelog (days={days})") # collecting last day of changes across all sources since = datetime.datetime.now() - datetime.timedelta(days=days) - since = since.strftime("%Y-%m-%dT%H:%M:%S") created = 0 existed = 0 @@ -25,8 +24,7 @@ def update_changelog(days=1): for entry in collect(since): files = entry.pop("files", []) # lame hack to remove TZ awareness - if entry["date"].endswith("Z"): - entry["date"] = entry["date"][:-1] + entry["date"] = entry["date"].isoformat() changelog, line_created = Changelog.objects.update_or_create(**entry) if not line_created: existed += 1 From 87e69a93e168d4545166a420b8b024f7439d9650 Mon Sep 17 00:00:00 2001 From: Alex Moses Date: Tue, 7 Jul 2026 10:34:15 +0000 Subject: [PATCH 4/5] Bug-2038705: Migrate ingest.query_data to PyGithub Migrated the `query_data` function in ingest.py to use the library written over PyGithub. Also, updated the unit tests for the same. --- tests/etl/test_ingest_command.py | 91 ++++++++++---------- treeherder/etl/management/commands/ingest.py | 88 ++++++++++++------- treeherder/utils/github.py | 8 ++ 3 files changed, 111 insertions(+), 76 deletions(-) diff --git a/tests/etl/test_ingest_command.py b/tests/etl/test_ingest_command.py index 2784fa78413..094d2acbb14 100644 --- a/tests/etl/test_ingest_command.py +++ b/tests/etl/test_ingest_command.py @@ -1,3 +1,5 @@ +from unittest.mock import MagicMock + from treeherder.etl.management.commands import ingest REPO_META = { @@ -10,64 +12,59 @@ def test_query_data_consumes_compare_dict(monkeypatch): - """query_data must read the GitHub compare REST response as a dict. + """query_data must use PyGithub objects correctly. - Regression guard for Bug 2009865, which switched ``compare_shas`` to return - a list of PyGithub commit objects (for the Pulse push loader) but left - query_data doing dict access (``.get("merge_base_commit")`` / ``["commits"]``), - breaking the ``ingest push`` command for GitHub repos. + Regression guard for Bug 2038705 refactor. """ - compare_by_range = { - # base branch vs head: the head isn't on the base branch, so the API - # reports a merge base whose parent is the real fork point. - "main...HEAD": { - "merge_base_commit": { - "sha": "BASE", - "commit": {"committer": {"date": "2026-01-01T00:00:00Z"}}, - "parents": [ - { - "sha": "PARENT", - "url": "https://api.github.com/repos/o/r/commits/PARENT", - } - ], - }, - "commits": [], - }, - # re-compare with the corrected base yields the push's commits - "PARENT...HEAD": { - "merge_base_commit": {"sha": "PARENT", "parents": []}, - "commits": [ - { - "sha": "C1", - "commit": { - "message": "Fix the thing", - "author": {"name": "Dev", "email": "dev@example.com"}, - "committer": {"date": "2026-02-02T00:00:00Z"}, - }, - } - ], - }, - } - def fake_fetch_api(path, params=None): - return compare_by_range[path.split("/compare/")[1]] + mock_repo = MagicMock() + + def create_mock_commit(sha, date, parents=None, message="Fix the issue"): + mock_commit = MagicMock() + mock_commit.sha = sha + mock_commit.commit.committer.name = "Dev" + mock_commit.commit.committer.email = "dev@example.com" + mock_commit.commit.committer.date = date + mock_commit.commit.author.name = "Dev" + mock_commit.commit.author.email = "dev@example.com" + mock_commit.commit.author.date = date + mock_commit.commit.message = message + mock_commit.parents = parents or [] + return mock_commit + + date1 = "2026-01-01T00:00:00Z" + date2 = "2026-02-02T00:00:00Z" + + parent_commit = create_mock_commit("PARENT", date2) + base_commit = create_mock_commit("BASE", date1, parents=[parent_commit]) + c1 = create_mock_commit("C1", date2) + + mock_comparison1 = MagicMock() + mock_comparison1.merge_base_commit = base_commit + mock_comparison1.commits = [] + + mock_comparison2 = MagicMock() + mock_comparison2.merge_base_commit = parent_commit + mock_comparison2.commits = [c1] - def fake_fetch_api_full_url(url, params=None): - # The merge-base parent, with a committer date different from the merge - # base so query_data takes the simple (non-recursive) branch. - return {"sha": "PARENT", "commit": {"committer": {"date": "2026-02-02T00:00:00Z"}}} + def fake_compare(base, head): + if base == "main": + return mock_comparison1 + if base == "PARENT": + return mock_comparison2 + return MagicMock() - monkeypatch.setattr(ingest, "fetch_api", fake_fetch_api) - monkeypatch.setattr(ingest, "fetch_api_full_url", fake_fetch_api_full_url) + mock_repo.compare.side_effect = fake_compare + monkeypatch.setattr(ingest.github, "get_repository", lambda owner, repo_name: mock_repo) event_base_sha, commits = ingest.query_data(REPO_META, "HEAD") assert event_base_sha == "PARENT" assert commits == [ { - "message": "Fix the thing", - "author": {"name": "Dev", "email": "dev@example.com"}, - "committer": {"date": "2026-02-02T00:00:00Z"}, + "message": "Fix the issue", + "author": {"name": "Dev", "email": "dev@example.com", "date": date2}, + "committer": {"name": "Dev", "email": "dev@example.com", "date": date2}, "id": "C1", } ] diff --git a/treeherder/etl/management/commands/ingest.py b/treeherder/etl/management/commands/ingest.py index e4219f47bb9..f4d188cf986 100644 --- a/treeherder/etl/management/commands/ingest.py +++ b/treeherder/etl/management/commands/ingest.py @@ -23,7 +23,6 @@ from treeherder.etl.taskcluster_pulse.handler import EXCHANGE_EVENT_MAP, handle_message from treeherder.model.models import Repository from treeherder.utils import github -from treeherder.utils.github import fetch_api, fetch_api_full_url logger = logging.getLogger(__name__) logger.setLevel(logging.INFO) @@ -275,54 +274,85 @@ def query_data(repo_meta, commit): # This is used for the `compare` API. The "event.base.sha" is only contained in Pulse events, thus, # we need to determine the correct value event_base_sha = repo_meta["branch"] - # First we try with `master` being the base sha - # e.g. https://api.github.com/repos/servo/servo/compare/master...1418c0555ff77e5a3d6cf0c6020ba92ece36be2e - compare_response = fetch_api( - f"repos/{repo_meta['owner']}/{repo_meta['repo']}/compare/{event_base_sha}...{commit}" + # First we try and get a comparsion with `master`` neing the base sha + comparison = github.get_comparison( + owner=repo_meta["owner"], repo_name=repo_meta["repo"], base=event_base_sha, head=commit ) - merge_base_commit = compare_response.get("merge_base_commit") + merge_base_commit = comparison.merge_base_commit + if merge_base_commit: - commiter_date = merge_base_commit["commit"]["committer"]["date"] + committer_date = merge_base_commit.commit.committer.date + # Since we don't use PushEvents that contain the "before" or "event.base.sha" fields [1] # we need to discover the right parent which existed in the base branch. # [1] https://github.com/taskcluster/taskcluster/blob/3dda0adf85619d18c5dcf255259f3e274d2be346/services/github/src/api.js#L55 - parents = compare_response["merge_base_commit"]["parents"] + parents = merge_base_commit.parents if len(parents) == 1: parent = parents[0] - commit_info = fetch_api_full_url(parent["url"]) - committer_date = commit_info["commit"]["committer"]["date"] - # All commits involved in a PR share the same committer's date - if merge_base_commit["commit"]["committer"]["date"] == committer_date: + parent_commit = github.get_commit( + owner=repo_meta["owner"], repo=repo_meta["repo"], sha=parent.sha + ) + parent_committer_date = parent_commit.commit.committer.date + + # All commits involvled in a PR share the same date + if committer_date == parent_committer_date: # Recursively find the forking parent - event_base_sha, _ = query_data(repo_meta, parent["sha"]) + event_base_sha, _ = query_data(repo_meta, parent.sha) else: - event_base_sha = parent["sha"] + event_base_sha = parent.sha + else: + event_base_sha = None # Initialize in case no suitable parent is found for parent in parents: - _commit = fetch_api_full_url(parent["url"]) + temp_commit = github.get_commit( + owner=repo_meta["owner"], repo=repo_meta["repo"], sha=parent.sha + ) # All commits involved in a merge share the same committer's date - if commiter_date != _commit["commit"]["committer"]["date"]: - event_base_sha = _commit["sha"] + if committer_date != temp_commit.commit.committer.date: + event_base_sha = temp_commit.sha break + if not event_base_sha: + # Fallback if no specific parent is found with different committer date + # This case might need more robust handling based on project-specific merge strategies + event_base_sha = repo_meta[ + "branch" + ] # default to branch if no specific parent found # This is to make sure that the value has changed - assert event_base_sha != repo_meta["branch"] - logger.info("We have a new base: %s", event_base_sha) - # When using the correct event_base_sha the "commits" field will be correct - compare_response = fetch_api( - f"repos/{repo_meta['owner']}/{repo_meta['repo']}/compare/{event_base_sha}...{commit}" - ) + if event_base_sha == repo_meta["branch"]: + logger.warning("Event base SHA did not change from repo branch: %s", event_base_sha) + else: + logger.info("We have a new base: %s", event_base_sha) + # When using the correct event_base_sha the "commits" field will be correct + compare_result = github.get_comparison( + repo_meta["owner"], repo_meta["repo"], event_base_sha, commit + ) + else: + # If no merge_base_commit, then the initial compare_result should be used + # and event_base_sha remains the initial branch. + event_base_sha = repo_meta["branch"] commits = [] - for _commit in compare_response["commits"]: + for temp_commit in compare_result.commits: commits.append( { - "message": _commit["commit"]["message"], - "author": _commit["commit"]["author"], - "committer": _commit["commit"]["committer"], - "id": _commit["sha"], + "message": temp_commit.commit.message, + "author": { + "name": temp_commit.commit.author.name, + "email": temp_commit.commit.author.email, + "date": temp_commit.commit.author.date, + } + if temp_commit.commit.author + else None, + "committer": { + "name": temp_commit.commit.committer.name, + "email": temp_commit.commit.committer.email, + "date": temp_commit.commit.committer.date, + } + if temp_commit.commit.committer + else None, + "id": temp_commit.sha, } ) - return event_base_sha, commits diff --git a/treeherder/utils/github.py b/treeherder/utils/github.py index 2cfe6b754f7..683d7cc2db3 100644 --- a/treeherder/utils/github.py +++ b/treeherder/utils/github.py @@ -1,5 +1,6 @@ from github import Auth, Github from github.Commit import Commit +from github.Comparison import Comparison from github.GitRelease import GitRelease from github.PullRequest import PullRequest from github.Repository import Repository @@ -52,6 +53,13 @@ def compare_shas(owner: str, repo_name: str, base: str, head: str) -> list[Commi ] +def get_comparison(owner: str, repo_name: str, base: str, head: str) -> Comparison: + """ + Returns a PyGithub Comparison for a comparison of the base branch against head revision. + """ + return get_repository(owner=owner, repo_name=repo_name).compare(base=base, head=head) + + def get_all_commits(owner: str, repo: str, params: None) -> list[Commit]: """ Returns a list of PyGithub Commit objects. From 9923475384aa5521820039acd2710be2d0a263d8 Mon Sep 17 00:00:00 2001 From: Alex Moses Date: Tue, 7 Jul 2026 11:53:10 +0000 Subject: [PATCH 5/5] Bug-2038705: Add tests for github.py utility - Added tests for treeherder/utils/github.py - Removed fetch_api and fetch_api_full_url --- tests/utils/test_github.py | 137 +++++++++++++++++++++++++++++++++++++ treeherder/utils/github.py | 22 +----- 2 files changed, 138 insertions(+), 21 deletions(-) create mode 100644 tests/utils/test_github.py diff --git a/tests/utils/test_github.py b/tests/utils/test_github.py new file mode 100644 index 00000000000..19c15ecb17e --- /dev/null +++ b/tests/utils/test_github.py @@ -0,0 +1,137 @@ +import unittest +from unittest.mock import Mock, patch + +from treeherder.utils import github + + +class TestGithubUtils(unittest.TestCase): + @patch("treeherder.utils.github.github_client") + def test_get_repository(self, mock_github_instance): + mock_repo = Mock() + mock_github_instance.get_repo.return_value = mock_repo + owner = "test_owner" + repo_name = "test_repo" + result = github.get_repository(owner, repo_name) + mock_github_instance.get_repo.assert_called_once_with( + full_name_or_id=f"{owner}/{repo_name}" + ) + self.assertEqual(result, mock_repo) + + @patch("treeherder.utils.github.get_repository") + def test_compare_shas(self, mock_get_repository): + mock_commit1 = Mock() + mock_commit2 = Mock() + mock_comparison = Mock() + mock_comparison.commits = [mock_commit1, mock_commit2] + mock_repo = Mock() + mock_repo.compare.return_value = mock_comparison + mock_get_repository.return_value = mock_repo + + owner = "test_owner" + repo_name = "test_repo" + base = "base_sha" + head = "head_sha" + result = github.compare_shas(owner, repo_name, base, head) + + mock_get_repository.assert_called_once_with(owner=owner, repo_name=repo_name) + mock_repo.compare.assert_called_once_with(base=base, head=head) + self.assertEqual(result, [mock_commit1, mock_commit2]) + + @patch("treeherder.utils.github.get_repository") + def test_get_comparison(self, mock_get_repository): + mock_comparison = Mock() + mock_repo = Mock() + mock_repo.compare.return_value = mock_comparison + mock_get_repository.return_value = mock_repo + + owner = "test_owner" + repo_name = "test_repo" + base = "base_sha" + head = "head_sha" + result = github.get_comparison(owner, repo_name, base, head) + + mock_get_repository.assert_called_once_with(owner=owner, repo_name=repo_name) + mock_repo.compare.assert_called_once_with(base=base, head=head) + self.assertEqual(result, mock_comparison) + + @patch("treeherder.utils.github.get_repository") + def test_get_all_commits(self, mock_get_repository): + mock_commit1 = Mock() + mock_commit2 = Mock() + mock_repo = Mock() + mock_repo.get_commits.return_value = [mock_commit1, mock_commit2] + mock_get_repository.return_value = mock_repo + + owner = "test_owner" + repo_name = "test_repo" + params = {"sha": "some_sha", "author": "test_author"} + result = github.get_all_commits(owner, repo_name, params) + + mock_get_repository.assert_called_once_with(owner=owner, repo_name=repo_name) + mock_repo.get_commits.assert_called_once_with(**params) + self.assertEqual(result, [mock_commit1, mock_commit2]) + + @patch("treeherder.utils.github.get_repository") + def test_get_commit(self, mock_get_repository): + mock_commit = Mock() + mock_repo = Mock() + mock_repo.get_commit.return_value = mock_commit + mock_get_repository.return_value = mock_repo + + owner = "test_owner" + repo_name = "test_repo" + sha = "test_sha" + result = github.get_commit(owner, repo_name, sha) + + mock_get_repository.assert_called_once_with(owner=owner, repo_name=repo_name) + mock_repo.get_commit.assert_called_once_with(sha=sha) + self.assertEqual(result, mock_commit) + + @patch("treeherder.utils.github.get_repository") + def test_get_pull_request(self, mock_get_repository): + mock_pr = Mock() + mock_repo = Mock() + mock_repo.get_pull.return_value = mock_pr + mock_get_repository.return_value = mock_repo + + owner = "test_owner" + repo_name = "test_repo" + pr_number = 123 + result = github.get_pull_request(owner, repo_name, pr_number) + + mock_get_repository.assert_called_once_with(owner, repo_name) + mock_repo.get_pull.assert_called_once_with(pr_number) + self.assertEqual(result, mock_pr) + + @patch("treeherder.utils.github.get_pull_request") + def test_get_pull_request_commits(self, mock_get_pull_request): + mock_commit1 = Mock() + mock_commit2 = Mock() + mock_pr = Mock() + mock_pr.get_commits.return_value = [mock_commit1, mock_commit2] + mock_get_pull_request.return_value = mock_pr + + owner = "test_owner" + repo_name = "test_repo" + pr_number = 123 + result = github.get_pull_request_commits(owner, repo_name, pr_number) + + mock_get_pull_request.assert_called_once_with(owner, repo_name, pr_number) + mock_pr.get_commits.assert_called_once_with() + self.assertEqual(result, [mock_commit1, mock_commit2]) + + @patch("treeherder.utils.github.get_repository") + def test_get_releases(self, mock_get_repository): + mock_release1 = Mock() + mock_release2 = Mock() + mock_repo = Mock() + mock_repo.get_releases.return_value = [mock_release1, mock_release2] + mock_get_repository.return_value = mock_repo + + owner = "test_owner" + repo_name = "test_repo" + result = github.get_releases(owner, repo_name) + + mock_get_repository.assert_called_once_with(owner=owner, repo_name=repo_name) + mock_repo.get_releases.assert_called_once_with() + self.assertEqual(result, [mock_release1, mock_release2]) diff --git a/treeherder/utils/github.py b/treeherder/utils/github.py index 683d7cc2db3..354d055d052 100644 --- a/treeherder/utils/github.py +++ b/treeherder/utils/github.py @@ -6,26 +6,6 @@ from github.Repository import Repository from treeherder.config.settings import GITHUB_TOKEN -from treeherder.utils.http import fetch_json - - -def fetch_api(path, params=None): - """ - Deprecated. Use the pygithub methods instead. - """ - return fetch_api_full_url(f"https://api.github.com/{path}", params) - - -def fetch_api_full_url(url, params=None): - """ - Deprecated. Use the pygithub methods instead. - """ - if GITHUB_TOKEN: - headers = {"Authorization": f"token {GITHUB_TOKEN}"} - else: - headers = {} - return fetch_json(url, params, headers) - if GITHUB_TOKEN: auth = Auth.Token(GITHUB_TOKEN) @@ -82,7 +62,7 @@ def get_pull_request(owner: str, repo_name: str, pr_number: int) -> PullRequest: return get_repository(owner, repo_name).get_pull(pr_number) -def get_pull_request_commits(owner: str, repo_name: str, pr_number: str) -> list[Commit]: +def get_pull_request_commits(owner: str, repo_name: str, pr_number: int) -> list[Commit]: """ Returns a list of PyGithub Commit objects associated with a given PR. """