[webkit-reviews] review denied: [Bug 180126] Add a test health page. : [Attachment 328751] 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 328751: Patch

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




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

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

This patch looks better but I think we need one more iteration of polishing
before we can land.

> Websites/perf.webkit.org/public/include/manifest-generator.php:47
> +	       'warningHourBaseline' => config('warningHourBaseline'),

I don't think this variable name makes much sense. What does "baseline" mean in
this context?

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

I would call this FreshnessIndicator or so.
There is nothing about this component that's specific to test other than the
fact it takes platform & metric just to create a label.
We're better off creating that label in the page label and just passing in a
label for the configuration here.

> Websites/perf.webkit.org/public/v3/components/test-health-status.js:20
> +	   if (!this._timeForLastDataPoint)
> +	       this.renderReplace(this.content('container'), new SpinnerIcon);

Just add an early exit here instead of having else.

> Websites/perf.webkit.org/public/v3/components/test-health-status.js:36
> +	   return `${this._noCurrentDataPoint ? 'More than ' :
''}${BuildRequest.formatTimeInterval((this._measurementSetFetchTime -
this._timeForLastDataPoint) / 1000)}` +
> +	       `since last data point on platform: "${this._platform.name()}"
test: "${this._metric.test().fullName()}"`;

These two lines are really hard to read.
Store the results of each JS expression into a separate local variable so that
the whole string is easier to read?
If you've made the above suggestion, then you can do `~ since the last data
point for {$this._configLabel}`
where this._configLabel is `"${this._metric.test().fullName()}" for
"${this._platform.name()}"`,
which is constructed in the page component. There is no need to say test or
platform.
Names such as Speedometer and macOS would make it obvious what we're talking
about.

> Websites/perf.webkit.org/public/v3/models/build-request.js:87
> +    static formatTimeInterval(intervalInSeconds) {

Could you add some unit tests for this function?
There are many tests for waitingTime in unit-tests/build-request-tests.js
ready.
So just add a few to make sure formatTimeInterval doest the same thing.

> Websites/perf.webkit.org/public/v3/models/build-request.js:125
> +	   const diffInSeconds = (referenceTime - this.createdAt()) / 1000;

Instead of forcing each caller to divide the number with 1000, just pass in
time in milliseconds.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:2
> +class TestHealthPage extends PageWithHeading {

I'd call this TestFreshnessPage instead because "health" can mean many things.
e.g. current test results being 50% worse than baseline might be "unhealthy"
but that's not what this page tells you.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:5
> +	   super('Test-Health', null);

Don't capitalize the component/page name. It should all be in lower case.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:7
> +	   this._checkTimeInterval = this._warningHourBaseline * 2 * 3600 *
1000;

I'd call this variable _timeDuration instead.
Time interval seems to imply more of a range (i.e. start & end) than the
duration.
e.g. https://en.wikipedia.org/wiki/Interval_(mathematics)

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:9
> +	   this._healthStatusForPlatformAndMetric = null;

This map doesn't store health status at all. It just stores data points in a
given platform & metric.
lastDataPointByConfiguration?

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:15
> +    _loadConfig(summaryPageConfiguration) {

Nit: { should be on a new line.

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

Instead of flattening arrays upfront like this, you can just have a nested for
loops that go over platformGroups and platforms.
That'd make the code a lot clearer.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:36
> +	       const metricIds = [].concat(...[].concat(...metricsInGroup));
> +	       for (const metricId of metricIds) {

Ditto. Two nested concat like this is impossible to reason.

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

Nit: _*fetch*DataPointsForTests, { should be on a new line.
Also, I'd call this _fetchTimeSeries or _fetchTestResults instead.
Data points is very specific kind of data returned by timeSeries.data() in our
codebase.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:67
> +	       const healthStatusByMetric = new Map;

This map doesn't store health status. It stores the time for the last data
point & url.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:77
> +		       let noCurrentDataPoint = false;

There is no need to have this separate boolean stored.
We can use timeForLastDataPoint === null as !noCurrentDataPoint.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:86
> +			   metric.id(), this._measurementSetFetchTime -
this._checkTimeInterval));

Please store the difference as a local variable, e.g. startTime instead of
duplicating the subtraction everywhere.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:87
> +		       healthStatusByMetric.set(metric, [timeForLastDataPoint,
noCurrentDataPoint, url]);

Use dictionary instead of an array for clarity.

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

Why are you creating div & span?
In general, creating wrapper elements like these is an anti-pattern.
We're better off using SVG and alike if you're trying to create a rectangle,
etc... with it.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:112
> +		   const [timeForLastDataPoint, noCurrentDataPoint, url] =
healthStatusForPlatformAndMetric.get(platform).get(metric) || [null, null,
null];

There is virtually no point in storing the URL earlier and using it here.
Here, we have platform, metric, and the startTime can be computed from the
instance variables.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:121
> +    static _isValidPlatformMetricCombination(platform, metric,
excludedConfigurations) {

Nit: { should be on a new line.
What's the point of making this function static?
I think we're better off making it an instance method so that you don't have to
pass in excludedConfigurations.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:127
> +    _constructTableCell(platform, metric, excludedConfigurations,
timeForLastDataPoint, noCurrentDataPoint, url) {

Nit: { should be on a new line.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:133
> +	   return element('td', {class: 'status-cell'}, new
TestHealthStatus(platform, metric, this._warningHourBaseline,
this._measurementSetFetchTime, timeForLastDataPoint, noCurrentDataPoint, url));

Instead of re-creating the entire table each time, we should simply construct
the table once,
and then for each status component, we should update its value in a separate
loop.
In subsequent calls to render(), we would only execute that second loop.

> Websites/perf.webkit.org/public/v3/pages/test-health-page.js:169
> +	       #test-health th.diagonal-header > div > span {

We shouldn't have a span just so that can have a border line this. That's an
anti-pattern.