Webpack Hot Module support + other options

[oguimbal]

TLDR / how to test

See this sample repository

Why ?

I am working on big typescript projects (hundreds of thousands of lines of codes, hundreds of tests).

This starts to be perticularly painful to me when it comes to testing. Your extension is great (thanks, by the way), but there is a huge overhead on every single test launch / test scan on big projects.

I modified (a bit :D ) your extension to leverage Webpack HMR in order to remove this overhead.

Example of the times I do not lose anymore (everything is amost instantaneous):

• When I change a file >10 sec to scan for test changes/new tests
• When I hit "start test" >10 sec to execute this test (mostly Typescript compilation)
• When I hit debug => same same

There are some tradeoffs (like losing guaranteed statelessness), but the resulting experience is perticularly joyful.

Given that this pull request modifies a lot of things, I would understand if you do not want to merge it. I created a fork of this extension that is quite OK for me.

However I tried to be as iso-functionality as possible, given that I dont have the time to test all the scenarios that this extension supports (launcher script, etc...)

Core changes & philosophy

When not enabling hmr support, there is no big change. However, when enabling it, there is only one worker that is spawned, and that stays up (it receives HMR updates).

A worker is now representated on the extension side by the class WorkerInstance (which is in the file I stupidely named listener-instance.ts)

You will notice that WorkerInstance has multiple IWorkerSessions.
When HMR is enabled, multiple run sessions can run in a single worker. This is the purpose of this interface, which has two instances types, found in load-session.ts and run-session.ts (respectively "test scanning" & "test run" functionalities).

Other details

I added two other extension options, which are unrelated to HMR:

• mochaExplorer.nodeArgs which allows you to pass optional arguments to nodejs (I am running node 8.10, and I need generators support, that is opt-in and I had no way to pass --harmony-async-iteration to the worker process)
• mochaExplorer.skipFrames which supports skipping frames in findCallLocation(...) ... this is useful when using helper functions that are wrapping methods such as describe() or it() (in which case, "goto file" was pointing to the wrapper, not the actual test)

Disclaimer

It works for us so far, but these changes are not (yet) extensively tested.

[hbenl]

Wow, that's impressive! However, since the adapter works so differently in HMR mode, I think it would make sense to keep your forked extension separate - especially because I don't know much about HMR, so you're certainly more qualified than me to maintain this.
Another option to work around the poor performance when using ts-node in a big project would be to write the compiled files to disk and then run those (with source-map-support the test locations would still be mapped to the typescript files). In this way, you also wouldn't have to recompile everything every time (and you could use babel with its typescript plugin for watching and compiling the files - since babel will only process the file(s) that have changed, it may work significantly faster for big projects). There is currently one problem with this approach: to get notified of test file changes, the adapter currently uses VS Code's DidSaveTextDocument event for the test files - that doesn't work in this scenario because VS Code will send an event for the typescript file but the adapter waits for an event for the javascript file. I'll have to find a fix for that (like optionally using a proper file watcher instead of realying on the DidSaveTextDocument event) so I don't have to push users to use ts-node.
If you tried this option I would be interested to hear how it compares to your HMR solution.

[oguimbal]

Thanks. That is up to you :)

I've noticed the use of DidSaveTextDocument ... actually, this is also a drawback when using HMR : All tests are retired when saving any document (but there is no problem being notified of new/changed tests, since modified/created files are being ran by HMR automatically).
I did not mention it, but I think i will investigate (as soon as I have some time) if it is possible to read the webpack dependency tree in order to retire the right tests when saving a file (which would retire exactly the right tests, since it would tell us which tests depends on which files). Regarding HMR, this approach seems more powerfull than a file watcher, but it is not applicable to other scenarios.

As for compiling to files, it could indeed remove a bit of overhead, but not all of it since a worker has to be spawned each time (when scanning test or running/debugging them), which has to parse everything (i.e. load every test dependency). This is also quite an overhead which takes seconds for us. When using HMR, the worker is already spawned, and only changed modules are reloaded from disk when something changes. Which is really fast.

Other (lighter) approaches that could work to detech changes on mapped files (since file watchers are hell), could be:

• Either to try to auto-detect "outDir" reading tsconfig.json or whatever, and map "DidSaveTextDocument" event accordingly.
• Or to hook stacktrace preparation before source-map-support modifies it, so you can access the original file names (non mapped) - see below

const originalSource = Symbol('original_source');
export function hookStack() {
	const originalPrepare: any = Error.prepareStackTrace;
	if (!originalPrepare) {
		return function(){};
	}
	Error.prepareStackTrace = function(this: any, error: any, stack: any[]) {
		// retreive orignal file names (non source mapped)
		// and set them on error object
		const sources = stack.map(f => f.isNative()
				? '<native>'
				:  (f.getFileName() || f.getScriptNameOrSourceURL()));
		error[originalSource] = sources;
		return originalPrepare.apply(this, arguments);
	};
	return () => Error.prepareStackTrace = originalPrepare;
}

And then, in patchMocha.ts : findCallLocation()

const dispose = hookStack();
const err = new Error();
const stackFrames = stackTrace.parse(err);
 // this contains the non source mapped file names
