Adding a watcher test #137

pana-cc · 2018-12-12T22:49:22Z

Not ready for PR yet, but I want to check what will happen with the build machines.

pana-cc · 2018-12-12T22:49:59Z

#135

pana-cc · 2018-12-12T23:07:13Z

Ok, PR triggered somewhat builds in AppVeyor.

silkentrance · 2018-12-13T17:19:48Z

You broke the build machines 😄

silkentrance · 2018-12-16T14:38:01Z

@pana-cc I also have tried various combinations, using a line queue and such.
It basically boils down to the fact that the build machines are running the tests at different speeds (in terms of ticks), so that line events eventually will be lost.

My solution to this would be to extend the watcher to accept yet another option (for testing purposes), that will terminate the watcher after N compilation runs.

This in place, one can then inject another source file into the watched project and wait for the watcher to terminate, which requires a spawnSync() instead of just a spawn().

After which one could compare the stdout of the watch process with some existing test fixture.

pana-cc · 2018-12-16T15:04:20Z

I’ll give it a few more attempts. I hope the line events are fired at different times and the subscribe/unsubscribe approach I used let some notifications slip while unsubscribed. The watcher supports interrupts of the current execution so I will need to trigger file changes after certain output from the watcher. For proper tests I really need to be able to do asserts / changes the way they are in the test.

pana-cc · 2018-12-16T15:15:41Z

Some of the machines actually executed the tests now. <3 I wish they were picking them up quicker.

How should we release the next version now? I think to bump the minor version and publish a preview. The master contains small breaking changes related to the custom test ui right?

pana-cc · 2018-12-16T15:23:00Z

It seems the tests passed but the .kill SIGINT interrupt didn’t kill properly the process. I need to simulate “Ctrl + C” sent to the child process.

pana-cc · 2018-12-16T18:38:08Z

The kill SIGINT should be enough to stop the child process, but it doesn't get killed, it doesn't raise "exit", it will turn out to be something stupid like travis' CI shell asking "yes / no"...

silkentrance · 2018-12-17T12:22:03Z

@pana-cc I think that would be the best idea, making this a release candidate / preview release first.

silkentrance · 2018-12-17T12:53:28Z

@pana-cc I think I found the reason for the seemingly erratic behaviour. There is a stdl standard readline from process.stdin. This remains open and will not cause the process to quit. Perhaps we should drop that for now as it is not being used?

silkentrance · 2018-12-17T13:02:03Z

@pana-cc otherwise I have here a solution that lets us run the watcher with spawn sync for a defined number of times, say 2:

#!/usr/bin/env node

// Run "tsc" with watch, upon successful compilation run mocha tests.

const chalk = require("chalk");
const spawn = require("cross-spawn");
const readline = require("readline");
const yargs = require("yargs");

const argv = yargs
    .options({
        p: {
            alias: "project",
            demand: false,
            default: ".",
            describe: "Path to tsconfig file or directory containing tsconfig, passed to `tsc -p <value>`.",
            type: "string",
        },
        t: {
            alias: "tsc",
            demand: false,
            default: "./node_modules/typescript/bin/tsc",
            describe: "Path to executable tsc, by default points to typescript installed as dev dependency. Set to 'tsc' for global tsc installation.",
            type: "string",
        },
        o: {
            alias: "opts",
            demand: false,
            default: "./test/mocha.opts",
            describe: "Path to mocha.opts file containing additional mocha configuration.",
            type: "string",
        },
        m: {
            alias: "mocha",
            demand: false,
            default: "./node_modules/mocha/bin/mocha",
            describe: "Path to executable mocha, by default points to mocha installed as dev dependency.",
            type: "string",
        },
        n: {
            alias: "times",
            demand: false,
            default: undefined,
            describe: "Number of iterations before the watcher process quits. For testing purposes only.",
            type: "number",
        },
        g: {
            alias: "grep",
            demand: false,
            default: undefined,
            describe: "Passed down to mocha: only run tests matching <pattern>",
            type: "string",
        },
        f: {
            alias: "fgrep",
            demand: false,
            default: undefined,
            describe: "Passed down to mocha: only run tests containing <string>",
            type: "string",
        },
    })
    .help("h")
    .alias("h", "help")
    .argv;

