test(options): un-skip the options suite and cover the revived validator

The suite was skipped wholesale, which is why the validator could sit dead
for five years without a single test noticing.

One assertion had genuinely rotted: `config` is declared in both
commonOptions (Array) and globalOptions (String), and the latter wins, so
the array-coercion test was asserting a type `config` no longer has. It now
exercises the coercion through a command-specific Array option, which is
what it was written to check.

The existing assertions describe the hard-fail behavior, so they run with
NS_STRICT_OPTIONS=error; new cases cover the staging itself, the vision-ng
camel/dashed pair, negated booleans, dot-notation object values, and the
skipOptionsValidation bypass.

Author: edusperoni

lib/options.ts

@@ -278,10 +278,9 @@ export class Options {
 				continue;
 			}
 
-			// yargs emits both the dashed and the camelCase spelling of every flag.
-			// Unknown options have no declaration to normalize against, so key the
-			// dedupe off the camelCase form both spellings collapse to.
-			const dedupeKey = this.getNonDashedOptionName(optionName);
+			// yargs emits every spelling of a flag: dashed, camelCase and, for an
+			// aliased option, the alias. Collapse them so one flag is reported once.
+			const dedupeKey = this.getCanonicalOptionName(optionName);
 			if (_.includes(validated, dedupeKey)) {
 				continue;
 			}
@@ -317,6 +316,21 @@ export class Options {
 		}
 	}
 
+	// The name every spelling of an option collapses to. Unknown options keep
+	// their own name; there is no declaration to resolve them against.
+	private getCanonicalOptionName(optionName: string): string {
+		const correctName = this.getCorrectOptionName(optionName);
+		if (this.options[correctName]) {
+			return this.getNonDashedOptionName(correctName);
+		}
+
+		const aliasedName = _.findKey(
+			this.options,
+			(opt) => opt.alias === correctName,
+		);
+		return this.getNonDashedOptionName(aliasedName || correctName);
+	}
+
 	// yargs strips the `no-` prefix off a negated flag, so an undeclared
 	// `--no-foo` surfaces as `foo` and would otherwise be reported under a name
 	// the user never typed.

test/commands-service.ts

@@ -0,0 +1,66 @@
+import { assert } from "chai";
+import { Yok } from "../lib/common/yok";
+import { CommandsService } from "../lib/common/services/commands-service";
+import { ICommand } from "../lib/common/definitions/commands";
+
+function createTestInjector(command: ICommand): {
+	injector: Yok;
+	validatedWith: { called: boolean };
+} {
+	const injector = new Yok();
+	const validatedWith = { called: false };
+
+	injector.register("errors", {
+		fail: (message: string): void => {
+			throw new Error(message);
+		},
+		failWithHelp: (message: string): void => {
+			throw new Error(message);
+		},
+	});
+	injector.register("hooksService", {});
+	injector.register("logger", { warn: (): void => undefined });
+	injector.register("options", {
+		validateOptions: (): void => {
+			validatedWith.called = true;
+		},
+	});
+	injector.register("staticConfig", {});
+	injector.register("extensibilityService", {});
+	injector.register("optionsTracker", {});
+
+	injector.resolveCommand = () => command;
+
+	return { injector, validatedWith };
+}
+
+describe("commands-service", () => {
+	describe("option validation", () => {
+		const baseCommand: ICommand = {
+			execute: async (): Promise<void> => undefined,
+			allowedParameters: [],
+			canExecute: async (): Promise<boolean> => true,
+		};
+
+		it("validates the options of an ordinary command", async () => {
+			const { injector, validatedWith } = createTestInjector(baseCommand);
+			const service = injector.resolve(CommandsService);
+
+			await (<any>service).tryExecuteCommandAction("info", []);
+
+			assert.isTrue(validatedWith.called);
+		});
+
+		it("skips validation for a command that forwards its options", async () => {
+			const { injector, validatedWith } = createTestInjector({
+				...baseCommand,
+				skipOptionsValidation: true,
+			});
+			const service = injector.resolve(CommandsService);
+
+			await (<any>service).tryExecuteCommandAction("preview", []);
+
+			assert.isFalse(validatedWith.called);
+		});
+	});
+});

test/options.ts

@@ -12,6 +12,7 @@ import { OptionType } from "../lib/common/enums";
 import * as _ from "lodash";
 
 let isExecutionStopped = false;
+let warnings: string[] = [];
 
 function createTestInjector(): IInjector {
 	const testInjector = new Yok();
@@ -23,6 +24,11 @@ function createTestInjector(): IInjector {
 		setSettings: (settings: IConfigurationSettings): any => undefined,
 		getProfileDir: () => "profileDir",
 	});
+	testInjector.register("logger", {
+		warn: (message: string): void => {
+			warnings.push(message);
+		},
+	});
 
 	return testInjector;
 }
