Skip to content

Commit 9b04dc3

Browse files
rbucktonrbuckton
committed
PR Feedback
1 parent f09c350 commit 9b04dc3

File tree

3 files changed

+115
-84
lines changed

3 files changed

+115
-84
lines changed

‎src/harness/compilerRunner.ts

+3-32
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ class CompilerBaselineRunner extends RunnerBase {
6868
public checkTestCodeOutput(fileName: string, test?: CompilerFileBasedTest) {
6969
if (test && test.configurations) {
7070
test.configurations.forEach(configuration => {
71-
describe(`${this.testSuiteName} tests for ${fileName}${configuration && configuration.name ? ` (${configuration.name})` : ``}`, () => {
71+
describe(`${this.testSuiteName} tests for ${fileName}${configuration ? ` (${Harness.getFileBasedTestConfigurationDescription(configuration)})` : ``}`, () => {
7272
this.runSuite(fileName, test, configuration);
7373
});
7474
});
@@ -82,7 +82,7 @@ class CompilerBaselineRunner extends RunnerBase {
8282
// Mocha holds onto the closure environment of the describe callback even after the test is done.
8383
// Everything declared here should be cleared out in the "after" callback.
8484
let compilerTest: CompilerTest | undefined;
85-
before(() => { compilerTest = new CompilerTest(fileName, test && test.payload, configuration && configuration.settings); });
85+
before(() => { compilerTest = new CompilerTest(fileName, test && test.payload, configuration); });
8686
it(`Correct errors for ${fileName}`, () => { compilerTest.verifyDiagnostics(); });
8787
it(`Correct module resolution tracing for ${fileName}`, () => { compilerTest.verifyModuleResolution(); });
8888
it(`Correct sourcemap content for ${fileName}`, () => { compilerTest.verifySourceMapRecord(); });
@@ -198,31 +198,7 @@ class CompilerTest {
198198
const rootDir = file.indexOf("conformance") === -1 ? "tests/cases/compiler/" : ts.getDirectoryPath(file) + "/";
199199
const payload = Harness.TestCaseParser.makeUnitsFromTest(content, file, rootDir);
200200
const settings = Harness.TestCaseParser.extractCompilerSettings(content);
201-
const scriptTargets = CompilerTest._split(settings.target);
202-
const moduleKinds = CompilerTest._split(settings.module);
203-
if (scriptTargets.length <= 1 && moduleKinds.length <= 1) {
204-
return { file, payload };
205-
}
206-
207-
const configurations: Harness.FileBasedTestConfiguration[] = [];
208-
for (const scriptTarget of scriptTargets) {
209-
for (const moduleKind of moduleKinds) {
210-
const settings: Record<string, any> = {};
211-
let name = "";
212-
if (moduleKinds.length > 1) {
213-
settings.module = moduleKind;
214-
name += `@module: ${moduleKind || "none"}`;
215-
}
216-
if (scriptTargets.length > 1) {
217-
settings.target = scriptTarget;
218-
if (name) name += ", ";
219-
name += `@target: ${scriptTarget || "none"}`;
220-
}
221-
222-
configurations.push({ name, settings });
223-
}
224-
}
225-
201+
const configurations = Harness.getFileBasedTestConfigurations(settings, /*varyBy*/ ["module", "target"]);
226202
return { file, payload, configurations };
227203
}
228204

@@ -291,11 +267,6 @@ class CompilerTest {
291267
this.toBeCompiled.concat(this.otherFiles).filter(file => !!this.result.program.getSourceFile(file.unitName)));
292268
}
293269

294-
private static _split(text: string) {
295-
const entries = text && text.split(",").map(s => s.toLowerCase().trim()).filter(s => s.length > 0);
296-
return entries && entries.length > 0 ? entries : [""];
297-
}
298-
299270
private makeUnitName(name: string, root: string) {
300271
const path = ts.toPath(name, root, ts.identity);
301272
const pathStart = ts.toPath(Harness.IO.getCurrentDirectory(), "", ts.identity);

‎src/harness/harness.ts

+68-10
Original file line numberDiff line numberDiff line change
@@ -499,16 +499,6 @@ namespace Utils {
499499
}
500500

501501
namespace Harness {
502-
export interface FileBasedTest {
503-
file: string;
504-
configurations?: FileBasedTestConfiguration[];
505-
}
506-
507-
export interface FileBasedTestConfiguration {
508-
name: string;
509-
settings?: Record<string, string>;
510-
}
511-
512502
// tslint:disable-next-line:interface-name
513503
export interface IO {
514504
newLine(): string;
@@ -1783,6 +1773,74 @@ namespace Harness {
17831773
}
17841774
}
17851775

1776+
export interface FileBasedTest {
1777+
file: string;
1778+
configurations?: FileBasedTestConfiguration[];
1779+
}
1780+
1781+
export interface FileBasedTestConfiguration {
1782+
[key: string]: string;
1783+
}
1784+
1785+
function splitVaryBySettingValue(text: string): string[] | undefined {
1786+
if (!text) return undefined;
1787+
const entries = text.split(/,/).map(s => s.trim().toLowerCase()).filter(s => s.length > 0);
1788+
return entries && entries.length > 1 ? entries : undefined;
1789+
}
1790+
1791+
function computeFileBasedTestConfigurationVariations(configurations: FileBasedTestConfiguration[], variationState: FileBasedTestConfiguration, varyByEntries: [string, string[]][], offset: number) {
1792+
if (offset >= varyByEntries.length) {
1793+
// make a copy of the current variation state
1794+
configurations.push({ ...variationState });
1795+
return;
1796+
}
1797+
1798+
const [varyBy, entries] = varyByEntries[offset];
1799+
for (const entry of entries) {
1800+
// set or overwrite the variation, then compute the next variation
1801+
variationState[varyBy] = entry;
1802+
computeFileBasedTestConfigurationVariations(configurations, variationState, varyByEntries, offset + 1);
1803+
}
1804+
}
1805+
1806+
/**
1807+
* Compute FileBasedTestConfiguration variations based on a supplied list of variable settings.
1808+
*/
1809+
export function getFileBasedTestConfigurations(settings: TestCaseParser.CompilerSettings, varyBy: string[]): FileBasedTestConfiguration[] | undefined {
1810+
let varyByEntries: [string, string[]][] | undefined;
1811+
for (const varyByKey of varyBy) {
1812+
if (ts.hasProperty(settings, varyByKey)) {
1813+
// we only consider variations when there are 2 or more variable entries.
1814+
const entries = splitVaryBySettingValue(settings[varyByKey]);
1815+
if (entries) {
1816+
if (!varyByEntries) varyByEntries = [];
1817+
varyByEntries.push([varyByKey, entries]);
1818+
}
1819+
}
1820+
}
1821+
1822+
if (!varyByEntries) return undefined;
1823+
1824+
const configurations: FileBasedTestConfiguration[] = [];
1825+
computeFileBasedTestConfigurationVariations(configurations, /*variationState*/ {}, varyByEntries, /*offset*/ 0);
1826+
return configurations;
1827+
}
1828+
1829+
/**
1830+
* Compute a description for this configuration based on its entries
1831+
*/
1832+
export function getFileBasedTestConfigurationDescription(configuration: FileBasedTestConfiguration) {
1833+
let name = "";
1834+
if (configuration) {
1835+
const keys = Object.keys(configuration).sort();
1836+
for (const key of keys) {
1837+
if (name) name += ", ";
1838+
name += `@${key}: ${configuration[key]}`;
1839+
}
1840+
}
1841+
return name;
1842+
}
1843+
17861844
export namespace TestCaseParser {
17871845
/** all the necessary information to set the right compiler settings */
17881846
export interface CompilerSettings {

‎tests/webTestServer.ts

+44-42
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,13 @@ let browser = "IE";
2424
let grep: string | undefined;
2525
let verbose = false;
2626

27-
interface FileTest {
27+
interface FileBasedTest {
2828
file: string;
29-
configurations?: FileTestConfiguration[];
29+
configurations?: FileBasedTestConfiguration[];
3030
}
3131

32-
interface FileTestConfiguration {
33-
name: string;
34-
settings: Record<string, string>;
32+
interface FileBasedTestConfiguration {
33+
[setting: string]: string;
3534
}
3635

3736
function isFileSystemCaseSensitive(): boolean {
@@ -669,7 +668,7 @@ function handleApiEnumerateTestFiles(req: http.ServerRequest, res: http.ServerRe
669668
try {
670669
if (err) return sendInternalServerError(res, err);
671670
if (!content) return sendBadRequest(res);
672-
const tests: (string | FileTest)[] = enumerateTestFiles(content);
671+
const tests: (string | FileBasedTest)[] = enumerateTestFiles(content);
673672
return sendJson(res, /*statusCode*/ 200, tests);
674673
}
675674
catch (e) {
@@ -699,59 +698,62 @@ function enumerateTestFiles(runner: string) {
699698
// Regex for parsing options in the format "@Alpha: Value of any sort"
700699
const optionRegex = /^[\/]{2}\s*@(\w+)\s*:\s*([^\r\n]*)/gm; // multiple matches on multiple lines
701700

702-
function extractCompilerSettings(content: string): Record<string, any> {
703-
const opts: Record<string, any> = {};
701+
function extractCompilerSettings(content: string): Record<string, string> {
702+
const opts: Record<string, string> = {};
704703

705704
let match: RegExpExecArray;
706-
/* tslint:disable:no-null-keyword */
707705
while ((match = optionRegex.exec(content)) !== null) {
708-
/* tslint:enable:no-null-keyword */
709706
opts[match[1]] = match[2].trim();
710707
}
711708

712709
return opts;
713710
}
714711

715-
function split(text: string) {
716-
const entries = text && text.split(",").map(s => s.toLowerCase().trim()).filter(s => s.length > 0);
717-
return entries && entries.length > 0 ? entries : [""];
712+
function splitVaryBySettingValue(text: string): string[] | undefined {
713+
if (!text) return undefined;
714+
const entries = text.split(/,/).map(s => s.trim().toLowerCase()).filter(s => s.length > 0);
715+
return entries && entries.length > 1 ? entries : undefined;
718716
}
719717

720-
function parseCompilerTestConfigurations(file: string): FileTest {
721-
const content = fs.readFileSync(path.join(rootDir, file), "utf8");
722-
const settings = extractCompilerSettings(content);
723-
const scriptTargets = split(settings.target);
724-
const moduleKinds = split(settings.module);
725-
if (scriptTargets.length <= 1 && moduleKinds.length <= 1) {
726-
return { file };
727-
}
728-
729-
const configurations: FileTestConfiguration[] = [];
730-
for (const scriptTarget of scriptTargets) {
731-
for (const moduleKind of moduleKinds) {
732-
const settings: Record<string, any> = {};
733-
let name = "";
734-
if (moduleKinds.length > 1) {
735-
settings.module = moduleKind;
736-
name += `@module: ${moduleKind || "none"}`;
737-
}
738-
if (scriptTargets.length > 1) {
739-
settings.target = scriptTarget;
740-
if (name) name += ", ";
741-
name += `@target: ${scriptTarget || "none"}`;
718+
function computeFileBasedTestConfigurationVariations(configurations: FileBasedTestConfiguration[], variationState: FileBasedTestConfiguration, varyByEntries: [string, string[]][], offset: number) {
719+
if (offset >= varyByEntries.length) {
720+
// make a copy of the current variation state
721+
configurations.push({ ...variationState });
722+
return;
723+
}
724+
725+
const [varyBy, entries] = varyByEntries[offset];
726+
for (const entry of entries) {
727+
// set or overwrite the variation
728+
variationState[varyBy] = entry;
729+
computeFileBasedTestConfigurationVariations(configurations, variationState, varyByEntries, offset + 1);
730+
}
731+
}
732+
733+
function getFileBasedTestConfigurations(settings: Record<string, string>, varyBy: string[]): FileBasedTestConfiguration[] | undefined {
734+
let varyByEntries: [string, string[]][] | undefined;
735+
for (const varyByKey of varyBy) {
736+
if (Object.prototype.hasOwnProperty.call(settings, varyByKey)) {
737+
const entries = splitVaryBySettingValue(settings[varyByKey]);
738+
if (entries) {
739+
if (!varyByEntries) varyByEntries = [];
740+
varyByEntries.push([varyByKey, entries]);
742741
}
743-
configurations.push({ name, settings });
744742
}
745743
}
746744

747-
return { file, configurations };
745+
if (!varyByEntries) return undefined;
746+
747+
const configurations: FileBasedTestConfiguration[] = [];
748+
computeFileBasedTestConfigurationVariations(configurations, {}, varyByEntries, 0);
749+
return configurations;
748750
}
749751

750-
function parseProjectTestConfigurations(file: string): FileTest {
751-
return { file, configurations: [
752-
{ name: `@module: commonjs`, settings: { module: "commonjs" } },
753-
{ name: `@module: amd`, settings: { module: "amd" } },
754-
] };
752+
function parseCompilerTestConfigurations(file: string): FileBasedTest {
753+
const content = fs.readFileSync(path.join(rootDir, file), "utf8");
754+
const settings = extractCompilerSettings(content);
755+
const configurations = getFileBasedTestConfigurations(settings, ["module", "target"]);
756+
return { file, configurations };
755757
}
756758

757759
function handleApiListFiles(req: http.ServerRequest, res: http.ServerResponse) {

0 commit comments

Comments
 (0)