🏗 ♻️ Fix type errors in /build-system/tasks/e2e (#33166)

* fix errors in /tasts/e2e * update typing in puppeteer-controller * solve controller-promise typing puzzle
ampproject · Apr 14, 2021 · 5ebf84e · 5ebf84e
1 parent cdc2855
commit 5ebf84e
Show file tree

Hide file tree

Showing 13 changed files with 199 additions and 185 deletions.
diff --git a/build-system/global.d.ts b/build-system/global.d.ts
@@ -14,6 +14,11 @@ declare global {
   interface EslintContext {
     report: (val: any) => void;
   }
+
+  interface Window {
+    queryXpath: (xpath: string, root: unknown /** Puppeteer.ElementHandle */) => unknown[] | null;
+    AMP: Function[];
+  }
 }
 
 export { }
diff --git a/build-system/tasks/e2e/amp-driver.js b/build-system/tasks/e2e/amp-driver.js
@@ -135,8 +135,7 @@ const EnvironmentBehaviorMap = {
 
 /**
  * @param {string} url
- * @param {Object=} opts
- * @param {boolean} opts.isEmail
+ * @param {{isEmail: boolean}=} opts
  * @return {string}
  */
 function getViewerUrl(url, {isEmail} = {isEmail: false}) {
@@ -163,7 +162,7 @@ function getViewerUrl(url, {isEmail} = {isEmail: false}) {
  */
 class AmpDriver {
   /**
-   * @param {!../functional-test-controller.FunctionalTestController} controller
+   * @param {!./functional-test-controller.FunctionalTestController} controller
    */
   constructor(controller) {
     /** @private @const */

diff --git a/build-system/tasks/e2e/controller-promise.js b/build-system/tasks/e2e/controller-promise.js
@@ -22,12 +22,12 @@
  * the new values that come from the browser.
  *
  * @template TYPE
- * @extends {Promise}
+ * @extends {Promise<TYPE>}
  */
 class ControllerPromise {
   /**
-   * @param {function(function(?TYPE):void, function(*):void):void|!Promise<TYPE>} executorOrPromise
-   * @param {function(TYPE,function(TYPE): ?TYPE): !Promise=} opt_waitForValue
+   * @param {!Promise<TYPE>|function(function(?TYPE):void, function(*):void):void} executorOrPromise
+   * @param {undefined|function(TYPE,function(TYPE): ?TYPE): Promise<TYPE>} opt_waitForValue
    */
   constructor(executorOrPromise, opt_waitForValue) {
     this.promise_ =
@@ -62,9 +62,8 @@ class ControllerPromise {
 
   /** @override */
   then(opt_onFulfilled, opt_onRejected) {
-    opt_onFulfilled = opt_onFulfilled || ((x) => x);
     // Allow this and future `then`s to update the wait value.
-    let wrappedWait = null;
+    let wrappedWait;
     if (this.waitForValue) {
       wrappedWait = wrapWait(this.waitForValue, opt_onFulfilled);
     }
@@ -80,14 +79,18 @@ class ControllerPromise {
  * Wrap the given wait function with the given mutation function,
  * while still allowing it to be mutated again in the future by
  * the inner opt_mutate function.
- * @param {function(TYPE,function(TYPE): ?TYPE): !Promise=} wait
- * @param {function(TYPE): TYPE} mutate
- * @return {!Promise}
- * @template TYPE
+ * @param {function(CONDITION, function(VALUE): ?DERIVED): Promise<RES>} wait
+ * @param {function(VALUE): MUTANT} mutate
+ * @return {function(CONDITION, function(MUTANT): ?DERIVED): Promise<RES>}
+ * @template CONDITION
+ * @template MUTANT
+ * @template DERIVED
+ * @template VALUE
+ * @template RES
  */
 function wrapWait(wait, mutate) {
   return (condition, opt_mutate) => {
-    opt_mutate = opt_mutate || ((x) => x);
+    opt_mutate = opt_mutate || ((x) => /** @type {*} */ (x));
     return wait(condition, (value) => opt_mutate(mutate(value)));
   };
 }

diff --git a/build-system/tasks/e2e/describes-e2e.js b/build-system/tasks/e2e/describes-e2e.js
@@ -22,6 +22,7 @@ const chrome = require('selenium-webdriver/chrome');
 const fetch = require('node-fetch');
 const firefox = require('selenium-webdriver/firefox');
 const puppeteer = require('puppeteer');
+const selenium = require('selenium-webdriver');
 const {
   clearLastExpectError,
   getLastExpectError,
@@ -31,11 +32,11 @@ const {
   SeleniumWebDriverController,
 } = require('./selenium-webdriver-controller');
 const {AmpDriver, AmpdocEnvironment} = require('./amp-driver');
-const {Builder, Capabilities, logging} = require('selenium-webdriver');
 const {HOST, PORT} = require('../serve');
 const {installRepl, uninstallRepl} = require('./repl');
 const {isCiBuild} = require('../../common/ci');
 const {PuppeteerController} = require('./puppeteer-controller');
+const {Builder, Capabilities, logging} = selenium;
 
 /** Should have something in the name, otherwise nothing is shown. */
 const SUB = ' ';
@@ -81,14 +82,14 @@ let DescribesConfigDef;
 
 /**
  * @typedef {{
- *  headless: boolean,
+ *  headless?: boolean,
  * }}
  */
 let PuppeteerConfigDef;
 
 /**
  * @typedef {{
- *  headless: boolean,
+ *  headless?: boolean,
  * }}
  */
 let SeleniumConfigDef;
@@ -126,7 +127,7 @@ function getConfig() {
 /**
  * Configure and launch a Puppeteer instance
  * @param {!PuppeteerConfigDef=} opt_config
- * @return {!Promise}
+ * @return {!Promise<puppeteer.Browser>}
  */
 async function createPuppeteer(opt_config = {}) {
   const browser = await puppeteer.launch({
@@ -142,16 +143,16 @@ async function createPuppeteer(opt_config = {}) {
  * Configure and launch a Selenium instance
  * @param {string} browserName
  * @param {!SeleniumConfigDef=} args
- * @param {?string} deviceName
- * @return {!WebDriver}
+ * @param {string=} deviceName
+ * @return {!selenium.WebDriver}
  */
 function createSelenium(browserName, args = {}, deviceName) {
   switch (browserName) {
     case 'safari':
       // Safari's only option is setTechnologyPreview
-      return createDriver(browserName, []);
+      return createDriver(browserName, [], deviceName);
     case 'firefox':
-      return createDriver(browserName, getFirefoxArgs(args));
+      return createDriver(browserName, getFirefoxArgs(args), deviceName);
     case 'chrome':
     default:
       return createDriver(browserName, getChromeArgs(args), deviceName);
@@ -161,9 +162,9 @@ function createSelenium(browserName, args = {}, deviceName) {
 /**
  *
  * @param {string} browserName
- * @param {!SeleniumConfigDef=} args
- * @param {?string} deviceName
- * @return {!WebDriver}
+ * @param {!string[]} args
+ * @param {string=} deviceName
+ * @return {!selenium.WebDriver}
  */
 function createDriver(browserName, args, deviceName) {
   const capabilities = Capabilities[browserName]();
@@ -239,7 +240,7 @@ function getFirefoxArgs(config) {
  *  environments: (!Array<!AmpdocEnvironment>|undefined),
  *  testUrl: string|undefined,
  *  fixture: string,
- *  initialRect: ({{width: number, height:number}}|undefined),
+ *  initialRect: ({width: number, height:number}|undefined),
  *  deviceName: string|undefined,
  *  version: string|undefined,
  * }}
@@ -252,9 +253,9 @@ let TestSpec;
  *  environments: (!Array<!AmpdocEnvironment>|undefined),
  *  testUrl: string|undefined,
  *  fixture: string,
- *  initialRect: ({{width: number, height:number}}|undefined),
+ *  initialRect: ({width: number, height:number}|undefined),
  *  deviceName: string|undefined,
- *  versions: {{[version: string]: TestSpec}}
+ *  versions: {[version: string]: TestSpec}
  * }}
  */
 let RootSpec;
@@ -328,11 +329,11 @@ envPresets['ampdoc-amp4ads-preset'] = envPresets['ampdoc-preset'].concat(
  */
 class ItConfig {
   /**
-   * @param {Function} it
+   * @param {function} it
    * @param {Object} env
    */
   constructor(it, env) {
-    this.it = it;
+    this.it = /** @type {Mocha.it} */ (it);
     this.env = env;
     this.skip = false;
   }
@@ -371,8 +372,8 @@ class ItConfig {
 
   /**
    * @param {string} name
-   * @param {Function} fn
-   * @return {ItConfig}
+   * @param {function(): void} fn
+   * @return {void|Mocha.Test}
    */
   run(name, fn) {
     if (this.skip) {
@@ -414,16 +415,16 @@ async function reportCoverage() {
  * Returns a wrapped version of Mocha's describe(), it() and only() methods
  * that also sets up the provided fixtures and returns the corresponding
  * environment objects of each fixture to the test method.
- * @param {function(!Object):!Array<?Fixture>} factory
- * @return {function()}
+ * @param {function(!TestSpec): EndToEndFixture} factory
+ * @return {function(string, RootSpec, function(!Object): void): void}
  */
 function describeEnv(factory) {
   /**
    * @param {string} suiteName
    * @param {!Object} spec
-   * @param {function(!Object)} fn
-   * @param {function(string, function())} describeFunc
-   * @return {function()}
+   * @param {function(!Object): void} fn
+   * @param {function(string, function(): void): void} describeFunc
+
    */
   const templateFunc = function (suiteName, spec, fn, describeFunc) {
     const fixture = factory(spec);
@@ -501,9 +502,10 @@ function describeEnv(factory) {
       }
     }
 
-    return describeFunc(suiteName, function () {
+    describeFunc(suiteName, function () {
       createBrowserDescribe();
     });
+    return;
 
     /**
      *
@@ -561,8 +563,7 @@ function describeEnv(factory) {
   /**
    * @param {string} name
    * @param {!RootSpec} spec
-   * @param {function(!Object)} fn
-   * @return {function()}
+   * @param {function(!Object): void} fn
    */
   const mainFunc = function (name, spec, fn) {
     const {versions, ...baseSpec} = spec;
@@ -594,15 +595,20 @@ function describeEnv(factory) {
   /**
    * @param {string} name
    * @param {!Object} spec
-   * @param {function(!Object)} fn
-   * @return {function()}
+   * @param {function(!Object): void} fn
    */
   mainFunc.only = function (name, spec, fn) {
-    return templateFunc(name, spec, fn, describe./*OK*/ only);
+    templateFunc(name, spec, fn, describe./*OK*/ only);
+    return;
   };
 
+  /**
+   * @param {string} name
+   * @param {!Object} variants
+   * @param {function(!Object): void} fn
+   */
   mainFunc.skip = function (name, variants, fn) {
-    return templateFunc(name, variants, fn, describe.skip);
+    templateFunc(name, variants, fn, describe.skip);
   };
 
   return mainFunc;
@@ -688,8 +694,8 @@ class EndToEndFixture {
  * Get the driver for the configured engine.
  * @param {!DescribesConfigDef} describesConfig
  * @param {string} browserName
- * @param {?string} deviceName
- * @return {!ThenableWebDriver}
+ * @param {string|undefined} deviceName
+ * @return {!selenium.WebDriver|Promise<puppeteer.Browser>}
  */
 function getDriver(
   {engine = EngineType.SELENIUM, headless = false},
@@ -706,7 +712,6 @@ function getDriver(
 }
 
 /**
- *
  * @param {{
  *  environment: *,
  *  ampDriver: *,

diff --git a/build-system/tasks/e2e/expect.js b/build-system/tasks/e2e/expect.js
@@ -136,8 +136,8 @@ const chaiMethodsAndProperties = [
 ];
 
 /**
- * @param {Chai.ChaiStatic} chai
- * @param {Chai.ChaiUtils} utils
+ * @param {chai.ChaiStatic} chai
+ * @param {chai.ChaiUtils} utils
  */
 function installWrappers(chai, utils) {
   const {METHOD, PROPERTY, CHAINABLE_METHOD} = ChaiType;
@@ -263,9 +263,8 @@ function installBrowserAssertions(_networkLogger) {
 }
 
 /**
- *
- * @param {Chai.ChaiStatic} chai
- * @param {Chai.ChaiUtils} utils
+ * @param {chai.ChaiStatic} chai
+ * @param {chai.ChaiUtils} utils
  */
 function installBrowserWrappers(chai, utils) {
   const {Assertion} = chai;

diff --git a/build-system/tasks/e2e/functional-test-controller.js b/build-system/tasks/e2e/functional-test-controller.js
@@ -26,10 +26,9 @@ class ElementHandle {
   /**
    * Wrap the framework-specific element handle object.
    * @param {!T} element
-   * @param {!T} unusedController
    * @package
    */
-  constructor(element, unusedController) {
+  constructor(element) {
     /** @private */
     this.element_ = element;
   }

diff --git a/build-system/tasks/e2e/index.js b/build-system/tasks/e2e/index.js
@@ -48,7 +48,7 @@ const COV_OUTPUT_HTML = path.resolve(COV_OUTPUT_DIR, 'lcov-report/index.html');
 
 /**
  * Set up the e2e testing environment.
- * @return {!Promise}
+ * @return {!Promise<void>}
  */
 async function setUpTesting_() {
   require('@babel/register')({caller: {name: 'test'}});
@@ -114,6 +114,7 @@ function addMochaFile_(mocha, file) {
 /**
  * Fetch aggregated coverage data from server.
  * @param {string} outDir relative path to coverage files directory.
+ * @return {Promise<void>}
  */
 async function fetchCoverage_(outDir) {
   // Note: We could access the coverage UI directly through the server started
@@ -127,6 +128,7 @@ async function fetchCoverage_(outDir) {
 
   const zipFilename = path.join(outDir, 'coverage.zip');
   const zipFile = fs.createWriteStream(zipFilename);
+
   await new Promise((resolve, reject) => {
     http
       .get(
@@ -137,7 +139,10 @@ async function fetchCoverage_(outDir) {
         },
         (response) => {
           response.pipe(zipFile);
-          zipFile.on('finish', () => zipFile.close(resolve));
+          zipFile.on('finish', () => {
+            zipFile.close();
+            resolve();
+          });
         }
       )
       .on('error', (err) => {
@@ -150,7 +155,7 @@ async function fetchCoverage_(outDir) {
 
 /**
  * Runs e2e tests on all files under test.
- * @return {!Promise}
+ * @return {!Promise<void>}
  */
 async function runTests_() {
   const mocha = createMocha_();
@@ -183,7 +188,7 @@ async function runTests_() {
 
 /**
  * Watches files a under test, running affected e2e tests on changes.
- * @return {!Promise}
+ * @return {!Promise<void>}
  */
 async function runWatch_() {
   const filesToWatch = argv.files ? getFilesFromArgv() : config.e2eTestPaths;

diff --git a/build-system/tasks/e2e/mocha-custom-json-reporter.js b/build-system/tasks/e2e/mocha-custom-json-reporter.js
@@ -28,8 +28,7 @@ const {Base} = require('mocha').reporters;
 const {inherits} = require('mocha').utils;
 
 /**
- *
- * @param {!Object} output
+ * @param {Object} output
  * @param {string} filename
  * @return {Promise<void>}
  */