[webkit-reviews] review denied: [Bug 180126] Add a test health page. : [Attachment 328163] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied dewei_zhu at apple.com's request for
review:
Bug 180126: Add a test health page.
https://bugs.webkit.org/show_bug.cgi?id=180126

Attachment 328163: Patch

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




--- 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.