From 85bdcbdd435d388479ad4496aaafd0403779410c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Mar 2012 17:06:55 -0700 Subject: [PATCH] Show coverage percentages in table of contents Summary: Rough cut -- this needs style / color / tooltips, etc. Show raw coverage and "modified coverage" (coverage on lines you touched) in the table of contents. https://secure.phabricator.com/file/data/id3apce5p5gevkee6tg2/PHID-FILE-kxcxlbsej454t4xiht2o/Screen_Shot_2012-03-12_at_3.30.30_PM.png Test Plan: See screenshot above. Reviewers: tuomaspelkonen, btrahan, zeeg Reviewed By: tuomaspelkonen CC: aran, epriestley Maniphest Tasks: T965 Differential Revision: https://secure.phabricator.com/D1864 --- src/__celerity_resource_map__.php | 108 +++++++++--------- .../DifferentialChangesetViewController.php | 10 +- .../DifferentialRevisionViewController.php | 2 + .../changeset/DifferentialChangesetParser.php | 45 ++++++++ .../DifferentialDiffTableOfContentsView.php | 64 +++++++++++ .../view/difftableofcontents/__init__.php | 2 + .../differential/table-of-contents.css | 31 +++++ .../differential/behavior-populate.js | 11 +- 8 files changed, 217 insertions(+), 56 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 112f78e245..2bd974dff5 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -250,7 +250,7 @@ celerity_register_resource_map(array( ), 'differential-table-of-contents-css' => array( - 'uri' => '/res/e4c089fe/rsrc/css/application/differential/table-of-contents.css', + 'uri' => '/res/a633259f/rsrc/css/application/differential/table-of-contents.css', 'type' => 'css', 'requires' => array( @@ -576,7 +576,7 @@ celerity_register_resource_map(array( ), 'javelin-behavior-differential-populate' => array( - 'uri' => '/res/6efe5cd2/rsrc/js/application/differential/behavior-populate.js', + 'uri' => '/res/3c430bff/rsrc/js/application/differential/behavior-populate.js', 'type' => 'js', 'requires' => array( @@ -1917,27 +1917,6 @@ celerity_register_resource_map(array( ), array( 'packages' => array( - '09c86840' => - array( - 'name' => 'differential.pkg.css', - 'symbols' => - array( - 0 => 'differential-core-view-css', - 1 => 'differential-changeset-view-css', - 2 => 'differential-revision-detail-css', - 3 => 'differential-revision-history-css', - 4 => 'differential-table-of-contents-css', - 5 => 'differential-revision-comment-css', - 6 => 'differential-revision-add-comment-css', - 7 => 'differential-revision-comment-list-css', - 8 => 'phabricator-object-selector-css', - 9 => 'aphront-headsup-action-list-view-css', - 10 => 'phabricator-content-source-view-css', - 11 => 'differential-local-commits-view-css', - ), - 'uri' => '/res/pkg/09c86840/differential.pkg.css', - 'type' => 'css', - ), '2af849fb' => array( 'name' => 'typeahead.pkg.js', @@ -2024,7 +2003,28 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/86fc0b0c/maniphest.pkg.js', 'type' => 'js', ), - 'e8b28c4a' => + '9d02b654' => + array( + 'name' => 'differential.pkg.css', + 'symbols' => + array( + 0 => 'differential-core-view-css', + 1 => 'differential-changeset-view-css', + 2 => 'differential-revision-detail-css', + 3 => 'differential-revision-history-css', + 4 => 'differential-table-of-contents-css', + 5 => 'differential-revision-comment-css', + 6 => 'differential-revision-add-comment-css', + 7 => 'differential-revision-comment-list-css', + 8 => 'phabricator-object-selector-css', + 9 => 'aphront-headsup-action-list-view-css', + 10 => 'phabricator-content-source-view-css', + 11 => 'differential-local-commits-view-css', + ), + 'uri' => '/res/pkg/9d02b654/differential.pkg.css', + 'type' => 'css', + ), + 'ffca9dae' => array( 'name' => 'differential.pkg.js', 'symbols' => @@ -2047,7 +2047,7 @@ celerity_register_resource_map(array( 15 => 'javelin-behavior-differential-dropdown-menus', 16 => 'javelin-behavior-buoyant', ), - 'uri' => '/res/pkg/e8b28c4a/differential.pkg.js', + 'uri' => '/res/pkg/ffca9dae/differential.pkg.js', 'type' => 'js', ), 31583232 => @@ -2092,7 +2092,7 @@ celerity_register_resource_map(array( 'aphront-crumbs-view-css' => '78e8854e', 'aphront-dialog-view-css' => '78e8854e', 'aphront-form-view-css' => '78e8854e', - 'aphront-headsup-action-list-view-css' => '09c86840', + 'aphront-headsup-action-list-view-css' => '9d02b654', 'aphront-list-filter-view-css' => '78e8854e', 'aphront-pager-view-css' => '78e8854e', 'aphront-panel-view-css' => '78e8854e', @@ -2100,40 +2100,40 @@ celerity_register_resource_map(array( 'aphront-table-view-css' => '78e8854e', 'aphront-tokenizer-control-css' => '78e8854e', 'aphront-typeahead-control-css' => '78e8854e', - 'differential-changeset-view-css' => '09c86840', - 'differential-core-view-css' => '09c86840', - 'differential-inline-comment-editor' => 'e8b28c4a', - 'differential-local-commits-view-css' => '09c86840', - 'differential-revision-add-comment-css' => '09c86840', - 'differential-revision-comment-css' => '09c86840', - 'differential-revision-comment-list-css' => '09c86840', - 'differential-revision-detail-css' => '09c86840', - 'differential-revision-history-css' => '09c86840', - 'differential-table-of-contents-css' => '09c86840', + 'differential-changeset-view-css' => '9d02b654', + 'differential-core-view-css' => '9d02b654', + 'differential-inline-comment-editor' => 'ffca9dae', + 'differential-local-commits-view-css' => '9d02b654', + 'differential-revision-add-comment-css' => '9d02b654', + 'differential-revision-comment-css' => '9d02b654', + 'differential-revision-comment-list-css' => '9d02b654', + 'differential-revision-detail-css' => '9d02b654', + 'differential-revision-history-css' => '9d02b654', + 'differential-table-of-contents-css' => '9d02b654', 'diffusion-commit-view-css' => '61f9d480', 'javelin-behavior' => '4fbae2af', 'javelin-behavior-aphront-basic-tokenizer' => '2af849fb', - 'javelin-behavior-aphront-drag-and-drop' => 'e8b28c4a', - 'javelin-behavior-aphront-drag-and-drop-textarea' => 'e8b28c4a', + 'javelin-behavior-aphront-drag-and-drop' => 'ffca9dae', + 'javelin-behavior-aphront-drag-and-drop-textarea' => 'ffca9dae', 'javelin-behavior-aphront-form-disable-on-submit' => '95944588', - 'javelin-behavior-buoyant' => 'e8b28c4a', - 'javelin-behavior-differential-accept-with-errors' => 'e8b28c4a', - 'javelin-behavior-differential-add-reviewers-and-ccs' => 'e8b28c4a', - 'javelin-behavior-differential-comment-jump' => 'e8b28c4a', - 'javelin-behavior-differential-diff-radios' => 'e8b28c4a', - 'javelin-behavior-differential-dropdown-menus' => 'e8b28c4a', - 'javelin-behavior-differential-edit-inline-comments' => 'e8b28c4a', - 'javelin-behavior-differential-feedback-preview' => 'e8b28c4a', - 'javelin-behavior-differential-keyboard-navigation' => 'e8b28c4a', - 'javelin-behavior-differential-populate' => 'e8b28c4a', - 'javelin-behavior-differential-show-more' => 'e8b28c4a', + 'javelin-behavior-buoyant' => 'ffca9dae', + 'javelin-behavior-differential-accept-with-errors' => 'ffca9dae', + 'javelin-behavior-differential-add-reviewers-and-ccs' => 'ffca9dae', + 'javelin-behavior-differential-comment-jump' => 'ffca9dae', + 'javelin-behavior-differential-diff-radios' => 'ffca9dae', + 'javelin-behavior-differential-dropdown-menus' => 'ffca9dae', + 'javelin-behavior-differential-edit-inline-comments' => 'ffca9dae', + 'javelin-behavior-differential-feedback-preview' => 'ffca9dae', + 'javelin-behavior-differential-keyboard-navigation' => 'ffca9dae', + 'javelin-behavior-differential-populate' => 'ffca9dae', + 'javelin-behavior-differential-show-more' => 'ffca9dae', 'javelin-behavior-maniphest-batch-selector' => '86fc0b0c', 'javelin-behavior-maniphest-transaction-controls' => '86fc0b0c', 'javelin-behavior-maniphest-transaction-expand' => '86fc0b0c', 'javelin-behavior-maniphest-transaction-preview' => '86fc0b0c', 'javelin-behavior-phabricator-autofocus' => '95944588', 'javelin-behavior-phabricator-keyboard-shortcuts' => '95944588', - 'javelin-behavior-phabricator-object-selector' => 'e8b28c4a', + 'javelin-behavior-phabricator-object-selector' => 'ffca9dae', 'javelin-behavior-phabricator-watch-anchor' => '95944588', 'javelin-behavior-refresh-csrf' => '95944588', 'javelin-behavior-workflow' => '95944588', @@ -2158,20 +2158,20 @@ celerity_register_resource_map(array( 'maniphest-task-summary-css' => '31583232', 'maniphest-transaction-detail-css' => '31583232', 'phabricator-app-buttons-css' => '78e8854e', - 'phabricator-content-source-view-css' => '09c86840', + 'phabricator-content-source-view-css' => '9d02b654', 'phabricator-core-buttons-css' => '78e8854e', 'phabricator-core-css' => '78e8854e', 'phabricator-directory-css' => '78e8854e', - 'phabricator-drag-and-drop-file-upload' => 'e8b28c4a', + 'phabricator-drag-and-drop-file-upload' => 'ffca9dae', 'phabricator-dropdown-menu' => '95944588', 'phabricator-jump-nav' => '78e8854e', 'phabricator-keyboard-shortcut' => '95944588', 'phabricator-keyboard-shortcut-manager' => '95944588', 'phabricator-menu-item' => '95944588', - 'phabricator-object-selector-css' => '09c86840', + 'phabricator-object-selector-css' => '9d02b654', 'phabricator-paste-file-upload' => '95944588', 'phabricator-remarkup-css' => '78e8854e', - 'phabricator-shaped-request' => 'e8b28c4a', + 'phabricator-shaped-request' => 'ffca9dae', 'phabricator-standard-page-view' => '78e8854e', 'phabricator-transaction-view-css' => '78e8854e', 'syntax-highlighting-css' => '78e8854e', diff --git a/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php b/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php index 599efa12a2..b99b24103f 100644 --- a/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php @@ -208,9 +208,17 @@ final class DifferentialChangesetViewController extends DifferentialController { $output = $parser->render($range_s, $range_e, $mask); + $mcov = $parser->renderModifiedCoverage(); + if ($request->isAjax()) { + $content = array( + 'coverage' => array( + 'differential-mcoverage-'.md5($changeset->getFilename()) => $mcov, + ), + 'changeset' => $output, + ); return id(new AphrontAjaxResponse()) - ->setContent($output); + ->setContent($content); } Javelin::initBehavior('differential-show-more', array( diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index c36569fe1e..c9104b602b 100644 --- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php @@ -68,6 +68,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $target, array( 'local:commits', + 'arc:unit', )); list($changesets, $vs_map, $rendering_references) = @@ -262,6 +263,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $toc_view = new DifferentialDiffTableOfContentsView(); $toc_view->setChangesets($changesets); + $toc_view->setUnitTestData(idx($props, 'arc:unit', array())); if ($repository) { $toc_view->setRepository($repository); } diff --git a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php index 525010fa04..874b64c10e 100644 --- a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php @@ -1714,4 +1714,49 @@ class DifferentialChangesetParser { return array($range_s, $range_e, $mask); } + /** + * Render "modified coverage" information; test coverage on modified lines. + * This synthesizes diff information with unit test information into a useful + * indicator of how well tested a change is. + */ + public function renderModifiedCoverage() { + $na = '-'; + + if (!$this->coverage) { + return $na; + } + + $covered = 0; + $not_covered = 0; + + foreach ($this->new as $k => $new) { + if (!$new['line']) { + continue; + } + + if (!$new['type']) { + continue; + } + + if (empty($this->coverage[$new['line'] - 1])) { + continue; + } + + switch ($this->coverage[$new['line']]) { + case 'C': + $covered++; + break; + case 'U': + $not_covered++; + break; + } + } + + if (!$covered && !$not_covered) { + return $na; + } + + return sprintf('%d%%', 100 * ($covered / ($covered + $not_covered))); + } + } diff --git a/src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php b/src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php index 54519fc0a0..9a46626011 100644 --- a/src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php +++ b/src/applications/differential/view/difftableofcontents/DifferentialDiffTableOfContentsView.php @@ -26,6 +26,7 @@ final class DifferentialDiffTableOfContentsView extends AphrontView { private $renderURI = '/differential/changeset/'; private $revisionID; private $whitespace; + private $unitTestData; public function setChangesets($changesets) { $this->changesets = $changesets; @@ -42,6 +43,11 @@ final class DifferentialDiffTableOfContentsView extends AphrontView { return $this; } + public function setUnitTestData($unit_test_data) { + $this->unitTestData = $unit_test_data; + return $this; + } + public function setUser(PhabricatorUser $user) { $this->user = $user; return $this; @@ -74,6 +80,23 @@ final class DifferentialDiffTableOfContentsView extends AphrontView { $rows = array(); + $coverage = array(); + if ($this->unitTestData) { + $coverage_by_file = array(); + foreach ($this->unitTestData as $result) { + $test_coverage = idx($result, 'coverage'); + if (!$test_coverage) { + continue; + } + foreach ($test_coverage as $file => $results) { + $coverage_by_file[$file][] = $results; + } + } + foreach ($coverage_by_file as $file => $coverages) { + $coverage[$file] = ArcanistUnitTestResult::mergeCoverage($coverages); + } + } + $changesets = $this->changesets; $paths = array(); foreach ($changesets as $changeset) { @@ -129,6 +152,20 @@ final class DifferentialDiffTableOfContentsView extends AphrontView { ? null : 'M'; + $fname = $changeset->getFilename(); + $cov = $this->renderCoverage($coverage, $fname); + if ($cov === null) { + $mcov = $cov = '-'; + } else { + $mcov = phutil_render_tag( + 'div', + array( + 'id' => 'differential-mcoverage-'.md5($fname), + 'class' => 'differential-mcoverage-loading', + ), + 'Loading...'); + } + $rows[] = ''. ''.$char. @@ -136,6 +173,8 @@ final class DifferentialDiffTableOfContentsView extends AphrontView { ''.$pchar.''. ''.$desc.''. ''.$link.$lines.''. + ''.$cov.''. + ''.$mcov.''. ''; if ($meta) { $rows[] = @@ -172,11 +211,36 @@ final class DifferentialDiffTableOfContentsView extends AphrontView { $editor_link. '

