Bug 66955 – garden-o-matic needs a way to report where and how tests are failing in the summary view.

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

[Dimitri Glazkov (Google)]

garden-o-matic needs a way to report where and how tests are failing in the summary view.

[Dimitri Glazkov (Google)]

Created attachment 105213 [details]
Patch

[Adam Barth]

Comment on attachment 105213 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/failures.js:63
> +        var titles = this.createTHead().insertRow();

Woah!  I never new about this API.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/failures.js:76
> +        row = this._resultRows[result] = this._body.insertRow(-1);

I don't use this pattern in C++ in WebKit, but it's probably fine to use here.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/failures.js:87
> +            this._resultRows = {};

Do you mean to blow away the entire dictionary for each key in the dictionary?  That seems slightly wrong.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/failures.js:95
> +            var cell = row.cells[configuration.debug ? 2 : 1];

I would have put "configuration.debug ? 2 : 1" into a function just to give the operation a name.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/notifications_unittests.js:117
> +            '<thead><tr><td>type</td><td>release</td><td>debug</td></tr></thead>' +
> +            '<tbody></tbody>' +

Should one of theres tests put something non-trivial into the tbody?

[Dimitri Glazkov (Google)]

Comment on attachment 105213 [details]
Patch

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

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/failures.js:87
>> +            this._resultRows = {};
> 
> Do you mean to blow away the entire dictionary for each key in the dictionary?  That seems slightly wrong.

I disagree. It's not slightly wrong. It's AWFULLY wrong :P Will fix.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/failures.js:95
>> +            var cell = row.cells[configuration.debug ? 2 : 1];
> 
> I would have put "configuration.debug ? 2 : 1" into a function just to give the operation a name.

Will do.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/scripts/ui/notifications_unittests.js:117
>> +            '<tbody></tbody>' +
> 
> Should one of theres tests put something non-trivial into the tbody?

I should add that.

[Dimitri Glazkov (Google)]

Committed r93801: <http://trac.webkit.org/changeset/93801>