@@ -32,8 +38,7 @@ function createOptions(testInjector: IInjector): IOptions {
 	return options;
 }
 
-// TODO: Igor and Nathan will make this work again
-describe.skip("options", () => {
+describe("options", () => {
 	let testInjector: IInjector;
 	beforeEach(() => {
 		testInjector = createTestInjector();
@@ -48,6 +53,14 @@ describe.skip("options", () => {
 
 		testInjector.register("errors", errors);
 		isExecutionStopped = false;
+		warnings = [];
+		// The assertions below describe the hard-fail behavior, which is opt-in
+		// while validation is staged. The staging itself is covered separately.
+		process.env.NS_STRICT_OPTIONS = "error";
+	});
+
+	afterEach(() => {
+		delete process.env.NS_STRICT_OPTIONS;
 	});
 
 	describe("validateOptions", () => {
@@ -180,13 +193,13 @@ describe.skip("options", () => {
 
 		it("converts string value to array when option type is array", () => {
 			const options: any = createOptions(testInjector);
-			process.argv.push("--config");
+			process.argv.push("--test1");
 			process.argv.push("value");
 			options.validateOptions({ test1: { type: OptionType.Array } });
 			process.argv.pop();
 			process.argv.pop();
 			assert.isFalse(isExecutionStopped);
-			assert.deepStrictEqual(["value"], <any>options["config"]);
+			assert.deepStrictEqual(["value"], <any>options.argv.test1);
 		});
 
 		it("does not break execution when valid commandSpecificOptions are passed", () => {
@@ -306,6 +319,91 @@ describe.skip("options", () => {
 						"Dashed options (special-dashed-v) are added to yargs.argv in two ways: special-dashed-v and specialDashedV",
 				);
 			});
+
+			// vision-ng and friends are declared with a literal dashed key, so the
+			// camelCase spelling yargs derives has no declaration of its own.
+			_.each(["--vision-ng", "--visionNg"], (arg) => {
+				it(`does not break execution when ${arg} is passed`, () => {
+					process.argv.push(arg);
+					const options = createOptions(testInjector);
+					options.validateOptions();
+					process.argv.pop();
+					assert.isFalse(isExecutionStopped);
+					assert.isEmpty(warnings);
+				});
+			});
+		});
+
+		describe("does not report valid options", () => {
+			it("accepts a negated declared boolean", () => {
+				process.argv.push("--no-hmr");
+				const options = createOptions(testInjector);
+				options.validateOptions();
+				process.argv.pop();
+				assert.isFalse(isExecutionStopped);
+				assert.isEmpty(warnings);
+				assert.isFalse(<boolean>options.argv.hmr);
+			});
+
+			it("accepts dot-notation values of an object option", () => {
+				process.argv.push("--env.production");
+				process.argv.push("--env.sourceMap");
+				const options = createOptions(testInjector);
+				options.validateOptions();
+				process.argv.pop();
+				process.argv.pop();
+				assert.isFalse(isExecutionStopped);
+				assert.isEmpty(warnings);
+				assert.deepStrictEqual(options.argv.env, {
+					production: true,
+					sourceMap: true,
+				});
+			});
+		});
+
+		describe("staged reporting", () => {
+			it("warns instead of failing when an option is not supported", () => {
+				delete process.env.NS_STRICT_OPTIONS;
+				process.argv.push("--unknownOption");
+				const options = createOptions(testInjector);
+				options.validateOptions();
+				process.argv.pop();
+				assert.isFalse(isExecutionStopped);
+				assert.lengthOf(warnings, 1);
+				assert.include(warnings[0], "'unknownOption' is not supported");
+				assert.include(warnings[0], "NS_STRICT_OPTIONS=error");
+			});
+
+			it("fails when an option is not supported and NS_STRICT_OPTIONS=error", () => {
+				process.env.NS_STRICT_OPTIONS = "error";
+				process.argv.push("--unknownOption");
+				const options = createOptions(testInjector);
+				options.validateOptions();
+				process.argv.pop();
+				assert.isTrue(isExecutionStopped);
+				assert.isEmpty(warnings);
+			});
+
+			it("warns instead of failing when a string option has no value", () => {
+				delete process.env.NS_STRICT_OPTIONS;
+				process.argv.push("--path");
+				const options = createOptions(testInjector);
+				options.validateOptions();
+				process.argv.pop();
+				assert.isFalse(isExecutionStopped);
+				assert.lengthOf(warnings, 1);
+				assert.include(warnings[0], "'path' requires non-empty value");
+			});
+
+			it("reports an undeclared negated flag under the spelling that was used", () => {
+				delete process.env.NS_STRICT_OPTIONS;
+				process.argv.push("--no-something");
+				const options = createOptions(testInjector);
+				options.validateOptions();
+				process.argv.pop();
+				assert.lengthOf(warnings, 1);
+				assert.include(warnings[0], "'no-something' is not supported");
+			});
 		});
 	});