Skip to content
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

Batch enumerateFiles into a single web request #23972

Merged
merged 2 commits into from
May 9, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
78 changes: 51 additions & 27 deletions src/harness/compilerRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ const enum CompilerTestType {
Test262
}

interface CompilerFileBasedTest extends Harness.FileBasedTest {
payload?: Harness.TestCaseParser.TestCaseContent;
}

class CompilerBaselineRunner extends RunnerBase {
private basePath = "tests/cases";
private testSuiteName: TestRunnerKind;
Expand Down Expand Up @@ -42,7 +46,8 @@ class CompilerBaselineRunner extends RunnerBase {
}

public enumerateTestFiles() {
return this.enumerateFiles(this.basePath, /\.tsx?$/, { recursive: true });
// see also: `enumerateTestFiles` in tests/webTestServer.ts
return this.enumerateFiles(this.basePath, /\.tsx?$/, { recursive: true }).map(CompilerTest.getConfigurations);
}

public initializeTests() {
Expand All @@ -52,24 +57,32 @@ class CompilerBaselineRunner extends RunnerBase {
});

// this will set up a series of describe/it blocks to run between the setup and cleanup phases
const files = this.tests.length > 0 ? this.tests : this.enumerateTestFiles();
files.forEach(file => { this.checkTestCodeOutput(vpath.normalizeSeparators(file)); });
const files = this.tests.length > 0 ? this.tests : Harness.IO.enumerateTestFiles(this);
files.forEach(test => {
const file = typeof test === "string" ? test : test.file;
this.checkTestCodeOutput(vpath.normalizeSeparators(file), typeof test === "string" ? CompilerTest.getConfigurations(test) : test);
});
});
}

