Batch enumerateFiles into a single web request by rbuckton · Pull Request #23972 · microsoft/TypeScript

rbuckton · 2018-05-08T20:24:04Z

One of the things we added in the VFS PR was the ability to define multiple values for // @module: and // @target: in compiler tests, to eventually migrate over the project tests. Because this was happening during test discovery, running the tests in the browser would take some time before the test run could start.

This PR changes the behavior of enumerateTestFiles to list the available test files and determine multiple configurations in a single HTTP request, speeding up initial load time when using runtests-browser.

Fixes #23877

…quest

ghost

Why is it important that we parse the test configurations early on? Couldn't we wait to handle that until we're actually running the test (which may not happen if just running one test)?

ghost · 2018-05-08T20:55:36Z

src/harness/compilerRunner.ts

+    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) + "/";


nit: ts.contains useful here

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

Sorry, ts.stringContains

ghost · 2018-05-08T20:57:45Z

src/harness/compilerRunner.ts

+        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 scriptTargets = CompilerTest._split(settings.target);


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

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

ghost · 2018-05-08T21:00:42Z

src/harness/compilerRunner.ts

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


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

ghost · 2018-05-08T21:02:28Z

src/harness/fourslashRunner.ts

+            this.tests.forEach(test => {
+                const file = typeof test === "string" ? test : test.file;
+                describe(file, () => {
+                    let fn = ts.normalizeSlashes(file);


Took me a minute to realize this meant fileName not function

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

ghost · 2018-05-08T21:04:04Z

src/harness/fourslashRunner.ts

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


Is this ts.getBaseFileName?

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

ghost · 2018-05-08T21:06:00Z

src/harness/fourslashRunner.ts

-                        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)) {


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

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

ghost · 2018-05-08T21:08:20Z

tests/webTestServer.ts

+        case "conformance":
+        case "compiler":
+            return listFiles(`tests/cases/${runner}`, /*serverDirname*/ undefined, /\.tsx?$/, { recursive: true }).map(parseCompilerTestConfigurations);
+        case "fourslash":


For these other cases only the directory name varies -- could use a map or switch statement for just that, then return listFiles(dir, ...); unconditionally.

True, but the code becomes more complicated and much less readable that way.

ghost · 2018-05-08T21:08:56Z

tests/webTestServer.ts

+
+    let match: RegExpExecArray;
+    /* tslint:disable:no-null-keyword */
+    while ((match = optionRegex.exec(content)) !== null) {


Nit: prefer // tslint:disable-line no-null-keyword on this line, or // tslint:disable-next-line no-null-keyword on the previous line

Actually, that's unneeded since I've disabled that rule throughout the whole file.

rbuckton · 2018-05-09T18:10:30Z

Why is it important that we parse the test configurations early on? Couldn't we wait to handle that until we're actually running the test (which may not happen if just running one test)?

When you are performing combinatorial testing of a feature, you want to ensure each combination is tested in isolation. So if you write a test that uses // @target: es2015, es5, you effectively need a different test suite for es2015 and es5, which is what we've always done previously for the Projects test runner.

To know whether we need to have a single or multiple configurations we need to parse the test option comments at the top of the file. We cannot do this when the test starts since we need to properly set up the various test suites and tests using describe, before, and it early for Mocha's test discovery.

Batch enumerateFiles for multiple configurations into a single web re…

f09c350

…quest

rbuckton requested review from a user and mhegazy May 8, 2018 20:24

ghost reviewed May 8, 2018

View reviewed changes

PR Feedback

9b04dc3

ghost approved these changes May 9, 2018

View reviewed changes

rbuckton merged commit 5af7e06 into master May 9, 2018

rbuckton deleted the batchTestConfigurationsForBrowser branch May 9, 2018 21:15

microsoft locked and limited conversation to collaborators Jul 31, 2018

Conversation

rbuckton commented May 8, 2018

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost May 8, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton commented May 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ghost May 8, 2018 •

edited by ghost

Loading