const originalFiles = (err as any)[originalSource];
dispose();

This way, "non source-mapped" file names could be sent to the extension, which could thus map DidSaveTextDocument events to actual test files.

[oguimbal]

Oh well, since I think about it, about reting exactly the right tests when changing a file:

The worker could hook require() function in order to keep track of the tests dependency tree (i.e. have a tree of which tests depends on which files). Doing so, we could tell which tests to retire when a non-test file is changed (instead of retiring ALL tests). This would be quite nice when "Enable autorun" is set (only impacted tests would re-run).

That should not be that hard to implement, and should be completely agnostic on configuration (ts-node, tsc --watch, etc...)

[hbenl]

I was also planning to use some kind of dependency analysis to retire only affected tests when a non-test file changes - when I find the time to do it. I was thinking about using an npm module like dependency-tree, but that would mean another round of parsing after every change... so creating a require() hook sounds like an interesting idea for that.

As for compiling to files, it could indeed remove a bit of overhead, but not all of it since a worker has to be spawned each time (when scanning test or running/debugging them), which has to parse everything (i.e. load every test dependency).

Ah, yes, I had not thought about that. Damn. Perhaps I could teach the adapter to read a single test file (the one that changed) and then to weave the newly discovered tests into the test tree from the last test discovery. This could work well when you have lots of unit tests, but if you have end-to-end tests, those will draw in large parts of the application and so it wouldn't help much in that case...

[jedwards1211]

@oguimbal since Webpack introduces all of this bundling and source mapping I think a better long-term solution for this would be to search for/create a system for hot reloading modules in node itself (it's totally possible, you can delete modules from require.cache and then reload them.)

Note, relying on hot reloading in a persistent worker would have other consequences on flexibility. For example, I want to use this extension but I currently can't because there's no way to configure different mocha options for different directories of tests (unit vs end-to-end tests, for example). To support different mocha options in this way, you'd have to have a separate persistent worker process for each directory configuration. (And of course, if you have multiple project roots in your workspace, one persistent worker for each).

but if you have end-to-end tests, those will draw in large parts of the application and so it wouldn't help much in that case...

Yup, in my end-to-end tests, most changes to server code would have to rerun everything, plus my server code doesn't require any of the client-side bundles since they're only referenced via <script> tags.

[oguimbal]

@jedwards1211 Sure, I get that. You're right, you would need several workers.

but if you have end-to-end tests, those will draw in large parts of the application and so it wouldn't help much in that case...

Regarding this... that's not exactly true. For two reasons:

1. Your worker will reload modules when you save your file, not when executing a test. Which means that you wont have to wait js interpretation + execution when executing your tests later.

2. While many of your files might reexecute when changing a file which burried deep in your codebase, everything is not reexecuted: only the require-chains that are linking the changed files to your tests.
   This is an important difference, mostlly because it excludes all your node_modules packages, which are not reexcuted (since they're already loaded, and do not depend on the changed file)... external modules turn out to eat up most of the overhead when running tests, even on million lines projects i'm working on.

An element of comparison: Running a single end-to-end test (when developing a feature by iteration ) has fallen for me from ~5-10 seconds to almost zero. Which is a very enjoyable experience, worthing the constraints of a background worker.

[jedwards1211]

I was talking more about the issue of figuring out exactly which tests need to be rerun when I change a file. Require analysis would mistakenly conclude none of my selenium integration tests need to be rerun when I change client side code for example, because the tests are only directly requiring server-side code.

As far as patching code changes into watch mode, it seems like it might be good for mocha itself to support this?

[oguimbal]

Yes right, this thing has been designed to test nodejs backend stuff. I've not given any thought about UI when writing this.

As far as patching code changes into watch mode, it seems like it might be good for mocha itself to support this?

I'm not sure to understand your question. Are you speaking about HMR patching ? This PR is agnostic on that... it is just executing a JS bundle, and waiting for something to happen (test runs, test discoveries, etc).

As it happens, I am using Weback for hot-reload, and it's webpack which does the job by reloading the required modules. HMR is an inside job of the generated bundle. This extension is not involved in it, nor is mocha.

I suppose HMR could be handled somehow else, as long as global['mocha-hot-reload']() is called after each hot reload (which can be done safely using this helper : require('vscode-mocha-hmr')() )

(Did this answer your question ? or am I going astray ? - i'm French, I sometime miss some subtleties in english sentences)

[jedwards1211]

Oh, I was just saying, it would be nice if Mocha's watch mode could support rerunning tests without reloading all modules, then not only could this VSCode extension be able to rerun tests faster, but so could anyone who has to use Mocha from the CLI

[oguimbal]

Right, I get you now 🎉 That's an interesting idea, I might give it some thought when I have time (I'm already giving a lot of time to opensource)... but maybe it's worth a pull request.

(that said, I know from experience that having a feature request merged into a repo such as mocha is pretty tough when not coming from an insider... whatever the added value is. I guess that's a sane behaviour to avoid going in all directions when maintaining a popular repository)

[jedwards1211]

Oh I don't mean to sound demanding, it's just a thought. Really the main thing I was concerned about here is whether a change like this would make it even less likely that I can someday use this extension on projects that need different mocha configs for different test categories.