const stdl = readline.createInterface({ input: process.stdin });
stdl.on("line", (line) => {
    // TODO: handle "g <pattern>" or "f <pattern>" to run mocha with pattern
    // Ctrl + R may restart mocha test run?
});

let mochap = null;
let errors = 0;

function compilationStarted() {
    if (mochap) {
        mochap.kill("SIGINT");
        console.log();
    }
    mochap = null;
    errors = 0;
}
function foundErrors() {
    errors ++;
}
function compilationComplete() {
    if (errors) {
        console.log(chalk.red("TS errors!"));
        return;
    } else {
        console.log(chalk.gray("Run mocha."));
    }

    const mochaOptions = ["--opts", argv.opts, "--colors"].concat(argv._);
    if (argv.g) {
        mochaOptions.push("-g");
        mochaOptions.push(argv.g);
    }
    if (argv.f) {
        mochaOptions.push("-f");
        mochaOptions.push(argv.f);
    }
    mochap = spawn(argv.mocha, mochaOptions);
    const source = mochap;
    mochap.on("close", (code) => {
        if (source === mochap) {
            if (code) {
                console.log(chalk.red("Exited with " + code));
            } else {
            }
        if (argv.times && (times >= argv.times)) {
            tscp.kill("SIGINT");
        }
            mochap = null;
        }
    });
    mochap.stdout.on("data", (chunk) => {
        // Ensure old processes won't interfere tsc, .pipe here may be good enough.
        if (source === mochap) {
            process.stdout.write(chunk);
        }
    });
    mochap.stderr.on("data", (chunk) => {
        // Ensure old processes won't interfere tsc, .pipe here may be good enough.
        if (source === mochap) {
            process.stderr.write(chunk);
        }
    });
}

const tscp = spawn(argv.tsc, ["-p", argv.project, "-w"]);
const tscl = readline.createInterface({ input: tscp.stdout });
let times = 0;
tscl.on("line", (line) => {
    if (line.indexOf("Compilation complete.") >= 0 || line.indexOf("Found ") >= 0) {
        console.log(line);
        times++;
        compilationComplete();
    } else if (line.indexOf("File change detected.") >= 0) {
        compilationStarted();
        console.log(line);
    } else if (line.indexOf(": error TS") >= 0) {
        console.log(line);
        foundErrors();
    }
});

tscl.on("close", () => {
    stdl.close();
    tscl.close();
});

This can be called with -n 2 for example for running the watch/test loop at most 2 times before it terminates itself.

In the test then we can use spawnSync and get stdout/stderr and compare that to a fixture for example.

pana-cc · 2018-12-17T19:55:35Z

terminate seems to exit the watcher processes on travis (linux?) now something is wrong with the line reader on appveyor (windows?).

silkentrance · 2019-01-05T21:42:48Z

I am about to set up a dedicated windoze machine for testing windoze related issues. Dunno when I am done with that, though, because windoze is giving me a bad time since the latest not-a-win-win-situation-10-update.

silkentrance · 2019-01-08T21:29:42Z

We have a similar problem over at node-tmp: raszi/node-tmp#159

Node on the Windows platform does not support required signals. There is a work around using a Power?Shell script, see nodejs/node#12378 (comment)

silkentrance · 2019-01-10T22:59:09Z

@pana-cc any progress on this?

silkentrance · 2018-12-13T18:04:39Z

test/it/WatcherPackageIT.ts

  @params({ fixture: "watcher", installTypesMocha: false }, "can run watcher")
  @params({ fixture: "watcher", installTypesMocha: true }, "can run watcher with @types/mocha")
