[feat] Guideline statistics #4365

cservakt · 2024-10-14T18:37:57Z

The new Guideline statistics tab on the Statistics page can list all rules for the selected guidelines. The user can select multiple guidelines (but currently the only one is sei-cert and it is the default).

The table can show the checker statistics that are related to the specified guideline rule. Rules may connect to more than one checker or may not have any checker.

The checker statistics are calculated for runs that are selected (or for all runs if no run selected) in the report filter. It can show guideline name, guideline rule, checker name, checker severity, checker status, number of closed and outstanding reports.

The status informs the user about how many runs the given checker was enabled or disabled. Closed and outstanding report counts depend on review and detection status.

New config dir was created to store guideline files. Each yaml file represents a guideline an contains its rules. The Guidelines class can parse the yamls. We can reach the guideline data via getGuidelineRules API endpoint that can return a list of Rules.

The new `Guideline statistics` tab on the Statistics page can list all rules for the selected guidelines. The user can select multiple guidelines (but currently the only one is `sei-cert` and it is the default). The table can show the checker statistics that are related to the specified guideline rule. Rules may connect to more than one checker or may not have any checker. The checker statistics are calculated for runs that are selected (or for all runs if no run selected) in the report filter. It can show guideline name, guideline rule, checker name, checker severity, checker status, number of closed and outstanding reports. The status informs the user about how many runs the given checker was enabled or disabled. Closed and outstanding report counts depend on review and detection status. New config dir was created to store guideline files. Each yaml file represents a guideline an contains its rules. The `Guidelines` class can parse the yamls. We can reach the guideline data via `getGuidelineRules` API endpoint that can return a list of `Rules`.

dkrupp

Thanks for the PR. I have some minor comments. otherwise looks good.

Remove the sei-cert rule titles as it may infringe copyright config/guidelines/sei-cert.yaml
Separate the guidelines to C and C++ (also 2 files sei-cert-c.yaml, sei-cert-cpp.yaml)
Call the guideline in the dropdown as SEI CERT C Coding Standard
Add a new field in the sei-cert.yaml to describe the above human readable title
include a short doc page with screenshot here https://codechecker-demo.eastus.cloudapp.azure.com/userguide#statistics-pages
I don't see the tests. shouldn't there be tests for parsing the yaml file and new getGuidelineRules() API function?

dkrupp · 2024-11-12T15:38:44Z

web/server/vue-cli/src/components/Statistics/Guideline/GuidelineStatisticsTable.vue

