From 7ea52c61d2c4cc62644b782e1d442911263cd1e8 Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Thu, 12 Dec 2024 11:15:57 +0100 Subject: [PATCH 1/3] refactor(IDX): update comment logic (#78) * wip * wip * remove * add test --- .../check_cla/check_cla_issue.py | 2 +- reusable_workflows/check_cla/check_cla_pr.py | 23 +++++++-------- reusable_workflows/tests/test_cla_issue.py | 4 +-- reusable_workflows/tests/test_cla_pr.py | 29 ++++++++++++------- 4 files changed, 32 insertions(+), 26 deletions(-) diff --git a/reusable_workflows/check_cla/check_cla_issue.py b/reusable_workflows/check_cla/check_cla_issue.py index 3a9abbf..1994659 100644 --- a/reusable_workflows/check_cla/check_cla_issue.py +++ b/reusable_workflows/check_cla/check_cla_issue.py @@ -20,7 +20,7 @@ def main() -> None: cla_signed = cla.check_if_cla_signed(issue, user) if not cla_signed: - cla.comment_on_issue(issue) + cla.leave_failed_comment_on_issue(issue) else: cla.handle_cla_signed(issue, user) diff --git a/reusable_workflows/check_cla/check_cla_pr.py b/reusable_workflows/check_cla/check_cla_pr.py index f6f9533..dde810e 100644 --- a/reusable_workflows/check_cla/check_cla_pr.py +++ b/reusable_workflows/check_cla/check_cla_pr.py @@ -12,33 +12,30 @@ APPROVED_LABEL = "cla:agreed" GH_WORKFLOW_LABEL = "cla:gh-wf-pending" -# keep all old bot names for backwards compatibility -CLA_BOT_NAMES = ["cla-idx-bot[bot]", "sa-github-api", "dfinity-droid-prod[bot]"] - class CLAHandler: def __init__(self, gh: github3.login) -> None: self.cla_repo = gh.repository(owner="dfinity", repository="cla") self.cla_link = f"{self.cla_repo.html_url}/blob/main/CLA.md" - def check_comment_already_exists( - self, comments: github3.structs.GitHubIterator + @staticmethod + def check_if_comment_already_exists( + search_comment: str, comments: github3.structs.GitHubIterator ) -> bool: for comment in comments: - if comment.user.login in CLA_BOT_NAMES: + if search_comment == comment.body: return True return False - def comment_on_issue(self, issue: GHIssue): + def leave_failed_comment_on_issue(self, issue: GHIssue) -> None: # check if bot has already left a message to avoid spam issue_comments = issue.comments() - bot_comment = self.check_comment_already_exists(issue_comments) - if not bot_comment: + if not self.check_if_comment_already_exists(messages.FAILED_COMMENT, issue_comments): issue.create_comment(messages.FAILED_COMMENT) - def comment_on_pr(self, pr: GHPullRequest, pr_comment): - bot_comment = self.check_comment_already_exists(pr.issue_comments()) - if not bot_comment: + def comment_on_pr(self, pr: GHPullRequest, pr_comment: str) -> None: + pr_comments = pr.comments() + if not self.check_if_comment_already_exists(pr_comment, pr_comments): pr.create_comment(pr_comment) def check_if_cla_signed(self, issue: GHIssue, user: str) -> bool: @@ -56,7 +53,7 @@ def check_if_cla_signed(self, issue: GHIssue, user: str) -> bool: def get_cla_issue(self, user: str) -> Optional[GHIssue]: for issue in self.cla_repo.issues(): - if issue.title == f"cla: @{user}" and issue.user.login in CLA_BOT_NAMES: + if issue.title == f"cla: @{user}": return issue print(f"No CLA issue for {user}") return None # to make linter happy diff --git a/reusable_workflows/tests/test_cla_issue.py b/reusable_workflows/tests/test_cla_issue.py index 5ce390b..2ac4150 100644 --- a/reusable_workflows/tests/test_cla_issue.py +++ b/reusable_workflows/tests/test_cla_issue.py @@ -33,7 +33,7 @@ def test_end_to_end_cla_signed(cla_mock, gh_login_mock): main() cla.check_if_cla_signed.assert_called_with(issue, "username") - cla.comment_on_issue.assert_not_called() + cla.leave_failed_comment_on_issue.assert_not_called() cla.handle_cla_signed.assert_called_once() @@ -56,7 +56,7 @@ def test_end_to_end_cla_not_signed(cla_mock, gh_login_mock): main() cla.check_if_cla_signed.assert_called_with(issue, "username") - cla.comment_on_issue.assert_called_once() + cla.leave_failed_comment_on_issue.assert_called_once() cla.handle_cla_signed.assert_not_called() diff --git a/reusable_workflows/tests/test_cla_pr.py b/reusable_workflows/tests/test_cla_pr.py index 65bff83..1ab3621 100644 --- a/reusable_workflows/tests/test_cla_pr.py +++ b/reusable_workflows/tests/test_cla_pr.py @@ -6,6 +6,7 @@ from shared.messages import ( AGREED_MESSAGE, CLA_AGREEMENT_MESSAGE, + FAILED_COMMENT, USER_AGREEMENT_MESSAGE, ) from check_cla.check_cla_pr import CLAHandler, main @@ -28,15 +29,14 @@ def test_bot_comment_exists(): cla = CLAHandler(mock.Mock()) comments_iterator = mock.Mock() comment1 = mock.Mock() - comment1.user.login = "username" + comment1.body = "comment1" comment2 = mock.Mock() - comment2.user.login = "sa-github-api" + comment2.body = "comment2" comments_iterator.__iter__ = mock.Mock( - return_value=iter([comment1, comment2, comment1]) + return_value=iter([comment1, comment2]) ) - # comments_iterator.return_value = [comment1, comment2, comment1] - bot_comment = cla.check_comment_already_exists(comments_iterator) + bot_comment = cla.check_if_comment_already_exists("comment1", comments_iterator) assert bot_comment is True @@ -45,14 +45,25 @@ def test_no_bot_comment(): cla = CLAHandler(mock.Mock()) issue_comments = mock.Mock() comment1 = mock.Mock() - comment1.user.login = "username" - issue_comments.__iter__ = mock.Mock(return_value=iter([comment1, comment1])) + comment1.body = "comment" + issue_comments.__iter__ = mock.Mock(return_value=iter([comment1])) - bot_comment = cla.check_comment_already_exists(issue_comments) + bot_comment = cla.check_if_comment_already_exists("comment2", issue_comments) assert bot_comment is False +def test_leave_failed_comment_on_issue(): + cla = CLAHandler(mock.Mock()) + issue = mock.Mock() + issue.comments.return_value = mock.Mock() + cla.check_if_comment_already_exists = mock.Mock(return_value=False) + + cla.leave_failed_comment_on_issue(issue) + + issue.create_comment.assert_called_once_with(FAILED_COMMENT) + + def test_cla_is_signed(capfd): cla = CLAHandler(mock.Mock()) issue = mock.Mock() @@ -88,7 +99,6 @@ def test_cla_is_not_signed(capfd): cla = CLAHandler(mock.Mock()) issue = mock.Mock() comment = mock.Mock() - comment.user.login = "bot" issue.comments.return_value = [mock.Mock(), comment] response = cla.check_if_cla_signed(issue, "username") @@ -103,7 +113,6 @@ def test_get_cla_issue_success(): cla_repo = mock.Mock() issue = mock.Mock() issue.title = "cla: @username" - issue.user.login = "sa-github-api" cla_repo.issues.return_value = [mock.Mock(), issue] cla.cla_repo = cla_repo From bfaaac1def0467d7774277e7e1e6057d325b8a02 Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Thu, 12 Dec 2024 11:29:26 +0100 Subject: [PATCH 2/3] Revert "refactor(IDX): update comment logic (#78)" (#89) This reverts commit 7ea52c61d2c4cc62644b782e1d442911263cd1e8. --- .../check_cla/check_cla_issue.py | 2 +- reusable_workflows/check_cla/check_cla_pr.py | 23 ++++++++------- reusable_workflows/tests/test_cla_issue.py | 4 +-- reusable_workflows/tests/test_cla_pr.py | 29 +++++++------------ 4 files changed, 26 insertions(+), 32 deletions(-) diff --git a/reusable_workflows/check_cla/check_cla_issue.py b/reusable_workflows/check_cla/check_cla_issue.py index 1994659..3a9abbf 100644 --- a/reusable_workflows/check_cla/check_cla_issue.py +++ b/reusable_workflows/check_cla/check_cla_issue.py @@ -20,7 +20,7 @@ def main() -> None: cla_signed = cla.check_if_cla_signed(issue, user) if not cla_signed: - cla.leave_failed_comment_on_issue(issue) + cla.comment_on_issue(issue) else: cla.handle_cla_signed(issue, user) diff --git a/reusable_workflows/check_cla/check_cla_pr.py b/reusable_workflows/check_cla/check_cla_pr.py index dde810e..f6f9533 100644 --- a/reusable_workflows/check_cla/check_cla_pr.py +++ b/reusable_workflows/check_cla/check_cla_pr.py @@ -12,30 +12,33 @@ APPROVED_LABEL = "cla:agreed" GH_WORKFLOW_LABEL = "cla:gh-wf-pending" +# keep all old bot names for backwards compatibility +CLA_BOT_NAMES = ["cla-idx-bot[bot]", "sa-github-api", "dfinity-droid-prod[bot]"] + class CLAHandler: def __init__(self, gh: github3.login) -> None: self.cla_repo = gh.repository(owner="dfinity", repository="cla") self.cla_link = f"{self.cla_repo.html_url}/blob/main/CLA.md" - @staticmethod - def check_if_comment_already_exists( - search_comment: str, comments: github3.structs.GitHubIterator + def check_comment_already_exists( + self, comments: github3.structs.GitHubIterator ) -> bool: for comment in comments: - if search_comment == comment.body: + if comment.user.login in CLA_BOT_NAMES: return True return False - def leave_failed_comment_on_issue(self, issue: GHIssue) -> None: + def comment_on_issue(self, issue: GHIssue): # check if bot has already left a message to avoid spam issue_comments = issue.comments() - if not self.check_if_comment_already_exists(messages.FAILED_COMMENT, issue_comments): + bot_comment = self.check_comment_already_exists(issue_comments) + if not bot_comment: issue.create_comment(messages.FAILED_COMMENT) - def comment_on_pr(self, pr: GHPullRequest, pr_comment: str) -> None: - pr_comments = pr.comments() - if not self.check_if_comment_already_exists(pr_comment, pr_comments): + def comment_on_pr(self, pr: GHPullRequest, pr_comment): + bot_comment = self.check_comment_already_exists(pr.issue_comments()) + if not bot_comment: pr.create_comment(pr_comment) def check_if_cla_signed(self, issue: GHIssue, user: str) -> bool: @@ -53,7 +56,7 @@ def check_if_cla_signed(self, issue: GHIssue, user: str) -> bool: def get_cla_issue(self, user: str) -> Optional[GHIssue]: for issue in self.cla_repo.issues(): - if issue.title == f"cla: @{user}": + if issue.title == f"cla: @{user}" and issue.user.login in CLA_BOT_NAMES: return issue print(f"No CLA issue for {user}") return None # to make linter happy diff --git a/reusable_workflows/tests/test_cla_issue.py b/reusable_workflows/tests/test_cla_issue.py index 2ac4150..5ce390b 100644 --- a/reusable_workflows/tests/test_cla_issue.py +++ b/reusable_workflows/tests/test_cla_issue.py @@ -33,7 +33,7 @@ def test_end_to_end_cla_signed(cla_mock, gh_login_mock): main() cla.check_if_cla_signed.assert_called_with(issue, "username") - cla.leave_failed_comment_on_issue.assert_not_called() + cla.comment_on_issue.assert_not_called() cla.handle_cla_signed.assert_called_once() @@ -56,7 +56,7 @@ def test_end_to_end_cla_not_signed(cla_mock, gh_login_mock): main() cla.check_if_cla_signed.assert_called_with(issue, "username") - cla.leave_failed_comment_on_issue.assert_called_once() + cla.comment_on_issue.assert_called_once() cla.handle_cla_signed.assert_not_called() diff --git a/reusable_workflows/tests/test_cla_pr.py b/reusable_workflows/tests/test_cla_pr.py index 1ab3621..65bff83 100644 --- a/reusable_workflows/tests/test_cla_pr.py +++ b/reusable_workflows/tests/test_cla_pr.py @@ -6,7 +6,6 @@ from shared.messages import ( AGREED_MESSAGE, CLA_AGREEMENT_MESSAGE, - FAILED_COMMENT, USER_AGREEMENT_MESSAGE, ) from check_cla.check_cla_pr import CLAHandler, main @@ -29,14 +28,15 @@ def test_bot_comment_exists(): cla = CLAHandler(mock.Mock()) comments_iterator = mock.Mock() comment1 = mock.Mock() - comment1.body = "comment1" + comment1.user.login = "username" comment2 = mock.Mock() - comment2.body = "comment2" + comment2.user.login = "sa-github-api" comments_iterator.__iter__ = mock.Mock( - return_value=iter([comment1, comment2]) + return_value=iter([comment1, comment2, comment1]) ) + # comments_iterator.return_value = [comment1, comment2, comment1] - bot_comment = cla.check_if_comment_already_exists("comment1", comments_iterator) + bot_comment = cla.check_comment_already_exists(comments_iterator) assert bot_comment is True @@ -45,25 +45,14 @@ def test_no_bot_comment(): cla = CLAHandler(mock.Mock()) issue_comments = mock.Mock() comment1 = mock.Mock() - comment1.body = "comment" - issue_comments.__iter__ = mock.Mock(return_value=iter([comment1])) + comment1.user.login = "username" + issue_comments.__iter__ = mock.Mock(return_value=iter([comment1, comment1])) - bot_comment = cla.check_if_comment_already_exists("comment2", issue_comments) + bot_comment = cla.check_comment_already_exists(issue_comments) assert bot_comment is False -def test_leave_failed_comment_on_issue(): - cla = CLAHandler(mock.Mock()) - issue = mock.Mock() - issue.comments.return_value = mock.Mock() - cla.check_if_comment_already_exists = mock.Mock(return_value=False) - - cla.leave_failed_comment_on_issue(issue) - - issue.create_comment.assert_called_once_with(FAILED_COMMENT) - - def test_cla_is_signed(capfd): cla = CLAHandler(mock.Mock()) issue = mock.Mock() @@ -99,6 +88,7 @@ def test_cla_is_not_signed(capfd): cla = CLAHandler(mock.Mock()) issue = mock.Mock() comment = mock.Mock() + comment.user.login = "bot" issue.comments.return_value = [mock.Mock(), comment] response = cla.check_if_cla_signed(issue, "username") @@ -113,6 +103,7 @@ def test_get_cla_issue_success(): cla_repo = mock.Mock() issue = mock.Mock() issue.title = "cla: @username" + issue.user.login = "sa-github-api" cla_repo.issues.return_value = [mock.Mock(), issue] cla.cla_repo = cla_repo From 63edbc808ce4c9a344fb5f9358b3caae1cfde76c Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Mon, 16 Dec 2024 16:17:21 +0100 Subject: [PATCH 3/3] feat(IDX): introduce integration tests (#92) * wip * wip * test * add test * fix * test * test output * lint * try again * use real token * updates * update --- .github/workflows/python_lint_test.yml | 11 +++- .gitignore | 1 + reusable_workflows/tests/conftest.py | 14 ++--- .../tests/test_check_external_contrib.py | 7 +++ .../tests/test_compliance_checks.py | 55 +++++++++++++++++++ reusable_workflows/tests/test_membership.py | 9 +++ reusable_workflows/tests/test_utils.py | 4 +- 7 files changed, 91 insertions(+), 10 deletions(-) diff --git a/.github/workflows/python_lint_test.yml b/.github/workflows/python_lint_test.yml index f320e2a..a83a726 100644 --- a/.github/workflows/python_lint_test.yml +++ b/.github/workflows/python_lint_test.yml @@ -32,5 +32,14 @@ jobs: black reusable_workflows/ flake8 reusable_workflows/ + - name: Create GitHub App Token + uses: actions/create-github-app-token@v1 + id: app-token + with: + app-id: ${{ vars.CLA_BOT_APP_ID }} + private-key: ${{ secrets.CLA_BOT_PRIVATE_KEY }} + - name: Run all tests - run: pytest --runslow reusable_workflows/ + run: pytest --integration-tests reusable_workflows/ + env: + GH_TOKEN: ${{ steps.app-token.outputs.token }} diff --git a/.gitignore b/.gitignore index 700939f..93b46df 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ sorting/target **/__pycache__ .env/** +.venv/** diff --git a/reusable_workflows/tests/conftest.py b/reusable_workflows/tests/conftest.py index e446d0a..8e719e8 100644 --- a/reusable_workflows/tests/conftest.py +++ b/reusable_workflows/tests/conftest.py @@ -3,19 +3,19 @@ def pytest_addoption(parser): parser.addoption( - "--runslow", action="store_true", default=False, help="run slow tests" + "--integration-tests", action="store_true", default=False, help="run integration tests" ) def pytest_configure(config): - config.addinivalue_line("markers", "slow: mark test as slow to run") + config.addinivalue_line("markers", "integration: mark test as integration test") def pytest_collection_modifyitems(config, items): - if config.getoption("--runslow"): - # --runslow given in cli: do not skip slow tests + if config.getoption("--integration-tests"): + # --integration-tests given in cli: do not skip integration tests return - skip_slow = pytest.mark.skip(reason="need --runslow option to run") + skip_integration = pytest.mark.skip(reason="need --integration-tests option to run") for item in items: - if "slow" in item.keywords: - item.add_marker(skip_slow) + if "integration" in item.keywords: + item.add_marker(skip_integration) diff --git a/reusable_workflows/tests/test_check_external_contrib.py b/reusable_workflows/tests/test_check_external_contrib.py index 4d44ce8..296720f 100644 --- a/reusable_workflows/tests/test_check_external_contrib.py +++ b/reusable_workflows/tests/test_check_external_contrib.py @@ -1,6 +1,7 @@ import os from unittest import mock +import github3 import pytest from check_membership.check_external_contrib import ( @@ -53,3 +54,9 @@ def test_github_token_not_passed_in(github_login_mock): assert ( str(exc.value) == "github login failed - maybe GH_TOKEN was not correctly set" ) + +@pytest.mark.integration +def test_check_repos_open_to_contributions_accessible(): + gh = github3.login(token=os.getenv("GH_TOKEN")) + + get_repos_open_to_contributions(gh) diff --git a/reusable_workflows/tests/test_compliance_checks.py b/reusable_workflows/tests/test_compliance_checks.py index 3cbeb98..7b9cd16 100644 --- a/reusable_workflows/tests/test_compliance_checks.py +++ b/reusable_workflows/tests/test_compliance_checks.py @@ -2,7 +2,9 @@ import sys from unittest import mock +import os import pytest +import github3 from github3.exceptions import NotFoundError from compliance.check_compliance.compliance_checks import ( @@ -318,3 +320,56 @@ def test_repo_permissions_team_name_not_found(get_team_name_mock): repo_permissions_check.check(helper) assert repo_permissions_check.succeeds is False assert repo_permissions_check.message == "Raised error: Only one team can be listed for repo-level codeowners." # fmt: skip + + +def integration_test_setup() -> ComplianceCheckHelper: + gh = github3.login(token=os.getenv("GH_TOKEN")) + repo = gh.repository("dfinity", "test-compliant-repository-public") + org = gh.organization("dfinity") + helper = ComplianceCheckHelper(repo, org) + return helper + +@pytest.mark.integration +def test_get_code_owners(): + helper = integration_test_setup() + + code_owners = helper.get_code_owners() + + assert code_owners == "* @dfinity/idx\n" + +@pytest.mark.integration +def test_get_repo_permissions(): + helper = integration_test_setup() + repo_permissions = RepoPermissions() + + repo_permissions.check(helper) + + assert repo_permissions.succeeds is True + assert repo_permissions.message == "Team idx has maintain permissions." + +@pytest.mark.integration +def test_branch_protection(): + helper = integration_test_setup() + branch_protection = BranchProtection() + + branch_protection.check(helper) + + assert branch_protection.succeeds is True + +@pytest.mark.integration +def test_license(): + helper = integration_test_setup() + license = License() + + license.check(helper) + + assert license.succeeds is True + +@pytest.mark.integration +def test_readme(): + helper = integration_test_setup() + readme = Readme() + + readme.check(helper) + + assert readme.succeeds is True diff --git a/reusable_workflows/tests/test_membership.py b/reusable_workflows/tests/test_membership.py index 18b1141..8632b99 100644 --- a/reusable_workflows/tests/test_membership.py +++ b/reusable_workflows/tests/test_membership.py @@ -1,6 +1,7 @@ import os from unittest import mock +import github3 from github3.exceptions import NotFoundError import pytest @@ -129,3 +130,11 @@ def test_github_token_not_passed_in(github_login_mock): assert ( str(exc.value) == "github login failed - maybe GH_TOKEN was not correctly set" ) + +@pytest.mark.integration +def test_is_member_of_org(): + gh = github3.login(token=os.getenv("GH_TOKEN")) + + is_member = is_member_of_org(gh, "dfinity", "sa-github-api") + + assert is_member is True diff --git a/reusable_workflows/tests/test_utils.py b/reusable_workflows/tests/test_utils.py index 3ae2aaf..f11283f 100644 --- a/reusable_workflows/tests/test_utils.py +++ b/reusable_workflows/tests/test_utils.py @@ -19,7 +19,7 @@ def test_download_file_succeeds_first_try(): assert repo.file_contents.call_count == 1 -@pytest.mark.slow +@pytest.mark.integration def test_download_file_succeeds_third_try(): repo = mock.MagicMock() file_content_obj = mock.Mock() @@ -35,7 +35,7 @@ def test_download_file_succeeds_third_try(): assert repo.file_contents.call_count == 3 -@pytest.mark.slow +@pytest.mark.integration @mock.patch("requests.get") def test_download_file_fails(mock_get): repo = mock.MagicMock()