Table of Contents

'. ''. + ''. + ''. + ''. + ''. + ''. + ''. + ''. + ''. implode("\n", $rows). '
PathCoverage (All)Coverage (Touched)
'. ''; } + private function renderCoverage(array $coverage, $file) { + $info = idx($coverage, $file); + if (!$info) { + return null; + } + + $not_covered = substr_count($info, 'U'); + $covered = substr_count($info, 'C'); + + if (!$not_covered && !$covered) { + return null; + } + + return sprintf('%d%%', 100 * ($covered / ($covered + $not_covered))); + } + + private function renderChangesetLink(DifferentialChangeset $changeset) { $display_file = $changeset->getDisplayFilename(); diff --git a/src/applications/differential/view/difftableofcontents/__init__.php b/src/applications/differential/view/difftableofcontents/__init__.php index 9d378f861d..cc2333699b 100644 --- a/src/applications/differential/view/difftableofcontents/__init__.php +++ b/src/applications/differential/view/difftableofcontents/__init__.php @@ -6,6 +6,8 @@ +phutil_require_module('arcanist', 'unit/result'); + phutil_require_module('phabricator', 'applications/differential/constants/changetype'); phutil_require_module('phabricator', 'infrastructure/celerity/api'); phutil_require_module('phabricator', 'view/base'); diff --git a/webroot/rsrc/css/application/differential/table-of-contents.css b/webroot/rsrc/css/application/differential/table-of-contents.css index 72b72cbdd5..cf25e9b1be 100644 --- a/webroot/rsrc/css/application/differential/table-of-contents.css +++ b/webroot/rsrc/css/application/differential/table-of-contents.css @@ -49,3 +49,34 @@ .diff-star-skip { color: #ff00aa; } + +.differential-toc table { + width: 100%; +} + +.differential-toc table td.differential-toc-cov, +.differential-toc table td.differential-toc-mcov { + width: 120px; + text-align: right; + padding-right: 6px; +} + +.differential-toc table th { + color: #666666; + font-size: 11px; + padding: 0 4px 4px; + white-space: nowrap; +} + +.differential-toc table th.differential-toc-cov, +.differential-toc table th.differential-toc-mcov { + text-align: right; +} + +.differential-toc table td em { + color: #666666; +} + +.differential-mcoverage-loading { + color: #888888; +} diff --git a/webroot/rsrc/js/application/differential/behavior-populate.js b/webroot/rsrc/js/application/differential/behavior-populate.js index ca68aea483..0430136feb 100644 --- a/webroot/rsrc/js/application/differential/behavior-populate.js +++ b/webroot/rsrc/js/application/differential/behavior-populate.js @@ -9,7 +9,16 @@ JX.behavior('differential-populate', function(config) { function onresponse(target, response) { - JX.DOM.replace(JX.$(target), JX.$H(response)); + JX.DOM.replace(JX.$(target), JX.$H(response.changeset)); + if (response.coverage) { + for (var k in response.coverage) { + try { + JX.DOM.replace(JX.$(k), JX.$H(response.coverage[k])); + } catch (ignored) { + // Not terribly important. + } + } + } } for (var k in config.registry) {