From f5c2a1922d596bcf37026d59b92794a82f23f658 Mon Sep 17 00:00:00 2001 From: driazati Date: Tue, 12 Apr 2022 14:58:55 -0700 Subject: [PATCH] Address comments --- .../ci/sample_prs/pr10786-missing-job.json | 123 ++++++++++++++++++ .../ci/sample_prs/pr10786-oldreview.json | 2 +- tests/python/ci/test_mergebot.py | 110 +++++++++------- tests/scripts/github_mergebot.py | 23 +++- 4 files changed, 206 insertions(+), 52 deletions(-) create mode 100644 tests/python/ci/sample_prs/pr10786-missing-job.json diff --git a/tests/python/ci/sample_prs/pr10786-missing-job.json b/tests/python/ci/sample_prs/pr10786-missing-job.json new file mode 100644 index 0000000000000..f702dc9e7a371 --- /dev/null +++ b/tests/python/ci/sample_prs/pr10786-missing-job.json @@ -0,0 +1,123 @@ +{ + "title": "[Hexagon] 2-d allocation cleanup", + "body": "- Added device validity check in allocation. HexagonDeviceAPI should only be called for CPU/Hexagon types.\r\n\r\n- Check for \"global.vtcm\" scope instead of \"vtcm\". The ccope of N-d allocations produced by `LowerVtcmAlloc` should be `\"global.vtcm\"`. The previous check allowed unsupported scope such as `\"local.vtcm\"`.\r\n\r\n- Remove `vtcmallocs` entry after calling free. Previously, the vtcm allocation map kept dangling pointers to `HexagonBuffer` objects after they had been freed.\r\n\r\n- Rename N-d alloc and free packed functions. Since most of the similar device functions use snake case, renaming `*.AllocND` to `*.alloc_nd` and `*.FreeND` to `*.free_nd`.\r\n\r\nCo-authored-by: Adam Straw ", + "state": "OPEN", + "comments": { + "pageInfo": { + "hasPreviousPage": false + }, + "nodes": [] + }, + "authorCommits": { + "nodes": [ + { + "commit": { + "authors": { + "nodes": [ + { + "name": "Eric Lunderberg", + "email": "elunderberg@octoml.ai" + }, + { + "name": "Adam Straw", + "email": "astraw@octoml.ai" + } + ] + } + } + } + ] + }, + "commits": { + "nodes": [ + { + "commit": { + "oid": "6f04bcf57d07f915a98fd91178f04d9e92a09fcd", + "statusCheckRollup": { + "contexts": { + "pageInfo": { + "hasNextPage": false + }, + "nodes": [ + { + "name": "MacOS", + "checkSuite": { + "workflowRun": { + "workflow": { + "name": "CI" + } + } + }, + "status": "COMPLETED", + "conclusion": "SUCCESS", + "url": "https://github.com/apache/tvm/runs/5694945392" + }, + { + "name": "cc-reviewers", + "checkSuite": { + "workflowRun": { + "workflow": { + "name": "PR" + } + } + }, + "status": "COMPLETED", + "conclusion": "SUCCESS", + "url": "https://github.com/apache/tvm/runs/5694945029" + }, + { + "name": "tag-teams", + "checkSuite": { + "workflowRun": { + "workflow": { + "name": "Teams" + } + } + }, + "status": "COMPLETED", + "conclusion": "SUCCESS", + "url": "https://github.com/apache/tvm/runs/5694945030" + }, + { + "name": "Windows", + "checkSuite": { + "workflowRun": { + "workflow": { + "name": "CI" + } + } + }, + "status": "COMPLETED", + "conclusion": "SUCCESS", + "url": "https://github.com/apache/tvm/runs/5694945524" + }, + { + "state": "SUCCESS", + "context": "tvm-ci/definitely-not-pr-merge", + "targetUrl": "https://ci.tlcpack.ai/job/tvm/job/PR-10786/1/display/redirect" + } + ] + } + } + } + } + ] + }, + "reviewDecision": "APPROVED", + "reviews": { + "pageInfo": { + "hasPreviousPage": false + }, + "nodes": [ + { + "body": "@tvm-bot merge", + "updatedAt": "2022-03-25T22:13:50Z", + "authorCanPushToRepository": true, + "commit": { + "oid": "6f04bcf57d07f915a98fd91178f04d9e92a09fcd" + }, + "state": "APPROVED" + } + ] + } +} \ No newline at end of file diff --git a/tests/python/ci/sample_prs/pr10786-oldreview.json b/tests/python/ci/sample_prs/pr10786-oldreview.json index a5e226f6403e4..52abf7a30d7a8 100644 --- a/tests/python/ci/sample_prs/pr10786-oldreview.json +++ b/tests/python/ci/sample_prs/pr10786-oldreview.json @@ -114,7 +114,7 @@ "updatedAt": "2022-03-25T22:13:50Z", "authorCanPushToRepository": true, "commit": { - "oid": "6f24bcf57d07f915a98fd91178f04d9e92a09fcd" + "oid": "abc12345" }, "state": "APPROVED" } diff --git a/tests/python/ci/test_mergebot.py b/tests/python/ci/test_mergebot.py index c62bde7c0c003..0dafb2a31f44c 100644 --- a/tests/python/ci/test_mergebot.py +++ b/tests/python/ci/test_mergebot.py @@ -35,60 +35,72 @@ def run(self, *args): raise RuntimeError(f"git command failed: '{args}'") -def test_mergebot(tmpdir_factory): - mergebot_script = REPO_ROOT / "tests" / "scripts" / "github_mergebot.py" - test_json_dir = Path(__file__).resolve().parent / "sample_prs" +test_data = { + "successful-merge": { + "number": 10786, + "filename": "pr10786-merges.json", + "expected": "Dry run, would have merged with url=pulls/10786/merge", + }, + "no-request": { + "number": 10786, + "filename": "pr10786-nottriggered.json", + "expected": "No merge requested, exiting", + }, + "bad-ci": { + "number": 10786, + "filename": "pr10786-badci.json", + "expected": "Cannot merge, these CI jobs are not successful on", + }, + "old-review": { + "number": 10786, + "filename": "pr10786-oldreview.json", + "expected": "Cannot merge, did not find any approving reviews", + }, + "missing-job": { + "number": 10786, + "filename": "pr10786-missing-job.json", + "expected": "Cannot merge, missing expected jobs", + }, +} - def run(number, filename, expected): - git = TempGit(tmpdir_factory.mktemp("tmp_git_dir")) - git.run("init") - git.run("checkout", "-b", "main") - git.run("remote", "add", "origin", "https://github.com/apache/tvm.git") - with open(test_json_dir / filename) as f: - test_data = json.load(f) - proc = subprocess.run( - [ - str(mergebot_script), - "--pr", - str(number), - "--dry-run", - "--run-url", - "https://example.com", - "--testing-pr-json", - json.dumps(test_data), - ], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - encoding="utf-8", - cwd=git.cwd, - ) - if proc.returncode != 0: - raise RuntimeError(f"Process failed:\nstdout:\n{proc.stdout}\n\nstderr:\n{proc.stderr}") +@pytest.mark.parametrize( + ["number", "filename", "expected"], + [tuple(d.values()) for d in test_data.values()], + ids=test_data.keys(), +) +def test_mergebot(tmpdir_factory, number, filename, expected): + mergebot_script = REPO_ROOT / "tests" / "scripts" / "github_mergebot.py" + test_json_dir = Path(__file__).resolve().parent / "sample_prs" - if expected not in proc.stderr: - raise RuntimeError(f"{proc.stderr}\ndid not contain\n{expected}") + git = TempGit(tmpdir_factory.mktemp("tmp_git_dir")) + git.run("init") + git.run("checkout", "-b", "main") + git.run("remote", "add", "origin", "https://github.com/apache/tvm.git") + with open(test_json_dir / filename) as f: + test_data = json.load(f) - run( - number=10786, - filename="pr10786-merges.json", - expected="Dry run, would have merged with url=pulls/10786/merge", - ) - run( - number=10786, - filename="pr10786-nottriggered.json", - expected="No merge requested, exiting", - ) - run( - number=10786, - filename="pr10786-badci.json", - expected="Cannot merge, these CI jobs failed on", - ) - run( - number=10786, - filename="pr10786-oldreview.json", - expected="Cannot merge, did not find any approving reviews", + proc = subprocess.run( + [ + str(mergebot_script), + "--pr", + str(number), + "--dry-run", + "--run-url", + "https://example.com", + "--testing-pr-json", + json.dumps(test_data), + ], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + encoding="utf-8", + cwd=git.cwd, ) + if proc.returncode != 0: + raise RuntimeError(f"Process failed:\nstdout:\n{proc.stdout}\n\nstderr:\n{proc.stderr}") + + if expected not in proc.stderr: + raise RuntimeError(f"{proc.stderr}\ndid not contain\n{expected}") if __name__ == "__main__": diff --git a/tests/scripts/github_mergebot.py b/tests/scripts/github_mergebot.py index a4ce7876195d5..8b2e783343256 100755 --- a/tests/scripts/github_mergebot.py +++ b/tests/scripts/github_mergebot.py @@ -150,12 +150,12 @@ def __init__( self.repo_name = repo self.dry_run = dry_run - self.github = GitHubRepo(user=owner, repo=repo, token=os.environ["GITHUB_TOKEN"]) - if dry_run and raw_data: # In test mode there is no need to fetch anything self.raw = raw_data + self.github = None else: + self.github = GitHubRepo(user=owner, repo=repo, token=os.environ["GITHUB_TOKEN"]) if os.getenv("DEBUG", "0") == "1": # For local runs fill in the requested data but cache it for # later use @@ -421,6 +421,25 @@ def merge_if_passed_checks(self): else: all_ci_passed = True + # Map of job name: has seen in completed jobs + seen_expected_jobs = { + "tvm-ci/pr-merge": False, + } + logging.info(f"Expected to see jobs: {seen_expected_jobs}") + + missing_expected_jobs = [] + for job in self.ci_jobs(): + seen_expected_jobs[job["name"]] = True + + for name, seen in seen_expected_jobs.items(): + if not seen: + missing_expected_jobs.append(name) + + if len(missing_expected_jobs) > 0: + missing_jobs_msg = "\n".join([f" * `{name}`" for name in missing_expected_jobs]) + self.comment(f"Cannot merge, missing expected jobs:\n{missing_jobs_msg}") + return + head_commit_reviews = self.head_commit_reviews() for review in head_commit_reviews: if review["state"] == "CHANGES_REQUESTED":