+          align: "center"
+        },
+        {
+          text: "Checker Status",


Consider renaming the column to "Checker Enabled/Disabled"

vodorok

Nice work, please see my remarks, and questions.

vodorok · 2024-11-05T12:51:08Z

web/api/report_server.thrift

+  1: string ruleId,                     // The identifier of the rule.
+  2: string title,                      // The rule summary.
+  3: string url,                        // The link of the rule page.
+  4: list<map<string, string>> checkers // List of checker names


This is a list of a map of strings, what are the maps for?

In this case the map represents one checker and contains the checker name and its severity. It is not the same as the struct Checker but I did not want to create another one checker struct.

vodorok · 2024-11-13T15:39:00Z

analyzer/codechecker_analyzer/analyzer_context.py

@@ -52,13 +53,17 @@ def __init__(self):
        if 'CC_TEST_LABELS_DIR' in os.environ:
            labels_dir = os.environ['CC_TEST_LABELS_DIR']

+        guidelines_dir = os.path.join(self._data_files_dir_path,


In the future, I would rather use Path from the pathlib module.

vodorok · 2024-11-13T15:42:14Z

codechecker_common/guidelines.py

+                f'{guidelines_dir} is not a directory.')
+
+        guideline_yaml_files = map(
+            lambda f: os.path.join(guidelines_dir, f),


Same comment from above applies to this os.path usage.

vodorok · 2024-11-13T15:47:12Z

codechecker_common/guidelines.py

+        Return the list of rules of a guideline.
+        """
+
+        guideline_rules = self.__all_rules.get(guideline_name, [])


If you are using the Defaultdict, then you don't need to provide an empty list for the get() function. You could also use self.__all_rules[guideline_name] which is more readable IMO.

vodorok · 2024-11-13T15:52:14Z

codechecker_common/guidelines.py

+import yaml
+
+
+class Guidelines:


I think a Guidlines class would benefit from an interface that would return all the available guideline names.

def get_guidelines(self): return self.__all_rules.keys()

There is a function that already provides all guidelines and their rules.

def all_guideline_rules(self) -> DefaultDict[str, List[Dict[str, str]]]: return self.__all_rules

Do you think it is enough or it would be better to have a dedicated guideline name function?

vodorok · 2024-11-13T15:53:00Z

web/codechecker_web/shared/webserver_context.py

@@ -70,7 +71,11 @@ def __init__(self):
        if 'CC_TEST_LABELS_DIR' in os.environ:
            labels_dir = os.environ['CC_TEST_LABELS_DIR']

+        guidelines_dir = os.path.join(self._data_files_dir_path,


Same comment about Path usage applies here. It is the recommended way to handle paths in python3.

vodorok · 2024-11-13T15:59:53Z

web/server/codechecker_server/api/report_server.py

+                    "checkerName": checker_name,
+                    "severity": self._context.checker_labels.severity(
+                        checker_name).lower()
+                    } for checker_name in


This part is really hard to understand. Can't you precompute a dict of checker_name -> checker labels instead of iterating it every time in the third nested cycle?

vodorok · 2024-11-13T16:02:39Z

web/server/vue-cli/src/components/CountChips.vue

@@ -89,7 +89,7 @@ export default {
  },
  props: {
    tag: { type: String, default: "span" },
-    numGood: { type: Number, required: true },
+    numGood: { type: Number, default: 0 },


required: true is not needed anymore?

Yes, it is not needed and there are some cases when we need to show just the numBad count chip.

vodorok · 2024-11-13T16:04:47Z

web/server/vue-cli/src/components/Report/ReportFilter/ReportFilter.vue

@@ -412,6 +413,15 @@ export default {
    }),
  },

+  watch: {


What issue does this auto refresh solve?

It is useful when the url changes and we want to update the report filter as well. For example, a user restricts runs that are not natively analyzed by CodeChecker 6.24 or above.

bruntib · 2024-11-15T13:36:34Z

codechecker_common/guidelines.py

+                guideline_name = guideline_data.get("guideline")
+                rules = guideline_data.get("rules")
+
+                all_rules[guideline_name].extend(rules)


Some format checking of the YAML file could be useful. The format checker function of checker label file may give some hint:

codechecker/codechecker_common/checker_labels.py

Line 98 in 3229b9b

def __check_json_format(self, data: dict):

bruntib · 2024-11-15T13:38:27Z

codechecker_common/guidelines.py

+        {
+            "guideline1": [
+                {
+                    "rule_id": ...


Consider using a dictionary where rule_id is a key:

"guideline1": { "rule_id1": { "rule_url": ... "title": ... }, "rule_id2": { ... } }

cservakt added API change 📄 Content of patch changes API! GUI 🎨 config ⚙️ new feature 👍 New feature request label-tool 🔖 Related to tooling that manages the analyzer/checker label configuration labels Oct 14, 2024

cservakt added this to the release 6.25.0 milestone Oct 14, 2024

cservakt requested a review from dkrupp October 14, 2024 18:37

cservakt requested review from bruntib and vodorok as code owners October 14, 2024 18:37

cservakt force-pushed the guideline-statistics branch 2 times, most recently from a5d4382 to 7fb79d6 Compare October 15, 2024 12:36

cservakt force-pushed the guideline-statistics branch from 7fb79d6 to 5a02fd4 Compare October 15, 2024 12:41

dkrupp requested changes Nov 12, 2024

View reviewed changes

vodorok requested changes Nov 14, 2024

View reviewed changes

bruntib reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] Guideline statistics #4365

[feat] Guideline statistics #4365

cservakt commented Oct 14, 2024

dkrupp left a comment •

edited

Loading

dkrupp Nov 12, 2024

vodorok left a comment

vodorok Nov 5, 2024

cservakt Nov 14, 2024

vodorok Nov 13, 2024

vodorok Nov 13, 2024

vodorok Nov 13, 2024

vodorok Nov 13, 2024

cservakt Nov 14, 2024

vodorok Nov 13, 2024

vodorok Nov 13, 2024

vodorok Nov 13, 2024

cservakt Nov 14, 2024

vodorok Nov 13, 2024

cservakt Nov 14, 2024

bruntib Nov 15, 2024

bruntib Nov 15, 2024

[feat] Guideline statistics #4365

Are you sure you want to change the base?

[feat] Guideline statistics #4365

Conversation

cservakt commented Oct 14, 2024

dkrupp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vodorok 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

dkrupp left a comment •

edited

Loading