chore(appium): review changes
some reformatting + import organization
renamed some unclear variables
created more constants
remove schema migration code
remove log-fiddling in `config-file-e2e-specs.js`
MODIFY
packages/appium/lib/ext-config-io.js
packages/appium/lib/ext-config-io.js
* Module containing {@link ExtConfigIO} which handles reading & writing of extension config files.
*/
-import _ from 'lodash';
import { fs, mkdirp } from '@appium/support';
+import _ from 'lodash';
import path from 'path';
import YAML from 'yaml';
@@ -16,16 +16,13 @@ const CONFIG_FILE_NAME = 'extensions.yaml';
*/
const CONFIG_SCHEMA_REV = 2;
-/**
- * This schema revision adds the `plugins` property.
- *
- * See {@link ExtConfigIO._applySchemaMigrations} for more details.
- */
-const CONFIG_SCHEMA_REV_2 = 2;
-
export const DRIVER_TYPE = 'driver';
export const PLUGIN_TYPE = 'plugin';
+/**
+ * Set of valid extension types.
+ * @type {Readonly<Set<ExtensionType>>}
+ */
const VALID_EXT_TYPES = new Set([DRIVER_TYPE, PLUGIN_TYPE]);
const CONFIG_DATA_DRIVER_KEY = `${DRIVER_TYPE}s`;
@@ -43,6 +40,7 @@ class ExtConfigIO {
* @private
*/
_dirty;
+
/**
* The entire contents of a parsed YAML extension config file.
* @type {object?}
@@ -51,37 +49,54 @@ class ExtConfigIO {
_data;
/**
- * A mapping of extension type to configuration data. Configuration data is keyed on extension name.
+ * A mapping of extension type to configuration data. Configuration data is
+ * keyed on extension name.
*
- * Consumers get the values of this `Map` and do not have access to the entire data object.
- * @type {Map<'driver'|'plugin',object>}
+ * Consumers get the values of this `Map` (corresponding to the
+ * `extensionType` of the consumer, which will be a subclass of
+ * `ExtensionConfig`) and do not have access to the entire data object.
* @private
+ * @type {Map<ExtensionType,object>}
*/
- _extensionTypeData = new Map();
+ _extDataByType = new Map();
/**
* Path to config file.
+ * @private
* @type {Readonly<string>}
*/
_filepath;
/**
* Path to `APPIUM_HOME`
+ * @private
* @type {Readonly<string>}
*/
_appiumHome;
/**
* Helps avoid writing multiple times.
+ *
+ * If this is `null`, calling {@link ExtConfigIO.write} will cause it to be
+ * set to a `Promise`. When the call to `write()` is complete, the `Promise`
+ * will resolve and then this value will be set to `null`. Concurrent calls
+ * made while this value is a `Promise` will return the `Promise` itself.
+ * @private
* @type {Promise<boolean>?}
*/
- _writing;
+ _writing = null;
/**
* Helps avoid reading multiple times.
+ *
+ * If this is `null`, calling {@link ExtConfigIO.read} will cause it to be
+ * set to a `Promise`. When the call to `read()` is complete, the `Promise`
+ * will resolve and then this value will be set to `null`. Concurrent calls
+ * made while this value is a `Promise` will return the `Promise` itself.
+ * @private
* @type {Promise<void>?}
*/
- _reading;
+ _reading = null;
/**
* @param {string} appiumHome
@@ -92,8 +107,13 @@ class ExtConfigIO {
}
/**
- * Creaes a proxy which watches for changes to the extension-type-specific config data.
- * @param {'driver'|'plugin'} extensionType
+ * Creaes a `Proxy` which watches for changes to the extension-type-specific
+ * config data.
+ *
+ * When changes are detected, it sets a `_dirty` flag. The next call to
+ * {@link ExtConfigIO.write} will check if this flag is `true` before
+ * proceeding.
+ * @param {ExtensionType} extensionType
* @param {Record<string,object>} data - Extension config data, keyed by name
* @private
* @returns {Record<string,object>}
@@ -117,7 +137,7 @@ class ExtConfigIO {
}
/**
- * Returns the path to the config file. This value is intended to be read-only.
+ * Returns the path to the config file.
*/
get filepath () {
return this._filepath;
@@ -129,14 +149,16 @@ class ExtConfigIO {
* Force-reading is _not_ supported, as it's likely to be a source of
* bugs--it's easy to mutate the data and then overwrite memory with the file
* contents
- * @param {'driver'|'plugin'} extensionType - Which bit of the config data we
+ *
+ * Ideally this will only ever read the file _once_.
+ * @param {ExtensionType} extensionType - Which bit of the config data we
* want
* @returns {Promise<object>} The data
*/
async read (extensionType) {
if (this._reading) {
await this._reading;
- return this._extensionTypeData.get(extensionType);
+ return this._extDataByType.get(extensionType);
}
this._reading = (async () => {
@@ -147,7 +169,7 @@ class ExtConfigIO {
].join(', ')}`,
);
}
- if (this._extensionTypeData.has(extensionType)) {
+ if (this._extDataByType.has(extensionType)) {
return;
}
@@ -156,7 +178,7 @@ class ExtConfigIO {
try {
await mkdirp(this._appiumHome);
const yaml = await fs.readFile(this.filepath, 'utf8');
- data = this._applySchemaMigrations(YAML.parse(yaml));
+ data = YAML.parse(yaml);
} catch (err) {
if (err.code === 'ENOENT') {
data = {
@@ -175,11 +197,11 @@ class ExtConfigIO {
}
this._data = data;
- this._extensionTypeData.set(
+ this._extDataByType.set(
DRIVER_TYPE,
this._createProxy(DRIVER_TYPE, data),
);
- this._extensionTypeData.set(
+ this._extDataByType.set(
PLUGIN_TYPE,
this._createProxy(PLUGIN_TYPE, data),
);
@@ -190,7 +212,7 @@ class ExtConfigIO {
})();
try {
await this._reading;
- return this._extensionTypeData.get(extensionType);
+ return this._extDataByType.get(extensionType);
} finally {
this._reading = null;
}
@@ -219,12 +241,16 @@ class ExtConfigIO {
const dataToWrite = {
...this._data,
- [CONFIG_DATA_DRIVER_KEY]: this._extensionTypeData.get(DRIVER_TYPE),
- [CONFIG_DATA_PLUGIN_KEY]: this._extensionTypeData.get(PLUGIN_TYPE),
+ [CONFIG_DATA_DRIVER_KEY]: this._extDataByType.get(DRIVER_TYPE),
+ [CONFIG_DATA_PLUGIN_KEY]: this._extDataByType.get(PLUGIN_TYPE),
};
try {
- await fs.writeFile(this.filepath, YAML.stringify(dataToWrite), 'utf8');
+ await fs.writeFile(
+ this.filepath,
+ YAML.stringify(dataToWrite),
+ 'utf8',
+ );
this._dirty = false;
return true;
} catch {
@@ -239,31 +265,6 @@ class ExtConfigIO {
})();
return await this._writing;
}
-
- /**
- * Normalizes the file, even if it was created with `schemaRev` < {@link CONFIG_SCHEMA_REV_2}.
- * At {@link CONFIG_SCHEMA_REV_2}, we started including plugins as well as drivers in the file,
- * so make sure we at least have an empty section for it.
- *
- * Returns a shallow copy of `yamlData`.
- * @param {Readonly<object>} yamlData - Parsed contents of YAML `extensions.yaml`
- * @private
- * @returns {object} A shallow copy of `yamlData`
- */
- _applySchemaMigrations (yamlData) {
- if (
- yamlData.schemaRev < CONFIG_SCHEMA_REV_2 &&
- yamlData[CONFIG_DATA_PLUGIN_KEY] === undefined
- ) {
- this._dirty = true;
- return {
- ...yamlData,
- [CONFIG_DATA_PLUGIN_KEY]: {},
- schemaRev: CONFIG_SCHEMA_REV,
- };
- }
- return {...yamlData};
- }
}
/**

MODIFY
packages/appium/lib/schema/schema.js
packages/appium/lib/schema/schema.js
@@ -5,8 +5,8 @@ import addFormats from 'ajv-formats';
import _ from 'lodash';
import path from 'path';
import resolveFrom from 'resolve-from';
-import appiumConfigSchema from './appium-config-schema';
import { APPIUM_HOME } from '../extension-config';
+import appiumConfigSchema from './appium-config-schema';
import { keywords } from './keywords';
/**
@@ -139,7 +139,9 @@ export function finalizeSchema () {
/** @type {SchemaObject} */
const finalExtSchema = {...extSchema, additionalProperties: false};
_.forEach(finalExtSchema?.properties ?? {}, (prop, propName) => {
- const schemaId = `${extensionType}-${name}-${_.kebabCase(prop.appiumCliDest ?? propName)}`;
+ const schemaId = `${extensionType}-${name}-${_.kebabCase(
+ prop.appiumCliDest ?? propName,
+ )}`;
ajv.addSchema(prop, schemaId);
createdSchemaIDs.add(schemaId);
});
@@ -163,8 +165,10 @@ export function finalizeSchema () {
const finalSchema = _.reduce(
_.fromPairs([...registeredSchemas]),
(baseSchema, extensionSchemas, extensionType) => {
- baseSchema.properties[extensionType].properties =
- combineExtSchemas(extensionSchemas, extensionType);
+ baseSchema.properties[extensionType].properties = combineExtSchemas(
+ extensionSchemas,
+ extensionType,
+ );
return baseSchema;
},
baseSchema,
@@ -317,7 +321,6 @@ export const readExtensionSchema = _.memoize(
`No \`schema\` property found in config for ${extType} ${pkgName} -- why is this function being called?`,
);
}
- if (argSchemaPath) {
const schemaPath = resolveFrom(
path.resolve(APPIUM_HOME, installPath),
// this path sep is fine because `resolveFrom` uses Node's module resolution
@@ -330,7 +333,6 @@ export const readExtensionSchema = _.memoize(
: moduleObject;
registerSchema(extType, extName, schema);
return schema;
- }
},
(extType, extData, extName) => `${extType}-${extName}`,
);

MODIFY
packages/appium/test/cli/cli-e2e-specs.js
packages/appium/test/cli/cli-e2e-specs.js
@@ -96,11 +96,13 @@ describe('CLI', function () {
list.should.eql(ret);
});
it('should install a driver from npm with a specific version/tag', async function () {
+ const currentFakeDriverVersionAsOfRightNow = '3.0.5';
await clear();
- const ret = JSON.parse(await run('install', ['@appium/fake-driver@3.0.5', '--source', 'npm', '--json']));
+ const installSpec = `@appium/fake-driver@${currentFakeDriverVersionAsOfRightNow}`;
+ const ret = JSON.parse(await run('install', [installSpec, '--source', 'npm', '--json']));
ret.fake.pkgName.should.eql('@appium/fake-driver');
ret.fake.installType.should.eql('npm');
- ret.fake.installSpec.should.eql('@appium/fake-driver@3.0.5');
+ ret.fake.installSpec.should.eql(installSpec);
const list = JSON.parse(await run('list', ['--installed', '--json']));
delete list.fake.installed;
list.should.eql(ret);

MODIFY
packages/appium/test/config-file-e2e-specs.js
packages/appium/test/config-file-e2e-specs.js
// @ts-check
-import { logger } from '@appium/support';
import { readConfigFile } from '../lib/config-file';
import { finalizeSchema, registerSchema, resetSchema } from '../lib/schema/schema';
import extSchema from './fixtures/driver.schema.js';
import { resolveFixture } from './helpers';
-const log = logger.log;
-
describe('config file behavior', function () {
const GOOD_FILEPATH = resolveFixture('config', 'appium.config.good.json');
const BAD_NODECONFIG_FILEPATH = resolveFixture(
@@ -40,13 +37,6 @@ describe('config file behavior', function () {
'appium.config.ext-good.json',
);
- let oldLogLevel;
- before(function () {
- // canonical way to do this?
- oldLogLevel = log.level;
- log.level = 'error';
- });
-
beforeEach(function () {
finalizeSchema();
});
@@ -55,10 +45,6 @@ describe('config file behavior', function () {
resetSchema();
});
- after(function () {
- log.level = oldLogLevel;
- });
-
describe('when provided a path to a config file', function () {
describe('when the config file is valid per the schema', function () {
it('should return a valid config object', async function () {
@@ -254,7 +240,7 @@ describe('config file behavior', function () {
});
});
it('should return an object containing errors', function () {
- result.errors.should.eql([
+ result.should.have.deep.property('errors', [
{
instancePath: '/driver/fake',
schemaPath:

MODIFY
packages/appium/test/config-file-specs.js
packages/appium/test/config-file-specs.js
// @ts-check
-import {rewiremock, resolveFixture} from './helpers';
-import YAML from 'yaml';
import fs from 'fs';
import sinon from 'sinon';
+import YAML from 'yaml';
import * as schema from '../lib/schema/schema';
+import { resolveFixture, rewiremock } from './helpers';
const expect = chai.expect;
@@ -56,7 +56,7 @@ describe('config-file', function () {
before(function () {
// generally called via the CLI parser, this needs to be done manually in tests.
// we don't need to do this before _each_ test, because we're not changing the schema.
- // if we did change the schema, this would need ot be in `beforeEach()` and `afterEach()`
+ // if we did change the schema, this would need to be in `beforeEach()` and `afterEach()`
// would need to call `schema.reset()`.
schema.finalizeSchema();
});

MODIFY
packages/appium/test/ext-config-io-specs.js
packages/appium/test/ext-config-io-specs.js
// @ts-check
-import {rewiremock} from './helpers';
+import { promises as fs } from 'fs';
import path from 'path';
import sinon from 'sinon';
-import {promises as fs} from 'fs';
import YAML from 'yaml';
+import { rewiremock } from './helpers';
const expect = chai.expect;
describe('ExtensionConfigIO', function () {
@@ -16,18 +16,11 @@ describe('ExtensionConfigIO', function () {
/** @type {string} */
let yamlFixture;
- /** @type {string} */
- let yamlFixtureRev1;
-
before(async function () {
yamlFixture = await fs.readFile(
path.join(__dirname, 'fixtures', 'extensions.yaml'),
'utf8',
);
- yamlFixtureRev1 = await fs.readFile(
- path.join(__dirname, 'fixtures', 'extensions-rev-1.yaml'),
- 'utf8',
- );
});
/**
@@ -98,7 +91,7 @@ describe('ExtensionConfigIO', function () {
});
describe('when called with a valid extension type', function () {
- describe('when the file does not exist', function () {
+ describe('when the file does not yet exist', function () {
beforeEach(async function () {
/** @type {NodeJS.ErrnoException} */
const err = new Error();
@@ -116,7 +109,7 @@ describe('ExtensionConfigIO', function () {
});
});
- describe('when `schemaRev` is up-to-date', function () {
+ describe('when the file already exists', function () {
beforeEach(async function () {
await io.read('driver');
});
@@ -133,39 +126,13 @@ describe('ExtensionConfigIO', function () {
).to.have.been.calledOnceWith(io.filepath, 'utf8');
});
});
-
- describe('when the `schemaRev` of the extension config file is less than 2', function () {
- let updatedYaml;
-
- beforeEach(async function () {
- mocks['@appium/support'].fs.readFile = sandbox
- .stub()
- .resolves(yamlFixtureRev1);
- updatedYaml = {
- ...YAML.parse(yamlFixtureRev1),
- schemaRev: 2,
- plugins: {},
- };
- await io.read('driver');
- });
-
- it('should write the file using the updated schema', async function () {
- await io.write();
- expect(
- mocks['@appium/support'].fs.writeFile,
- ).to.have.been.calledOnceWith(
- io.filepath,
- YAML.stringify(updatedYaml),
- 'utf8',
- );
- });
- });
});
- describe('when called with an unknown `extensionType`', function () {
+ describe('when called with an unknown extension type`', function () {
it('should reject', async function () {
// @ts-ignore
- return await expect(io.read('unknown')).to.be.rejectedWith(
+ const promise = io.read('unknown');
+ return await expect(promise).to.be.rejectedWith(
TypeError,
/invalid extension type/i,
);

DELETE
packages/appium/test/fixtures/extensions-rev-1.yaml
-drivers:
- fake:
- automationName: Fake
- platformNames:
- - Fake
- mainClass: FakeDriver
- schema: ./build/lib/fake-driver-schema.js
- scripts:
- fake-error: ./build/lib/scripts/fake-error.js
- fake-success: ./build/lib/scripts/fake-success.js
- pkgName: '@appium/fake-driver'
- version: 3.0.5
- installPath: '@appium/fake-driver'
- installType: local
- installSpec: /Users/alice/projects/appium/packages/fake-driver
-schemaRev: 1

chore(appium): review changes

- some reformatting + import organization
- renamed some unclear variables
- created more constants
- remove schema migration code
- remove log-fiddling in `config-file-e2e-specs.js`