runTests.d.ts spec not consistent with code

[aeschli]

Testing #74436

• create a TS extension as described in Test @types/vscode and vscode-test #74436 and look at runTest.ts
  'runTests' is called with testRunnerPath.
  The hover of testRunnerPath says:

    /**
     * Absolute path to the test suite folder. Must include an `index.js` that
     * exports a test runner, such as:
     *
     * ```ts
     * import * as testRunner from 'vscode/lib/testrunner';
     * module.exports = testRunner;
     * ```
     */

Looking at suite/index.ts I don't see a testRunner exported.

[aeschli]

Ok, thanks.
But now my question is about
run(testsRoot: string, cb: (error: any, failures?: number) => void)

What is the testsRoot? How does the user define it? Is it necessary?

Why do we use the cb style. Our APIs typically use Promises...

[aeschli]

So the testRunner is the module name passed to --extensionTestsPath.

Did we change the contract? Before it had to export a testrunner, now it has to export a run?

[octref]

We didn't. That's what the old test runner does: https://github.com/microsoft/vscode-extension-vscode/blob/master/lib/testrunner.ts.

We use the old API since we don't want to break backwards compat. I'd suggest we come up with a better API (see my comment here: microsoft/vscode-extension-samples#186), but it has to be a different name, such as runTest.

Here's the relevant code:

vscode/src/vs/workbench/api/node/extHostExtensionService.ts

Lines 521 to 560 in ac6237c

	private _doHandleExtensionTests(): Promise<void> {
	const { extensionDevelopmentLocationURI: extensionDevelopmentLocationURI, extensionTestsLocationURI } = this._initData.environment;
	if (!(extensionDevelopmentLocationURI && extensionTestsLocationURI && extensionTestsLocationURI.scheme === Schemas.file)) {
	return Promise.resolve(undefined);
	}
	const extensionTestsPath = originalFSPath(extensionTestsLocationURI);
	// Require the test runner via node require from the provided path
	let testRunner: ITestRunner | undefined;
	let requireError: Error | undefined;
	try {
	testRunner = <any>require.__$__nodeRequire(extensionTestsPath);
	} catch (error) {
	requireError = error;
	}
	// Execute the runner if it follows our spec
	if (testRunner && typeof testRunner.run === 'function') {
	return new Promise<void>((c, e) => {
	testRunner!.run(extensionTestsPath, (error, failures) => {
	if (error) {
	e(error.toString());
	} else {
	c(undefined);
	}
	// after tests have run, we shutdown the host
	this._gracefulExit(error || (typeof failures === 'number' && failures > 0) ? 1 /* ERROR */ : 0 /* OK */);
	});
	});
	}
	// Otherwise make sure to shutdown anyway even in case of an error
	else {
	this._gracefulExit(1 /* ERROR */);
	}
	return Promise.reject(new Error(requireError ? requireError.toString() : nls.localize('extensionTestError', "Path {0} does not point to a valid extension test runner.", extensionTestsPath)));
	}

.

[octref]

I don't really like us having to have this API, but for people who are still importing from vscode/lib/testrunner, if they don't migrate we don't want to break them.

[octref]

Actually, for people who have been importing from vscode/lib/testrunner, they haven't used the run signature directly. So if we do add a better signature with a different name such as runTest, we might not need to expose run.

[octref]

The change is to add a new function signature for run:

interface ITestRunner {
	/** Old test runner API, as exported from `vscode/lib/testrunner` */
	run(testsRoot: string, clb: (error: Error, failures?: number) => void): void;
}

interface INewTestRunner {
	/** New test runner API, as explained in the extension test doc */
	run(): Promise<void>;
}

When the function returns a promise, wait for the promise, otherwise use the old logic.