Merge pull request #173 from Polymer/measure-refactor

Improve representation of measurements

Author: aomarks

CHANGELOG.md

@@ -14,9 +14,26 @@ project adheres to [Semantic Versioning](http://semver.org/).
   "benchmarks": [
     {
       "measurement": {
-        "performanceEntry": {
-          "name": "foo"
-        }
+        "kind": "performance",
+        "entryName": "foo"
+      }
+    }
+  ]
+  ```
+
+- Add new syntax for specifying benchmarks:
+
+  ```
+  "benchmarks": [
+    {
+      "measurement": {
+        "kind": "callback"
+      }
+    },
+    {
+      "measurement": {
+        "kind": "expression",
+        "expression": "window.tachometerResult"
       }
     }
   ]

README.md

@@ -107,9 +107,8 @@ And in your config file:
 "benchmarks": [
   {
     "measurement": {
-      "performanceEntry": {
-        "name": "foo"
-      }
+      "kind": "performance",
+      "entryName": "foo"
     }
   }
 ]

config.schema.json

@@ -2,6 +2,21 @@
     "$schema": "http://json-schema.org/draft-07/schema#",
     "additionalProperties": false,
     "definitions": {
+        "CallbackMeasurement": {
+            "additionalProperties": false,
+            "properties": {
+                "kind": {
+                    "enum": [
+                        "callback"
+                    ],
+                    "type": "string"
+                }
+            },
+            "required": [
+                "kind"
+            ],
+            "type": "object"
+        },
         "ChromeConfig": {
             "additionalProperties": false,
             "properties": {
@@ -90,16 +105,13 @@
                 "measurement": {
                     "anyOf": [
                         {
-                            "additionalProperties": false,
-                            "properties": {
-                                "performanceEntry": {
-                                    "$ref": "#/definitions/PerformanceEntryCriteria"
-                                }
-                            },
-                            "required": [
-                                "performanceEntry"
-                            ],
-                            "type": "object"
+                            "$ref": "#/definitions/CallbackMeasurement"
+                        },
+                        {
+                            "$ref": "#/definitions/PerformanceEntryMeasurement"
+                        },
+                        {
+                            "$ref": "#/definitions/ExpressionMeasurement"
                         },
                         {
                             "enum": [
@@ -173,6 +185,25 @@
             ],
             "type": "object"
         },
+        "ExpressionMeasurement": {
+            "additionalProperties": false,
+            "properties": {
+                "expression": {
+                    "type": "string"
+                },
+                "kind": {
+                    "enum": [
+                        "expression"
+                    ],
+                    "type": "string"
+                }
+            },
+            "required": [
+                "expression",
+                "kind"
+            ],
+            "type": "object"
+        },
         "FirefoxConfig": {
             "additionalProperties": false,
             "properties": {
@@ -254,16 +285,22 @@
             "description": "A mapping from NPM package name to version specifier, as used in a\npackage.json's \"dependencies\" and \"devDependencies\".",
             "type": "object"
         },
-        "PerformanceEntryCriteria": {
+        "PerformanceEntryMeasurement": {
             "additionalProperties": false,
-            "description": "Criteria for matching a Performance Entry.",
             "properties": {
-                "name": {
+                "entryName": {
+                    "type": "string"
+                },
+                "kind": {
+                    "enum": [
+                        "performance"
+                    ],
                     "type": "string"
                 }
             },
             "required": [
-                "name"
+                "entryName",
+                "kind"
             ],
             "type": "object"
         },

src/config.ts

@@ -123,7 +123,9 @@ export async function makeConfig(opts: Opts): Promise<Config> {
   }
 
   for (const spec of config.benchmarks) {
-    if (spec.measurement === 'fcp' && !fcpBrowsers.has(spec.browser.name)) {
+    if (spec.measurement.kind === 'performance' &&
+        spec.measurement.entryName === 'first-contentful-paint' &&
+        !fcpBrowsers.has(spec.browser.name)) {
       throw new Error(
           `Browser ${spec.browser.name} does not support the ` +
           `first contentful paint (FCP) measurement`);

src/configfile.ts

@@ -114,7 +114,7 @@ interface ConfigFileBenchmark {
    *       }
    *     }
    */
-  measurement?: Measurement;
+  measurement?: ConfigFileMeasurement;
 
   /**
    * Expression to use to retrieve global result.  Defaults to
@@ -136,6 +136,8 @@ interface ConfigFileBenchmark {
   expand?: ConfigFileBenchmark[];
 }
 
+type ConfigFileMeasurement = 'callback'|'fcp'|'global'|Measurement;
+
 type BrowserConfigs =
     ChromeConfig|FirefoxConfig|SafariConfig|EdgeConfig|IEConfig;
 
@@ -338,13 +340,24 @@ async function parseBenchmark(benchmark: ConfigFileBenchmark, root: string):
     spec.browser = browser;
   }
 
-  if (benchmark.measurement !== undefined) {
+  if (benchmark.measurement === 'callback') {
+    spec.measurement = {
+      kind: 'callback',
+    };
+  } else if (benchmark.measurement === 'fcp') {
+    spec.measurement = {
+      kind: 'performance',
+      entryName: 'first-contentful-paint',
+    };
+  } else if (benchmark.measurement === 'global') {
+    spec.measurement = {
+      kind: 'expression',
+      expression:
+          benchmark.measurementExpression || defaults.measurementExpression,
+    };
+  } else {
     spec.measurement = benchmark.measurement;
   }
-  if (spec.measurement === 'global' &&
-      benchmark.measurementExpression !== undefined) {
-    spec.measurementExpression = benchmark.measurementExpression;
-  }
 
   const url = benchmark.url;
   if (url !== undefined) {
@@ -460,15 +473,7 @@ function applyDefaults(partialSpec: Partial<BenchmarkSpec>): BenchmarkSpec {
   if (measurement === undefined) {
     measurement = defaults.measurement(url);
   }
-  const spec: BenchmarkSpec = {name, url, browser, measurement};
-  if (measurement === 'global') {
-    if (partialSpec.measurementExpression === undefined) {
-      spec.measurementExpression = defaults.measurementExpression;
-    } else {
-      spec.measurementExpression = partialSpec.measurementExpression;
-    }
-  }
-  return spec;
+  return {name, url, browser, measurement};
 }
 
 export async function writeBackSchemaIfNeeded(

src/defaults.ts

@@ -27,7 +27,10 @@ export const measurementExpression = 'window.tachometerResult';
 
 export function measurement(url: LocalUrl|RemoteUrl): Measurement {
   if (url.kind === 'remote') {
-    return 'fcp';
+    return {
+      kind: 'performance',
+      entryName: 'first-contentful-paint',
+    };
   }
-  return 'callback';
+  return {kind: 'callback'};
 }

src/flags.ts

@@ -14,7 +14,7 @@ import * as path from 'path';
 
 import {supportedBrowsers} from './browser';
 import * as defaults from './defaults';
-import {Measurement, measurements} from './types';
+import {CommandLineMeasurements, measurements} from './types';
 
 import commandLineArgs = require('command-line-args');
 import commandLineUsage = require('command-line-usage');
@@ -216,7 +216,7 @@ export interface Opts {
   'sample-size': number|undefined;
   manual: boolean;
   save: string;
-  measure: Measurement|undefined;
+  measure: CommandLineMeasurements|undefined;
   'measurement-expression': string|undefined;
   horizon: string|undefined;
   timeout: number|undefined;

src/measure.ts

@@ -12,7 +12,7 @@
 import * as webdriver from 'selenium-webdriver';
 
 import {Server} from './server';
-import {BenchmarkSpec, PerformanceEntryCriteria} from './types';
+import {Measurement, PerformanceEntryMeasurement} from './types';
 import {throwUnreachable} from './util';
 
 /**
@@ -22,25 +22,18 @@ import {throwUnreachable} from './util';
  */
 export async function measure(
     driver: webdriver.WebDriver,
-    {measurement, measurementExpression}: BenchmarkSpec,
+    measurement: Measurement,
     server: Server|undefined): Promise<number|undefined> {
-  if (measurement === 'fcp') {
-    return queryForPerformanceEntry(driver, {name: 'first-contentful-paint'});
-  }
-  if (measurement === 'callback') {
-    if (server === undefined) {
-      throw new Error('Internal error: no server for spec');
-    }
-    return (await server.nextResults()).millis;
-  }
-  if (measurement === 'global') {
-    if (!measurementExpression) {
-      throw new Error('Internal error: no measurement expression');
-    }
-    return queryForExpression(driver, measurementExpression);
-  }
-  if (measurement && 'performanceEntry' in measurement) {
-    return queryForPerformanceEntry(driver, measurement.performanceEntry);
+  switch (measurement.kind) {
+    case 'callback':
+      if (server === undefined) {
+        throw new Error('Internal error: no server for spec');
+      }
+      return (await server.nextResults()).millis;
+    case 'expression':
+      return queryForExpression(driver, measurement.expression);
+    case 'performance':
+      return queryForPerformanceEntry(driver, measurement);
   }
   throwUnreachable(
       measurement,
@@ -71,8 +64,8 @@ interface PerformanceEntry {
  */
 async function queryForPerformanceEntry(
     driver: webdriver.WebDriver,
-    criteria: PerformanceEntryCriteria): Promise<number|undefined> {
-  const escaped = escapeStringLiteral(criteria.name);
+    measurement: PerformanceEntryMeasurement): Promise<number|undefined> {
+  const escaped = escapeStringLiteral(measurement.entryName);
   const script = `return window.performance.getEntriesByName(\`${escaped}\`);`;
   const entries = await driver.executeScript(script) as PerformanceEntry[];
   if (entries.length === 0) {

src/runner.ts

@@ -203,8 +203,9 @@ export class Runner {
       await driver.get(url);
       for (let waited = 0; millis === undefined && waited <= 10000;
            waited += 50) {
+        // TODO(aomarks) You don't have to wait in callback mode!
         await wait(50);
-        millis = await measure(driver, spec, server);
+        millis = await measure(driver, spec.measurement, server);
       }
 
       // Close the active tab (but not the whole browser, since the
@@ -224,21 +225,15 @@ export class Runner {
 
       console.log(
           `\n\nFailed ${attempt}/${maxAttempts} times ` +
-          `to get a ${spec.measurement} measurement ` +
-          (spec.measurement === 'global' ?
-               `(from '${spec.measurementExpression}') ` :
-               '') +
+          `to get a measurement ` +
           `in ${spec.browser.name} from ${url}. Retrying.`);
     }
 
     if (millis === undefined) {
       console.log();
       throw new Error(
           `\n\nFailed ${maxAttempts}/${maxAttempts} times ` +
-          `to get a ${spec.measurement} measurement ` +
-          (spec.measurement === 'global' ?
-               `(from '${spec.measurementExpression}') ` :
-               '') +
+          `to get a measurement ` +
           `in ${spec.browser.name} from ${url}. Retrying.`);
     }