Detection status #724

bruntib · 2017-07-13T15:55:51Z

#709 should be merged first!

whisperity · 2017-07-19T19:27:18Z

libcodechecker/analyze/analyzers/result_handler_plist_to_db.py

-    def handle_results(self, client=None):
+        return report_ids
+
+    def handle_results(self, client):
        """
        Send the plist content to the database.
        Server API calls should be used in one connection.


The same one connection thing pointed out by @dkrupp earlier is here. We should clarify this, somehow.

whisperity · 2017-07-19T19:30:26Z

libcodechecker/orm_model.py

+    detection_status = Column(Enum('new',
+                                   'unresolved',
+                                   'resolved',
+                                   'reopened'))


So after all, should we keep the reopened status?

Yes, we keep reopened. Moreover in the latest implementation a report stays reopened unit it becomes resolved again.

whisperity · 2017-07-19T19:35:18Z

libcodechecker/libhandlers/store.py

+
+
+def consumed_results(client, run_id):
+    def markFixed(l):


(Let's keep not introducing style warnings and keep to the Pythonic nomenclature, mark_fixed. Also comment below is not a full sentence. I'm Vice Xazax now! 😛 )

whisperity · 2017-07-19T19:38:49Z

www/scripts/codecheckerviewer/BugViewer.js

-      //--- Details ---//
-
-      this.addChild(new Button({
-        label : 'Details',


Ah, this is that ugly window of #566. Good riddance, I think. I'm okay with losing this information, didn't seem too useful in the first place. If analysing a file failed, where could have the user click Details to find the failure output and such in the first place?

whisperity · 2017-07-19T19:41:23Z

libcodechecker/libhandlers/server.py

-                                           i['workspace']) ==
-                                           os.path.abspath(
-                                               args.config_directory))):
+                    args.stop and not (i['port'] == args.view_port and


This change is not relevant.

whisperity · 2017-07-19T19:41:35Z

Doxyfile.in

@@ -34,7 +34,7 @@ PROJECT_NAME           = "CodeChecker"
 # This could be handy for archiving the generated documentation or
 # if some version control system is used.

-PROJECT_NUMBER         = 5.9.1
+PROJECT_NUMBER         = 6.0.0


This change is not relevant.

whisperity

Please rebase the topic branch, a lot of commits already in Ericsson/version6 are duplicated.

whisperity · 2017-08-16T13:31:04Z

www/scripts/codecheckerviewer/util.js

@@ -50,6 +50,10 @@ function (locale, dom, style) {
          return key.toLowerCase();
    },

+    detectionStatusToString : function (detectionStatus) {


This appears to be an unused method.

whisperity · 2017-08-16T13:31:48Z

tests/functional/detection_status/test_detection_status.py

+from libtest import env
+
+
+class TestSkeleton(unittest.TestCase):


Test class uses template name.

whisperity · 2017-08-16T13:33:04Z

tests/functional/detection_status/test_detection_status.py

+
+    def _create_source_file(self, version):
+        if version == 1:
+            source = """


Perhaps we could put the later hardcoded bug hashes here at least as a comment to see which means which bug in the code.

whisperity · 2017-08-16T13:34:21Z

libcodechecker/server/client_db_access_handler.py

+def unzip(b64zip):
+    """
+    This function unzips the base64 encoded zip file which should contain
+    exactly one directory. This directory is extracted to the given workspace.


This method no longer uses a "workspace", the docstring should reflect that.

whisperity · 2017-08-16T13:35:50Z

libcodechecker/server/client_db_access_handler.py

@@ -20,6 +27,10 @@
 from codeCheckerDBAccess import constants
 from codeCheckerDBAccess.ttypes import *

+from libcodechecker import generic_package_context
+from libcodechecker import suppress_handler
+from libcodechecker.analyze import plist_parser


Please add a comment at the .analyze import

# TODO: Cross-subpackage import here.

Ok. But what's this?

Back when the subcommands and the whole of libcodechecker was split and reorganised, imports which violated subcommand boundaries (such as server depending on a class defined in analyze) were marked with the same(-esque) TODO.

It's a marker for us that there are architectural mishaps in CodeChecker. It's not a problem as long as CodeChecker is a single and whole being in itself, but the moment we want to create separate builds from CodeChecker, e.g. one that can only log and analyze, or one that is only a webserver, these imports make easy creation of such packages problematic, and these markers show us that "Yeah, there should one day be a refactoring here!". (There were discussion about this package separation around 7-9 months ago, but that discussion died due to the plethora of other features to be made.)

whisperity · 2017-08-16T13:37:44Z

libcodechecker/analyze/store_handler.py

+import tempfile
+import zlib
+
+import shared


Import order is: systemwide, external, API, internal, local.

So:

import zlib

(empty line)

import sqlalchemy

(empty line)

import shared

from codeCheckerDBAcess...

whisperity · 2017-08-16T13:39:29Z

api/report_server.thrift

-  bool setRunDuration(1: i64 run_id,
-                      2: i64 duration)
-                      throws (1: shared.RequestFailed requestError),
-
  bool stopServer()


This method is absolutely not used anywhere in the code. We should remove it too, if we are already throwing out a big hunk here.

whisperity · 2017-08-16T15:21:42Z

tests/functional/detection_status/test_detection_status.py

+                          'hello',
+                          self._test_dir)
+
+    def test_skel(self):


This is template text

whisperity · 2017-08-16T15:21:49Z

tests/functional/detection_status/test_detection_status.py

+
+    def test_skel(self):
+        """
+        Test some feature.


