From 50e88d129268c82493d9540683fadf53c478b055 Mon Sep 17 00:00:00 2001 From: ionutgoldan Date: Tue, 26 Jan 2021 09:52:45 +0200 Subject: [PATCH] Bug 1680433 - Fix wrong pushlog from alert dropdown * pick the correct previous push * add backend validation * provide test coverage for it * ensure data points maintain chronological order --- .../api/test_performance_alertsummary_api.py | 50 ++++++++++++++++--- tests/webapp/api/test_performance_data_api.py | 22 ++++++++ treeherder/webapp/api/performance_data.py | 5 ++ ui/perfherder/graphs/GraphTooltip.jsx | 11 ++-- 4 files changed, 78 insertions(+), 10 deletions(-) diff --git a/tests/webapp/api/test_performance_alertsummary_api.py b/tests/webapp/api/test_performance_alertsummary_api.py index 1deefb46e..101290041 100644 --- a/tests/webapp/api/test_performance_alertsummary_api.py +++ b/tests/webapp/api/test_performance_alertsummary_api.py @@ -209,7 +209,7 @@ def test_alert_summaries_put( assert PerformanceAlertSummary.objects.get(id=1).assignee == test_user -def test_alert_summary_post( +def test_auth_for_alert_summary_post( client, test_repository, test_issue_tracker, @@ -218,8 +218,6 @@ def test_alert_summary_post( test_user, test_sheriff, ): - # this blob should be sufficient to create a new alert summary (assuming - # the user of this API is authorized to do so!) post_blob = { 'repository_id': test_repository.id, 'framework_id': test_perf_signature.framework.id, @@ -238,9 +236,25 @@ def test_alert_summary_post( assert resp.status_code == 403 assert PerformanceAlertSummary.objects.count() == 0 + +def test_alert_summary_post( + authorized_sheriff_client, + test_repository, + test_issue_tracker, + push_stored, + test_perf_signature, + test_user, + test_sheriff, +): + post_blob = { + 'repository_id': test_repository.id, + 'framework_id': test_perf_signature.framework.id, + 'prev_push_id': 1, + 'push_id': 2, + } + # verify that we succeed if authenticated + staff - client.force_authenticate(user=test_sheriff) - resp = client.post(reverse('performance-alert-summaries-list'), post_blob) + resp = authorized_sheriff_client.post(reverse('performance-alert-summaries-list'), post_blob) assert resp.status_code == 200 assert PerformanceAlertSummary.objects.count() == 1 @@ -253,11 +267,35 @@ def test_alert_summary_post( # verify that we don't create a new performance alert summary if one # already exists (but also don't throw an error) - resp = client.post(reverse('performance-alert-summaries-list'), post_blob) + resp = authorized_sheriff_client.post(reverse('performance-alert-summaries-list'), post_blob) assert resp.status_code == 200 assert PerformanceAlertSummary.objects.count() == 1 +def test_push_range_validation_for_alert_summary_post( + authorized_sheriff_client, + test_repository, + test_issue_tracker, + push_stored, + test_perf_signature, + test_user, + test_sheriff, +): + identical_push = 1 + post_blob = { + 'repository_id': test_repository.id, + 'framework_id': test_perf_signature.framework.id, + 'prev_push_id': identical_push, + 'push_id': identical_push, + } + + # verify that we succeed if authenticated + staff + resp = authorized_sheriff_client.post(reverse('performance-alert-summaries-list'), post_blob) + assert resp.status_code == 400 + + assert PerformanceAlertSummary.objects.count() == 0 + + @pytest.mark.parametrize( 'modification', [{'notes': 'human created notes'}, {'bug_number': 123456, 'issue_tracker': 1}] ) diff --git a/tests/webapp/api/test_performance_data_api.py b/tests/webapp/api/test_performance_data_api.py index 8a6d8586b..dcf478315 100644 --- a/tests/webapp/api/test_performance_data_api.py +++ b/tests/webapp/api/test_performance_data_api.py @@ -523,6 +523,28 @@ def test_perf_summary(client, test_perf_signature, test_perf_data): assert resp2.json() == expected +def test_data_points_from_same_push_are_ordered_chronologically( + client, test_perf_signature, test_perf_data +): + """ + The chronological order for data points associated to a single push + is based upon the order of their related job. If related jobs are + ordered, the data points are considered ordered. + + As job ids are auto incremented, older jobs have smaller ids than newer ones. + Thus, these ids are sufficient to check for chronological order. + """ + query_params = '?repository={}&framework={}&interval=172800&no_subtests=true&startday=2013-11-01T23%3A28%3A29&endday=2013-11-30T23%3A28%3A29'.format( + test_perf_signature.repository.name, test_perf_signature.framework_id + ) + + response = client.get(reverse('performance-summary') + query_params) + assert response.status_code == 200 + + job_ids = response.json()[0]['job_ids'] + assert job_ids == sorted(job_ids) + + def test_no_retriggers_perf_summary( client, push_stored, test_perf_signature, test_perf_signature_2, test_perf_data ): diff --git a/treeherder/webapp/api/performance_data.py b/treeherder/webapp/api/performance_data.py index 56f71fb7b..6184ed75f 100644 --- a/treeherder/webapp/api/performance_data.py +++ b/treeherder/webapp/api/performance_data.py @@ -457,6 +457,11 @@ class PerformanceAlertSummaryViewSet(viewsets.ModelViewSet): def create(self, request, *args, **kwargs): data = request.data + if data['push_id'] == data['prev_push_id']: + return Response( + "IDs of push & previous push cannot be identical", status=HTTP_400_BAD_REQUEST + ) + alert_summary, _ = PerformanceAlertSummary.objects.get_or_create( repository_id=data['repository_id'], framework=PerformanceFramework.objects.get(id=data['framework_id']), diff --git a/ui/perfherder/graphs/GraphTooltip.jsx b/ui/perfherder/graphs/GraphTooltip.jsx index 8ae2d508a..4f0e35d2f 100644 --- a/ui/perfherder/graphs/GraphTooltip.jsx +++ b/ui/perfherder/graphs/GraphTooltip.jsx @@ -80,13 +80,16 @@ const GraphTooltip = ({ let prevRevision; let prevPushId; let pushUrl; - const firstTriggerIndex = testDetails.data.findIndex( + const originalDataPointIdx = testDetails.data.findIndex( (e) => e.revision === dataPointDetails.revision, ); - if (prevFlotDataPointIndex !== -1 && firstTriggerIndex > 0) { - prevRevision = testDetails.data[firstTriggerIndex - 1].revision; - prevPushId = testDetails.data[prevFlotDataPointIndex].pushId; + + if (prevFlotDataPointIndex !== -1 && originalDataPointIdx > 0) { + const prevDataPointIdx = originalDataPointIdx - 1; const repoModel = new RepositoryModel(repositoryName); + + prevRevision = testDetails.data[prevDataPointIdx].revision; + prevPushId = testDetails.data[prevDataPointIdx].pushId; pushUrl = repoModel.getPushLogRangeHref({ fromchange: prevRevision, tochange: dataPointDetails.revision,