Bug 144814 – Add support to the botwatchers dashboard for a static analyzer bot

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dana Burkart]

Add support for displaying the number of bugs a static analyzer bot has found to the dashboard. While we're at it, we might as well get rid of the performance column and merge it with the 'Other' column.

[Dana Burkart]

<rdar://problem/20878234>

[Dana Burkart]

Created attachment 253065 [details]
Add support for static analyzer bots to the dashboard.

[Dana Burkart]

Created attachment 253068 [details]
Add support for static analyzer bots to the dashboard.

Add missing class (forgot to git add)

[Alexey Proskuryakov]

Comment on attachment 253068 [details]
Add support for static analyzer bots to the dashboard.

View in context: https://bugs.webkit.org/attachment.cgi?id=253068&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:90
> +    "scan build" : "scan build",

No need to add it to TestSteps, only BuildbotTesterQueueView uses that.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotStaticAnalyzerQueueView.js:59
> +                var url = iteration.queue.buildbot.buildPageURLForIteration(iteration);

Is this the best URL that we have? No way to link directly to static analyzer output when the build succeeds with issues?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotStaticAnalyzerQueueView.js:66
> +                    text = "no bugs found";

Should we call these "issues", not "bugs"?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotStaticAnalyzerQueueView.js:69
> +                } else if (!iteration.productive) {
> +                    statusLineViewColor = StatusLineView.Status.Danger;
> +                } else if (iteration.failed && /failed to build/.test(iteration.text)) {

WebKit style is to not have braces around single-line blocks.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotStaticAnalyzerQueueView.js:74
> +                var failedStep = iteration.failedTestSteps[0];

OK, you are adding a usage of TestSteps here, but I don't think that I like it. Seems better to get it by name.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotStaticAnalyzerQueueView.js:77
> +                var status = new StatusLineView(messageElement, statusLineViewColor, (text) ? text : "found " + failedStep.bugsCount + " bugs", failedStep.bugsCount, url);

Same question about "issues".

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTestResults.js:97
>  

I wish we had a less generic way to identify these.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:158
> -    header.textContent = "Performance";
> +    header.textContent = "Other";

I don't understand how changes in Main.js affect the internal dashboard. Please test.

[Dana Burkart]

Comment on attachment 253068 [details]
Add support for static analyzer bots to the dashboard.

View in context: https://bugs.webkit.org/attachment.cgi?id=253068&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotStaticAnalyzerQueueView.js:59
>> +                var url = iteration.queue.buildbot.buildPageURLForIteration(iteration);
> 
> Is this the best URL that we have? No way to link directly to static analyzer output when the build succeeds with issues?

That's a good point, agreed.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotStaticAnalyzerQueueView.js:66
>> +                    text = "no bugs found";
> 
> Should we call these "issues", not "bugs"?

I don't really have a preference. To me, an "issue" and a "bug" are synonymous, at least in the context of static analysis.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:158
>> +    header.textContent = "Other";
> 
> I don't understand how changes in Main.js affect the internal dashboard. Please test.

I did test, and this is the only way to change the heading. If you look in the internal index.html, you find:

<script src="https://build.webkit.org/dashboard/Scripts/Main.js"></script>

so if you're saying it does nothing, then we should also be removing that script tag.

[Alexey Proskuryakov]

> I don't really have a preference. To me, an "issue" and a "bug" are synonymous, at least in the context of static analysis.

Most static analyzer issues are not really bugs. Even "issue" may be to strong.

That said, what terminology does Xcode use? We should just match that.

> so if you're saying it does nothing, then we should also be removing that script tag.

I was unclear. What I was saying was that I wasn't sure what this change will break on the internal dashboard.

Main.js is definitely used everywhere.

[Dana Burkart]

(In reply to comment #6)
> > I don't really have a preference. To me, an "issue" and a "bug" are synonymous, at least in the context of static analysis.
> 
> Most static analyzer issues are not really bugs. Even "issue" may be to
> strong.
> 
> That said, what terminology does Xcode use? We should just match that.

AnalyzeShallow calls them "warnings", and the scan-build scripts refers to them as "bugs" in one place, and "analyzer issues" in another.

[Alexey Proskuryakov]

This was landed in <http://trac.webkit.org/changeset/184804>, plus follow-up fixes.

[Dana Burkart]

Reopening to further fix fallout from related dashboard change.

[Dana Burkart]

Created attachment 253744 [details]
Make the static analyzer builder queue show up.

[Alexey Proskuryakov]

Comment on attachment 253744 [details]
Make the static analyzer builder queue show up.

View in context: https://bugs.webkit.org/attachment.cgi?id=253744&action=review

I was wondering if the step name in BuildbotIteration.js is correct ("scan build").

> Tools/ChangeLog:-293
> -        * MiniBrowser/efl/main.c: 

Code changes look good, but ChangeLog re-formatting shouldn't happen.

[Dana Burkart]

Committed r184882.