chore(appium): peer review changes
add note about argparse tomfoolery
refactor `list` command impl for better types
rewrite exception in `driver-config`
add `hasDriver`, `hasPlugin`, and small refactors in `manifest`
add note about args in `grid-register`
remove invalid comment in `main`
more test coverage for `manifest`
MODIFY
packages/appium/lib/cli/args.js
packages/appium/lib/cli/args.js
@@ -8,7 +8,12 @@ import { toParserArgs } from '../schema/cli-args';
const DRIVER_EXAMPLE = 'xcuitest';
const PLUGIN_EXAMPLE = 'find_by_image';
+/**
+ * This is necessary because we pass the array into `argparse`. `argparse` is bad and mutates things. We don't want that.
+ * Bad `argparse`! Bad!
+ */
const INSTALL_TYPES_ARRAY = [...INSTALL_TYPES];
+
/** @type {Set<ExtensionType>} */
const EXTENSION_TYPES = new Set([DRIVER_TYPE, PLUGIN_TYPE]);

MODIFY
packages/appium/lib/cli/extension-command.js
packages/appium/lib/cli/extension-command.js
@@ -17,24 +17,38 @@ class NoUpdatesAvailableError extends Error {}
* @template {ExtensionType} ExtType
*/
export default class ExtensionCommand {
+ /**
+ * This is the `DriverConfig` or `PluginConfig`, depending on `ExtType`.
+ * @type {ExtensionConfig<ExtType>}
+ */
+ config;
/**
- * Build an ExtensionCommand
- * @param {ExtensionCommandOptions<ExtType>} opts
+ * {@linkcode Record} of official plugins or drivers.
+ * @type {ExtType extends import('../extension/manifest').DriverType ? typeof import('../constants').KNOWN_DRIVERS : typeof import('../constants').KNOWN_PLUGINS}
*/
- constructor ({config, json, type}) {
- /** @type {ExtensionConfig<ExtType>} */
- this.config = config;
+ knownExtensions;
- /** @type {ExtType} */
- this.type = type;
/**
+ * If `true`, command output has been requested as JSON.
* @type {boolean}
*/
+ isJsonOutput;
+
+ /**
+ * Build an ExtensionCommand
+ * @param {ExtensionCommandOptions<ExtType>} opts
+ */
+ constructor ({config, json}) {
+ this.config = config;
this.isJsonOutput = json;
+ }
- /** @type {Record<string, string>} */
- this.knownExtensions = {}; // this needs to be overridden in final class
+ /**
+ * `driver` or `plugin`, depending on the `ExtensionConfig`.
+ */
+ get type () {
+ return this.config.extensionType;
}
/**
@@ -85,8 +99,7 @@ export default class ExtensionCommand {
return;
}
for (const [ext, data] of _.toPairs(exts)) {
- const {installed, installType} = data;
- if (!installed || installType !== INSTALL_TYPE_NPM) {
+ if (!data.installed || data.installType !== INSTALL_TYPE_NPM) {
// don't need to check for updates on exts that aren't installed
// also don't need to check for updates on non-npm exts
continue;
@@ -106,8 +119,14 @@ export default class ExtensionCommand {
for (const [
name,
- {installType, installSpec, installed, updateVersion, unsafeUpdateVersion, version, upToDate}
+ data
] of _.toPairs(exts)) {
+ let installTxt = ' [not installed]'.grey;
+ let updateTxt = '';
+ let upToDateTxt = '';
+ let unsafeUpdateTxt = '';
+ if (data.installed) {
+ const {installType, installSpec, updateVersion, unsafeUpdateVersion, version, upToDate} = data;
let typeTxt;
switch (installType) {
case INSTALL_TYPE_GIT:
@@ -120,18 +139,20 @@ export default class ExtensionCommand {
default:
typeTxt = '(NPM)';
}
- const installTxt = installed ?
- `@${/** @type {string} */(version).yellow} ${('[installed ' + typeTxt + ']').green}` :
- ' [not installed]'.grey;
- const updateTxt = showUpdates && updateVersion ?
- ` [${updateVersion} available]`.magenta :
- '';
- const upToDateTxt = showUpdates && upToDate ?
- ` [Up to date]`.green :
- '';
- const unsafeUpdateTxt = showUpdates && unsafeUpdateVersion ?
- ` [${unsafeUpdateVersion} available (potentially unsafe)]`.cyan :
- '';
+ installTxt = `@${version.yellow} ${('[installed ' + typeTxt + ']').green}`;
+
+ if (showUpdates) {
+ if (updateVersion) {
+ updateTxt = ` [${updateVersion} available]`.magenta;
+ }
+ if (upToDate) {
+ upToDateTxt = ` [Up to date]`.green;
+ }
+ if (unsafeUpdateVersion) {
+ unsafeUpdateTxt = ` [${unsafeUpdateVersion} available (potentially unsafe)]`.cyan;
+ }
+ }
+ }
console.log(`- ${name.yellow}${installTxt}${updateTxt}${upToDateTxt}${unsafeUpdateTxt}`);
}

MODIFY
packages/appium/lib/extension/driver-config.js
packages/appium/lib/extension/driver-config.js
@@ -160,7 +160,7 @@ export class DriverConfig extends ExtensionConfig {
log.info(`Will require it at ${this.getInstallPath(driverName)}`);
const driver = this.require(driverName);
if (!driver) {
- throw new Error(`MainClass ${mainClass} did not result in a driver object`);
+ throw new Error(`Driver '${driverName}' did not export a class with name '${mainClass}'. Contact the author of the driver!`);
}
return {driver, version, driverName};
} catch (err) {

MODIFY
packages/appium/lib/extension/manifest.js
packages/appium/lib/extension/manifest.js
@@ -194,11 +194,13 @@ export class Manifest {
try {
const pkg = await env.readPackageInDir(filepath);
if (pkg && isExtension(pkg)) {
- this.addExtensionFromPackage(
+ // it's possible that this extension already exists in the manifest,
+ // so only update `didChange` if it's new.
+ const added = this.addExtensionFromPackage(
pkg,
path.join(filepath, 'package.json'),
);
- didChange = true;
+ didChange = didChange || added;
}
} catch {}
}
@@ -206,6 +208,24 @@ export class Manifest {
return didChange;
}
+ /**
+ * Returns `true` if driver with name `name` is registered.
+ * @param {string} name - Driver name
+ * @returns {boolean}
+ */
+ hasDriver (name) {
+ return Boolean(this._data.drivers[name]);
+ }
+
+ /**
+ * Returns `true` if plugin with name `name` is registered.
+ * @param {string} name - Plugin name
+ * @returns {boolean}
+ */
+ hasPlugin (name) {
+ return Boolean(this._data.plugins[name]);
+ }
+
/**
* Given a path to a `package.json`, add it as either a driver or plugin to the manifest.
*
@@ -213,13 +233,9 @@ export class Manifest {
* @template {ExtensionType} ExtType
* @param {ExtensionPackageJson<ExtType>} pkgJson
* @param {string} pkgPath
- * @returns {void}
+ * @returns {boolean} - `true` upon success, `false` if the extension is already registered.
*/
addExtensionFromPackage (pkgJson, pkgPath) {
- if (!isExtension(pkgJson)) {
- return;
- }
-
/**
* @type {InternalData}
*/
@@ -231,19 +247,23 @@ export class Manifest {
};
if (isDriver(pkgJson)) {
- if (!this._data.drivers[pkgJson.appium.driverName]) {
- this._data.drivers[pkgJson.appium.driverName] = {
+ if (!this.hasDriver(pkgJson.appium.driverName)) {
+ this.addExtension(DRIVER_TYPE, pkgJson.appium.driverName, {
..._.omit(pkgJson.appium, 'driverName'),
- ...internal,
- };
+ ...internal
+ });
+ return true;
}
+ return false;
} else if (isPlugin(pkgJson)) {
- if (!this._data.plugins[pkgJson.appium.pluginName]) {
- this._data.plugins[pkgJson.appium.pluginName] = {
+ if (!this.hasPlugin(pkgJson.appium.pluginName)) {
+ this.addExtension(PLUGIN_TYPE, pkgJson.appium.pluginName, {
..._.omit(pkgJson.appium, 'pluginName'),
...internal,
- };
+ });
+ return true;
}
+ return false;
} else {
throw new TypeError(
`The extension in ${path.dirname(
@@ -254,14 +274,14 @@ export class Manifest {
}
/**
- * Adds an extension to the manifest as was installed by the `appium` CLI. It determines the
- * `extData`, `extType`, `extName` itself.
+ * Adds an extension to the manifest as was installed by the `appium` CLI. The
+ * `extData`, `extType`, and `extName` have already been determined.
*
* See {@link Manifest.addExtensionFromPackage} for adding an extension from an on-disk package.
* @template {ExtensionType} ExtType
- * @param {ExtType} extType
- * @param {string} extName
- * @param {ExtData<ExtType>} extData
+ * @param {ExtType} extType - `driver` or `plugin`
+ * @param {string} extName - Name of extension
+ * @param {ExtData<ExtType>} extData - Extension metadata
* @returns {void}
*/
addExtension (extType, extName, extData) {

MODIFY
packages/appium/lib/grid-register.js
packages/appium/lib/grid-register.js
@@ -11,7 +11,7 @@ const hubUri = (config) => {
/**
* Registers a new node with a selenium grid
- * @param {string|object} data - Path or object representing selenium grid node config file
+ * @param {string|object} data - Path or object representing selenium grid node config file. If a `string`, all subsequent arguments are required!
* @param {string} [addr] - Bind to this address
* @param {number} [port] - Bind to this port
* @param {string} [basePath] - Base path for the grid

MODIFY
packages/appium/lib/main.js
packages/appium/lib/main.js
@@ -246,8 +246,6 @@ async function init (args) {
* @returns {Promise<import('http').Server|undefined>}
*/
async function main (args) {
- // Appium drivers often use process events to schedule proper exit cleanup
- // The default value of 10 causes an ugly warning to appear in logs on server startup
const {appiumDriver, parsedArgs, pluginConfig, driverConfig} = /** @type {ServerInitResult} */(await init(args));
if (!appiumDriver || !parsedArgs || !pluginConfig || !driverConfig) {

MODIFY
packages/appium/test/unit/extension/manifest.spec.js
packages/appium/test/unit/extension/manifest.spec.js
// @ts-check
import B from 'bluebird';
import { promises as fs } from 'fs';
+import { DRIVER_TYPE, PLUGIN_TYPE } from '../../../lib/constants';
import { resolveFixture, rewiremock } from '../../helpers';
import { initMocks } from './mocks';
@@ -68,15 +69,53 @@ describe('Manifest', function () {
});
describe('property', function () {
- describe('filepath', function () {
+ describe('appiumHome', function () {
+ it('should return the `appiumHome` path', function () {
+ expect(Manifest.getInstance('/some/path').appiumHome).to.equal(
+ '/some/path',
+ );
+ });
+
it('should not be writable', function () {
const instance = Manifest.getInstance('/some/path');
expect(() => {
- // @ts-ignore
+ // @ts-expect-error
instance.appiumHome = '/some/other/path';
}).to.throw(TypeError);
});
});
+
+ describe('manifestPath', function () {
+ describe('before `read()` has been called', function () {
+ it('should be undefined', function () {
+ expect(Manifest.getInstance('/some/path').manifestPath).to.be
+ .undefined;
+ });
+ });
+
+ describe('after `read()` has been called', function () {
+ let manifest;
+ beforeEach(async function () {
+ manifest = Manifest.getInstance('/some/path');
+ await manifest.read();
+ });
+
+ it('should return the manifest file path', function () {
+ // this path is not the actual path; it's mocked in `MockAppiumSupport.env.resolveManifestPath`.
+ expect(Manifest.getInstance('/some/path').manifestPath).to.equal(
+ '/some/path/extensions.yaml',
+ );
+ });
+ });
+
+ it('should not be writable', function () {
+ const instance = Manifest.getInstance('/some/path');
+ expect(() => {
+ // @ts-expect-error
+ instance.manifestPath = '/some/other/path';
+ }).to.throw(TypeError);
+ });
+ });
});
describe('instance method', function () {
@@ -220,38 +259,35 @@ describe('Manifest', function () {
});
});
- describe('when called after adding a property', function () {
- beforeEach(function () {
- data.drivers.foo = extData;
- });
-
- it('should write the file', async function () {
+ describe('when the manifest file was successfully written to', function () {
+ it('should return `true`', async function () {
expect(await manifest.write()).to.be.true;
});
});
- describe('when called after deleting a property', function () {
- beforeEach(async function () {
+ describe('when the manifest file could not be written', function () {
+ beforeEach(function () {
+ MockAppiumSupport.fs.writeFile.rejects();
data.drivers.foo = extData;
- await manifest.write();
- delete data.drivers.foo;
});
- it('should write the file', async function () {
- expect(await manifest.write()).to.be.true;
+ it('should reject', async function () {
+ await expect(manifest.write()).to.be.rejectedWith(
+ Error,
+ /Appium could not write to manifest/i,
+ );
});
});
- describe('when the manifest file could not be written', function () {
+ describe('when the manifest directory could not be created', function () {
beforeEach(function () {
- MockAppiumSupport.fs.writeFile.rejects();
- data.drivers.foo = extData;
+ MockAppiumSupport.fs.mkdirp.rejects();
});
it('should reject', async function () {
await expect(manifest.write()).to.be.rejectedWith(
Error,
- /Appium could not write to manifest/i,
+ /could not create the directory for the manifest file/i,
);
});
});
@@ -295,18 +331,23 @@ describe('Manifest', function () {
describe('addExtensionFromPackage()', function () {
describe('when provided a valid package.json for a driver and its path', function () {
- it('should add an extension to the internal data', function () {
- const packageJson = {
+ /** @type {ExtensionPackageJson<DriverType>} */
+ let packageJson;
+
+ beforeEach(function () {
+ packageJson = {
name: 'derp',
version: '1.0.0',
appium: {
automationName: 'derp',
mainClass: 'SomeClass',
- pkgName: 'derp',
platformNames: ['dogs', 'cats'],
driverName: 'myDriver',
},
};
+ });
+
+ it('should add an extension to the internal data', function () {
manifest.addExtensionFromPackage(
packageJson,
'/some/path/to/package.json',
@@ -323,19 +364,50 @@ describe('Manifest', function () {
},
});
});
+
+ it('should return `true`', function () {
+ expect(
+ manifest.addExtensionFromPackage(
+ packageJson,
+ '/some/path/to/package.json',
+ ),
+ ).to.be.true;
+ });
+
+ describe('when the driver has already been registered', function () {
+ beforeEach(function () {
+ manifest.addExtensionFromPackage(
+ packageJson,
+ '/some/path/to/package.json',
+ );
+ });
+
+ it('should return `false`', function () {
+ expect(
+ manifest.addExtensionFromPackage(
+ packageJson,
+ '/some/path/to/package.json',
+ ),
+ ).to.be.false;
+ });
+ });
});
describe('when provided a valid package.json for a plugin and its path', function () {
- it('should add an extension to the internal data', function () {
- const packageJson = {
+ /** @type {ExtensionPackageJson<PluginType>} */
+ let packageJson;
+ beforeEach(function () {
+ packageJson = {
name: 'derp',
version: '1.0.0',
appium: {
mainClass: 'SomeClass',
- pkgName: 'derp',
pluginName: 'myPlugin',
},
};
+ });
+
+ it('should add an extension to the internal data', function () {
manifest.addExtensionFromPackage(
packageJson,
'/some/path/to/package.json',
@@ -350,26 +422,41 @@ describe('Manifest', function () {
},
});
});
+
+ it('should return `true`', function () {
+ expect(
+ manifest.addExtensionFromPackage(
+ packageJson,
+ '/some/path/to/package.json',
+ ),
+ ).to.be.true;
});
- describe('when provided a non-extension', function () {
- it('should ignore', function () {
+ describe('when the plugin has already been registered', function () {
+ beforeEach(function () {
manifest.addExtensionFromPackage(
- // @ts-expect-error
- {herp: 'derp'},
+ packageJson,
'/some/path/to/package.json',
);
- expect(manifest.getExtensionData('plugin')).to.deep.equal({});
- expect(manifest.getExtensionData('driver')).to.deep.equal({});
+ });
+
+ it('should return `false`', function () {
+ expect(
+ manifest.addExtensionFromPackage(
+ packageJson,
+ '/some/path/to/package.json',
+ ),
+ ).to.be.false;
+ });
});
});
- describe('when provided an unrecognizable extension', function () {
+ describe('when provided a non-extension', function () {
it('should throw', function () {
expect(() =>
manifest.addExtensionFromPackage(
// @ts-expect-error
- {name: 'derp', version: '123', appium: {}},
+ {herp: 'derp'},
'/some/path/to/package.json',
),
).to.throw(/neither a valid driver nor a valid plugin/);
@@ -422,6 +509,44 @@ describe('Manifest', function () {
);
});
});
+
+ describe('hasDriver()', function () {
+ describe('when the driver is registered', function () {
+ beforeEach(function () {
+ // @ts-expect-error
+ manifest.addExtension(DRIVER_TYPE, 'foo', {});
+ });
+
+ it('should return `true`', function () {
+ expect(manifest.hasDriver('foo')).to.be.true;
+ });
+ });
+
+ describe('when the driver is not registered', function () {
+ it('should return `false`', function () {
+ expect(manifest.hasDriver('foo')).to.be.false;
+ });
+ });
+ });
+
+ describe('hasPlugin()', function () {
+ describe('when the plugin is registered', function () {
+ beforeEach(function () {
+ // @ts-expect-error
+ manifest.addExtension(PLUGIN_TYPE, 'foo', {});
+ });
+
+ it('should return `true`', function () {
+ expect(manifest.hasPlugin('foo')).to.be.true;
+ });
+ });
+
+ describe('when the plugin is not registered', function () {
+ it('should return `false`', function () {
+ expect(manifest.hasPlugin('foo')).to.be.false;
+ });
+ });
+ });
});
});
@@ -430,6 +555,12 @@ describe('Manifest', function () {
* @typedef {import('../../../lib/extension/manifest').ExtData<T>} ExtData
*/
+/**
+ * @template T
+ * @typedef {import('../../../lib/extension/manifest').ExtensionPackageJson<T>} ExtensionPackageJson
+ */
+
/**
* @typedef {import('../../../lib/extension/manifest').DriverType} DriverType
+ * @typedef {import('../../../lib/extension/manifest').PluginType} PluginType
*/

chore(appium): peer review changes

- add note about argparse tomfoolery
- refactor `list` command impl for better types
- rewrite exception in `driver-config`
- add `hasDriver`, `hasPlugin`, and small refactors in `manifest`
- add note about args in `grid-register`
- remove invalid comment in `main`
- more test coverage for `manifest`