areusch commented on code in PR #12361:
URL: https://github.com/apache/tvm/pull/12361#discussion_r961098378
##########
tests/python/ci/test_tvmbot.py:
##########
@@ -37,167 +36,262 @@
""".strip()
-TEST_DATA = {
- "successful-merge": {
- "number": 10786,
- "filename": "pr10786-merges.json",
- "expected": SUCCESS_EXPECTED_OUTPUT,
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Everything is fine so this PR will merge",
- },
- "no-request": {
- "number": 10786,
- "filename": "pr10786-nottriggered.json",
- "expected": "Command 'do something else' did not match anything",
- "comment": "@tvm-bot do something else",
- "user": "abc",
- "detail": "A PR for which the mergebot runs but no merge is requested",
- },
- "bad-ci": {
- "number": 10786,
- "filename": "pr10786-badci.json",
- "expected": "Cannot merge, these CI jobs are not successful on",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "A PR which failed CI and cannot merge",
- },
- "old-review": {
- "number": 10786,
- "filename": "pr10786-oldreview.json",
- "expected": "Cannot merge, did not find any approving reviews",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "A PR with passing CI and approving reviews on an old commit
so it cannot merge",
- },
- "missing-job": {
- "number": 10786,
- "filename": "pr10786-missing-job.json",
- "expected": "Cannot merge, missing expected jobs",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "PR missing an expected CI job and cannot merge",
- },
- "invalid-author": {
- "number": 10786,
- "filename": "pr10786-invalid-author.json",
- "expected": "Failed auth check 'collaborators', quitting",
- "comment": "@tvm-bot merge",
- "user": "not-abc",
- "detail": "Merge requester is not a committer and cannot merge",
- },
- "unauthorized-comment": {
- "number": 11244,
- "filename": "pr11244-unauthorized-comment.json",
- "expected": "Failed auth check 'collaborators'",
- "comment": "@tvm-bot merge",
- "user": "not-abc2",
- "detail": "Check that a merge comment not from a CONTRIBUTOR is
rejected",
- },
- "no-review": {
- "number": 11267,
- "filename": "pr11267-no-review.json",
- "expected": "Cannot merge, did not find any approving reviews from
users with write access",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Check that a merge request without any reviews is rejected",
- },
- "changes-requested": {
- "number": 10786,
- "filename": "pr10786-changes-requested.json",
- "expected": "Cannot merge, found [this review]",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Check that a merge request with a 'Changes Requested'
review is rejected",
- },
- "co-authors": {
- "number": 10786,
- "filename": "pr10786-co-authors.json",
- "expected": "Co-authored-by: Some One <[email protected]>",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Check that a merge request with co-authors generates the
correct commit message",
- },
- "rerun-ci": {
- "number": 11442,
- "filename": "pr11442-rerun-ci.json",
- "expected": "Rerunning ci with",
- "comment": "@tvm-bot rerun",
- "user": "abc",
- "detail": "Start a new CI job",
- },
- "ignore-jobs": {
- "number": 10786,
- "filename": "pr10786-ignore-jobs.json",
- "expected": "Dry run, would have merged",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Ignore GitHub Actions jobs that don't start with CI / ",
- },
-}
+class _TvmBotTest:
+ number = 10786
Review Comment:
consider NUMBER =?
##########
tests/python/ci/test_utils.py:
##########
@@ -30,12 +31,41 @@ class TempGit:
def __init__(self, cwd):
self.cwd = cwd
+ # Jenkins git is too old and doesn't have 'git init --initial-branch'
+ self.run("init", stderr=subprocess.PIPE, stdout=subprocess.PIPE)
+ self.run("checkout", "-b", "main", stderr=subprocess.PIPE)
+ self.run("remote", "add", "origin",
"https://github.com/apache/tvm.git";)
def run(self, *args, **kwargs):
+ """
+ Run a git command based on *args
+ """
proc = subprocess.run(
["git"] + list(args), encoding="utf-8", cwd=self.cwd, check=False,
**kwargs
)
if proc.returncode != 0:
raise RuntimeError(f"git command failed: '{args}'")
return proc
+
+
+def run_script(command: List[Any], check: bool = True, **kwargs):
Review Comment:
i thought subprocess.check_output did this, does that work?
##########
tests/python/ci/test_tvmbot.py:
##########
@@ -37,167 +36,262 @@
""".strip()
-TEST_DATA = {
- "successful-merge": {
- "number": 10786,
- "filename": "pr10786-merges.json",
- "expected": SUCCESS_EXPECTED_OUTPUT,
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Everything is fine so this PR will merge",
- },
- "no-request": {
- "number": 10786,
- "filename": "pr10786-nottriggered.json",
- "expected": "Command 'do something else' did not match anything",
- "comment": "@tvm-bot do something else",
- "user": "abc",
- "detail": "A PR for which the mergebot runs but no merge is requested",
- },
- "bad-ci": {
- "number": 10786,
- "filename": "pr10786-badci.json",
- "expected": "Cannot merge, these CI jobs are not successful on",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "A PR which failed CI and cannot merge",
- },
- "old-review": {
- "number": 10786,
- "filename": "pr10786-oldreview.json",
- "expected": "Cannot merge, did not find any approving reviews",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "A PR with passing CI and approving reviews on an old commit
so it cannot merge",
- },
- "missing-job": {
- "number": 10786,
- "filename": "pr10786-missing-job.json",
- "expected": "Cannot merge, missing expected jobs",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "PR missing an expected CI job and cannot merge",
- },
- "invalid-author": {
- "number": 10786,
- "filename": "pr10786-invalid-author.json",
- "expected": "Failed auth check 'collaborators', quitting",
- "comment": "@tvm-bot merge",
- "user": "not-abc",
- "detail": "Merge requester is not a committer and cannot merge",
- },
- "unauthorized-comment": {
- "number": 11244,
- "filename": "pr11244-unauthorized-comment.json",
- "expected": "Failed auth check 'collaborators'",
- "comment": "@tvm-bot merge",
- "user": "not-abc2",
- "detail": "Check that a merge comment not from a CONTRIBUTOR is
rejected",
- },
- "no-review": {
- "number": 11267,
- "filename": "pr11267-no-review.json",
- "expected": "Cannot merge, did not find any approving reviews from
users with write access",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Check that a merge request without any reviews is rejected",
- },
- "changes-requested": {
- "number": 10786,
- "filename": "pr10786-changes-requested.json",
- "expected": "Cannot merge, found [this review]",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Check that a merge request with a 'Changes Requested'
review is rejected",
- },
- "co-authors": {
- "number": 10786,
- "filename": "pr10786-co-authors.json",
- "expected": "Co-authored-by: Some One <[email protected]>",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Check that a merge request with co-authors generates the
correct commit message",
- },
- "rerun-ci": {
- "number": 11442,
- "filename": "pr11442-rerun-ci.json",
- "expected": "Rerunning ci with",
- "comment": "@tvm-bot rerun",
- "user": "abc",
- "detail": "Start a new CI job",
- },
- "ignore-jobs": {
- "number": 10786,
- "filename": "pr10786-ignore-jobs.json",
- "expected": "Dry run, would have merged",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Ignore GitHub Actions jobs that don't start with CI / ",
- },
-}
+class _TvmBotTest:
Review Comment:
confirming, is this an abstract base class?
##########
tests/python/ci/test_tvmbot.py:
##########
@@ -37,167 +36,262 @@
""".strip()
-TEST_DATA = {
- "successful-merge": {
- "number": 10786,
- "filename": "pr10786-merges.json",
- "expected": SUCCESS_EXPECTED_OUTPUT,
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Everything is fine so this PR will merge",
- },
- "no-request": {
- "number": 10786,
- "filename": "pr10786-nottriggered.json",
- "expected": "Command 'do something else' did not match anything",
- "comment": "@tvm-bot do something else",
- "user": "abc",
- "detail": "A PR for which the mergebot runs but no merge is requested",
- },
- "bad-ci": {
- "number": 10786,
- "filename": "pr10786-badci.json",
- "expected": "Cannot merge, these CI jobs are not successful on",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "A PR which failed CI and cannot merge",
- },
- "old-review": {
- "number": 10786,
- "filename": "pr10786-oldreview.json",
- "expected": "Cannot merge, did not find any approving reviews",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "A PR with passing CI and approving reviews on an old commit
so it cannot merge",
- },
- "missing-job": {
- "number": 10786,
- "filename": "pr10786-missing-job.json",
- "expected": "Cannot merge, missing expected jobs",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "PR missing an expected CI job and cannot merge",
- },
- "invalid-author": {
- "number": 10786,
- "filename": "pr10786-invalid-author.json",
- "expected": "Failed auth check 'collaborators', quitting",
- "comment": "@tvm-bot merge",
- "user": "not-abc",
- "detail": "Merge requester is not a committer and cannot merge",
- },
- "unauthorized-comment": {
- "number": 11244,
- "filename": "pr11244-unauthorized-comment.json",
- "expected": "Failed auth check 'collaborators'",
- "comment": "@tvm-bot merge",
- "user": "not-abc2",
- "detail": "Check that a merge comment not from a CONTRIBUTOR is
rejected",
- },
- "no-review": {
- "number": 11267,
- "filename": "pr11267-no-review.json",
- "expected": "Cannot merge, did not find any approving reviews from
users with write access",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Check that a merge request without any reviews is rejected",
- },
- "changes-requested": {
- "number": 10786,
- "filename": "pr10786-changes-requested.json",
- "expected": "Cannot merge, found [this review]",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Check that a merge request with a 'Changes Requested'
review is rejected",
- },
- "co-authors": {
- "number": 10786,
- "filename": "pr10786-co-authors.json",
- "expected": "Co-authored-by: Some One <[email protected]>",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Check that a merge request with co-authors generates the
correct commit message",
- },
- "rerun-ci": {
- "number": 11442,
- "filename": "pr11442-rerun-ci.json",
- "expected": "Rerunning ci with",
- "comment": "@tvm-bot rerun",
- "user": "abc",
- "detail": "Start a new CI job",
- },
- "ignore-jobs": {
- "number": 10786,
- "filename": "pr10786-ignore-jobs.json",
- "expected": "Dry run, would have merged",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Ignore GitHub Actions jobs that don't start with CI / ",
- },
-}
+class _TvmBotTest:
+ number = 10786
+
+ def load_data(self, data: Dict[str, Any]):
Review Comment:
should this and `number` be required to be declared in the test? consider
`abc.abstractmethod` here. also, what if we call this preprocess_data?
##########
tests/python/ci/test_tvmbot.py:
##########
@@ -37,167 +36,262 @@
""".strip()
-TEST_DATA = {
- "successful-merge": {
- "number": 10786,
- "filename": "pr10786-merges.json",
- "expected": SUCCESS_EXPECTED_OUTPUT,
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Everything is fine so this PR will merge",
- },
- "no-request": {
- "number": 10786,
- "filename": "pr10786-nottriggered.json",
- "expected": "Command 'do something else' did not match anything",
- "comment": "@tvm-bot do something else",
- "user": "abc",
- "detail": "A PR for which the mergebot runs but no merge is requested",
- },
- "bad-ci": {
- "number": 10786,
- "filename": "pr10786-badci.json",
- "expected": "Cannot merge, these CI jobs are not successful on",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "A PR which failed CI and cannot merge",
- },
- "old-review": {
- "number": 10786,
- "filename": "pr10786-oldreview.json",
- "expected": "Cannot merge, did not find any approving reviews",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "A PR with passing CI and approving reviews on an old commit
so it cannot merge",
- },
- "missing-job": {
- "number": 10786,
- "filename": "pr10786-missing-job.json",
- "expected": "Cannot merge, missing expected jobs",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "PR missing an expected CI job and cannot merge",
- },
- "invalid-author": {
- "number": 10786,
- "filename": "pr10786-invalid-author.json",
- "expected": "Failed auth check 'collaborators', quitting",
- "comment": "@tvm-bot merge",
- "user": "not-abc",
- "detail": "Merge requester is not a committer and cannot merge",
- },
- "unauthorized-comment": {
- "number": 11244,
- "filename": "pr11244-unauthorized-comment.json",
- "expected": "Failed auth check 'collaborators'",
- "comment": "@tvm-bot merge",
- "user": "not-abc2",
- "detail": "Check that a merge comment not from a CONTRIBUTOR is
rejected",
- },
- "no-review": {
- "number": 11267,
- "filename": "pr11267-no-review.json",
- "expected": "Cannot merge, did not find any approving reviews from
users with write access",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Check that a merge request without any reviews is rejected",
- },
- "changes-requested": {
- "number": 10786,
- "filename": "pr10786-changes-requested.json",
- "expected": "Cannot merge, found [this review]",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Check that a merge request with a 'Changes Requested'
review is rejected",
- },
- "co-authors": {
- "number": 10786,
- "filename": "pr10786-co-authors.json",
- "expected": "Co-authored-by: Some One <[email protected]>",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Check that a merge request with co-authors generates the
correct commit message",
- },
- "rerun-ci": {
- "number": 11442,
- "filename": "pr11442-rerun-ci.json",
- "expected": "Rerunning ci with",
- "comment": "@tvm-bot rerun",
- "user": "abc",
- "detail": "Start a new CI job",
- },
- "ignore-jobs": {
- "number": 10786,
- "filename": "pr10786-ignore-jobs.json",
- "expected": "Dry run, would have merged",
- "comment": "@tvm-bot merge",
- "user": "abc",
- "detail": "Ignore GitHub Actions jobs that don't start with CI / ",
- },
-}
+class _TvmBotTest:
+ number = 10786
+
+ def load_data(self, data: Dict[str, Any]):
+ """
+ Used to pre-process PR data before running the test. Override as
+ necessary to edit data for specific test cases.
+ """
+ return data
+
+ @tvm.testing.skip_if_wheel_test
+ def test(self, tmpdir_factory):
+ """
+ Run the tvm-bot script using the data from load_data
+ """
+ mergebot_script = REPO_ROOT / "ci" / "scripts" / "github_tvmbot.py"
+ test_json_dir = Path(__file__).resolve().parent / "sample_prs"
+ with open(test_json_dir / f"pr{self.number}.json") as f:
+ test_data = json.load(f)
+
+ # Update testing data with replacements / additions
+ test_data = self.load_data(test_data)
+
+ git = TempGit(tmpdir_factory.mktemp("tmp_git_dir"))
+
+ comment = {
+ "body": self.comment,
+ "id": 123,
+ "user": {
+ "login": self.user,
+ },
+ }
+ allowed_users = [{"login": "abc"}, {"login": "other-abc"}]
+
+ proc = run_script(
+ [
+ mergebot_script,
+ "--pr",
+ self.number,
+ "--dry-run",
+ "--run-url",
+ "https://example.com";,
+ "--testing-pr-json",
+ json.dumps(test_data),
+ "--testing-collaborators-json",
+ json.dumps(allowed_users),
+ "--testing-mentionable-users-json",
+ json.dumps(allowed_users),
+ "--trigger-comment-json",
+ json.dumps(comment),
+ ],
+ env={
+ "TVM_BOT_JENKINS_TOKEN": "123",
+ "GH_ACTIONS_TOKEN": "123",
+ },
+ cwd=git.cwd,
+ )
+
+ if self.expected not in proc.stderr:
+ raise RuntimeError(f"{proc.stderr}\ndid not
contain\n{self.expected}")
+
+
+class TestNoRequest(_TvmBotTest):
+ """
+ A PR for which the mergebot runs but no merge is requested
+ """
+
+ comment = "@tvm-bot do something else"
+ user = "abc"
+ expected = "Command 'do something else' did not match anything"
+
+ def load_data(self, data: Dict[str, Any]):
+ data["reviews"]["nodes"][0]["body"] = "nothing"
+ return data
+
+
+class TestSuccessfulMerge(_TvmBotTest):
+ """
+ Everything is fine so this PR will merge
+ """
+
+ comment = "@tvm-bot merge"
Review Comment:
also wondering if these should be CONSTANT-style names?
##########
tests/python/ci/test_utils.py:
##########
@@ -30,12 +31,41 @@ class TempGit:
def __init__(self, cwd):
self.cwd = cwd
+ # Jenkins git is too old and doesn't have 'git init --initial-branch'
Review Comment:
maybe update the class docstring?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
