Change testsupport to not use child processes

felixfbecker commented 8 years ago

I have tests for my debugadapter for quite some time now, but spawning the debuadapter for every test has only proven to cause problems:

It is extremely slow, especially on Windows
Constantly there are issues with timeouts or hanging that only happen in CI. I have to test against 3 PHP versions x 3 XDebug versions and these random failures mean that at least one job always fails, breaking the build. I never had a stable green build ever since I added tests.
It is impossible to generate code coverage with istanbul/nyc. Even though nyc claims to work with child processes, my debug adapter file is getting 0% coverage in the report.

This could be made much easier. In the mock debug extension, the main file declares a class and then directly calls run() to run the adapter. This could be changed to

export class MyDebugSession extends DebugSession {
  // ...
}
if (require.main === module) {
  DebugSession.run(MyDebugSession);
}

making it only run when the file was the entry point of the application. Then in the tests, we could just import the session, instantiate it and call the methods directly on it, providing object literals as parameters that conform to the debug protocol interfaces. sendResponse() and sendErrorResponse() can easily be stubbed by sinon, asserting that they have been called with specific parameters.

That way, we only test the actual unit under test, our own debug adapter, and trust vscode-debugadapter-node to function correctly. Currently we basically test the whole stack including vscode-debugadapter-node's protocol parsing, method dispatching and response handling, which is not a good practice.

weinand commented 8 years ago

@felixfbecker yes, your suggestion is a useful option for node based debug adapters. But most debug adapters are not node based, so there your approach doesn't help.

felixfbecker commented 8 years ago

True, but those debug adapters don't use the vscode-debugadapter-node module either. I don't know the reality but I imagine that the C# debug adapter would write its tests in C# aswell and not in TypeScript? Because then they can use the same approach, just ported to their language (they have to port a bunch of stuff anyway).

weinand commented 8 years ago

@felixfbecker we've never experienced any problems when using the testsupport, so we do not intend to 'change' this.

felixfbecker commented 8 years ago

Hm, then there must be something wrong with how I use it...

felixfbecker commented 8 years ago

we've never experienced any problems when using the testsupport

Have you tried generating a coverage report?

weinand commented 8 years ago

This is planned. We are using https://coveralls.io for VS Code already...

felixfbecker commented 8 years ago

It currently does not work with testsupport, because of the child processes. Coverage output is always 0%.

Btw I would suggest you to take a look at https://codecov.io/ - coveralls doesn't support merging coverage of multiple CI providers for the same commit, which is a problem if you want to run Linux tests on Travis and Windows tests on AppVeyor. Recently faced this when setting up windows tests for https://github.com/sequelize/sequelize. We are much happier with codecov, the interface is much nicer too - you get coverage diffs and can navigate the project with a file browser.

microsoft / vscode-debugadapter-node

Change testsupport to not use child processes #56