From 4b8b84362b534394ee1f9fb48a53511e5b241e80 Mon Sep 17 00:00:00 2001 From: driazati Date: Fri, 10 Jun 2022 10:35:14 -0700 Subject: [PATCH] [ci] Enable pylint for tests/python/ci This fixes up the pylint issues as part of #11414 for the CI tests --- tests/lint/pylint.sh | 1 + tests/python/ci/__init__.py | 17 ++++ tests/python/ci/test_ci.py | 114 +++++++++++++---------- tests/python/ci/test_mergebot.py | 36 ++++--- tests/python/ci/test_script_converter.py | 22 ++++- tests/python/ci/test_utils.py | 24 ++++- 6 files changed, 140 insertions(+), 74 deletions(-) create mode 100644 tests/python/ci/__init__.py diff --git a/tests/lint/pylint.sh b/tests/lint/pylint.sh index b442c33c0ff67..5517dc7076b52 100755 --- a/tests/lint/pylint.sh +++ b/tests/lint/pylint.sh @@ -21,4 +21,5 @@ python3 -m pylint python/tvm --rcfile="$(dirname "$0")"/pylintrc python3 -m pylint vta/python/vta --rcfile="$(dirname "$0")"/pylintrc python3 -m pylint tests/python/unittest/test_tvmscript_type.py --rcfile="$(dirname "$0")"/pylintrc python3 -m pylint tests/python/contrib/test_cmsisnn --rcfile="$(dirname "$0")"/pylintrc +python3 -m pylint tests/python/ci --rcfile="$(dirname "$0")"/pylintrc diff --git a/tests/python/ci/__init__.py b/tests/python/ci/__init__.py new file mode 100644 index 0000000000000..0c5f28c1f2f89 --- /dev/null +++ b/tests/python/ci/__init__.py @@ -0,0 +1,17 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Infrastructure and tests for CI scripts""" diff --git a/tests/python/ci/test_ci.py b/tests/python/ci/test_ci.py index 7ef2f0cd58452..43c9042d10c4f 100644 --- a/tests/python/ci/test_ci.py +++ b/tests/python/ci/test_ci.py @@ -14,32 +14,21 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. - +"""Test various CI scripts and GitHub Actions workflows""" import subprocess -import sys import json -from tempfile import tempdir import textwrap -import pytest -import tvm.testing from pathlib import Path -from test_utils import REPO_ROOT - - -class TempGit: - def __init__(self, cwd): - self.cwd = cwd - - def run(self, *args, **kwargs): - proc = subprocess.run(["git"] + list(args), encoding="utf-8", cwd=self.cwd, **kwargs) - if proc.returncode != 0: - raise RuntimeError(f"git command failed: '{args}'") - - return proc +import pytest +import tvm.testing +from .test_utils import REPO_ROOT, TempGit def test_cc_reviewers(tmpdir_factory): + """ + Test that reviewers are added from 'cc @someone' messages in PRs + """ reviewers_script = REPO_ROOT / "tests" / "scripts" / "github_cc_reviewers.py" def run(pr_body, requested_reviewers, existing_review_users, expected_reviewers): @@ -49,7 +38,7 @@ def run(pr_body, requested_reviewers, existing_review_users, expected_reviewers) git.run("remote", "add", "origin", "https://github.com/apache/tvm.git") reviews = [{"user": {"login": r}} for r in existing_review_users] requested_reviewers = [{"login": r} for r in requested_reviewers] - proc = subprocess.run( + proc = subprocess.run( # pylint: disable=subprocess-run-check [str(reviewers_script), "--dry-run", "--testing-reviews-json", json.dumps(reviews)], stdout=subprocess.PIPE, stderr=subprocess.PIPE, @@ -113,6 +102,9 @@ def run(pr_body, requested_reviewers, existing_review_users, expected_reviewers) def test_update_branch(tmpdir_factory): + """ + Test that the last-successful branch script updates successfully + """ update_script = REPO_ROOT / "tests" / "scripts" / "update_branch.py" def run(statuses, expected_rc, expected_output): @@ -132,7 +124,7 @@ def run(statuses, expected_rc, expected_output): } } } - proc = subprocess.run( + proc = subprocess.run( # pylint: disable=subprocess-run-check [str(update_script), "--dry-run", "--testonly-json", json.dumps(data)], stdout=subprocess.PIPE, stderr=subprocess.PIPE, @@ -214,6 +206,9 @@ def run(statuses, expected_rc, expected_output): def test_skip_ci(tmpdir_factory): + """ + Test that CI is skipped when it should be + """ skip_ci_script = REPO_ROOT / "tests" / "scripts" / "git_skip_ci.py" def test(commands, should_skip, pr_title, why): @@ -228,7 +223,7 @@ def test(commands, should_skip, pr_title, why): for command in commands: git.run(*command) pr_number = "1234" - proc = subprocess.run( + proc = subprocess.run( # pylint: disable=subprocess-run-check [str(skip_ci_script), "--pr", pr_number, "--pr-title", pr_title], cwd=git.cwd ) expected = 0 if should_skip else 1 @@ -267,7 +262,8 @@ def test(commands, should_skip, pr_title, why): ], should_skip=False, pr_title="[no skip ci] test", - why="ci should not be skipped on a branch with [skip ci] in the last commit but not the PR title", + why="ci should not be skipped on a branch with " + "[skip ci] in the last commit but not the PR title", ) test( @@ -307,6 +303,9 @@ def test(commands, should_skip, pr_title, why): def test_skip_globs(tmpdir_factory): + """ + Test that CI is skipped if only certain files are edited + """ script = REPO_ROOT / "tests" / "scripts" / "git_skip_ci_globs.py" def run(files, should_skip): @@ -316,7 +315,7 @@ def run(files, should_skip): git.run("checkout", "-b", "main") git.run("remote", "add", "origin", "https://github.com/apache/tvm.git") - proc = subprocess.run( + proc = subprocess.run( # pylint: disable=subprocess-run-check [ str(script), "--files", @@ -342,9 +341,12 @@ def run(files, should_skip): def test_ping_reviewers(tmpdir_factory): + """ + Test that reviewers are messaged after a time period of inactivity + """ reviewers_script = REPO_ROOT / "tests" / "scripts" / "ping_reviewers.py" - def run(pr, check): + def run(pr, check): # pylint: disable=invalid-name git = TempGit(tmpdir_factory.mktemp("tmp_git_dir")) # Jenkins git is too old and doesn't have 'git init --initial-branch' git.run("init") @@ -361,7 +363,7 @@ def run(pr, check): } } } - proc = subprocess.run( + proc = subprocess.run( # pylint: disable=subprocess-run-check [ str(reviewers_script), "--dry-run", @@ -486,14 +488,20 @@ def all_time_keys(time): def assert_in(needle: str, haystack: str): + """ + Check that 'needle' is in 'haystack' + """ if needle not in haystack: raise AssertionError(f"item not found:\n{needle}\nin:\n{haystack}") def test_github_tag_teams(tmpdir_factory): + """ + Check that individuals are tagged from team headers + """ tag_script = REPO_ROOT / "tests" / "scripts" / "github_tag_teams.py" - def run(type, data, check): + def run(source_type, data, check): git = TempGit(tmpdir_factory.mktemp("tmp_git_dir")) git.run("init") git.run("checkout", "-b", "main") @@ -528,9 +536,9 @@ def run(type, data, check): } } env = { - type: json.dumps(data), + source_type: json.dumps(data), } - proc = subprocess.run( + proc = subprocess.run( # pylint: disable=subprocess-run-check [ str(tag_script), "--dry-run", @@ -549,8 +557,8 @@ def run(type, data, check): assert_in(check, proc.stdout) run( - "ISSUE", - { + source_type="ISSUE", + data={ "title": "A title", "number": 1234, "user": { @@ -563,12 +571,12 @@ def run(type, data, check): """.strip() ), }, - "No one to cc, exiting", + check="No one to cc, exiting", ) run( - "ISSUE", - { + source_type="ISSUE", + data={ "title": "A title", "number": 1234, "user": { @@ -583,11 +591,11 @@ def run(type, data, check): """.strip() ), }, - "No one to cc, exiting", + check="No one to cc, exiting", ) run( - type="ISSUE", + source_type="ISSUE", data={ "title": "A title", "number": 1234, @@ -602,11 +610,12 @@ def run(type, data, check): something""" ), }, - check="would have updated issues/1234 with {'body': '\\nhello\\n\\nsomething\\n\\ncc @person1 @person2 @person4'}", + check="would have updated issues/1234 with {'body': " + "'\\nhello\\n\\nsomething\\n\\ncc @person1 @person2 @person4'}", ) run( - type="ISSUE", + source_type="ISSUE", data={ "title": "A title", "number": 1234, @@ -625,7 +634,7 @@ def run(type, data, check): ) run( - type="ISSUE", + source_type="ISSUE", data={ "title": "[something] A title", "number": 1234, @@ -640,11 +649,12 @@ def run(type, data, check): something""" ), }, - check="would have updated issues/1234 with {'body': '\\nhello\\n\\nsomething\\n\\ncc @person1 @person2 @person4'}", + check="would have updated issues/1234 with {'body': " + "'\\nhello\\n\\nsomething\\n\\ncc @person1 @person2 @person4'}", ) run( - type="ISSUE", + source_type="ISSUE", data={ "title": "[something] A title", "number": 1234, @@ -663,7 +673,7 @@ def run(type, data, check): ) run( - type="PR", + source_type="PR", data={ "title": "[something] A title", "number": 1234, @@ -683,7 +693,7 @@ def run(type, data, check): ) run( - type="PR", + source_type="PR", data={ "title": "[something] A title", "number": 1234, @@ -703,7 +713,7 @@ def run(type, data, check): ) run( - type="ISSUE", + source_type="ISSUE", data={ "title": "[something] A title", "number": 1234, @@ -711,6 +721,7 @@ def run(type, data, check): "login": "person5", }, "labels": [{"name": "something2"}], + # pylint: disable=line-too-long "body": textwrap.dedent( """ `mold` and `lld` can be a much faster alternative to `ld` from gcc. We should modify our CMakeLists.txt to detect and use these when possible. cc @person1 @@ -718,12 +729,15 @@ def run(type, data, check): cc @person4 """ ), + # pylint: enable=line-too-long }, - check="would have updated issues/1234 with {'body': '\\n`mold` and `lld` can be a much faster alternative to `ld` from gcc. We should modify our CMakeLists.txt to detect and use these when possible. cc @person1\\n\\ncc @person2 @person4\\n'}", + check="would have updated issues/1234 with {'body': '\\n`mold` and `lld` can be a much" + " faster alternative to `ld` from gcc. We should modify our CMakeLists.txt to " + "detect and use these when possible. cc @person1\\n\\ncc @person2 @person4\\n'}", ) run( - type="ISSUE", + source_type="ISSUE", data={ "title": "[something3] A title", "number": 1234, @@ -733,11 +747,12 @@ def run(type, data, check): "labels": [{"name": "something2"}], "body": "@person2 @SOME1-ONE-", }, - check="Dry run, would have updated issues/1234 with {'body': '@person2 @SOME1-ONE-\\n\\ncc @person1'}", + check="Dry run, would have updated issues/1234 with" + " {'body': '@person2 @SOME1-ONE-\\n\\ncc @person1'}", ) run( - type="ISSUE", + source_type="ISSUE", data={ "title": "[] A title", "number": 1234, @@ -781,6 +796,9 @@ def run(type, data, check): ], ) def test_should_rebuild_docker(tmpdir_factory, changed_files, name, check, expected_code): + """ + Check that the Docker images are built when necessary + """ tag_script = REPO_ROOT / "tests" / "scripts" / "should_rebuild_docker.py" git = TempGit(tmpdir_factory.mktemp("tmp_git_dir")) @@ -824,7 +842,7 @@ def test_should_rebuild_docker(tmpdir_factory, changed_files, name, check, expec }, } - proc = subprocess.run( + proc = subprocess.run( # pylint: disable=subprocess-run-check [ str(tag_script), "--testing-docker-data", diff --git a/tests/python/ci/test_mergebot.py b/tests/python/ci/test_mergebot.py index a565cc76a5c14..7a3ea83a1f1bc 100644 --- a/tests/python/ci/test_mergebot.py +++ b/tests/python/ci/test_mergebot.py @@ -14,25 +14,17 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +""" +Test the @tvm-bot merge code +""" import subprocess import json -import sys -import pytest - from pathlib import Path -from test_utils import REPO_ROOT - - -class TempGit: - def __init__(self, cwd): - self.cwd = cwd - - def run(self, *args, **kwargs): - proc = subprocess.run(["git"] + list(args), cwd=self.cwd, **kwargs) - if proc.returncode != 0: - raise RuntimeError(f"git command failed: '{args}'") +import pytest +import tvm +from .test_utils import REPO_ROOT, TempGit SUCCESS_EXPECTED_OUTPUT = """ @@ -45,7 +37,7 @@ def run(self, *args, **kwargs): """.strip() -test_data = { +TEST_DATA = { "successful-merge": { "number": 10786, "filename": "pr10786-merges.json", @@ -116,7 +108,7 @@ def run(self, *args, **kwargs): "expected": "Cannot merge, found [this review]", "comment": "@tvm-bot merge", "user": "abc", - "detail": "Check that a merge request with a 'Changes Requested' review on HEAD is rejected", + "detail": "Check that a merge request with a 'Changes Requested' review is rejected", }, "co-authors": { "number": 10786, @@ -139,10 +131,13 @@ def run(self, *args, **kwargs): @pytest.mark.parametrize( ["number", "filename", "expected", "comment", "user", "detail"], - [tuple(d.values()) for d in test_data.values()], - ids=test_data.keys(), + [tuple(d.values()) for d in TEST_DATA.values()], + ids=TEST_DATA.keys(), ) def test_mergebot(tmpdir_factory, number, filename, expected, comment, user, detail): + """ + Test the mergebot test cases + """ mergebot_script = REPO_ROOT / "tests" / "scripts" / "github_tvmbot.py" test_json_dir = Path(__file__).resolve().parent / "sample_prs" @@ -162,7 +157,8 @@ def test_mergebot(tmpdir_factory, number, filename, expected, comment, user, det } collaborators = [] - proc = subprocess.run( + # return code is checked manually, so disable pylint + proc = subprocess.run( # pylint: disable=subprocess-run-check [ str(mergebot_script), "--pr", @@ -193,4 +189,4 @@ def test_mergebot(tmpdir_factory, number, filename, expected, comment, user, det if __name__ == "__main__": - sys.exit(pytest.main([__file__] + sys.argv[1:])) + tvm.testing.main() diff --git a/tests/python/ci/test_script_converter.py b/tests/python/ci/test_script_converter.py index a792c135811e4..7cb4b16d73be3 100644 --- a/tests/python/ci/test_script_converter.py +++ b/tests/python/ci/test_script_converter.py @@ -14,17 +14,19 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +""" +Test the conversion of bash to rst +""" import sys -import pytest - +import tvm from tvm.contrib import utils - -from test_utils import REPO_ROOT +from .test_utils import REPO_ROOT sys.path.insert(0, str(REPO_ROOT / "docs")) -from script_convert import ( +# this has to be after the sys.path patching, so ignore pylint +from script_convert import ( # pylint: disable=wrong-import-position bash_to_python, BASH, BASH_IGNORE, @@ -34,6 +36,7 @@ def test_bash_cmd(): + """Test that a bash command gets turned into a rst code block""" temp = utils.tempdir() src_path = temp / "src.sh" dest_path = temp / "dest.py" @@ -56,6 +59,7 @@ def test_bash_cmd(): def test_bash_ignore_cmd(): + """Test that ignored bash commands are not turned into code blocks""" temp = utils.tempdir() src_path = temp / "src.sh" dest_path = temp / "dest.py" @@ -79,6 +83,7 @@ def test_bash_ignore_cmd(): def test_no_command(): + """Test a file with no code blocks""" temp = utils.tempdir() src_path = temp / "src.sh" dest_path = temp / "dest.py" @@ -98,6 +103,7 @@ def test_no_command(): def test_text_and_bash_command(): + """Test a file with a bash code block""" temp = utils.tempdir() src_path = temp / "src.sh" dest_path = temp / "dest.py" @@ -122,6 +128,7 @@ def test_text_and_bash_command(): def test_last_line_break(): + """Test that line endings are correct""" temp = utils.tempdir() src_path = temp / "src.sh" dest_path = temp / "dest.py" @@ -141,6 +148,7 @@ def test_last_line_break(): def test_multiline_comment(): + """Test that bash comments are inserted correctly""" temp = utils.tempdir() src_path = temp / "src.sh" dest_path = temp / "dest.py" @@ -160,3 +168,7 @@ def test_multiline_comment(): expected_cmd = '"""\n' "comment\n" '"""\n' assert generated_cmd == expected_cmd + + +if __name__ == "__main__": + tvm.testing.main() diff --git a/tests/python/ci/test_utils.py b/tests/python/ci/test_utils.py index 0ad88f19f4cdc..e647b1c8670fe 100644 --- a/tests/python/ci/test_utils.py +++ b/tests/python/ci/test_utils.py @@ -14,7 +14,29 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. - +""" +Constants used in various CI tests +""" +import subprocess import pathlib REPO_ROOT = pathlib.Path(__file__).resolve().parent.parent.parent.parent + + +class TempGit: + """ + A wrapper to run commands in a directory + """ + + def __init__(self, cwd): + self.cwd = cwd + + def run(self, *args, **kwargs): + # the returncode is checked manually, ignore pylint + proc = subprocess.run( # pylint: disable=subprocess-run-check + ["git"] + list(args), encoding="utf-8", cwd=self.cwd, **kwargs + ) + if proc.returncode != 0: + raise RuntimeError(f"git command failed: '{args}'") + + return proc