Checker summary view #870

csordasmarton · 2017-09-04T08:21:42Z

Closes #826

whisperity · 2017-09-04T08:23:31Z

libcodechecker/libhandlers/cmd.py

@@ -594,6 +594,15 @@ def add_arguments_to_parser(parser):
    results.set_defaults(func=cmd_line_client.handle_list_results)
    __add_common_arguments(results, has_matrix_output=True)

+    results = subcommands.add_parser(
+        'statistics',


Why is this a new subcommand instead of CodeChecker cmd sum redesigned to give this output?

@gyorb

whisperity · 2017-09-04T10:35:39Z

www/scripts/codecheckerviewer/CheckerStatistics.js

+  'dojo/promise/all',
+  'dojo/store/Memory',
+  'dojo/store/Observable',
+  'dojo/data/ItemFileWriteStore',


Imports are in a strange order here.

whisperity · 2017-09-06T09:48:22Z

api/report_server.thrift

@@ -42,6 +42,13 @@ struct RunReportCount{
 }
 typedef list<RunReportCount> RunReportCounts

+struct CheckerDataCount {
+  1: string             name,     // Checker name


The nomenclature pull-request will make this file also be formatted as full sentences, this should be so from now on.

whisperity · 2017-09-06T09:48:45Z

api/report_server.thrift

@@ -443,7 +450,7 @@ service codeCheckerDBAccess {
  // for all of the runs and in compare mode all of the runs
  // will be used as a baseline excluding the runs in compare data.
  // PERMISSION: PRODUCT_ACCESS
-  map<string, i64> getCheckerCounts(1: list<i64> runIds,
+  CheckerDataCounts getCheckerCounts(1: list<i64> runIds,
                                    2: ReportFilter_v2 reportFilter,


The length of the type changed, please align the arguments to be under each other (introduce a single space).

whisperity · 2017-09-06T09:52:21Z

www/scripts/codecheckerviewer/CheckerStatistics.js

+
+      this.structure = [
+        { name : 'Checker Name', field : 'checker', width : '100%'},
+        { name : 'Severity', field : 'severity', styles : 'text-align: center;', width : '10%'},


Please make this column a bit bigger, so on relatively medium-sized screens the Severity header is not split.

whisperity · 2017-09-06T09:54:47Z

libcodechecker/cmd/cmd_line_client.py

-        print(CmdLineOutputEncoder().encode(results_collector))
+        print(CmdLineOutputEncoder().encode(all_results))
+    else:
+        header = ['Checker', 'Severity', 'Reports', 'Unreviewed', 'Confirmed',


I think this table looks a bit strange, as the "detection status" is too far.

I believe the following format would be better for the eye:

Checker

Severity

All reports (let's emphasise this)

Resolved

Unreviewed

Confirmed

False positive

Won't fix

whisperity · 2017-09-06T09:55:36Z

www/scripts/codecheckerviewer/CheckerStatistics.js

+      this._runFilter = new RunFilter({
+        store: this._runStore,
+        labelAttr : 'label',
+        label : 'Get Statistics only for these runs...',


Get statistics only for runs... would be better here. Also, if this is a user-string, perhaps " should be used instead of '.

For the sake of readability, please align the :s under each other.

whisperity · 2017-09-06T09:58:03Z

libcodechecker/libhandlers/cmd.py

-        description="Show the count of checker reports per checker for some "
-                    "analysis runs.",
-        help="Show number of reports per checker.")
+        description="Show checker statistics for some analysis runs.",


As the description of this command was changed, the docs/user_guide.md file should also be updated to reflect the new help.

whisperity · 2017-09-06T09:58:36Z

api/report_server.thrift

@@ -42,6 +42,13 @@ struct RunReportCount{
 }
 typedef list<RunReportCount> RunReportCounts

+struct CheckerDataCount {


Why is Data part of this type's name?

whisperity · 2017-09-06T09:59:02Z

www/scripts/codecheckerviewer/CheckerStatistics.js

+      this.structure = [
+        { name : 'Checker Name', field : 'checker', width : '100%'},
+        { name : 'Severity', field : 'severity', styles : 'text-align: center;', width : '10%'},
+        { name : '<span class="customIcon detection-status-unresolved"></span> Reports', field : 'reports', width : '20%'},


Of course the same header layout I proposed applies to the web interface too.

gyorb

We should note that this will change the command line output format, some scripts might be broken because of the changed output.

whisperity · 2017-09-06T11:21:36Z

@gyorb I added a note to the 6.0 patchnotes.

gyorb · 2017-09-06T11:28:40Z

Could we changed the tab order to be like list of runs all issues checker statistics?
What if there are many runs and I select a lot in the drop down? Will the selection menu length increase with the names of all the selected runs?
What do you think @csordasmarton?

gyorb · 2017-09-06T11:34:59Z

I think ordering (or viewing) by severity does not work properly (High, Low, Medium) should be (High, Medium, Low) @csordasmarton.

csordasmarton · 2017-09-06T14:33:58Z

@gyorb

I don' think we should change the tab order. What if I opened a couple of run in a new tab:
1.) list of runs, all reports, checker statistics, reports1, reports2
2.) list of runs, checker statistics, all reports, reports1, reports2
I think the second one is better, because we keep run tabs after each other.
The selection menu will automatically increase it's size if I'm selecting multiple runs.
I fixed the severity ordering problem.

whisperity · 2017-09-07T07:58:23Z

Just pitching in a minor thought here. For me, a lot of zeroes in a huge table, especially on some website, is painful for the eyes.

Perhaps we could change the web server (the command-line output could stay as it is) so that if the number if 0, nothing is printed?

csordasmarton requested review from whisperity, dkrupp and gyorb September 4, 2017 08:21

whisperity reviewed Sep 4, 2017

View reviewed changes

whisperity added enhancement 🌟 GUI 🎨 labels Sep 4, 2017

csordasmarton force-pushed the summary_view branch from d287779 to fc354ec Compare September 4, 2017 08:44

whisperity reviewed Sep 4, 2017

View reviewed changes

csordasmarton force-pushed the summary_view branch 7 times, most recently from 907cf30 to 1f6019d Compare September 5, 2017 07:09

whisperity force-pushed the version6 branch from 14a3ed2 to 4c8f84d Compare September 5, 2017 08:09

csordasmarton changed the base branch from version6 to master September 5, 2017 08:11

csordasmarton force-pushed the summary_view branch from 1f6019d to 9619b2e Compare September 5, 2017 08:38

whisperity requested changes Sep 6, 2017

View reviewed changes

gyorb reviewed Sep 6, 2017

View reviewed changes

csordasmarton force-pushed the summary_view branch from 9619b2e to 32fbebd Compare September 6, 2017 14:33

csordasmarton force-pushed the summary_view branch from 32fbebd to 9b745b8 Compare September 7, 2017 08:41

Checker summary view

9681054

csordasmarton force-pushed the summary_view branch from 9b745b8 to 9681054 Compare September 7, 2017 08:42

csordasmarton requested a review from whisperity September 7, 2017 08:43

whisperity approved these changes Sep 7, 2017

View reviewed changes

gyorb approved these changes Sep 7, 2017

View reviewed changes

gyorb merged commit d38b48e into Ericsson:master Sep 7, 2017

csordasmarton deleted the summary_view branch September 7, 2017 12:34

whisperity mentioned this pull request Sep 7, 2017

Checkers Summary View #826

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checker summary view #870

Checker summary view #870

csordasmarton commented Sep 4, 2017

whisperity Sep 4, 2017

whisperity Sep 4, 2017

whisperity Sep 6, 2017

whisperity Sep 6, 2017

whisperity Sep 6, 2017

whisperity Sep 6, 2017

whisperity Sep 6, 2017

whisperity Sep 6, 2017

whisperity Sep 6, 2017

whisperity Sep 6, 2017

gyorb left a comment

whisperity commented Sep 6, 2017

gyorb commented Sep 6, 2017

gyorb commented Sep 6, 2017

csordasmarton commented Sep 6, 2017

whisperity commented Sep 7, 2017

Checker summary view #870

Checker summary view #870

Conversation

csordasmarton commented Sep 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyorb left a comment

Choose a reason for hiding this comment

whisperity commented Sep 6, 2017

gyorb commented Sep 6, 2017

gyorb commented Sep 6, 2017

csordasmarton commented Sep 6, 2017

whisperity commented Sep 7, 2017