From eeed1d759fa8a3d3893b21e2e4d3dc9f52048b44 Mon Sep 17 00:00:00 2001 From: Cameron Dawson Date: Fri, 22 Nov 2019 10:19:04 -0800 Subject: [PATCH] Bug 1597835 - Refactor Push Health to ease adding new metric support (#5661) The push_health.py file is now 'tests.py' where we will add 'linting.py', etc. Changed a couple function names to make them a little more explicit. * Change the 'metrics' field in the result to an object instead of an array. This way the UI can ask for each metric more easily. It'd rather have the UI decide what order to display the metrics than the API. This also makes the 'Metric.jsx' component simpler and just control expand / collapse and display of basic pass / fail. --- .../commands/cache_failure_history.py | 6 +- .../push_health/{push_health.py => tests.py} | 6 +- treeherder/webapp/api/push.py | 36 ++++----- ui/push-health/Health.jsx | 59 ++++++++------ ui/push-health/Metric.jsx | 47 +---------- ui/push-health/TestFailures.jsx | 78 ++++++++++--------- 6 files changed, 103 insertions(+), 129 deletions(-) rename treeherder/push_health/{push_health.py => tests.py} (97%) diff --git a/treeherder/model/management/commands/cache_failure_history.py b/treeherder/model/management/commands/cache_failure_history.py index ddf1ae007..4235a513e 100644 --- a/treeherder/model/management/commands/cache_failure_history.py +++ b/treeherder/model/management/commands/cache_failure_history.py @@ -3,9 +3,9 @@ import datetime from django.core.management.base import BaseCommand from treeherder.model.models import OptionCollection -from treeherder.push_health.push_health import (fixed_by_commit_history_days, - get_history, - intermittent_history_days) +from treeherder.push_health.tests import (fixed_by_commit_history_days, + get_history, + intermittent_history_days) from treeherder.webapp.api.utils import REPO_GROUPS diff --git a/treeherder/push_health/push_health.py b/treeherder/push_health/tests.py similarity index 97% rename from treeherder/push_health/push_health.py rename to treeherder/push_health/tests.py index 3fb115937..b22f254dc 100644 --- a/treeherder/push_health/push_health.py +++ b/treeherder/push_health/tests.py @@ -79,7 +79,7 @@ def get_history(failure_classification_id, push_date, num_days, option_map, repo # though the job failed, we count it as a 'pass' for the test in question. This 'pass' is # used for the pass/fail ratio on the test to help determine if it is intermittent. # -def get_push_failures(push, option_map): +def get_current_test_failures(push, option_map): all_testfailed = Job.objects.filter( push=push, tier__lte=2, @@ -168,7 +168,7 @@ def has_line(failure_line, log_line_list): return next((find_line for find_line in log_line_list if find_line['line_number'] == failure_line.line), False) -def get_push_health_test_failures(push, repository_ids): +def get_test_failures(push, repository_ids): # query for jobs for the last two weeks excluding today # find tests that have failed in the last 14 days # this is very cache-able for reuse on other pushes. @@ -186,7 +186,7 @@ def get_push_health_test_failures(push, repository_ids): fixed_by_commit_history_days, option_map, repository_ids) - push_failures, unsupported_jobs = get_push_failures(push, option_map) + push_failures, unsupported_jobs = get_current_test_failures(push, option_map) filtered_push_failures = [ failure for failure in push_failures if filter_failure(failure) ] diff --git a/treeherder/webapp/api/push.py b/treeherder/webapp/api/push.py index fb462c3e5..632356fa0 100644 --- a/treeherder/webapp/api/push.py +++ b/treeherder/webapp/api/push.py @@ -13,7 +13,7 @@ from treeherder.model.models import (Job, JobType, Push, Repository) -from treeherder.push_health.push_health import get_push_health_test_failures +from treeherder.push_health.tests import get_test_failures from treeherder.webapp.api.serializers import PushSerializer from treeherder.webapp.api.utils import (REPO_GROUPS, to_datetime, @@ -211,7 +211,7 @@ class PushViewSet(viewsets.ViewSet): except Push.DoesNotExist: return Response("No push with id: {0}".format(pk), status=HTTP_404_NOT_FOUND) - push_health_test_failures = get_push_health_test_failures(push, REPO_GROUPS['trunk']) + push_health_test_failures = get_test_failures(push, REPO_GROUPS['trunk']) return Response({'needInvestigation': len(push_health_test_failures['needInvestigation'])}) @@ -227,7 +227,7 @@ class PushViewSet(viewsets.ViewSet): except Push.DoesNotExist: return Response("No push with revision: {0}".format(revision), status=HTTP_404_NOT_FOUND) - push_health_test_failures = get_push_health_test_failures(push, REPO_GROUPS['trunk']) + push_health_test_failures = get_test_failures(push, REPO_GROUPS['trunk']) test_result = 'pass' if len(push_health_test_failures['unsupported']): test_result = 'indeterminate' @@ -238,37 +238,37 @@ class PushViewSet(viewsets.ViewSet): 'revision': revision, 'id': push.id, 'result': test_result, - 'metrics': [ - { + 'metrics': { + 'tests': { 'name': 'Tests', 'result': test_result, - 'failures': push_health_test_failures, + 'details': push_health_test_failures, }, - { + 'linting': { + 'name': 'Linting (Not yet implemented)', + 'result': 'none', + 'details': ['lint stuff would be good'], + }, + 'builds': { 'name': 'Builds (Not yet implemented)', - 'result': 'pass', + 'result': 'none', 'details': ['Wow, everything passed!'], }, - { - 'name': 'Linting (Not yet implemented)', - 'result': 'pass', - 'details': ['Gosh, this code is really nicely formatted.'], - }, - { + 'coverage': { 'name': 'Coverage (Not yet implemented)', - 'result': 'pass', + 'result': 'none', 'details': [ 'Covered 42% of the tests that are needed for feature ``foo``.', 'Covered 100% of the tests that are needed for feature ``bar``.', 'The ratio of people to cake is too many...', ], }, - { + 'performance': { 'name': 'Performance (Not yet implemented)', - 'result': 'pass', + 'result': 'none', 'details': ['Ludicrous Speed'], }, - ], + }, }) @cache_memoize(60 * 60) diff --git a/ui/push-health/Health.jsx b/ui/push-health/Health.jsx index 6e1611b26..178aa7cb9 100644 --- a/ui/push-health/Health.jsx +++ b/ui/push-health/Health.jsx @@ -14,6 +14,7 @@ import PushModel from '../models/push'; import { resultColorMap } from './helpers'; import Metric from './Metric'; import Navigation from './Navigation'; +import TestFailures from './TestFailures'; export default class Health extends React.PureComponent { constructor(props) { @@ -25,7 +26,8 @@ export default class Health extends React.PureComponent { user: { isLoggedIn: false }, revision: params.get('revision'), repo: params.get('repo'), - healthData: null, + metrics: {}, + result: null, failureMessage: null, notifications: [], }; @@ -55,9 +57,7 @@ export default class Health extends React.PureComponent { updatePushHealth = async () => { const { repo, revision } = this.state; const { data, failureStatus } = await PushModel.getHealth(repo, revision); - const newState = !failureStatus - ? { healthData: data } - : { failureMessage: data }; + const newState = !failureStatus ? data : { failureMessage: data }; this.setState(newState); }; @@ -85,17 +85,17 @@ export default class Health extends React.PureComponent { render() { const { - healthData, + metrics, + result, user, repo, revision, failureMessage, notifications, } = this.state; + const { tests, linting, builds, coverage, performance } = metrics; const { currentRepo } = this.props; - const overallResult = healthData - ? resultColorMap[healthData.result] - : 'none'; + const overallResult = result ? resultColorMap[result] : 'none'; return ( @@ -105,7 +105,7 @@ export default class Health extends React.PureComponent { notifications={notifications} clearNotification={this.clearNotification} /> - {healthData && ( + {!!tests && !!currentRepo && (

@@ -121,20 +121,25 @@ export default class Health extends React.PureComponent {

- {healthData.metrics.map(metric => ( - - + + + + {[linting, builds, coverage, performance].map(metric => ( + + +
+ {metric.details.map(detail => ( +
{detail}
+ ))} +
+ ))} @@ -142,7 +147,7 @@ export default class Health extends React.PureComponent { )} {failureMessage && } - {!failureMessage && !healthData && } + {!failureMessage && !tests && } ); @@ -151,5 +156,9 @@ export default class Health extends React.PureComponent { Health.propTypes = { location: PropTypes.object.isRequired, - currentRepo: PropTypes.object.isRequired, + currentRepo: PropTypes.object, +}; + +Health.defaultProps = { + currentRepo: null, }; diff --git a/ui/push-health/Metric.jsx b/ui/push-health/Metric.jsx index 1eb0062db..c0102c082 100644 --- a/ui/push-health/Metric.jsx +++ b/ui/push-health/Metric.jsx @@ -8,7 +8,6 @@ import { import { Button, Badge, Row, Col, Collapse, Card, CardBody } from 'reactstrap'; import { resultColorMap } from './helpers'; -import TestFailures from './TestFailures'; export default class Metric extends React.PureComponent { constructor(props) { @@ -17,7 +16,7 @@ export default class Metric extends React.PureComponent { const { result } = this.props; this.state = { - detailsShowing: result !== 'pass', + detailsShowing: !['pass', 'none'].includes(result), }; } @@ -27,17 +26,7 @@ export default class Metric extends React.PureComponent { render() { const { detailsShowing } = this.state; - const { - result, - name, - details, - failures, - repo, - revision, - user, - notify, - currentRepo, - } = this.props; + const { result, name, children } = this.props; const resultColor = resultColorMap[result]; const expandIcon = detailsShowing ? faMinusSquare : faPlusSquare; @@ -63,24 +52,7 @@ export default class Metric extends React.PureComponent { - - {name === 'Tests' && ( - - )} - {details && - details.map(detail => ( -
- {detail} -
- ))} - + {children} @@ -91,18 +63,7 @@ export default class Metric extends React.PureComponent { } Metric.propTypes = { - repo: PropTypes.string.isRequired, - currentRepo: PropTypes.object.isRequired, - revision: PropTypes.string.isRequired, result: PropTypes.string.isRequired, name: PropTypes.string.isRequired, - user: PropTypes.object.isRequired, - notify: PropTypes.func.isRequired, - details: PropTypes.array, - failures: PropTypes.object, -}; - -Metric.defaultProps = { - details: null, - failures: null, + children: PropTypes.object.isRequired, }; diff --git a/ui/push-health/TestFailures.jsx b/ui/push-health/TestFailures.jsx index 392f116ee..e15da2009 100644 --- a/ui/push-health/TestFailures.jsx +++ b/ui/push-health/TestFailures.jsx @@ -3,54 +3,58 @@ import PropTypes from 'prop-types'; import ClassificationGroup from './ClassificationGroup'; import UnsupportedGroup from './UnsupportedGroup'; +import Metric from './Metric'; export default class TestFailures extends React.PureComponent { render() { - const { failures, repo, revision, user, notify, currentRepo } = this.props; - const { needInvestigation, intermittent, unsupported } = failures; + const { data, repo, revision, user, notify, currentRepo } = this.props; + const { name, result, details } = data; + const { needInvestigation, intermittent, unsupported } = details; const needInvestigationLength = Object.keys(needInvestigation).length; return ( -
- - - -
+ +
+ + + +
+ ); } } TestFailures.propTypes = { - failures: PropTypes.object.isRequired, + data: PropTypes.object.isRequired, user: PropTypes.object.isRequired, repo: PropTypes.string.isRequired, currentRepo: PropTypes.object.isRequired,