public checkTestCodeOutput(fileName: string) {
for (const { name, payload } of CompilerTest.getConfigurations(fileName)) {
describe(`${this.testSuiteName} tests for ${fileName}${name ? ` (${name})` : ``}`, () => {
this.runSuite(fileName, payload);
public checkTestCodeOutput(fileName: string, test?: CompilerFileBasedTest) {
if (test && test.configurations) {
test.configurations.forEach(configuration => {
describe(`${this.testSuiteName} tests for ${fileName}${configuration && configuration.name ? ` (${configuration.name})` : ``}`, () => {
this.runSuite(fileName, test, configuration);
});
});
}
describe(`${this.testSuiteName} tests for ${fileName}}`, () => {
this.runSuite(fileName, test);
});
}

private runSuite(fileName: string, testCaseContent: Harness.TestCaseParser.TestCaseContent) {
private runSuite(fileName: string, test?: CompilerFileBasedTest, configuration?: Harness.FileBasedTestConfiguration) {
// Mocha holds onto the closure environment of the describe callback even after the test is done.
// Everything declared here should be cleared out in the "after" callback.
let compilerTest: CompilerTest | undefined;
before(() => { compilerTest = new CompilerTest(fileName, testCaseContent); });
before(() => { compilerTest = new CompilerTest(fileName, test && test.payload, configuration && configuration.settings); });
it(`Correct errors for ${fileName}`, () => { compilerTest.verifyDiagnostics(); });
it(`Correct module resolution tracing for ${fileName}`, () => { compilerTest.verifyModuleResolution(); });
it(`Correct sourcemap content for ${fileName}`, () => { compilerTest.verifySourceMapRecord(); });
Expand Down Expand Up @@ -97,11 +110,6 @@ class CompilerBaselineRunner extends RunnerBase {
}
}

interface CompilerTestConfiguration {
name: string;
payload: Harness.TestCaseParser.TestCaseContent;
}

class CompilerTest {
private fileName: string;
private justName: string;
Expand All @@ -116,10 +124,20 @@ class CompilerTest {
// equivalent to other files on the file system not directly passed to the compiler (ie things that are referenced by other files)
private otherFiles: Harness.Compiler.TestFile[];

constructor(fileName: string, testCaseContent: Harness.TestCaseParser.TestCaseContent) {
constructor(fileName: string, testCaseContent?: Harness.TestCaseParser.TestCaseContent, configurationOverrides?: Harness.TestCaseParser.CompilerSettings) {
this.fileName = fileName;
this.justName = vpath.basename(fileName);

const rootDir = fileName.indexOf("conformance") === -1 ? "tests/cases/compiler/" : ts.getDirectoryPath(fileName) + "/";

if (testCaseContent === undefined) {
testCaseContent = Harness.TestCaseParser.makeUnitsFromTest(Harness.IO.readFile(fileName), fileName, rootDir);
}

if (configurationOverrides) {
testCaseContent = { ...testCaseContent, settings: { ...testCaseContent.settings, ...configurationOverrides } };
}

const units = testCaseContent.testUnitData;
this.harnessSettings = testCaseContent.settings;
let tsConfigOptions: ts.CompilerOptions;
Expand Down Expand Up @@ -174,32 +192,38 @@ class CompilerTest {
this.options = this.result.options;
}

public static getConfigurations(fileName: string) {
const content = Harness.IO.readFile(fileName);
const rootDir = fileName.indexOf("conformance") === -1 ? "tests/cases/compiler/" : ts.getDirectoryPath(fileName) + "/";
const testCaseContent = Harness.TestCaseParser.makeUnitsFromTest(content, fileName, rootDir);
const configurations: CompilerTestConfiguration[] = [];
const scriptTargets = this._split(testCaseContent.settings.target);
const moduleKinds = this._split(testCaseContent.settings.module);
public static getConfigurations(file: string): CompilerFileBasedTest {
// also see `parseCompilerTestConfigurations` in tests/webTestServer.ts
const content = Harness.IO.readFile(file);
const rootDir = file.indexOf("conformance") === -1 ? "tests/cases/compiler/" : ts.getDirectoryPath(file) + "/";
Copy link

Choose a reason for hiding this comment

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

nit: ts.contains useful here

Copy link
Member Author

Choose a reason for hiding this comment

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

ts.contains is for arrays. file is a string.

Copy link

Choose a reason for hiding this comment

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

Sorry, ts.stringContains

const payload = Harness.TestCaseParser.makeUnitsFromTest(content, file, rootDir);
const settings = Harness.TestCaseParser.extractCompilerSettings(content);
const scriptTargets = CompilerTest._split(settings.target);
Copy link

@ghost ghost May 8, 2018

Choose a reason for hiding this comment

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

Nit: _foo indicates an unused variable, why not split?

Copy link
Member Author

Choose a reason for hiding this comment

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

While we chose to make _x indicate an unused variable, its also traditionally used to indicate a private member of a type.

const moduleKinds = CompilerTest._split(settings.module);
if (scriptTargets.length <= 1 && moduleKinds.length <= 1) {
return { file, payload };
}

const configurations: Harness.FileBasedTestConfiguration[] = [];
for (const scriptTarget of scriptTargets) {
for (const moduleKind of moduleKinds) {
const settings: Record<string, any> = {};
let name = "";
if (moduleKinds.length > 1) {
settings.module = moduleKind;
name += `@module: ${moduleKind || "none"}`;
}
if (scriptTargets.length > 1) {
settings.target = scriptTarget;
if (name) name += ", ";
name += `@target: ${scriptTarget || "none"}`;
}

const settings = { ...testCaseContent.settings };
if (scriptTarget) settings.target = scriptTarget;
if (moduleKind) settings.module = moduleKind;
configurations.push({ name, payload: { ...testCaseContent, settings } });
configurations.push({ name, settings });
Copy link

Choose a reason for hiding this comment

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

If the name is derived from the settings, why not compute it at its use?

}
}

return configurations;
return { file, payload, configurations };
}

public verifyDiagnostics() {
Expand Down
2 changes: 1 addition & 1 deletion src/harness/externalCompileRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ abstract class ExternalCompileRunnerBase extends RunnerBase {

describe(`${this.kind()} code samples`, () => {
for (const test of testList) {
this.runTest(test);
this.runTest(typeof test === "string" ? test : test.file);
}
});
}
Expand Down
24 changes: 13 additions & 11 deletions src/harness/fourslashRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ class FourSlashRunner extends RunnerBase {
}

public enumerateTestFiles() {
// see also: `enumerateTestFiles` in tests/webTestServer.ts
return this.enumerateFiles(this.basePath, /\.ts/i, { recursive: false });
}

Expand All @@ -45,22 +46,23 @@ class FourSlashRunner extends RunnerBase {

public initializeTests() {
if (this.tests.length === 0) {
this.tests = this.enumerateTestFiles();
this.tests = Harness.IO.enumerateTestFiles(this);
}

describe(this.testSuiteName + " tests", () => {
this.tests.forEach((fn: string) => {
describe(fn, () => {
fn = ts.normalizeSlashes(fn);
const justName = fn.replace(/^.*[\\\/]/, "");
this.tests.forEach(test => {
const file = typeof test === "string" ? test : test.file;
describe(file, () => {
let fn = ts.normalizeSlashes(file);
Copy link

Choose a reason for hiding this comment

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

Took me a minute to realize this meant fileName not function

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but in general I am just trying to preserve as much of the original source here as I can.

const justName = fn.replace(/^.*[\\\/]/, "");
Copy link

Choose a reason for hiding this comment

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

Is this ts.getBaseFileName?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly, but that source is unchanged from the original (aside from indentation).


// Convert to relative path
const testIndex = fn.indexOf("tests/");
if (testIndex >= 0) fn = fn.substr(testIndex);
// Convert to relative path
const testIndex = fn.indexOf("tests/");
if (testIndex >= 0) fn = fn.substr(testIndex);

if (justName && !justName.match(/fourslash\.ts$/i) && !justName.match(/\.d\.ts$/i)) {
it(this.testSuiteName + " test " + justName + " runs correctly", () => {
FourSlash.runFourSlashTest(this.basePath, this.testType, fn);
if (justName && !justName.match(/fourslash\.ts$/i) && !justName.match(/\.d\.ts$/i)) {
Copy link

Choose a reason for hiding this comment

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

If we somehow got an empty name we don't run the test -- maybe this should be an assert instead of an if?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly, but that source is unchanged from the original (aside from indentation).

it(this.testSuiteName + " test " + justName + " runs correctly", () => {
FourSlash.runFourSlashTest(this.basePath, this.testType, fn);
});
}
});
Expand Down
28 changes: 24 additions & 4 deletions src/harness/harness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,16 @@ namespace Utils {
}

namespace Harness {
export interface FileBasedTest {
file: string;
configurations?: FileBasedTestConfiguration[];
}

export interface FileBasedTestConfiguration {
name: string;
settings?: Record<string, string>;
}

// tslint:disable-next-line:interface-name
export interface IO {
newLine(): string;
Expand All @@ -514,6 +524,7 @@ namespace Harness {
fileExists(fileName: string): boolean;
directoryExists(path: string): boolean;
deleteFile(fileName: string): void;
enumerateTestFiles(runner: RunnerBase): (string | FileBasedTest)[];
listFiles(path: string, filter?: RegExp, options?: { recursive?: boolean }): string[];
log(text: string): void;
args(): string[];
Expand Down Expand Up @@ -559,6 +570,10 @@ namespace Harness {
return dirPath === path ? undefined : dirPath;
}

function enumerateTestFiles(runner: RunnerBase) {
return runner.enumerateTestFiles();
}

function listFiles(path: string, spec: RegExp, options?: { recursive?: boolean }) {
options = options || {};

Expand Down Expand Up @@ -639,6 +654,7 @@ namespace Harness {
directoryExists: path => ts.sys.directoryExists(path),
deleteFile,
listFiles,
enumerateTestFiles,
log: s => console.log(s),
args: () => ts.sys.args,
getExecutingFilePath: () => ts.sys.getExecutingFilePath(),
Expand Down Expand Up @@ -913,6 +929,11 @@ namespace Harness {
return ts.getDirectoryPath(ts.normalizeSlashes(url.pathname || "/"));
}

function enumerateTestFiles(runner: RunnerBase): (string | FileBasedTest)[] {
const response = send(HttpRequestMessage.post(new URL("/api/enumerateTestFiles", serverRoot), HttpContent.text(runner.kind())));
return hasJsonContent(response) ? JSON.parse(response.content.content) : [];
}

function listFiles(dirname: string, spec?: RegExp, options?: { recursive?: boolean }): string[] {
if (spec || (options && !options.recursive)) {
let results = IO.listFiles(dirname);
Expand Down Expand Up @@ -959,6 +980,7 @@ namespace Harness {
directoryExists,
deleteFile,
listFiles: Utils.memoize(listFiles, (path, spec, options) => `${path}|${spec}|${options ? options.recursive === true : true}`),
enumerateTestFiles: Utils.memoize(enumerateTestFiles, runner => runner.kind()),
log: s => console.log(s),
args: () => [],
getExecutingFilePath: () => "",
Expand Down Expand Up @@ -1779,7 +1801,7 @@ namespace Harness {
// Regex for parsing options in the format "@Alpha: Value of any sort"
const optionRegex = /^[\/]{2}\s*@(\w+)\s*:\s*([^\r\n]*)/gm; // multiple matches on multiple lines

function extractCompilerSettings(content: string): CompilerSettings {
export function extractCompilerSettings(content: string): CompilerSettings {
const opts: CompilerSettings = {};

let match: RegExpExecArray;
Expand All @@ -1800,9 +1822,7 @@ namespace Harness {
}

/** Given a test file containing // @FileName directives, return an array of named units of code to be added to an existing compiler instance */
export function makeUnitsFromTest(code: string, fileName: string, rootDir?: string): TestCaseContent {
const settings = extractCompilerSettings(code);

export function makeUnitsFromTest(code: string, fileName: string, rootDir?: string, settings = extractCompilerSettings(code)): TestCaseContent {
// List of all the subfiles we've parsed out
const testUnitData: TestUnitData[] = [];

Expand Down
3 changes: 2 additions & 1 deletion src/harness/parallel/host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ namespace Harness.Parallel.Host {
const { statSync }: { statSync(path: string): { size: number }; } = require("fs");
const path: { join: (...args: string[]) => string } = require("path");
for (const runner of runners) {
for (const file of runner.enumerateTestFiles()) {
for (const test of runner.enumerateTestFiles()) {
const file = typeof test === "string" ? test : test.file;
let size: number;
if (!perfData) {
try {
Expand Down
2 changes: 1 addition & 1 deletion src/harness/projectsRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ namespace project {
describe("projects tests", () => {
const tests = this.tests.length === 0 ? this.enumerateTestFiles() : this.tests;
for (const test of tests) {
this.runProjectTestCase(test);
this.runProjectTestCase(typeof test === "string" ? test : test.file);
}
});
}
Expand Down
7 changes: 2 additions & 5 deletions src/harness/runnerbase.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
/// <reference path="harness.ts" />


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

abstract class RunnerBase {
// contains the tests to run
public tests: string[] = [];
public tests: (string | Harness.FileBasedTest)[] = [];

/** Add a source file to the runner's list of tests that need to be initialized with initializeTests */
public addTest(fileName: string) {
Expand All @@ -20,7 +17,7 @@ abstract class RunnerBase {

abstract kind(): TestRunnerKind;

abstract enumerateTestFiles(): string[];
abstract enumerateTestFiles(): (string | Harness.FileBasedTest)[];

/** 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 = "";
Expand Down
3 changes: 2 additions & 1 deletion src/harness/rwcRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ namespace RWC {

class RWCRunner extends RunnerBase {
public enumerateTestFiles() {
// see also: `enumerateTestFiles` in tests/webTestServer.ts
return Harness.IO.getDirectories("internal/cases/rwc/");
}

Expand All @@ -245,7 +246,7 @@ class RWCRunner extends RunnerBase {
public initializeTests(): void {
// Read in and evaluate the test list
for (const test of this.tests && this.tests.length ? this.tests : this.enumerateTestFiles()) {
this.runTest(test);
this.runTest(typeof test === "string" ? test : test.file);
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/harness/test262Runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ class Test262BaselineRunner extends RunnerBase {
}

public enumerateTestFiles() {
// see also: `enumerateTestFiles` in tests/webTestServer.ts
return ts.map(this.enumerateFiles(Test262BaselineRunner.basePath, Test262BaselineRunner.testFileExtensionRegex, { recursive: true }), ts.normalizePath);
}

Expand All @@ -113,7 +114,7 @@ class Test262BaselineRunner extends RunnerBase {
});
}
else {
this.tests.forEach(test => this.runTest(test));
this.tests.forEach(test => this.runTest(typeof test === "string" ? test : test.file));
}
}
}
Loading