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.

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,
+from treeherder.push_health.tests import (fixed_by_commit_history_days,
-                                                get_history,
+                                          get_history,
-                                                intermittent_history_days)
+                                          intermittent_history_days)
 from treeherder.webapp.api.utils import REPO_GROUPS
 
 

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)
     ]

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!'],
                 },
-                {
+                'coverage': {
-                    'name': 'Linting (Not yet implemented)',
-                    'result': 'pass',
-                    'details': ['Gosh, this code is really nicely formatted.'],
-                },
-                {
                     '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)

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
+    const newState = !failureStatus ? data : { failureMessage: data };
-      ? { healthData: 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
+    const overallResult = result ? resultColorMap[result] : 'none';
-      ? resultColorMap[healthData.result]
-      : 'none';
 
     return (
       <React.Fragment>
@@ -105,7 +105,7 @@ export default class Health extends React.PureComponent {
             notifications={notifications}
             clearNotification={this.clearNotification}
           />
-          {healthData && (
+          {!!tests && !!currentRepo && (
             <div className="d-flex flex-column">
               <h3 className="text-center">
                 <span className={`badge badge-xl mb-3 badge-${overallResult}`}>
@@ -121,20 +121,25 @@ export default class Health extends React.PureComponent {
               </h3>
               <Table size="sm" className="table-fixed">
                 <tbody>
-                  {healthData.metrics.map(metric => (
+                  <tr>
-                    <tr key={metric.name}>
+                    <TestFailures
-                      <Metric
+                      data={tests}
-                        name={metric.name}
+                      repo={repo}
-                        result={metric.result}
+                      currentRepo={currentRepo}
-                        value={metric.value}
+                      revision={revision}
-                        details={metric.details}
+                      user={user}
-                        failures={metric.failures}
+                      notify={this.notify}
-                        repo={repo}
+                    />
-                        currentRepo={currentRepo}
+                  </tr>
-                        revision={revision}
+                  {[linting, builds, coverage, performance].map(metric => (
-                        user={user}
+                    <tr>
-                        notify={this.notify}
+                      <Metric result={metric.result} name={metric.name}>
-                      />
+                        <div>
+                          {metric.details.map(detail => (
+                            <div>{detail}</div>
+                          ))}
+                        </div>
+                      </Metric>
                     </tr>
                   ))}
                 </tbody>
@@ -142,7 +147,7 @@ export default class Health extends React.PureComponent {
             </div>
           )}
           {failureMessage && <ErrorMessages failureMessage={failureMessage} />}
-          {!failureMessage && !healthData && <Spinner />}
+          {!failureMessage && !tests && <Spinner />}
         </Container>
       </React.Fragment>
     );
@@ -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,
 };

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 {
+    const { result, name, children } = this.props;
-      result,
-      name,
-      details,
-      failures,
-      repo,
-      revision,
-      user,
-      notify,
-      currentRepo,
-    } = this.props;
     const resultColor = resultColorMap[result];
     const expandIcon = detailsShowing ? faMinusSquare : faPlusSquare;
 
@@ -63,24 +52,7 @@ export default class Metric extends React.PureComponent {
             </Row>
             <Collapse isOpen={detailsShowing}>
               <Card>
-                <CardBody>
+                <CardBody>{children}</CardBody>
-                  {name === 'Tests' && (
-                    <TestFailures
-                      failures={failures}
-                      repo={repo}
-                      currentRepo={currentRepo}
-                      revision={revision}
-                      user={user}
-                      notify={notify}
-                    />
-                  )}
-                  {details &&
-                    details.map(detail => (
-                      <div key={detail} className="ml-3">
-                        {detail}
-                      </div>
-                    ))}
-                </CardBody>
               </Card>
             </Collapse>
           </Col>
@@ -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,
+  children: PropTypes.object.isRequired,
-  notify: PropTypes.func.isRequired,
-  details: PropTypes.array,
-  failures: PropTypes.object,
-};
-
-Metric.defaultProps = {
-  details: null,
-  failures: null,
 };

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 { data, repo, revision, user, notify, currentRepo } = this.props;
-    const { needInvestigation, intermittent, unsupported } = failures;
+    const { name, result, details } = data;
+    const { needInvestigation, intermittent, unsupported } = details;
     const needInvestigationLength = Object.keys(needInvestigation).length;
 
     return (
-      <div className="border-bottom border-secondary">
+      <Metric name={name} result={result}>
-        <ClassificationGroup
+        <div className="border-bottom border-secondary">
-          group={needInvestigation}
+          <ClassificationGroup
-          name="Need Investigation"
+            group={needInvestigation}
-          repo={repo}
+            name="Need Investigation"
-          currentRepo={currentRepo}
+            repo={repo}
-          revision={revision}
+            currentRepo={currentRepo}
-          className="mb-5"
+            revision={revision}
-          headerColor={needInvestigationLength ? 'danger' : 'secondary'}
+            className="mb-5"
-          user={user}
+            headerColor={needInvestigationLength ? 'danger' : 'secondary'}
-          hasRetriggerAll
+            user={user}
-          notify={notify}
+            hasRetriggerAll
-        />
+            notify={notify}
-        <ClassificationGroup
+          />
-          group={intermittent}
+          <ClassificationGroup
-          name="Known Intermittent"
+            group={intermittent}
-          repo={repo}
+            name="Known Intermittent"
-          currentRepo={currentRepo}
+            repo={repo}
-          revision={revision}
+            currentRepo={currentRepo}
-          className="mb-5"
+            revision={revision}
-          headerColor="secondary"
+            className="mb-5"
-          expanded={false}
+            headerColor="secondary"
-          user={user}
+            expanded={false}
-          notify={notify}
+            user={user}
-        />
+            notify={notify}
-        <UnsupportedGroup
+          />
-          group={unsupported}
+          <UnsupportedGroup
-          name="Unsupported"
+            group={unsupported}
-          repo={repo}
+            name="Unsupported"
-          revision={revision}
+            repo={repo}
-          className="mb-5"
+            revision={revision}
-          headerColor="warning"
+            className="mb-5"
-        />
+            headerColor="warning"
-      </div>
+          />
+        </div>
+      </Metric>
     );
   }
 }
 
 TestFailures.propTypes = {
-  failures: PropTypes.object.isRequired,
+  data: PropTypes.object.isRequired,
   user: PropTypes.object.isRequired,
   repo: PropTypes.string.isRequired,
   currentRepo: PropTypes.object.isRequired,