Add DefinitelyTyped runner #19815

sandersn · 2017-11-07T18:11:39Z

Adds a test runner that compiles DefinitelyTyped packages with the currently built compiler. You must have a DefinitelyTyped repo named DefinitelyTyped next to your Typescript repo.

The test only outputs a log file to baselines/local/definitely/ when there is a failure. Currently about 75 packages have compile failures. I expect this number to fluctuate up and down rapidly based on DefinitelyTyped's amount of traffic, so I have not included the baselines here, after discussion with @mhegazy. A run takes about 15 minutes on my machine, so it's simple, if slow, to baseline from master when needed.

I just tested on #17765 and the experience is many times better and faster than the shell scripts I used before.

Assumes that ../DefinitelyTyped holds the DefinitelyTyped repo.

1. Only `npm install` packages with a package.json 2. Add `workingDirectory` to runnerBase to differentiate input directory from output directory (which should be different for definitelyRunner). 3. Don't output anything on success.

sandersn · 2017-11-07T18:20:27Z

By the way, for @andy-ms, 31 of the 78 errors that DefinitelyTyped has when compiling on master were from missing lib entries from dependencies. The others are a random mish-mash of errors that have been introduced since 2.0.

weswigham

Also, maybe dt instead of definitely mostly because I think t=dt is shorter and I'm liable to misspell t=definitely a few times.

weswigham · 2017-11-07T18:28:44Z

src/harness/definitelyRunner.ts

