Skip to content

Batch enumerateFiles into a single web request #23972

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.

Sign up for GitHub

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 2 commits into from
May 9, 2018
Merged

Batch enumerateFiles into a single web request #23972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f09c350
Batch enumerateFiles for multiple configurations into a single web re…
rbuckton May 8, 2018
9b04dc3
PR Feedback
rbuckton May 9, 2018
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
87 changes: 41 additions & 46 deletions src/harness/compilerRunner.ts
View file
Open in desktop
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 ? ` (${Harness.getFileBasedTestConfigurationDescription(configuration)})` : ``}`, () => {
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); });
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,14 @@ 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);
for (const scriptTarget of scriptTargets) {
for (const moduleKind of moduleKinds) {
let name = "";
if (moduleKinds.length > 1) {
name += `@module: ${moduleKind || "none"}`;
}
if (scriptTargets.length > 1) {
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 } });
}
}

return configurations;
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) + "/";
const payload = Harness.TestCaseParser.makeUnitsFromTest(content, file, rootDir);
const settings = Harness.TestCaseParser.extractCompilerSettings(content);
const configurations = Harness.getFileBasedTestConfigurations(settings, /*varyBy*/ ["module", "target"]);
return { file, payload, configurations };
}

public verifyDiagnostics() {
Expand Down Expand Up @@ -267,11 +267,6 @@ class CompilerTest {
this.toBeCompiled.concat(this.otherFiles).filter(file => !!this.result.program.getSourceFile(file.unitName)));
}

private static _split(text: string) {
const entries = text && text.split(",").map(s => s.toLowerCase().trim()).filter(s => s.length > 0);
return entries && entries.length > 0 ? entries : [""];
}

private makeUnitName(name: string, root: string) {
const path = ts.toPath(name, root, ts.identity);
const pathStart = ts.toPath(Harness.IO.getCurrentDirectory(), "", ts.identity);
Expand Down
2 changes: 1 addition & 1 deletion src/harness/externalCompileRunner.ts
View file
Open in desktop
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
View file
Open in desktop
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
Contributor 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
Contributor 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
Contributor 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
86 changes: 82 additions & 4 deletions src/harness/harness.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,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 +560,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 +644,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 +919,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 +970,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 @@ -1761,6 +1773,74 @@ namespace Harness {
}
}

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

export interface FileBasedTestConfiguration {
[key: string]: string;
}

function splitVaryBySettingValue(text: string): string[] | undefined {
if (!text) return undefined;
const entries = text.split(/,/).map(s => s.trim().toLowerCase()).filter(s => s.length > 0);
return entries && entries.length > 1 ? entries : undefined;
}

function computeFileBasedTestConfigurationVariations(configurations: FileBasedTestConfiguration[], variationState: FileBasedTestConfiguration, varyByEntries: [string, string[]][], offset: number) {
if (offset >= varyByEntries.length) {
// make a copy of the current variation state
configurations.push({ ...variationState });
return;
}

const [varyBy, entries] = varyByEntries[offset];
for (const entry of entries) {
// set or overwrite the variation, then compute the next variation
variationState[varyBy] = entry;
computeFileBasedTestConfigurationVariations(configurations, variationState, varyByEntries, offset + 1);
}
}

/**
* Compute FileBasedTestConfiguration variations based on a supplied list of variable settings.
*/
export function getFileBasedTestConfigurations(settings: TestCaseParser.CompilerSettings, varyBy: string[]): FileBasedTestConfiguration[] | undefined {
let varyByEntries: [string, string[]][] | undefined;
for (const varyByKey of varyBy) {
if (ts.hasProperty(settings, varyByKey)) {
// we only consider variations when there are 2 or more variable entries.
const entries = splitVaryBySettingValue(settings[varyByKey]);
if (entries) {
if (!varyByEntries) varyByEntries = [];
varyByEntries.push([varyByKey, entries]);
}
}
}

if (!varyByEntries) return undefined;

const configurations: FileBasedTestConfiguration[] = [];
computeFileBasedTestConfigurationVariations(configurations, /*variationState*/ {}, varyByEntries, /*offset*/ 0);
return configurations;
}

/**
* Compute a description for this configuration based on its entries
*/
export function getFileBasedTestConfigurationDescription(configuration: FileBasedTestConfiguration) {
let name = "";
if (configuration) {
const keys = Object.keys(configuration).sort();
for (const key of keys) {
if (name) name += ", ";
name += `@${key}: ${configuration[key]}`;
}
}
return name;
}

export namespace TestCaseParser {
/** all the necessary information to set the right compiler settings */
export interface CompilerSettings {
Expand All @@ -1779,7 +1859,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 +1880,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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading