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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 12 16:31:10 PST 2017


Ryosuke Niwa <rniwa at webkit.org> changed:

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

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

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.

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

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/20171213/db6e40aa/attachment-0001.html>

More information about the webkit-unassigned mailing list