-  runTest(params: PackageTestParams) {
+  async runTest(params: PackageTestParams) {
    super.runTest(params);


I don't think that you need to call super here as the watcher will run the same tests from the package.

I want to be sure that if it fails it will be because of the watcher issues, not because of issues with mocha-typescript. Further the watcher will need the dependencies installed I think that's handled in the super call.

silkentrance · 2018-12-13T18:05:40Z

test/it/WatcherPackageIT.ts

+    assert((await this.line()).includes("1 failing"));
+  }
+
+  line(): Promise<string> {


While this looks nice, I think it is also the cause for some race conditions, where multiple line events will be send and get lost while awaiting the next line(). Especially on the much slower vms over at appveyor or the much faster ones on travis.

The last version attaches a single listener for the linereader events and the promises will either resolve if the lines queue is not empty or will wait for a line event but should handle the racing.

silkentrance · 2018-12-13T18:20:46Z

test/it/WatcherPackageIT.ts

 class WatcherPackage extends AbstractPackageITBase {
+
+  watch: ChildProcess;
+  readline: readline.Interface;


Shouldn't this be readline.ReadLine?

type ReadLine = Interface; // type forwarded for backwards compatiblity

I think the "modern stuff" is Interface.

silkentrance · 2019-01-12T16:58:42Z

@pana-cc see also #125 for the conditional skips.

pana-cc · 2019-01-17T21:22:00Z

I gave up signaling node processes for windows, probably I can just throw a "quit[enter]" in the std in but that would be next year.

Add watcher test Add some whitespace, trigger AppVeyor Run only the watcher test on build machines with some more logs Add queue with lines Run all tests

…nding based on platform

…ndows

silkentrance · 2019-01-17T22:42:32Z

@pana-cc I will have a look into this during the weekend. I cannot believe that signalling an external process is that difficult with node on windows. But you never know...

pana-cc · 2019-01-18T20:24:07Z

That would be awesome, either try to run the test for windows or just skip it under win32 and let's prepare the package for v2.0.0 removing the custom ui.

silkentrance · 2019-01-21T20:38:08Z

sry, I did not run the tests on a dedicated windows machine yet.

pana-cc · 2019-02-02T17:38:55Z

I've skipped the watcher test for windows.
I am going to merge it after successful builds, @silkentrance you can take another look at the final version.

silkentrance · 2019-02-05T20:02:05Z

@pana-cc very good. I will take a look at how lerna implemented their system that watches over external processes and terminate them if they do not exit in a timely manner.

pana-cc · 2019-02-05T20:05:29Z

I think we are good for v2 release? I was thinking of RC version but we don’t have a newsletter to announce it so nobody would try an RC anyway. Do you want to bump the package to v2 and npm publish?

silkentrance · 2019-02-05T22:59:15Z

To be honest, I still have no clue on how to do a publish on npm for a package where I am only a contributor for. Let me have a look at the available documentation and I will come back to you tomorrow.

silkentrance · 2019-02-05T23:26:41Z

@pana-cc also, with the work being done in gh-157, we would have to release a 3.x.x soon after, since it moves typedi support into a separate package.

pana-cc force-pushed the more-tests branch from da33a0e to fdcfd0d Compare December 12, 2018 22:53

pana-cc force-pushed the more-tests branch 3 times, most recently from 4ea52dc to 7e6f1a8 Compare December 16, 2018 18:36

pana-cc force-pushed the more-tests branch 9 times, most recently from 84e994f to 27d0fb7 Compare December 17, 2018 07:00

pana-cc force-pushed the more-tests branch 3 times, most recently from b5ac51b to 1464a24 Compare December 17, 2018 19:54

pana-cc force-pushed the more-tests branch 2 times, most recently from e8da54c to 4921c88 Compare December 19, 2018 20:09

silkentrance requested changes Jan 12, 2019

View reviewed changes

PanayotCankov added 6 commits January 18, 2019 00:11

Run tsc on pretest

d2c4ebb

Add watcher test Add some whitespace, trigger AppVeyor Run only the watcher test on build machines with some more logs Add queue with lines Run all tests

Add some logging to figure out what's wrong with windows

c0ddd73

Make the modifiers accept boolean as first argument so we can skip/pe…

ee31562

…nding based on platform

Add a @Skip(isWin) support and skip the watcher tests on windows.

83d188b

Add a conditional skip, some test for it, remove watcher tests for wi…

79dd372

…ndows

Log the platform, what is it on appveyor?

8b1e508

pana-cc force-pushed the more-tests branch from d547ee1 to a119305 Compare January 17, 2019 22:18

pana-cc force-pushed the more-tests branch from a119305 to 40ecbb0 Compare February 2, 2019 16:56

Remove the mocha-typescript custom ui from the watcher

0477898

pana-cc force-pushed the more-tests branch from 40ecbb0 to 0477898 Compare February 2, 2019 17:36

pana-cc merged commit a271dab into master Feb 5, 2019

pana-cc deleted the more-tests branch February 5, 2019 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a watcher test #137

Adding a watcher test #137

pana-cc commented Dec 12, 2018

pana-cc commented Dec 12, 2018

pana-cc commented Dec 12, 2018

silkentrance commented Dec 13, 2018

silkentrance commented Dec 16, 2018

pana-cc commented Dec 16, 2018

pana-cc commented Dec 16, 2018

pana-cc commented Dec 16, 2018

pana-cc commented Dec 16, 2018

silkentrance commented Dec 17, 2018

silkentrance commented Dec 17, 2018

silkentrance commented Dec 17, 2018

pana-cc commented Dec 17, 2018 •

edited

silkentrance commented Jan 5, 2019

silkentrance commented Jan 8, 2019

silkentrance commented Jan 10, 2019

silkentrance Dec 13, 2018

pana-cc Jan 17, 2019

silkentrance Dec 13, 2018

pana-cc Jan 17, 2019

silkentrance Dec 13, 2018

pana-cc Jan 17, 2019

silkentrance commented Jan 12, 2019

pana-cc commented Jan 17, 2019

silkentrance commented Jan 17, 2019

pana-cc commented Jan 18, 2019 •

edited

silkentrance commented Jan 21, 2019

pana-cc commented Feb 2, 2019

silkentrance commented Feb 5, 2019

pana-cc commented Feb 5, 2019

silkentrance commented Feb 5, 2019

silkentrance commented Feb 5, 2019

Adding a watcher test #137

Adding a watcher test #137

Conversation

pana-cc commented Dec 12, 2018

pana-cc commented Dec 12, 2018

pana-cc commented Dec 12, 2018

silkentrance commented Dec 13, 2018

silkentrance commented Dec 16, 2018

pana-cc commented Dec 16, 2018

pana-cc commented Dec 16, 2018

pana-cc commented Dec 16, 2018

pana-cc commented Dec 16, 2018

silkentrance commented Dec 17, 2018

silkentrance commented Dec 17, 2018

silkentrance commented Dec 17, 2018

pana-cc commented Dec 17, 2018 • edited

silkentrance commented Jan 5, 2019

silkentrance commented Jan 8, 2019

silkentrance commented Jan 10, 2019

silkentrance Dec 13, 2018

Choose a reason for hiding this comment

pana-cc Jan 17, 2019

Choose a reason for hiding this comment

silkentrance Dec 13, 2018

Choose a reason for hiding this comment

pana-cc Jan 17, 2019

Choose a reason for hiding this comment

silkentrance Dec 13, 2018

Choose a reason for hiding this comment

pana-cc Jan 17, 2019

Choose a reason for hiding this comment

silkentrance commented Jan 12, 2019

pana-cc commented Jan 17, 2019

silkentrance commented Jan 17, 2019

pana-cc commented Jan 18, 2019 • edited

silkentrance commented Jan 21, 2019

pana-cc commented Feb 2, 2019

silkentrance commented Feb 5, 2019

pana-cc commented Feb 5, 2019

silkentrance commented Feb 5, 2019

silkentrance commented Feb 5, 2019

pana-cc commented Dec 17, 2018 •

edited

pana-cc commented Jan 18, 2019 •

edited