This is a text from the test template

whisperity · 2017-08-16T15:22:02Z

tests/functional/detection_status/test_detection_status.py

+
+    def setUp(self):
+        """
+        WARNING!!!


This text is from the test template

whisperity · 2017-08-16T15:22:11Z

tests/functional/detection_status/test_detection_status.py

+
+""" detection_status function test.
+
+WARNING!!!


This test remained here from the test template

whisperity · 2017-08-16T15:22:25Z

tests/functional/detection_status/__init__.py

+
+"""Setup for the test package detection_status.
+
+WARNING!!!


This text remained here from the test template

whisperity · 2017-08-16T15:23:32Z

www/scripts/codecheckerviewer/ListOfRuns.js

@@ -15,9 +15,10 @@ define([
  'dijit/form/TextBox',
  'dijit/layout/BorderContainer',
  'dijit/layout/ContentPane',
-  'dojox/grid/DataGrid'],
+  'dojox/grid/DataGrid',


We should keep an import order in the JS code too: dijit/form/..., dijit/grid/..., dijit/layout/....

Don't forget to align the variable names in the function decl below!

This is dojox/grid, not dijit/grid.

This is my bad, then. 😇

gyorb · 2017-08-16T20:17:31Z

libcodechecker/analyze/store_handler.py

+
+    events = filter(lambda i: i.get('kind') == 'event', report.bug_path)
+
+    # TODO: skip handling


Right now the idea is that skip handling will be done after the analysis so the storage does not have to skip the results.

gyorb · 2017-08-16T20:53:59Z

libcodechecker/server/client_db_access_handler.py

@@ -823,6 +1007,9 @@ def __queryDiffResults(self,
                .outerjoin(File, Report.file_id == File.id) \
                .outerjoin(ReviewStatus,
                           ReviewStatus.bug_hash == Report.bug_id) \
+                .outerjoin(SuppressBug,


Didn't we remove the SuppressBug table?

We did, in #768.

gyorb · 2017-08-16T21:11:21Z

tests/functional/detection_status/test_detection_status.py

+
+        # Get the CodeChecker cmd if needed for the tests.
+        self._codechecker_cmd = env.codechecker_cmd()
+        self._test_dir = os.path.join(os.path.dirname(__file__), 'test_files')


I would recommend to use the workspace created for the tests to generate test files. Not to the test file directory.

gyorb · 2017-08-16T21:28:00Z

api/report_server.thrift

@@ -31,6 +31,8 @@ struct RunData{
  6: string         runCmd,       // the used check command
  7: optional bool  can_delete    // true if codeCheckerDBAccess::removeRunResults()
                                  // is allowed on this run (see issue 151)
+  8: map<shared.DetectionStatus, i32> detectionStatusCount


There will be a separate count api with filtering if the schema is ready. I'm not sure we should return this value here. For now we can keep it.

whisperity

Apart from the inline comments, considering that we are only packaging in store, the --jobs argument is irrelevant. It should be removed.

It's argument forwarding in the wrapper check should also be removed.

whisperity · 2017-08-17T09:02:50Z

libcodechecker/libhandlers/store.py

+
+                shutil.copyfile(f, target_file)
+
+        zipf = zipfile.ZipFile(zip_file, 'w')


zipfile.ZipFile can be used as a context manager:

with zipfile.ZipFile(zip_file, 'w') as zipf: for root, _, files in os.walk(temp_dir): # ...

whisperity · 2017-08-17T09:06:07Z

libcodechecker/libhandlers/store.py

+
+                shutil.copyfile(f, target_file)
+
+        zipf = zipfile.ZipFile(zip_file, 'w')


This ZIP file in its current state does not deflate the files, only store them. If this is intended, it's allright, you create a compression later on, actually creating a .zip.gz.

If we'd rather individually compress the files, we should add a 3rd argument to this constructor: zipfile.DEFLATED.

The .zip file is compressed later in one piece when it's already containing everything. I'm not writing the compressed file to the disk to a .zip.gz, but I'm sending the data directly to the server.

Xazax-hun · 2017-08-17T16:30:56Z

libcodechecker/analyze/store_handler.py

+LOG = LoggerFactory.get_new_logger('STORE HANDLER')
+
+
+def process_compile_db(compile_db):


Maybe a more descriptive name like make_absolute_path_compile_db. Process is overly generic in my opinion.

Xazax-hun · 2017-08-17T16:31:45Z

libcodechecker/analyze/store_handler.py

+    if not os.path.exists(compile_db):
+        LOG.warning("Compilation command file '" + compile_db +
+                    "' is missing.")
+        LOG.warning("Update mode will not work, without compilation "


Somewhere we should have a ticket to extend the plist with a working directory entry to overcome this issue in the long term.

Xazax-hun · 2017-08-17T16:34:51Z

libcodechecker/analyze/store_handler.py

+        metadata_dict = json.load(metadata)
+        LOG.debug(metadata_dict)
+
+        if 'command' in metadata_dict:


Do we need this code to be in the with block?

Xazax-hun · 2017-08-17T16:35:05Z

libcodechecker/analyze/store_handler.py

+
+    with open(metadata_file, 'r') as metadata:
+        metadata_dict = json.load(metadata)
+        LOG.debug(metadata_dict)


Do we want to dump the content of this file?

Xazax-hun · 2017-08-17T16:39:27Z

libcodechecker/analyze/store_handler.py

+                start2_line,
+                start2_col,
+                file_ids[source_file_path]))
+        except IndexError:


I think this worth a comment what kind of lookups do we expect to fail and why.

Xazax-hun · 2017-08-17T16:42:00Z

libcodechecker/analyze/store_handler.py

+    try:
+        LOG.debug("adding checker run")
+
+        run = session.query(Run).filter(Run.name == name).first()


Shouldn't we use one_or_none to enforce that there are no multiple runs with the same name?

Store detection status for the reports in the database.

dkrupp

Looks good.

Please clean up the naming in the API and add a few more test cases.

dkrupp · 2017-08-18T11:55:15Z

api/report_server.thrift

-  9: ReviewData           review           // bug review status informations.
+  1: string              checkerId,       // the qualified id of the checker that reported this
+  2: string              bugHash,         // This is unique id of the concrete report.
+  3: string              checkedFile,     // this is a filepath


checkedFile is confusing indeed. Maybe sourceFileName would be a better name...

dkrupp · 2017-08-18T12:01:23Z

api/report_server.thrift

+  // The "version" parameter is the used CodeChecker version which checked this
+  // run.
+  // The "force" parameter removes existing analysis results for a run.
+  i64 storeZip(


Sorry a naming comment again: Please rename this function: storeRun(...)
or maybe massStoreRun() to better express the functionality.

Because that's what this function is really functionally doing.
Requiring zip format is an accidental design choice, it could have been rar as well, but still the function stores a run...

dkrupp · 2017-08-18T12:03:57Z

api/report_server.thrift

+               1: string run_name,
+               2: string version,
+               3: string zipfile,
+               4: bool force)


maybe clean is a better name as it expresses that previous results will be cleaned for this run.

dkrupp · 2017-08-18T13:06:26Z

tests/functional/detection_status/test_detection_status.py

+
+  int* p = 0;
+
+  i = *p + 42;


please add a version 4 of the file where you insert 2 blank lines after int main(){ to test if the bugs are not duplicated with line shift.

dkrupp · 2017-08-18T13:09:07Z

tests/functional/detection_status/test_detection_status.py

+                                 shared.ttypes.DetectionStatus.REOPENED)
+            elif report.bugHash == 'ac147b31a745d91be093bd70bbc5567c':
+                self.assertEqual(report.detectionStatus,
+                                 shared.ttypes.DetectionStatus.RESOLVED)


please also verify the source file content referred to by the bug.
for the resolved bug (i.e. i=1/0 we should see version 2) we should see the last version where the bug occurred, for new, unresolved, and reopened, we should always see the latest file content.

whisperity · 2017-08-18T15:15:00Z

tests/functional/report_viewer_api/test_files/metadata.json

+    "analyze",
+    "asd.json"],
+  "failed": {},
+  "working_directory": "/home/etibbru/CodeCompass/projects/hello",


Will this work for other users?

whisperity · 2017-08-18T15:15:08Z

tests/functional/report_viewer_api/test_files/metadata.json

+  "failed": {},
+  "working_directory": "/home/etibbru/CodeCompass/projects/hello",
+  "timestamps": {"begin": 1502964665.566252, "end": 1502964665.777198},
+  "output_path": "/home/etibbru/.codechecker/reports"}


A user-related path is added here too.

…rver

whisperity

Let's roll this at this point. Some issues remain, they will be fixed later on.

bruntib added the WIP 💣 Work In Progress label Jul 13, 2017

bruntib mentioned this pull request Jul 13, 2017

Diag section #725

Closed

dkrupp added this to the 6.0 pre1 milestone Jul 13, 2017

bruntib force-pushed the version6 branch from 502343d to a4e53b2 Compare July 17, 2017 11:53

bruntib mentioned this pull request Jul 17, 2017

Removing BuildAction table and related elements. #709

Merged

bruntib force-pushed the detection_status branch from 8b0b361 to cb00d86 Compare July 17, 2017 17:38

whisperity force-pushed the version6 branch from a4e53b2 to ed9b726 Compare July 19, 2017 06:31

whisperity reviewed Jul 19, 2017

View reviewed changes

bruntib force-pushed the detection_status branch from cb00d86 to fc57e7c Compare July 21, 2017 14:23

whisperity self-requested a review July 21, 2017 15:03

bruntib force-pushed the detection_status branch 4 times, most recently from 84b58f9 to a46ae01 Compare July 23, 2017 16:02

bruntib removed the WIP 💣 Work In Progress label Jul 23, 2017

bruntib force-pushed the detection_status branch 2 times, most recently from bf31491 to 613897c Compare July 23, 2017 21:38

whisperity mentioned this pull request Jul 24, 2017

Add incremental mode to analysis command. #719

Merged

dkrupp modified the milestones: 6.0 pre1 , 6.0 pre2 Jul 25, 2017

whisperity force-pushed the version6 branch 2 times, most recently from d47cf59 to 6c167b7 Compare July 27, 2017 12:47

whisperity requested changes Aug 2, 2017

View reviewed changes

bruntib force-pushed the detection_status branch from a130824 to 3501d3c Compare August 2, 2017 11:39

whisperity added the WIP 💣 Work In Progress label Aug 3, 2017

bruntib force-pushed the detection_status branch 2 times, most recently from 6be5d63 to 3f576c0 Compare August 11, 2017 13:31

bruntib requested a review from Xazax-hun August 11, 2017 13:32

bruntib force-pushed the detection_status branch 3 times, most recently from 1ca8382 to c78983a Compare August 16, 2017 13:08

whisperity requested changes Aug 16, 2017

View reviewed changes

bruntib force-pushed the detection_status branch from c78983a to b8b108f Compare August 16, 2017 14:06

whisperity requested changes Aug 16, 2017

View reviewed changes

bruntib force-pushed the detection_status branch from b8b108f to 43f3b8d Compare August 16, 2017 17:04

gyorb reviewed Aug 16, 2017

View reviewed changes

whisperity requested changes Aug 17, 2017

View reviewed changes

Xazax-hun reviewed Aug 17, 2017

View reviewed changes

bruntib force-pushed the detection_status branch 2 times, most recently from 1de081b to ef70176 Compare August 18, 2017 09:58

Detection status

a112911

Store detection status for the reports in the database.

dkrupp requested changes Aug 18, 2017

View reviewed changes

bruntib force-pushed the detection_status branch 2 times, most recently from 99d53ad to 859844b Compare August 18, 2017 13:51

whisperity mentioned this pull request Aug 18, 2017

Test case for suppress comments #819

Closed

bruntib force-pushed the detection_status branch 2 times, most recently from f8da1a5 to 73aad57 Compare August 18, 2017 15:10

whisperity requested changes Aug 18, 2017

View reviewed changes

The store command sends the analysis results in a .zip file to the se…

77601a7

…rver

bruntib force-pushed the detection_status branch from 73aad57 to 77601a7 Compare August 18, 2017 15:50

whisperity approved these changes Aug 18, 2017

View reviewed changes

whisperity merged commit 81d8764 into Ericsson:version6 Aug 18, 2017

bruntib deleted the detection_status branch August 18, 2017 16:16

This was referenced Aug 21, 2017

Fix logging with nonexistant G++ on macOS #824

Merged

Fix store using /tmp for ZIP paths #825

Merged

bruntib mentioned this pull request Aug 30, 2017

Store suppression messages #848

Merged

whisperity mentioned this pull request Aug 30, 2017

Product management (1st part) #773

Merged

		from libtest import env


		class TestSkeleton(unittest.TestCase):


		events = filter(lambda i: i.get('kind') == 'event', report.bug_path)

		# TODO: skip handling


		shutil.copyfile(f, target_file)

		zipf = zipfile.ZipFile(zip_file, 'w')

		LOG = LoggerFactory.get_new_logger('STORE HANDLER')


		def process_compile_db(compile_db):

Detection status #724

Detection status #724

Conversation

bruntib commented Jul 13, 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

whisperity left a 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

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

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

whisperity left a 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

Choose a reason for hiding this comment

dkrupp left a comment • edited Loading

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

whisperity left a comment

Choose a reason for hiding this comment

dkrupp left a comment •

edited

Loading