From f8a1cbd1b6e7dc567ec279d15b3ae4e78f0a0344 Mon Sep 17 00:00:00 2001 From: Michelle Tran Date: Thu, 19 Sep 2024 12:03:16 -0400 Subject: [PATCH 1/2] Refactor tests to use fixtures This is because the tests were failing on duplicate keys. Fixtures reuses existing objects, which avoids creating objects with duplicate keys. --- database/tests/factories/core.py | 2 +- tasks/tests/unit/test_brolly_stats_rollup.py | 59 +- tasks/tests/unit/test_sync_pull.py | 1065 +++++++++--------- 3 files changed, 552 insertions(+), 574 deletions(-) diff --git a/database/tests/factories/core.py b/database/tests/factories/core.py index 55fd30bf7..d705387e7 100644 --- a/database/tests/factories/core.py +++ b/database/tests/factories/core.py @@ -112,7 +112,7 @@ class Meta: ).hexdigest() ) ci_passed = True - pullid = 1 + pullid = None timestamp = datetime(2019, 2, 1, 17, 59, 47) author = factory.SubFactory(OwnerFactory) repository = factory.SubFactory(RepositoryFactory) diff --git a/tasks/tests/unit/test_brolly_stats_rollup.py b/tasks/tests/unit/test_brolly_stats_rollup.py index 80af2f0a1..af8ccaf74 100644 --- a/tasks/tests/unit/test_brolly_stats_rollup.py +++ b/tasks/tests/unit/test_brolly_stats_rollup.py @@ -3,6 +3,7 @@ import uuid import httpx +import pytest import respx from mock import Mock, patch @@ -10,6 +11,7 @@ from database.tests.factories import ( CommitFactory, ConstantsFactory, + ReportFactory, RepositoryFactory, UploadFactory, UserFactory, @@ -17,6 +19,26 @@ from tasks.brolly_stats_rollup import DEFAULT_BROLLY_ENDPOINT, BrollyStatsRollupTask +@pytest.fixture +def version(dbsession) -> str: + version = dbsession.query(Constants).filter_by(key="version").scalar() + if version is None: + version = ConstantsFactory.create(key="version", value="hello") + dbsession.add(version) + dbsession.flush() + return version + + +@pytest.fixture +def install_id(dbsession) -> int: + install_id = dbsession.query(Constants).filter_by(key="install_id").scalar() + if install_id is None: + install_id = ConstantsFactory.create(key="install_id", value=str(uuid.uuid4())) + dbsession.add(install_id) + dbsession.flush() + return install_id + + def _get_n_hours_ago(n): return datetime.datetime.now() - datetime.timedelta(hours=n) @@ -38,9 +60,7 @@ def test_get_min_seconds_interval_between_executions(self, dbsession): ) @patch("tasks.brolly_stats_rollup.get_config", return_value=False) - def test_run_cron_task_while_disabled(self, mocker, dbsession): - task = BrollyStatsRollupTask() - + def test_run_cron_task_while_disabled(self, dbsession): result = BrollyStatsRollupTask().run_cron_task(dbsession) assert result == { "uploaded": False, @@ -48,7 +68,7 @@ def test_run_cron_task_while_disabled(self, mocker, dbsession): } @respx.mock - def test_run_cron_task_http_ok(self, mocker, dbsession): + def test_run_cron_task_http_ok(self, dbsession, install_id, version): users = [UserFactory.create(name=name) for name in ("foo", "bar", "baz")] for user in users: dbsession.add(user) @@ -73,8 +93,9 @@ def test_run_cron_task_http_ok(self, mocker, dbsession): for commit in commits: dbsession.add(commit) + report = ReportFactory.create(commit=commits[0]) uploads = [ - UploadFactory.create(created_at=created_at, report=None) + UploadFactory.create(created_at=created_at, report=report) for created_at in ( _get_n_hours_ago(5), _get_n_hours_ago(16), @@ -84,17 +105,12 @@ def test_run_cron_task_http_ok(self, mocker, dbsession): for upload in uploads: dbsession.add(upload) - install_id = ConstantsFactory.create(key="install_id", value=str(uuid.uuid4())) - version = ConstantsFactory.create(key="version", value="hello") - dbsession.add(install_id) - dbsession.add(version) + dbsession.flush() install_id_val = dbsession.query(Constants).get("install_id").value version_val = dbsession.query(Constants).get("version").value print("mattmatt", install_id_val, version_val) - dbsession.flush() - mock_request = respx.post(DEFAULT_BROLLY_ENDPOINT).mock( return_value=httpx.Response(200) ) @@ -117,18 +133,10 @@ def test_run_cron_task_http_ok(self, mocker, dbsession): } @respx.mock - def test_run_cron_task_not_ok(self, mocker, dbsession): + def test_run_cron_task_not_ok(self, dbsession, install_id, version): mock_request = respx.post(DEFAULT_BROLLY_ENDPOINT).mock( return_value=httpx.Response(500) ) - - install_id = ConstantsFactory.create(key="install_id", value=str(uuid.uuid4())) - version = ConstantsFactory.create(key="version", value="hello") - dbsession.add(install_id) - dbsession.add(version) - - dbsession.flush() - task = BrollyStatsRollupTask() result = task.run_cron_task(dbsession) assert mock_request.called @@ -146,7 +154,9 @@ def test_run_cron_task_not_ok(self, mocker, dbsession): } @respx.mock - def test_run_cron_task_include_admin_email_if_populated(self, mocker, dbsession): + def test_run_cron_task_include_admin_email_if_populated( + self, mocker, dbsession, install_id, version + ): mock_request = respx.post(DEFAULT_BROLLY_ENDPOINT).mock( return_value=httpx.Response(200) ) @@ -155,13 +165,6 @@ def test_run_cron_task_include_admin_email_if_populated(self, mocker, dbsession) BrollyStatsRollupTask, "_get_admin_email", return_value="hello" ) - install_id = ConstantsFactory.create(key="install_id", value=str(uuid.uuid4())) - version = ConstantsFactory.create(key="version", value="hello") - dbsession.add(install_id) - dbsession.add(version) - - dbsession.flush() - task = BrollyStatsRollupTask() result = task.run_cron_task(dbsession) assert mock_request.called diff --git a/tasks/tests/unit/test_sync_pull.py b/tasks/tests/unit/test_sync_pull.py index 49b84f153..97839ca51 100644 --- a/tasks/tests/unit/test_sync_pull.py +++ b/tasks/tests/unit/test_sync_pull.py @@ -9,6 +9,7 @@ from shared.reports.types import Change from shared.torngit.exceptions import TorngitClientError +from database.models import Commit, Pull, Repository from database.tests.factories import CommitFactory, PullFactory, RepositoryFactory from database.tests.factories.reports import TestFactory from helpers.exceptions import NoConfiguredAppsAvailable, RepositoryWithoutValidBotError @@ -19,582 +20,556 @@ here = Path(__file__) -class TestPullSyncTask(object): - @pytest.mark.parametrize( - "flake_detection_config, flake_detection,flaky_shadow_mode,tests_exist,outcome", - [ - (False, True, True, True, False), - (True, False, False, False, False), - (True, False, False, True, False), - (True, True, False, True, True), - (True, False, True, True, True), - (True, True, True, True, True), - ], - ) - def test_update_pull_commits_merged( - self, - dbsession, - mocker, - flake_detection_config, - flake_detection, - flaky_shadow_mode, - tests_exist, - outcome, - ): - if flake_detection: - mock_feature = mocker.patch("services.test_results.FLAKY_TEST_DETECTION") - mock_feature.check_value.return_value = True - - if flaky_shadow_mode: - _flaky_shadow_mode_feature = mocker.patch( - "services.test_results.FLAKY_SHADOW_MODE" - ) - _flaky_shadow_mode_feature.check_value.return_value = True +@pytest.fixture +def repository(dbsession) -> Repository: + repository = RepositoryFactory.create() + dbsession.add(repository) + dbsession.flush() + return repository - repository = RepositoryFactory.create() - dbsession.add(repository) - dbsession.flush() - if tests_exist: - t = TestFactory(repository=repository) - dbsession.add(t) - dbsession.flush() - - base_commit = CommitFactory.create(repository=repository) - head_commit = CommitFactory.create(repository=repository) - pull = PullFactory.create( - repository=repository, - base=base_commit.commitid, - head=head_commit.commitid, - state="merged", - ) - pullid = pull.pullid - base_commit.pullid = pullid - head_commit.pullid = pullid - first_commit = CommitFactory.create( - repository=repository, pullid=pullid, merged=False - ) - second_commit = CommitFactory.create( - repository=repository, pullid=pullid, merged=False - ) - third_commit = CommitFactory.create( - repository=repository, pullid=pullid, merged=False - ) - fourth_commit = CommitFactory.create( - repository=repository, pullid=pullid, merged=False - ) - dbsession.add(pull) - dbsession.add(first_commit) - dbsession.add(second_commit) - dbsession.add(third_commit) - dbsession.add(fourth_commit) - dbsession.add(base_commit) - dbsession.add(head_commit) - dbsession.flush() - task = PullSyncTask() - enriched_pull = EnrichedPull( - database_pull=pull, - provider_pull=dict( - base=dict(branch="lookatthis"), head=dict(branch="thing") - ), - ) - commits = [first_commit.commitid, third_commit.commitid] - commits_at_base = { - "commitid": first_commit.commitid, - "parents": [{"commitid": third_commit.commitid, "parents": []}], - } +@pytest.fixture +def base_commit(dbsession, repository) -> Commit: + commit = CommitFactory.create(repository=repository) + dbsession.add(commit) + dbsession.flush() + return commit - apply_async: MagicMock = mocker.patch.object( - task.app.tasks["app.tasks.flakes.ProcessFlakesTask"], "apply_async" - ) - current_yaml = UserYaml.from_dict( - { - "test_analytics": { - "flake_detection": flake_detection_config, - } - } - ) - mock_repo_provider = MagicMock( - get_commit=MagicMock(return_value=dict(parents=["1", "2"])) - ) - res = task.update_pull_commits( - mock_repo_provider, enriched_pull, commits, commits_at_base, current_yaml - ) +@pytest.fixture +def head_commit(dbsession, repository) -> Commit: + commit = CommitFactory.create(repository=repository) + dbsession.add(commit) + dbsession.flush() + return commit - if outcome: - apply_async.assert_called_once_with( - kwargs=dict( - repo_id=repository.repoid, - commit_id_list=[head_commit.commitid], - branch="thing", - ) - ) - else: - apply_async.assert_not_called() - - assert res == {"merged_count": 2, "soft_deleted_count": 2} - dbsession.refresh(first_commit) - dbsession.refresh(second_commit) - dbsession.refresh(third_commit) - dbsession.refresh(fourth_commit) - assert not first_commit.deleted - assert second_commit.deleted - assert not third_commit.deleted - assert fourth_commit.deleted - assert first_commit.merged - assert not second_commit.merged - assert third_commit.merged - assert not fourth_commit.merged - assert first_commit.branch == "lookatthis" - assert third_commit.branch == "lookatthis" - - def test_update_pull_commits_not_merged(self, dbsession, mocker): - repository = RepositoryFactory.create() - dbsession.add(repository) - dbsession.flush() - base_commit = CommitFactory.create(repository=repository) - head_commit = CommitFactory.create(repository=repository) - pull = PullFactory.create( - repository=repository, - base=base_commit.commitid, - head=head_commit.commitid, - state="open", - ) - pullid = pull.pullid - base_commit.pullid = pullid - head_commit.pullid = pullid - first_commit = CommitFactory.create( - repository=repository, pullid=pullid, merged=False - ) - second_commit = CommitFactory.create( - repository=repository, pullid=pullid, merged=False - ) - third_commit = CommitFactory.create( - repository=repository, pullid=pullid, merged=False - ) - fourth_commit = CommitFactory.create( - repository=repository, pullid=pullid, merged=False - ) - dbsession.add(pull) - dbsession.add(first_commit) - dbsession.add(second_commit) - dbsession.add(third_commit) - dbsession.add(fourth_commit) - dbsession.add(base_commit) - dbsession.add(head_commit) - dbsession.flush() - task = PullSyncTask() - enriched_pull = EnrichedPull( - database_pull=pull, provider_pull=dict(base=dict(branch="lookatthis")) - ) - commits = [first_commit.commitid, third_commit.commitid] - commits_at_base = { - "commitid": first_commit.commitid, - "parents": [{"commitid": third_commit.commitid, "parents": []}], - } - current_yaml = UserYaml.from_dict(dict()) - mock_repo_provider = MagicMock( - get_commit=MagicMock(return_value=dict(parents=["1", "2"])) - ) - res = task.update_pull_commits( - mock_repo_provider, enriched_pull, commits, commits_at_base, current_yaml - ) - assert res == {"merged_count": 0, "soft_deleted_count": 2} - dbsession.refresh(first_commit) - dbsession.refresh(second_commit) - dbsession.refresh(third_commit) - dbsession.refresh(fourth_commit) - assert not first_commit.deleted - assert second_commit.deleted - assert not third_commit.deleted - assert fourth_commit.deleted - assert not first_commit.merged - assert not second_commit.merged - assert not third_commit.merged - assert not fourth_commit.merged - - def test_call_pullsync_task_no_head_commit(self, dbsession, mocker, mock_redis): - task = PullSyncTask() - pull = PullFactory.create(head="head_commit_nonexistent_sha", state="open") - dbsession.add(pull) - dbsession.flush() - mocked_fetch_pr = mocker.patch( - "tasks.sync_pull.fetch_and_update_pull_request_information" - ) - mocked_fetch_pr.return_value = EnrichedPull( - database_pull=pull, provider_pull={} - ) - res = task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) - assert res == { - "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, - "notifier_called": False, - "pull_updated": False, - "reason": "no_head", - } - - def test_call_pullsync_task_nolock(self, dbsession, mock_redis): - task = PullSyncTask() - pull = PullFactory.create(state="open") - dbsession.add(pull) - dbsession.flush() - mock_redis.lock.return_value.__enter__.side_effect = LockError - res = task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) - assert res == { - "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, - "notifier_called": False, - "pull_updated": False, - "reason": "unable_fetch_lock", - } - - def test_call_pullsync_task_no_database_pull(self, dbsession, mocker, mock_redis): - repository = RepositoryFactory.create() - dbsession.add(repository) - dbsession.flush() - task = PullSyncTask() - mocked_fetch_pr = mocker.patch( - "tasks.sync_pull.fetch_and_update_pull_request_information" - ) - mocked_fetch_pr.return_value = EnrichedPull( - database_pull=None, provider_pull=None - ) - res = task.run_impl(dbsession, repoid=repository.repoid, pullid=99) - assert res == { - "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, - "notifier_called": False, - "pull_updated": False, - "reason": "no_db_pull", - } - def test_call_pullsync_task_no_provider_pull_only( - self, dbsession, mocker, mock_redis - ): - repository = RepositoryFactory.create() - dbsession.add(repository) - dbsession.flush() - pull = PullFactory.create(state="open", repository=repository) - dbsession.add(pull) +@pytest.fixture +def pull(dbsession, repository, base_commit, head_commit) -> Pull: + pull = PullFactory.create( + repository=repository, + base=base_commit.commitid, + head=head_commit.commitid, + ) + dbsession.add(pull) + dbsession.flush() + return pull + + +@pytest.mark.parametrize( + "flake_detection_config, flake_detection,flaky_shadow_mode,tests_exist,outcome", + [ + (False, True, True, True, False), + (True, False, False, False, False), + (True, False, False, True, False), + (True, True, False, True, True), + (True, False, True, True, True), + (True, True, True, True, True), + ], +) +def test_update_pull_commits_merged( + dbsession, + mocker, + flake_detection_config, + flake_detection, + flaky_shadow_mode, + tests_exist, + outcome, + repository, + head_commit, + base_commit, + pull, +): + if flake_detection: + mock_feature = mocker.patch("services.test_results.FLAKY_TEST_DETECTION") + mock_feature.check_value.return_value = True + + if flaky_shadow_mode: + _flaky_shadow_mode_feature = mocker.patch( + "services.test_results.FLAKY_SHADOW_MODE" + ) + _flaky_shadow_mode_feature.check_value.return_value = True + + if tests_exist: + t = TestFactory(repository=repository) + dbsession.add(t) dbsession.flush() - task = PullSyncTask() - mocked_fetch_pr = mocker.patch( - "tasks.sync_pull.fetch_and_update_pull_request_information" - ) - mocked_fetch_pr.return_value = EnrichedPull( - database_pull=pull, provider_pull=None - ) - res = task.run_impl(dbsession, repoid=repository.repoid, pullid=99) - assert res == { - "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, - "notifier_called": False, - "pull_updated": False, - "reason": "not_in_provider", - } - def test_call_pullsync_no_bot(self, dbsession, mock_redis, mocker): - task = PullSyncTask() - pull = PullFactory.create(state="open") - dbsession.add(pull) - dbsession.flush() - mocker.patch( - "tasks.sync_pull.get_repo_provider_service", - side_effect=RepositoryWithoutValidBotError(), - ) - res = task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) - assert res == { - "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, - "notifier_called": False, - "pull_updated": False, - "reason": "no_bot", - } + pull.state = "merged" + pullid = pull.pullid + base_commit.pullid = pullid + head_commit.pullid = pullid + first_commit = CommitFactory.create( + repository=repository, pullid=pullid, merged=False + ) + second_commit = CommitFactory.create( + repository=repository, pullid=pullid, merged=False + ) + third_commit = CommitFactory.create( + repository=repository, pullid=pullid, merged=False + ) + fourth_commit = CommitFactory.create( + repository=repository, pullid=pullid, merged=False + ) + dbsession.add(pull) + dbsession.add(first_commit) + dbsession.add(second_commit) + dbsession.add(third_commit) + dbsession.add(fourth_commit) + dbsession.add(base_commit) + dbsession.add(head_commit) + dbsession.flush() + task = PullSyncTask() + enriched_pull = EnrichedPull( + database_pull=pull, + provider_pull=dict(base=dict(branch="lookatthis"), head=dict(branch="thing")), + ) + commits = [first_commit.commitid, third_commit.commitid] + commits_at_base = { + "commitid": first_commit.commitid, + "parents": [{"commitid": third_commit.commitid, "parents": []}], + } + + apply_async: MagicMock = mocker.patch.object( + task.app.tasks["app.tasks.flakes.ProcessFlakesTask"], "apply_async" + ) - def test_call_pullsync_no_apps_available_rate_limit( - self, dbsession, mock_redis, mocker - ): - task = PullSyncTask() - pull = PullFactory.create(state="open") - dbsession.add(pull) - dbsession.flush() - mocker.patch( - "tasks.sync_pull.get_repo_provider_service", - side_effect=NoConfiguredAppsAvailable( - apps_count=1, rate_limited_count=1, suspended_count=0 - ), - ) - with pytest.raises(Retry): - task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) - - def test_call_pullsync_no_apps_available_suspended( - self, dbsession, mock_redis, mocker - ): - task = PullSyncTask() - pull = PullFactory.create(state="open") - dbsession.add(pull) - dbsession.flush() - mocker.patch( - "tasks.sync_pull.get_repo_provider_service", - side_effect=NoConfiguredAppsAvailable( - apps_count=1, rate_limited_count=0, suspended_count=1 - ), - ) - res = task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) - assert res == { - "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, - "notifier_called": False, - "pull_updated": False, - "reason": "no_configured_apps_available", + current_yaml = UserYaml.from_dict( + { + "test_analytics": { + "flake_detection": flake_detection_config, + } } + ) + mock_repo_provider = MagicMock( + get_commit=MagicMock(return_value=dict(parents=["1", "2"])) + ) + res = task.update_pull_commits( + mock_repo_provider, enriched_pull, commits, commits_at_base, current_yaml + ) - def test_call_pullsync_no_permissions_get_compare( - self, dbsession, mock_redis, mocker, mock_repo_provider, mock_storage - ): - mocker.patch.object(PullSyncTask, "app") - task = PullSyncTask() - repository = RepositoryFactory.create() - dbsession.add(repository) - dbsession.flush() - base_commit = CommitFactory.create(repository=repository) - head_commit = CommitFactory.create(repository=repository) - dbsession.add(base_commit) - dbsession.add(head_commit) - pull = PullFactory.create( - state="open", - repository=repository, - base=base_commit.commitid, - head=head_commit.commitid, - ) - dbsession.add(pull) - dbsession.flush() - mocked_fetch_pr = mocker.patch( - "tasks.sync_pull.fetch_and_update_pull_request_information" - ) - mocked_fetch_pr.return_value = EnrichedPull( - database_pull=pull, provider_pull={"head"} - ) - mock_repo_provider.get_compare.side_effect = TorngitClientError( - 403, "response", "message" - ) - mock_repo_provider.get_pull_request_commits.side_effect = TorngitClientError( - 403, "response", "message" + if outcome: + apply_async.assert_called_once_with( + kwargs=dict( + repo_id=repository.repoid, + commit_id_list=[head_commit.commitid], + branch="thing", + ) ) - res = task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) - assert res == { - "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, - "notifier_called": True, - "pull_updated": True, - "reason": "success", - } + else: + apply_async.assert_not_called() + + assert res == {"merged_count": 2, "soft_deleted_count": 2} + dbsession.refresh(first_commit) + dbsession.refresh(second_commit) + dbsession.refresh(third_commit) + dbsession.refresh(fourth_commit) + assert not first_commit.deleted + assert second_commit.deleted + assert not third_commit.deleted + assert fourth_commit.deleted + assert first_commit.merged + assert not second_commit.merged + assert third_commit.merged + assert not fourth_commit.merged + assert first_commit.branch == "lookatthis" + assert third_commit.branch == "lookatthis" + + +def test_update_pull_commits_not_merged( + dbsession, repository, base_commit, head_commit, pull +): + pull.state = "open" + pullid = pull.pullid + base_commit.pullid = pullid + head_commit.pullid = pullid + first_commit = CommitFactory.create( + repository=repository, pullid=pullid, merged=False + ) + second_commit = CommitFactory.create( + repository=repository, pullid=pullid, merged=False + ) + third_commit = CommitFactory.create( + repository=repository, pullid=pullid, merged=False + ) + fourth_commit = CommitFactory.create( + repository=repository, pullid=pullid, merged=False + ) + dbsession.add(pull) + dbsession.add(first_commit) + dbsession.add(second_commit) + dbsession.add(third_commit) + dbsession.add(fourth_commit) + dbsession.add(base_commit) + dbsession.add(head_commit) + dbsession.flush() + task = PullSyncTask() + enriched_pull = EnrichedPull( + database_pull=pull, provider_pull=dict(base=dict(branch="lookatthis")) + ) + commits = [first_commit.commitid, third_commit.commitid] + commits_at_base = { + "commitid": first_commit.commitid, + "parents": [{"commitid": third_commit.commitid, "parents": []}], + } + current_yaml = UserYaml.from_dict(dict()) + mock_repo_provider = MagicMock( + get_commit=MagicMock(return_value=dict(parents=["1", "2"])) + ) + res = task.update_pull_commits( + mock_repo_provider, enriched_pull, commits, commits_at_base, current_yaml + ) + assert res == {"merged_count": 0, "soft_deleted_count": 2} + dbsession.refresh(first_commit) + dbsession.refresh(second_commit) + dbsession.refresh(third_commit) + dbsession.refresh(fourth_commit) + assert not first_commit.deleted + assert second_commit.deleted + assert not third_commit.deleted + assert fourth_commit.deleted + assert not first_commit.merged + assert not second_commit.merged + assert not third_commit.merged + assert not fourth_commit.merged + + +def test_call_pullsync_task_no_head_commit(dbsession, mocker, mock_redis): + task = PullSyncTask() + pull = PullFactory.create(head="head_commit_nonexistent_sha", state="open") + dbsession.add(pull) + dbsession.flush() + mocked_fetch_pr = mocker.patch( + "tasks.sync_pull.fetch_and_update_pull_request_information" + ) + mocked_fetch_pr.return_value = EnrichedPull(database_pull=pull, provider_pull={}) + res = task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) + assert res == { + "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, + "notifier_called": False, + "pull_updated": False, + "reason": "no_head", + } + + +def test_call_pullsync_task_nolock(dbsession, mock_redis, pull): + task = PullSyncTask() + mock_redis.lock.return_value.__enter__.side_effect = LockError + res = task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) + assert res == { + "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, + "notifier_called": False, + "pull_updated": False, + "reason": "unable_fetch_lock", + } + + +def test_call_pullsync_task_no_database_pull(dbsession, mocker, mock_redis, repository): + task = PullSyncTask() + mocked_fetch_pr = mocker.patch( + "tasks.sync_pull.fetch_and_update_pull_request_information" + ) + mocked_fetch_pr.return_value = EnrichedPull(database_pull=None, provider_pull=None) + res = task.run_impl(dbsession, repoid=repository.repoid, pullid=99) + assert res == { + "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, + "notifier_called": False, + "pull_updated": False, + "reason": "no_db_pull", + } + + +def test_call_pullsync_task_no_provider_pull_only( + dbsession, mocker, mock_redis, repository, pull +): + task = PullSyncTask() + mocked_fetch_pr = mocker.patch( + "tasks.sync_pull.fetch_and_update_pull_request_information" + ) + mocked_fetch_pr.return_value = EnrichedPull(database_pull=pull, provider_pull=None) + res = task.run_impl(dbsession, repoid=repository.repoid, pullid=99) + assert res == { + "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, + "notifier_called": False, + "pull_updated": False, + "reason": "not_in_provider", + } + + +def test_call_pullsync_no_bot(dbsession, mock_redis, mocker, pull): + task = PullSyncTask() + mocker.patch( + "tasks.sync_pull.get_repo_provider_service", + side_effect=RepositoryWithoutValidBotError(), + ) + res = task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) + assert res == { + "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, + "notifier_called": False, + "pull_updated": False, + "reason": "no_bot", + } + + +def test_call_pullsync_no_apps_available_rate_limit( + dbsession, mock_redis, mocker, pull +): + task = PullSyncTask() + mocker.patch( + "tasks.sync_pull.get_repo_provider_service", + side_effect=NoConfiguredAppsAvailable( + apps_count=1, rate_limited_count=1, suspended_count=0 + ), + ) + with pytest.raises(Retry): + task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) - def test_run_impl_unobtainable_lock(self, dbsession, mocker, mock_redis): - pull = PullFactory.create() - dbsession.add(pull) - dbsession.flush() - mock_redis.lock.side_effect = LockError() - mock_redis.exists.return_value = True - task = PullSyncTask() - task.request.retries = 0 - res = task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) - assert res == { - "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, - "notifier_called": False, - "pull_updated": False, - "reason": "unable_fetch_lock", - } - def test_was_pr_merged_with_squash(self): - ancestors_tree = { - "commitid": "c739768fcac68144a3a6d82305b9c4106934d31a", - "parents": [ - { - "commitid": "b33e12816cc3f386dae8add4968cedeff5155021", - "parents": [ - { - "commitid": "743b04806ea677403aa2ff26c6bdeb85005de658", - "parents": [], - }, - { - "commitid": "some_commit", - "parents": [{"commitid": "paaaaaaaaaaa", "parents": []}], - }, - ], - } - ], - } - task = PullSyncTask() - assert not task.was_squash_via_ancestor_tree( - ["c739768fcac68144a3a6d82305b9c4106934d31a"], ancestors_tree - ) - assert task.was_squash_via_ancestor_tree(["some_other_stuff"], ancestors_tree) - assert not task.was_squash_via_ancestor_tree( - ["some_other_stuff", "some_commit"], ancestors_tree - ) +def test_call_pullsync_no_apps_available_suspended(dbsession, mock_redis, mocker, pull): + task = PullSyncTask() + mocker.patch( + "tasks.sync_pull.get_repo_provider_service", + side_effect=NoConfiguredAppsAvailable( + apps_count=1, rate_limited_count=0, suspended_count=1 + ), + ) + res = task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) + assert res == { + "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, + "notifier_called": False, + "pull_updated": False, + "reason": "no_configured_apps_available", + } + + +def test_call_pullsync_no_permissions_get_compare( + dbsession, + mock_redis, + mocker, + mock_repo_provider, + mock_storage, + repository, + base_commit, + head_commit, + pull, +): + mocker.patch.object(PullSyncTask, "app") + task = PullSyncTask() + mocked_fetch_pr = mocker.patch( + "tasks.sync_pull.fetch_and_update_pull_request_information" + ) + mocked_fetch_pr.return_value = EnrichedPull( + database_pull=pull, provider_pull={"head"} + ) + mock_repo_provider.get_compare.side_effect = TorngitClientError( + 403, "response", "message" + ) + mock_repo_provider.get_pull_request_commits.side_effect = TorngitClientError( + 403, "response", "message" + ) + res = task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) + assert res == { + "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, + "notifier_called": True, + "pull_updated": True, + "reason": "success", + } + + +def test_run_impl_unobtainable_lock(dbsession, mock_redis, pull): + mock_redis.lock.side_effect = LockError() + mock_redis.exists.return_value = True + task = PullSyncTask() + task.request.retries = 0 + res = task.run_impl(dbsession, repoid=pull.repoid, pullid=pull.pullid) + assert res == { + "commit_updates_done": {"merged_count": 0, "soft_deleted_count": 0}, + "notifier_called": False, + "pull_updated": False, + "reason": "unable_fetch_lock", + } + + +def test_was_pr_merged_with_squash(): + ancestors_tree = { + "commitid": "c739768fcac68144a3a6d82305b9c4106934d31a", + "parents": [ + { + "commitid": "b33e12816cc3f386dae8add4968cedeff5155021", + "parents": [ + { + "commitid": "743b04806ea677403aa2ff26c6bdeb85005de658", + "parents": [], + }, + { + "commitid": "some_commit", + "parents": [{"commitid": "paaaaaaaaaaa", "parents": []}], + }, + ], + } + ], + } + task = PullSyncTask() + assert not task.was_squash_via_ancestor_tree( + ["c739768fcac68144a3a6d82305b9c4106934d31a"], ancestors_tree + ) + assert task.was_squash_via_ancestor_tree(["some_other_stuff"], ancestors_tree) + assert not task.was_squash_via_ancestor_tree( + ["some_other_stuff", "some_commit"], ancestors_tree + ) - def test_cache_changes_stores_changed_files_in_redis_if_owner_is_whitelisted( - self, dbsession, mock_redis, mock_repo_provider, mocker - ): - repository = RepositoryFactory.create() - dbsession.add(repository) - dbsession.flush() - base_commit = CommitFactory.create(repository=repository) - head_commit = CommitFactory.create(repository=repository) - dbsession.add(base_commit) - dbsession.add(head_commit) - pull = PullFactory.create( - state="open", - repository=repository, - base=base_commit.commitid, - head=head_commit.commitid, - ) - os.environ["OWNERS_WITH_CACHED_CHANGES"] = f"{pull.repository.owner.ownerid}" +def test_cache_changes_stores_changed_files_in_redis_if_owner_is_whitelisted( + dbsession, + mock_redis, + mock_repo_provider, + mocker, + repository, + base_commit, + head_commit, + pull, +): + os.environ["OWNERS_WITH_CACHED_CHANGES"] = f"{pull.repository.owner.ownerid}" + + changes = [Change(path="f.py")] + mocker.patch( + "tasks.sync_pull.get_changes", + lambda base_report, head_report, diff: changes, + ) - changes = [Change(path="f.py")] - mocker.patch( - "tasks.sync_pull.get_changes", - lambda base_report, head_report, diff: changes, - ) + task = PullSyncTask() + task.cache_changes(pull, changes) + + mock_redis.set.assert_called_once_with( + "/".join( + ( + "compare-changed-files", + pull.repository.owner.service, + pull.repository.owner.username, + pull.repository.name, + f"{pull.pullid}", + ) + ), + json.dumps(["f.py"]), + ex=86400, + ) - task = PullSyncTask() - task.cache_changes(pull, changes) - - mock_redis.set.assert_called_once_with( - "/".join( - ( - "compare-changed-files", - pull.repository.owner.service, - pull.repository.owner.username, - pull.repository.name, - f"{pull.pullid}", - ) - ), - json.dumps(["f.py"]), - ex=86400, - ) - def test_trigger_ai_pr_review(self, dbsession, mocker): - repository = RepositoryFactory.create() - dbsession.add(repository) - dbsession.flush() - base_commit = CommitFactory.create(repository=repository) - head_commit = CommitFactory.create(repository=repository) - pull = PullFactory.create( - repository=repository, - base=base_commit.commitid, - head=head_commit.commitid, - ) - pullid = pull.pullid - base_commit.pullid = pullid - head_commit.pullid = pullid - dbsession.add(pull) - dbsession.add(base_commit) - dbsession.add(head_commit) - dbsession.flush() - task = PullSyncTask() - apply_async = mocker.patch.object( - task.app.tasks["app.tasks.ai_pr_review.AiPrReview"], "apply_async" - ) - enriched_pull = EnrichedPull( - database_pull=pull, - provider_pull=dict(base=dict(branch="lookatthis"), labels=["ai-pr-review"]), - ) - current_yaml = UserYaml.from_dict( - { - "ai_pr_review": { - "enabled": True, - "method": "label", - "label_name": "ai-pr-review", - } +def test_trigger_ai_pr_review( + dbsession, mocker, repository, base_commit, head_commit, pull +): + pullid = pull.pullid + base_commit.pullid = pullid + head_commit.pullid = pullid + dbsession.add(pull) + dbsession.add(base_commit) + dbsession.add(head_commit) + dbsession.flush() + task = PullSyncTask() + apply_async = mocker.patch.object( + task.app.tasks["app.tasks.ai_pr_review.AiPrReview"], "apply_async" + ) + enriched_pull = EnrichedPull( + database_pull=pull, + provider_pull=dict(base=dict(branch="lookatthis"), labels=["ai-pr-review"]), + ) + current_yaml = UserYaml.from_dict( + { + "ai_pr_review": { + "enabled": True, + "method": "label", + "label_name": "ai-pr-review", } - ) - task.trigger_ai_pr_review(enriched_pull, current_yaml) - apply_async.assert_called_once_with( - kwargs=dict(repoid=pull.repoid, pullid=pull.pullid) - ) + } + ) + task.trigger_ai_pr_review(enriched_pull, current_yaml) + apply_async.assert_called_once_with( + kwargs=dict(repoid=pull.repoid, pullid=pull.pullid) + ) - apply_async.reset_mock() - current_yaml = UserYaml.from_dict( - { - "ai_pr_review": { - "enabled": True, - } + apply_async.reset_mock() + current_yaml = UserYaml.from_dict( + { + "ai_pr_review": { + "enabled": True, } - ) - task.trigger_ai_pr_review(enriched_pull, current_yaml) - apply_async.assert_called_once_with( - kwargs=dict(repoid=pull.repoid, pullid=pull.pullid) - ) + } + ) + task.trigger_ai_pr_review(enriched_pull, current_yaml) + apply_async.assert_called_once_with( + kwargs=dict(repoid=pull.repoid, pullid=pull.pullid) + ) - apply_async.reset_mock() - current_yaml = UserYaml.from_dict( - { - "ai_pr_review": { - "enabled": True, - "method": "auto", - } + apply_async.reset_mock() + current_yaml = UserYaml.from_dict( + { + "ai_pr_review": { + "enabled": True, + "method": "auto", } - ) - task.trigger_ai_pr_review(enriched_pull, current_yaml) - apply_async.assert_called_once_with( - kwargs=dict(repoid=pull.repoid, pullid=pull.pullid) - ) + } + ) + task.trigger_ai_pr_review(enriched_pull, current_yaml) + apply_async.assert_called_once_with( + kwargs=dict(repoid=pull.repoid, pullid=pull.pullid) + ) - apply_async.reset_mock() - current_yaml = UserYaml.from_dict( - { - "ai_pr_review": { - "enabled": True, - "method": "label", - "label_name": "other", - } + apply_async.reset_mock() + current_yaml = UserYaml.from_dict( + { + "ai_pr_review": { + "enabled": True, + "method": "label", + "label_name": "other", } - ) - task.trigger_ai_pr_review(enriched_pull, current_yaml) - assert not apply_async.called - - @pytest.mark.parametrize( - "flake_detection", [None, "FLAKY_TEST_DETECTION", "FLAKY_SHADOW_MODE"] + } ) - def test_trigger_process_flakes(self, dbsession, mocker, flake_detection): - current_yaml = UserYaml.from_dict(dict()) - if flake_detection: - mock_feature = mocker.patch(f"services.test_results.{flake_detection}") - mock_feature.check_value.return_value = True + task.trigger_ai_pr_review(enriched_pull, current_yaml) + assert not apply_async.called - if flake_detection == "FLAKY_TEST_DETECTION": - current_yaml = UserYaml.from_dict( - { - "test_analytics": { - "flake_detection": flake_detection, - } + +@pytest.mark.parametrize( + "flake_detection", [None, "FLAKY_TEST_DETECTION", "FLAKY_SHADOW_MODE"] +) +def test_trigger_process_flakes(dbsession, mocker, flake_detection, repository): + current_yaml = UserYaml.from_dict(dict()) + if flake_detection: + mock_feature = mocker.patch(f"services.test_results.{flake_detection}") + mock_feature.check_value.return_value = True + + if flake_detection == "FLAKY_TEST_DETECTION": + current_yaml = UserYaml.from_dict( + { + "test_analytics": { + "flake_detection": flake_detection, } - ) + } + ) - repository = RepositoryFactory.create() - dbsession.add(repository) - dbsession.flush() - commit = CommitFactory.create(repository=repository) - dbsession.add(commit) - dbsession.flush() + commit = CommitFactory.create(repository=repository) + dbsession.add(commit) + dbsession.flush() - dbsession.flush() - task = PullSyncTask() + dbsession.flush() + task = PullSyncTask() - apply_async: MagicMock = mocker.patch.object( - task.app.tasks["app.tasks.flakes.ProcessFlakesTask"], "apply_async" - ) + apply_async: MagicMock = mocker.patch.object( + task.app.tasks["app.tasks.flakes.ProcessFlakesTask"], "apply_async" + ) - task.trigger_process_flakes( - repository.repoid, - commit.commitid, - "main", - current_yaml, - ) - if flake_detection: - apply_async.assert_called_once_with( - kwargs=dict( - repo_id=repository.repoid, - commit_id_list=[commit.commitid], - branch="main", - ) + task.trigger_process_flakes( + repository.repoid, + commit.commitid, + "main", + current_yaml, + ) + if flake_detection: + apply_async.assert_called_once_with( + kwargs=dict( + repo_id=repository.repoid, + commit_id_list=[commit.commitid], + branch="main", ) - else: - apply_async.assert_not_called() + ) + else: + apply_async.assert_not_called() From 8d85121d035f3d8305a3159d47536eeae2802564 Mon Sep 17 00:00:00 2001 From: Michelle Tran Date: Thu, 17 Oct 2024 15:55:21 -0400 Subject: [PATCH 2/2] Explicitly set pullid Because pullid=1 was removed from the CommitFactory (due to unintended consequences of creating a Pull object), we now need to be explicit about setting it in a few places. --- tasks/tests/unit/test_commit_update.py | 1 + tasks/tests/unit/test_upload_task.py | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/tasks/tests/unit/test_commit_update.py b/tasks/tests/unit/test_commit_update.py index be8a6f3ff..e51fb41df 100644 --- a/tasks/tests/unit/test_commit_update.py +++ b/tasks/tests/unit/test_commit_update.py @@ -31,6 +31,7 @@ def test_update_commit( repository__owner__service="github", repository__owner__service_id="104562106", repository__name="test_example", + pullid=1, ) dbsession.add(commit) dbsession.flush() diff --git a/tasks/tests/unit/test_upload_task.py b/tasks/tests/unit/test_upload_task.py index eda92ad40..3dd42115f 100644 --- a/tasks/tests/unit/test_upload_task.py +++ b/tasks/tests/unit/test_upload_task.py @@ -144,6 +144,7 @@ def test_upload_task_call( repository__owner__service="github", repository__yaml={"codecov": {"max_report_age": "1y ago"}}, repository__name="example-python", + pullid=1, ) dbsession.add(commit) dbsession.flush() @@ -244,6 +245,7 @@ def test_upload_task_call_bundle_analysis( repository__owner__service="github", repository__yaml={"codecov": {"max_report_age": "1y ago"}}, repository__name="example-python", + pullid=1, ) dbsession.add(commit) dbsession.flush() @@ -309,6 +311,7 @@ def test_upload_task_call_bundle_analysis_no_upload( repository__owner__service="github", repository__yaml={"codecov": {"max_report_age": "1y ago"}}, repository__name="example-python", + pullid=1, ) dbsession.add(commit) dbsession.flush() @@ -374,6 +377,7 @@ def test_upload_task_call_test_results( repository__owner__service="github", repository__yaml={"codecov": {"max_report_age": "1y ago"}}, repository__name="example-python", + pullid=1, ) dbsession.add(commit) dbsession.flush() @@ -439,6 +443,7 @@ def test_upload_task_call_no_jobs( repository__owner__username="ThiagoCodecov", repository__yaml={"codecov": {"max_report_age": "1y ago"}}, repository__name="example-python", + pullid=1, ) dbsession.add(commit) dbsession.flush() @@ -483,6 +488,7 @@ def test_upload_task_upload_processing_delay_not_enough_delay( repository__owner__username="ThiagoCodecov", repository__yaml={"codecov": {"max_report_age": "1y ago"}}, repository__name="example-python", + pullid=1, ) dbsession.add(commit) dbsession.flush() @@ -521,6 +527,7 @@ def test_upload_task_upload_processing_delay_enough_delay( repository__owner__username="ThiagoCodecov", repository__yaml={"codecov": {"max_report_age": "1y ago"}}, repository__name="example-python", + pullid=1, ) mock_redis.keys[f"latest_upload/{commit.repoid}/{commit.commitid}"] = ( datetime.now() - timedelta(seconds=1200) @@ -574,6 +581,7 @@ def test_upload_task_upload_processing_delay_upload_is_none( repository__owner__username="ThiagoCodecov", repository__yaml={"codecov": {"max_report_age": "1y ago"}}, repository__name="example-python", + pullid=1, ) dbsession.add(commit) dbsession.flush() @@ -620,6 +628,7 @@ def test_upload_task_call_multiple_processors( repository__owner__service="github", repository__yaml={"codecov": {"max_report_age": "1y ago"}}, repository__name="example-python", + pullid=1, ) dbsession.add(commit) dbsession.flush() @@ -718,12 +727,14 @@ def test_upload_task_proper_parent( message="", commitid="c5b67303452bbff57cc1f49984339cde39eb1db5", repository=repo, + pullid=1, ) commit = CommitFactory.create( message="", commitid="abf6d4df662c47e32460020ab14abf9303581429", repository=repo, + pullid=1, ) dbsession.add(parent_commit) dbsession.add(commit)