[Webkit-unassigned] [Bug 180126] Add a test health page.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 5 11:40:09 PST 2017 

https://bugs.webkit.org/show_bug.cgi?id=180126

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #328163|review?                     |review-
              Flags|                            |

--- Comment #7 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 328163
  --> https://bugs.webkit.org/attachment.cgi?id=328163
Patch

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

> Websites/perf.webkit.org/ChangeLog:8
> +        Added a tet health page which shows freshness of a test.

Typo: tet -> test.
Also, this page isn't showing the freshness of tests, it's showing the freshness of test results.

> Websites/perf.webkit.org/ChangeLog:9
> +        Shows the health status of tests which are specified in summary pages.

It's not that summary page specifies which tests' health to reported.
It's that we're reporting the health status of the tests shown in the summary page; or put in another way,
the health status page reports on the same set of tests as the one shown in the summary page.

> Websites/perf.webkit.org/ChangeLog:13
> +        (TestHealthCell): A cell of the health status table, color will transit from green to red base on the time of last data point for a test.

This is a really long line. Wrap the line at some point?

> Websites/perf.webkit.org/ChangeLog:33
> +        * server-tests/api-manifest-tests.js: Added 'warningHourBaseline' and 'ratingCurveCoefficient' so that we can config the parameter of sigmoid funciton.

Wrap this line?

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:1
> +class TestHealthCell extends ComponentBase {

Since this is not a table cell, we should call it TestHealthStatus or something instead.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:13
> +        this._checkTimeInterval = 2 * 24 * 3600 * 1000;

It seems like this shouldn't be hard-coded here.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:23
> +            this._measurementSet.fetchBetween(this._measurementSetFetchTime - this._checkTimeInterval, this._measurementSetFetchTime).then(() => {

render() function shouldn't be the one doing fetching.
render() function in general shouldn't have that kind of side effect.
r- because this needs to be moved to elsewhere.

I think it's a cleaner design for the constructor of this class to take a measurement set,
and then it had a separate function like update() which does fetching.
We can then call it in loadConfig of TestHealthPage.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:29
> +                }
> +                else

Nit: } and else should be on the same line.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:35
> +        }
> +        else {

Ditto.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:37
> +            const rating = 1 / (1 + Math.pow(this._ratingCurveCoefficient, this.hoursSinceLastDataPoint() - this._warningHourBaseline));
> +            const hue = Math.round(120 * rating);

What is this function about? Please explain in the change log.
Also, I don't think it makes sense to make ratingCurveCoefficient configurable.
It's just to be simply hard-coded since it's not something the user would be keep adjusting over time.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:65
> +        for (const key in state)
> +            params.push(key + '=' + this._serializeHashQueryValue(state[key]));
> +        const query = params.length ? ('?' + params.join('&')) : '';
> +        return `/v3/#/charts${query}`;

This is still all wrong. See DashboardPage's line 101.
You just copy & paste that code here, not duplicate the logic to construct a magic string.
r- because of this.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:70
> +        return `${this._noDataWithinInterval ? 'More than ' : ''}${this.hoursSinceLastDataPoint().toFixed(2)} hours since last data point on platform: "${this._platform.name()}" test: "${this._metric.test().fullName()}"`;

Why don't we generalize BuildRequest's waitingTime and use it here instead?
That'd make the time interval label look much nicer.

> Websites/perf.webkit.org/public/v3/components/test-health-cell.js:80
> +        return `<div id='cell'></div>`

Nit: Needs a semicolon at the end.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:3
> +    constructor(configs, warningHourBaseline, ratingCurveCoefficient)

summaryPages isn't a list of configurations.
I think it's better to call this explicitly summaryPageConfiguration instead.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:9
> +        this.loadConfig(configs);

We should call this after having set this.constructTableLazily.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:13
> +    loadConfig(configs) {

Nit: { should be on the new line.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:15
> +        const platformWithOrder = [];
> +        const metricWithOrder = [];

What's the point of having separate array?
You can just use spread operator at where you're calling a map as in:
[...platformSet].map((platformId) => Platform.findById(platformId)).

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:20
> +            const platforms = [].concat(...config.platformGroups.map((platformGroup) => platformGroup.platforms));

These aren't platforms but rather platform IDs.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:25
> +                platformSet.add(platform);
> +                platformWithOrder.push(platform);

You should resolve platform IDs to platform right here.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:29
> +            const metrics = [].concat(...config.metricGroups.map((metricGroup) =>
> +                [].concat(...metricGroup.subgroups.map((subgroup) => subgroup.metrics))));

Ditto. These are metric IDs.
Having two nested concat like this is really hard to read.
Why don't we extract the lambda to a separate line with a descriptive name like metricsInGroup.
By the way, it's probably better to resolve metric ID to Metric objects right here instead.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:38
> +            for (const platform of Object.keys(excludedConfigs)) {

Just do: for (const platformID in excludedConfigs) instead.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:46
> +        this._platforms = platformWithOrder.map(Platform.findById.bind(Platform));
> +        this._metrics = metricWithOrder.map(Metric.findById.bind(Metric));

This is not a great way to resolve platform & metric IDs.
FWIW, some metric & platform IDs may not exist.
If anything it should be resolved when we're adding things to the set instead.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:52
> +        this.renderReplace(this.content().querySelector('.test-health'),

Use id="test-health" instead in the markup so that we can do this.content('test-health').
querySelector is the old way. I'm trying to get rid of its use.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:60
> +        const tableHeadElements = [element('th',  {class: 'table-corner'},'Platform \\ Test')];

Nit: Need a space before ,.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:63
> +            tableHeadElements.push(element('th', {class: 'diagonal-header'}, element('div', element('span', metric.test().path()[0].name()))));

Why are we not calling metric.test.label() instead?

Not that for LabeledObject, label() is the human readable name and name() is some machine readable name
even though Test object currently aliases label() to name().
Nevertheless, we should be calling label() since what we want here is the human readable name.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:66
> +            const row = [element('th', platform.name())];

Ditto: platform.label().

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:69
> +            metrics.forEach((metric) => {
> +                row.push(this.constructTableCell(platform, metric, excludedConfigurations));
> +            });

More canonical way to write this in ES6 would be:
const row = [element('th', platform.name()), ...metrics.map((metric) => this.constructTableCell(platform, metric, excludedConfigurations))]

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:81
> +            return element('td', {class: 'blank-cell'}, element('div'));

Instead of manually inserting a div just to show a grey box,
generate that synthetically in CSS using either ::before or ::after.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:84
> +            return element('td', {class: 'blank-cell'}, element('div'));

Ditto.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:121
> +            .test-health th.diagonal-header > div {
> +                transform:
> +                  translate(1rem, 4rem)
> +                  rotate(315deg);

Nit: typically, the entire transform is writing in a single line.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171205/9a2e20b6/attachment-0001.html>

More information about the webkit-unassigned mailing list