Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(gui): Checker status auditing #4156

Merged

Conversation

whisperity
Copy link
Contributor

@whisperity whisperity commented Jan 16, 2024

✔️ Closes #4049.
⚠️⛔ Blocked by #4089!

Summary

This patch extends the visualisation in the Analysis information widget by reporting the status of the checkers that were available in an executed analyser at the time of analysis. This data is gathered by the analysing client's emitted metadata.json file and stored in the database, and is subsequently made available over the getAnalysisInfo() API in the following format: map<AnalyserName, map<CheckerName, bool>>. See #4089 for details!

Each checker that was present at analysis time will receive either a ✔️ or an ❌ based on whether it was enabled or disabled during the analysis. The "auditing" feature comes from two facts. First, if a checker is not named here, it is reasonable to assume that it was not available in the analyser (e.g. because it was a different version of that analyser). Second, the data survives write access to the Reports structure (e.g., deleting reports that match a filter), so it should always be clear from this point onwards what checkers executed when an analysis was done.

Demo (good outcome)

Normally, you will get an expandable widget grouped by analyser first. All the data is loaded in a single pass; the "closedness" of these widgets is only to make sure the user does not have to scroll 10 screens of checker names to find something! It also shows the total of how many checkers were enabled, disabled, and available from what that analyser offered.

Initial view grouped by analyser name, showing totals

Expanding the widget opens up the list of the checkers, unless there are checker groups, which there usually are. This information is heuristically calculated by the front-end only, based on the usual rules:

  1. clang-diagnostic is its own group. D'oh! 🍩
  2. The first . or - separates the checker's name from a group (bugprone-easily-swappable-parametersbugprone, core.DivisionByZerocore), unless…
  3. …the first - separated the analyser's name, in which case the second - will separate whatever will be believed as a checker "group": gcc-fd-leaksfd.
  4. If everything else fails, there will be no group associated. See later!

These groups are their own further expandable widgets. The same tally of enabled-disabled-total is shown for the group if needed. If an entire group is enabled or disabled, this information is shown with a different icon without littering the screen further with counters.

Checkers belonging to a particular analyser grouped into further expandable widgets by checker "group" collection

These groups open, showing the list of checkers with their full names in the group. There is no second-level grouping or tree structure! Although the result over the API is a map, the GUI shows everything sorted alphabetically. In case a checker cannot be assigned a group (e.g., cppcheck-ZERODIV), it will be shown directly as a child of the analyser before any group. (These checkers are counted only in the analyser's totals!)

Checker listing expanded on multiple levels, also showing a checker without a group heuristically calculated

The list automatically separates itself into columns (keeping the alphabetical sort and left-right top-down order) based on the width of the viewport… assuming the browser was released after 2016-ish.

Every icon has a mouse-over tooltip in case it is not immediately obvious what the symbols mean.

Tooltip: ✘ means disabled, duh!
Tooltip: ✓ means enabled, duh!!!

Demo (bad outcomes)

Unfortunately, there are two major cases where the above visualisation won't work. If a server was upgraded with past data, there would be no way to conjure which checkers had had been available at the time the stored analyses were performed. The decision in #4089 was that we shouldn't just blindly accept "whatever reports belong to the run" = "set of enabled checkers" and not show any disabled ones.

If the map over the API is empty, it is still a perfectly valid response. However, instead of simply not showing anything, I implemented a way to show an error message, and this error message tracks down a few conditions to give the most accurate error message possible. [†] Emphasis on likely: using an older client (as long as it is not too old) is not a deterministic way of losing the information.

Error message for runs that came through a server upgrade

The other edge case is when users store a run that does not have a metadata.json. This is the most generic error that can happen here and will likely be the most populous as time passes.

Error message for cases without any analysis info

[†]: There is an edge case. An AnalysisInfo can be queried for a Report directly. This is patched through the API with a relation table, but there is no organic way (from the front-end) to figure out which RunHistory moment inserted the Report. As this new feature works not just for the newest Runs but also RunHistorys, the error message is a little bit different when the viewport is opened from a Report.

Error message for Report-induced analysis infos without data

ℹ️💡 Note: If there is data available from a Report's position, everything will work normally:

Report-induced analysis infos with actual data

@whisperity whisperity added this to the release 6.24.0 milestone Jan 16, 2024
@whisperity whisperity requested a review from cservakt January 16, 2024 19:44
@whisperity whisperity marked this pull request as draft January 16, 2024 19:45
@whisperity whisperity force-pushed the feat/show-zero-checkers/patch-frontend branch from ed3348f to 43e4077 Compare January 17, 2024 14:00
@whisperity whisperity removed the WIP 💣 Work In Progress label Jan 17, 2024
@whisperity whisperity marked this pull request as ready for review January 17, 2024 15:14
@whisperity whisperity force-pushed the feat/show-zero-checkers/patch-frontend branch from 43e4077 to 702ee60 Compare January 21, 2024 12:01
Extend the "Analysis Info" dialog on the Web UI to be capable of
rendering the new `AnalysisInfo.checkers` data structure from the API.

If the data is available, renders an expanding widget first grouped by
analyser, then heuristically by checker "group". If the group is not
entirely enabled or disabled, shows the number of enabled, disabled, and
total checkers. Does the same for the analyser as a whole.

If the data is not available, it tries doing its best to give the user a
reason why. Likely it is because of two things: runs upgraded from older
server versions that did not collect the data, or runs done not with
`CodeChecker analyze`.
@whisperity whisperity force-pushed the feat/show-zero-checkers/patch-frontend branch from 702ee60 to ce4d280 Compare March 2, 2024 17:38
@whisperity
Copy link
Contributor Author

@cservakt I'm working on refactoring this to have some of the logic reusable as we discussed! It's just a non-trivial process to get everything right.

@whisperity
Copy link
Contributor Author

@cservakt I did the refactoring and now the getAnalysisInfo() in the Dialog's code is much simpler, and deals with only calling transformation codes. So for your feature, you should be able to reuse the mixin from other locations, which will do the simple checks (such as heuristically reasoning why the checker list is not available) and load the raw values (analysis command and raw, unsorted and ungrouped checker list — so essentially only what is received from the API), but doing the "fancy" transformations is a separate opt-in call, which is driven by the main Dialog's code.

@whisperity whisperity force-pushed the feat/show-zero-checkers/patch-frontend branch from 51e3507 to 3987127 Compare March 22, 2024 16:38
Copy link
Collaborator

@cservakt cservakt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the rebase! Now, it looks good to me.

@whisperity
Copy link
Contributor Author

I've created tests for this feature, but I will upload them as a separate patch after this is merged s.t. this review is not derailed by concerns specific only to the tests.

@cservakt cservakt merged commit 887c1ec into Ericsson:master Mar 26, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants