From 212ad4a203c0e6b5c16cc15f74c206e04cbc8249 Mon Sep 17 00:00:00 2001 From: joseph-sentry Date: Tue, 3 Sep 2024 10:07:01 -0400 Subject: [PATCH 1/3] feat: turn on flaky read path for everyone --- services/test_results.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/services/test_results.py b/services/test_results.py index 326155484..5811a4c6f 100644 --- a/services/test_results.py +++ b/services/test_results.py @@ -408,6 +408,8 @@ def should_write_flaky_detection(repoid: int, commit_yaml: UserYaml) -> bool: def should_read_flaky_detection(repoid: int, commit_yaml: UserYaml) -> bool: - return FLAKY_TEST_DETECTION.check_value( - identifier=repoid, default=False - ) and read_yaml_field(commit_yaml, ("test_analytics", "flake_detection"), False) + return ( + FLAKY_TEST_DETECTION.check_value(identifier=repoid, default=False) + and read_yaml_field(commit_yaml, ("test_analytics", "flake_detection"), False) + or FLAKY_SHADOW_MODE.check_value(identifier=repoid, default=False) + ) From 83357e6016091f0f7808388e65bbd9dc760fb2b5 Mon Sep 17 00:00:00 2001 From: joseph-sentry Date: Tue, 3 Sep 2024 10:07:29 -0400 Subject: [PATCH 2/3] fix: call process flakes in sync pulls only when tests exist on repo --- database/tests/factories/reports.py | 13 ++++++++++- tasks/sync_pull.py | 10 ++++---- tasks/tests/unit/test_sync_pull.py | 36 ++++++++++++++++++++++++++--- 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/database/tests/factories/reports.py b/database/tests/factories/reports.py index f3ae16266..7adf59a86 100644 --- a/database/tests/factories/reports.py +++ b/database/tests/factories/reports.py @@ -1,6 +1,6 @@ import factory -from database.models.reports import CompareFlag, RepositoryFlag +from database.models.reports import CompareFlag, RepositoryFlag, Test from database.tests.factories.core import CompareCommitFactory, RepositoryFactory @@ -18,3 +18,14 @@ class Meta: commit_comparison = factory.SubFactory(CompareCommitFactory) repositoryflag = factory.SubFactory(RepositoryFlagFactory) + + +class TestFactory(factory.Factory): + class Meta: + model = Test + + name = factory.Sequence(lambda n: f"test_{n}") + testsuite = "testsuite" + flags_hash = "flags_hash" + id_ = factory.Sequence(lambda n: f"id_{n}") + repository = factory.SubFactory(RepositoryFactory) diff --git a/tasks/sync_pull.py b/tasks/sync_pull.py index 22508e469..23fdd64a9 100644 --- a/tasks/sync_pull.py +++ b/tasks/sync_pull.py @@ -17,7 +17,7 @@ from shared.yaml.user_yaml import OwnerContext from app import celery_app -from database.models import Commit, Pull, Repository +from database.models import Commit, Pull, Repository, Test from helpers.exceptions import RepositoryWithoutValidBotError from helpers.github_installation import get_installation_name_for_owner_for_task from helpers.metrics import metrics @@ -377,9 +377,11 @@ def update_pull_commits( synchronize_session=False, ) ) - self.trigger_process_flakes( - repoid, pull.head, pull_dict["head"]["branch"], current_yaml - ) + + if db_session.query(Test).filter(Test.repoid == repoid).count() > 0: + self.trigger_process_flakes( + repoid, pull.head, pull_dict["head"]["branch"], current_yaml + ) # set the rest of the commits to deleted (do not show in the UI) deleted_count = ( diff --git a/tasks/tests/unit/test_sync_pull.py b/tasks/tests/unit/test_sync_pull.py index 9f3eae829..b28dacdff 100644 --- a/tasks/tests/unit/test_sync_pull.py +++ b/tasks/tests/unit/test_sync_pull.py @@ -9,6 +9,7 @@ from shared.torngit.exceptions import TorngitClientError from database.tests.factories import CommitFactory, PullFactory, RepositoryFactory +from database.tests.factories.reports import TestFactory from helpers.exceptions import RepositoryWithoutValidBotError from services.repository import EnrichedPull from services.yaml import UserYaml @@ -18,15 +19,44 @@ class TestPullSyncTask(object): - @pytest.mark.parametrize("flake_detection", [False, True]) - def test_update_pull_commits_merged(self, dbsession, mocker, flake_detection): + @pytest.mark.parametrize( + "flake_detection,flaky_shadow_mode,tests_exist,outcome", + [ + (False, False, False, False), + (False, True, False, False), + (True, False, False, False), + (False, True, True, True), + (True, True, True, True), + ], + ) + def test_update_pull_commits_merged( + self, + dbsession, + mocker, + 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 + 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( @@ -89,7 +119,7 @@ def test_update_pull_commits_merged(self, dbsession, mocker, flake_detection): mock_repo_provider, enriched_pull, commits, commits_at_base, current_yaml ) - if flake_detection: + if outcome: apply_async.assert_called_once_with( kwargs=dict( repo_id=repository.repoid, From 4e2ba9bfafb1e9000b733c87d3d48fc29f5c1833 Mon Sep 17 00:00:00 2001 From: joseph-sentry Date: Tue, 3 Sep 2024 12:27:14 -0400 Subject: [PATCH 3/3] fix: redefine should_read|write_flaky_tests we should write if: FLAKY_SHADOW_MODE or FLAKE_DETECTION feature flags are enabled AND the user has not disabled flake detection via the config we should read if: FLAKE_DETECTION feature flag is enabled AND the user has not disabled flake detection via the config --- services/test_results.py | 11 +++---- tasks/tests/unit/test_sync_pull.py | 16 ++++++---- .../tests/unit/test_test_results_finisher.py | 32 ++++++++++++------- 3 files changed, 33 insertions(+), 26 deletions(-) diff --git a/services/test_results.py b/services/test_results.py index 5811a4c6f..d934096f9 100644 --- a/services/test_results.py +++ b/services/test_results.py @@ -402,14 +402,11 @@ def latest_test_instances_for_a_given_commit(db_session, commit_id): def should_write_flaky_detection(repoid: int, commit_yaml: UserYaml) -> bool: return ( FLAKY_TEST_DETECTION.check_value(identifier=repoid, default=False) - and read_yaml_field(commit_yaml, ("test_analytics", "flake_detection"), False) or FLAKY_SHADOW_MODE.check_value(identifier=repoid, default=False) - ) + ) and read_yaml_field(commit_yaml, ("test_analytics", "flake_detection"), True) def should_read_flaky_detection(repoid: int, commit_yaml: UserYaml) -> bool: - return ( - FLAKY_TEST_DETECTION.check_value(identifier=repoid, default=False) - and read_yaml_field(commit_yaml, ("test_analytics", "flake_detection"), False) - or FLAKY_SHADOW_MODE.check_value(identifier=repoid, default=False) - ) + return FLAKY_TEST_DETECTION.check_value( + identifier=repoid, default=False + ) and read_yaml_field(commit_yaml, ("test_analytics", "flake_detection"), True) diff --git a/tasks/tests/unit/test_sync_pull.py b/tasks/tests/unit/test_sync_pull.py index b28dacdff..614cd9065 100644 --- a/tasks/tests/unit/test_sync_pull.py +++ b/tasks/tests/unit/test_sync_pull.py @@ -20,19 +20,21 @@ class TestPullSyncTask(object): @pytest.mark.parametrize( - "flake_detection,flaky_shadow_mode,tests_exist,outcome", + "flake_detection_config, flake_detection,flaky_shadow_mode,tests_exist,outcome", [ - (False, False, False, False), - (False, True, False, False), - (True, False, False, False), - (False, True, True, True), - (True, True, True, True), + (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, @@ -108,7 +110,7 @@ def test_update_pull_commits_merged( current_yaml = UserYaml.from_dict( { "test_analytics": { - "flake_detection": flake_detection, + "flake_detection": flake_detection_config, } } ) diff --git a/tasks/tests/unit/test_test_results_finisher.py b/tasks/tests/unit/test_test_results_finisher.py index 2032e27e8..9b20970a1 100644 --- a/tasks/tests/unit/test_test_results_finisher.py +++ b/tasks/tests/unit/test_test_results_finisher.py @@ -903,7 +903,7 @@ def test_upload_finisher_task_call_with_flaky( @pytest.mark.integration @pytest.mark.parametrize( - "flake_detection", ["FLAKY_TEST_DETECTION", "FLAKY_SHADOW_MODE"] + "flake_detection", [None, "FLAKY_TEST_DETECTION", "FLAKY_SHADOW_MODE"] ) def test_upload_finisher_task_call_main_branch( self, @@ -919,13 +919,16 @@ def test_upload_finisher_task_call_main_branch( test_results_setup, flake_detection, ): - mock_feature = mocker.patch(f"services.test_results.{flake_detection}") - mock_feature.check_value.return_value = True + if flake_detection: + mock_feature = mocker.patch(f"services.test_results.{flake_detection}") + mock_feature.check_value.return_value = True commit_yaml = { "codecov": {"max_report_age": False}, } if flake_detection == "FLAKY_TEST_DETECTION": commit_yaml["test_analytics"] = {"flake_detection": True} + elif flake_detection is None: + commit_yaml["test_analytics"] = {"flake_detection": False} repoid, commit, pull, test_instances = test_results_setup @@ -949,12 +952,17 @@ def test_upload_finisher_task_call_main_branch( assert expected_result == result - test_results_mock_app.tasks[ - "app.tasks.flakes.ProcessFlakesTask" - ].apply_async.assert_called_with( - kwargs={ - "repo_id": repoid, - "commit_id_list": [commit.commitid], - "branch": "main", - }, - ) + if flake_detection is None: + test_results_mock_app.tasks[ + "app.tasks.flakes.ProcessFlakesTask" + ].apply_async.assert_not_called() + else: + test_results_mock_app.tasks[ + "app.tasks.flakes.ProcessFlakesTask" + ].apply_async.assert_called_with( + kwargs={ + "repo_id": repoid, + "commit_id_list": [commit.commitid], + "branch": "main", + }, + )