+                const timeout = 600000; // 600s = 10 minutes
+                if (fs.existsSync(path.join(cwd, 'package.json'))) {
+                    const stdio = isWorker ? "pipe" : "inherit";
+                    const install = cp.spawnSync(`npm`, ["i"], { cwd, timeout, shell: true, stdio });


Should we delete a package-lock.json before running npm i? As is, if a DT package updates a dep, I don't think it'd get tested here.

Ugggg. Is there a way not to emit package-lock.json? I'd rather do that instead.

Nope! Guess I'll have to remove it!

weswigham · 2017-11-07T18:35:11Z

If we're not committing the baselines, I suppose we have no interest in adding it to the daily cron build? Would this be something we'd be more comfortable with as like a weekly build? Or is it because the DT repo already has similar associated tests?

sandersn · 2017-11-07T18:53:07Z

Yes, at this point we don't want to baseline for a couple of reasons. (1) DefinitelyTyped isn't clean and will likely fluctuate a lot until (if) it's clean; (2) this is currently intended as an automatic way to check new changes with DT faster (3) also that dt-runner claims DT is clean, even if the compiler itself disagrees.
I'll switch to dt. I don't like acronyms, but definitely is hard to type.

mhegazy · 2017-11-07T19:15:33Z

If we're not committing the baselines, I suppose we have no interest in adding it to the daily cron build? Would this be something we'd be more comfortable with as like a weekly build? Or is it because the DT repo already has similar associated tests?

we already have another test run going against DT. this one is more of a local validation after a change.

mhegazy · 2017-11-07T19:17:08Z

src/harness/dtRunner.ts

+    public workingDirectory = DefinitelyTypedRunner.testDir;
+
+    public enumerateTestFiles() {
+        return Harness.IO.getDirectories(DefinitelyTypedRunner.testDir);


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.

weswigham · 2017-11-07T19:17:27Z

src/harness/dtRunner.ts

+    public workingDirectory = DefinitelyTypedRunner.testDir;
+
+    public enumerateTestFiles() {
+        return Harness.IO.getDirectories(DefinitelyTypedRunner.testDir);


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.

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

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

mhegazy · 2017-11-07T19:18:59Z

@andy-ms can you take a look as well.

ghost · 2017-11-08T17:26:09Z

src/harness/dtRunner.ts

+        // Read in and evaluate the test list
+        const testList = this.tests && this.tests.length ? this.tests : this.enumerateTestFiles();
+
+        describe(`${this.kind()} code samples`, () => {


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

ghost · 2017-11-08T17:27:02Z

src/harness/dtRunner.ts

+                }
+                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


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

ghost · 2017-11-08T17:28:05Z

src/harness/tsconfig.json

@@ -93,6 +93,7 @@
        "loggedIO.ts",
        "rwcRunner.ts",
        "userRunner.ts",
+        "definitelyRunner.ts",


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

Just by gulp. Fixed.

sandersn · 2017-11-08T21:32:17Z

OK, I pulled everything up into a superclass except a couple of properties. I stuck all three classes in one file since the subclasses are now, essentially, just pieces of configuration (that still needs to be newed).

ghost · 2017-11-08T21:37:49Z

src/harness/externalCompileRunner.ts

+    }
+}
+
+class DefinitelyTypedRunner extends ExternalCompileRunnerBase {


This is starting to look like it should be just another instance of a class instead of its own class.

True, except that Wesley is shortly going to want to put in more code into the UserCodeRunner, and I already need to put more code into the DefinitelyTypedRunner to skip ExpectErrors. I'll keep it the way it is for now and we can simplify later if neither of those efforts work out.

weswigham · 2017-11-08T22:01:23Z

src/harness/externalCompileRunner.ts

+                Harness.Baseline.runBaseline(`${this.kind()}/${directoryName}.log`, () => {
+                    const result = cp.spawnSync(`node`, [path.join(__dirname, "tsc.js")], { cwd, timeout, shell: true });
+                    // tslint:disable-next-line:no-null-keyword
+                    return result.status === 0 ? null : `Exit Code: ${result.status}


As mentioned in person, check result.status and result.stdout and result.stderr instead of just result.status - we want to capture any output even when we report success.

I've got git problems and I'm not even on Windows!

mihailik · 2017-11-09T10:51:38Z

Jakefile.js

@@ -105,7 +105,7 @@ var harnessCoreSources = [
    "projectsRunner.ts",
    "loggedIO.ts",
    "rwcRunner.ts",
-    "userRunner.ts",
+    "externalCompileRunner.ts",


Jakefile, but not gulp?

The official TS homepage refers to gulp. Does it mean both gulp/jake need to be used, depending on area of the project?

If they don't behave the same it's a bug.

Gulp uses the tsconfig edited below.

sandersn added 3 commits November 6, 2017 15:16

Add DefinitelyTyped test runner

e6c38bf

Assumes that ../DefinitelyTyped holds the DefinitelyTyped repo.

Update Jakefile with definitelyRunner.ts

f2d4b36

DefinitelyRunner cleanup and speedup

9a415a2

1. Only `npm install` packages with a package.json 2. Add `workingDirectory` to runnerBase to differentiate input directory from output directory (which should be different for definitelyRunner). 3. Don't output anything on success.

sandersn requested review from weswigham and mhegazy November 7, 2017 18:19

weswigham reviewed Nov 7, 2017

View reviewed changes

Fix lint and allow null keyword

2378ff3

sandersn added 2 commits November 7, 2017 10:57

Change runner name from 'definitely' to 'dt'

88a31d6

Remove package-lock.json before npm install

5e5b565

mhegazy reviewed Nov 7, 2017

View reviewed changes

weswigham reviewed Nov 7, 2017

View reviewed changes

mhegazy approved these changes Nov 7, 2017

View reviewed changes

mhegazy requested a review from a user November 7, 2017 19:18

ghost reviewed Nov 8, 2017

View reviewed changes

sandersn added 2 commits November 8, 2017 13:28

Refactor user+dt runners into externalCompilerRunner

d64a8f6

Remove positive baselines for user tests

397b549

ghost approved these changes Nov 8, 2017

View reviewed changes

weswigham reviewed Nov 8, 2017

View reviewed changes

Use CRLF and emit test output for stdout/stderr

bb79308

I've got git problems and I'm not even on Windows!

sandersn merged commit 594ac01 into master Nov 8, 2017

sandersn deleted the add-definitely-typed-runner branch November 8, 2017 23:45

mihailik reviewed Nov 9, 2017

View reviewed changes

microsoft locked and limited conversation to collaborators Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DefinitelyTyped runner #19815

Add DefinitelyTyped runner #19815

sandersn commented Nov 7, 2017

sandersn commented Nov 7, 2017

weswigham left a comment

weswigham Nov 7, 2017

sandersn Nov 7, 2017

sandersn Nov 7, 2017

weswigham commented Nov 7, 2017

sandersn commented Nov 7, 2017

mhegazy commented Nov 7, 2017

mhegazy Nov 7, 2017

weswigham Nov 7, 2017

sandersn Nov 7, 2017

ghost Nov 8, 2017

mhegazy commented Nov 7, 2017

ghost Nov 8, 2017

ghost Nov 8, 2017

ghost Nov 8, 2017

sandersn Nov 8, 2017

sandersn commented Nov 8, 2017

ghost Nov 8, 2017

sandersn Nov 8, 2017

weswigham Nov 8, 2017 •

edited

Loading

sandersn Nov 8, 2017

mihailik Nov 9, 2017

ghost Nov 9, 2017

sandersn Nov 9, 2017

Add DefinitelyTyped runner #19815

Add DefinitelyTyped runner #19815

Conversation

sandersn commented Nov 7, 2017

sandersn commented Nov 7, 2017

weswigham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham commented Nov 7, 2017

sandersn commented Nov 7, 2017

mhegazy commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham Nov 8, 2017 •

edited

Loading