Skip to content

Add DefinitelyTyped runner #19815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Nov 8, 2017
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Jakefile.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ var harnessCoreSources = [
"loggedIO.ts",
"rwcRunner.ts",
"userRunner.ts",
"dtRunner.ts",
"test262Runner.ts",
"./parallel/shared.ts",
"./parallel/host.ts",
Expand Down
62 changes: 62 additions & 0 deletions src/harness/dtRunner.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/// <reference path="harness.ts"/>
/// <reference path="runnerbase.ts" />
class DefinitelyTypedRunner extends RunnerBase {
private static readonly testDir = "../DefinitelyTyped/types/";

public workingDirectory = DefinitelyTypedRunner.testDir;

public enumerateTestFiles() {
return Harness.IO.getDirectories(DefinitelyTypedRunner.testDir);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to add an assert here or something to let users know that DefinitllyTyped was not found in the expected place instead of silently not running any tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth throwing an error if DefinitelyTypedRunner.testDir is empty/does not exist (rather than the usual behavior of getDirectories, which I believe just produces undefined or an empty list). After all, without that, an empty or missing folder is a passing DT suite, which could be misleading if you run this with DefinitelyTyped cloned in the wrong location.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out getDirectories throws already, which I think is good enough.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does a function called enumerateTestFiles return a list of directories?

}

public kind(): TestRunnerKind {
return "dt";
}

/** Setup the runner's tests so that they are ready to be executed by the harness
* The first test should be a describe/it block that sets up the harness's compiler instance appropriately
*/
public initializeTests(): void {
// Read in and evaluate the test list
const testList = this.tests && this.tests.length ? this.tests : this.enumerateTestFiles();

describe(`${this.kind()} code samples`, () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like code copied from elsewhere -- can you just inherit?

for (const test of testList) {
this.runTest(test);
}
});
}

private runTest(directoryName: string) {
describe(directoryName, () => {
const cp = require("child_process");
const path = require("path");
const fs = require("fs");

it("should build successfully", () => {
const cwd = path.join(__dirname, "../../", DefinitelyTypedRunner.testDir, directoryName);
const timeout = 600000; // 600s = 10 minutes
if (fs.existsSync(path.join(cwd, "package.json"))) {
if (fs.existsSync(path.join(cwd, "package-lock.json"))) {
fs.unlinkSync(path.join(cwd, "package-lock.json"));
}
const stdio = isWorker ? "pipe" : "inherit";
const install = cp.spawnSync(`npm`, ["i"], { cwd, timeout, shell: true, stdio });
if (install.status !== 0) throw new Error(`NPM Install for ${directoryName} failed!`);
}
Harness.Baseline.runBaseline(`${this.kind()}/${directoryName}.log`, () => {
const result = cp.spawnSync(`node`, [path.join(__dirname, "tsc.js")], { cwd, timeout, shell: true });
// tslint:disable:no-null-keyword
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: use // tslint:disable-next-line, and indent it with the other statements.
Also, can this code be shared with code elsewhere?

return result.status === 0 ? null : `Exit Code: ${result.status}
Standard output:
${result.stdout.toString().replace(/\r\n/g, "\n")}


Standard error:
${result.stderr.toString().replace(/\r\n/g, "\n")}`;
// tslint:enable:no-null-keyword
});
});
});
}
}
8 changes: 4 additions & 4 deletions src/harness/parallel/host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,18 +77,18 @@ namespace Harness.Parallel.Host {
console.log("Discovering runner-based tests...");
const discoverStart = +(new Date());
const { statSync }: { statSync(path: string): { size: number }; } = require("fs");
const path: { join: (...args: string[]) => string } = require("path");
for (const runner of runners) {
const files = runner.enumerateTestFiles();
for (const file of files) {
for (const file of runner.enumerateTestFiles()) {
let size: number;
if (!perfData) {
try {
size = statSync(file).size;
size = statSync(path.join(runner.workingDirectory, file)).size;
}
catch {
// May be a directory
try {
size = Harness.IO.listFiles(file, /.*/g, { recursive: true }).reduce((acc, elem) => acc + statSync(elem).size, 0);
size = Harness.IO.listFiles(path.join(runner.workingDirectory, file), /.*/g, { recursive: true }).reduce((acc, elem) => acc + statSync(elem).size, 0);
}
catch {
// Unknown test kind, just return 0 and let the historical analysis take over after one run
Expand Down
6 changes: 6 additions & 0 deletions src/harness/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
/// <reference path="projectsRunner.ts" />
/// <reference path="rwcRunner.ts" />
/// <reference path="userRunner.ts" />
/// <reference path="dtRunner.ts" />
/// <reference path="harness.ts" />
/// <reference path="./parallel/shared.ts" />

Expand Down Expand Up @@ -62,6 +63,8 @@ function createRunner(kind: TestRunnerKind): RunnerBase {
return new Test262BaselineRunner();
case "user":
return new UserCodeRunner();
case "dt":
return new DefinitelyTypedRunner();
}
ts.Debug.fail(`Unknown runner kind ${kind}`);
}
Expand Down Expand Up @@ -183,6 +186,9 @@ function handleTestConfig() {
case "user":
runners.push(new UserCodeRunner());
break;
case "dt":
runners.push(new DefinitelyTypedRunner());
break;
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion src/harness/runnerbase.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/// <reference path="harness.ts" />


type TestRunnerKind = CompilerTestKind | FourslashTestKind | "project" | "rwc" | "test262" | "user";
type TestRunnerKind = CompilerTestKind | FourslashTestKind | "project" | "rwc" | "test262" | "user" | "dt";
type CompilerTestKind = "conformance" | "compiler";
type FourslashTestKind = "fourslash" | "fourslash-shims" | "fourslash-shims-pp" | "fourslash-server";

Expand All @@ -24,6 +24,9 @@ abstract class RunnerBase {

abstract enumerateTestFiles(): string[];

/** The working directory where tests are found. Needed for batch testing where the input path will differ from the output path inside baselines */
public workingDirectory = "";

/** Setup the runner's tests so that they are ready to be executed by the harness
* The first test should be a describe/it block that sets up the harness's compiler instance appropriately
*/
Expand Down
1 change: 1 addition & 0 deletions src/harness/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
"loggedIO.ts",
"rwcRunner.ts",
"userRunner.ts",
"definitelyRunner.ts",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file you added is named dtRunner instead, which implies this isn't being used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just by gulp. Fixed.

"test262Runner.ts",
"./parallel/shared.ts",
"./parallel/host.ts",
Expand Down
2 changes: 1 addition & 1 deletion src/harness/userRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class UserCodeRunner extends RunnerBase {
const install = cp.spawnSync(`npm`, ["i"], { cwd, timeout, shell: true, stdio });
if (install.status !== 0) throw new Error(`NPM Install for ${directoryName} failed!`);
Harness.Baseline.runBaseline(`${this.kind()}/${directoryName}.log`, () => {
const result = cp.spawnSync(`node`, ["../../../../built/local/tsc.js"], { cwd, timeout, shell: true });
const result = cp.spawnSync(`node`, [path.join(__dirname, "tsc.js")], { cwd, timeout, shell: true });
return `Exit Code: ${result.status}
Standard output:
${result.stdout.toString().replace(/\r\n/g, "\n")}
Expand Down