Test runner performance could be improved (Export Runner)

[NoelAbrahams]

Hi,

Version: VS 2015/ NTVS

We use ExportRunner to run TypeScript tests. The time taken to run a single test normally ranges from 250 ms to 1 second.

The test itself should run under 5 ms, because they are all unit tests and there is no reason for them to take much longer than that.

It appears that the test launch/preparation code is adding about 200 ms per test.

This is a problem, because for just 100 tests we're looking at 20 seconds plus.

Can this be fixed?

[mousetraps]

This is probably because we run each test in separate processes and have to spin up a new node process every time. Duplicate of #223

[NoelAbrahams]

@mousetraps, issue #223 appears to have disappeared off the face of the earth.

Should this issue be reopened? Are there any plans to fix the test runner performance problems or should we look elsewhere for a solution?

As one can see from the image below, each test is taking one second or more to run, while it should in reality complete in 10MS or less.

[ozyx]

Hi all,
I'm currently investigating a fix for this issue and for #1157.
I do have some questions before I dive in, though. Is it known whether or not fixing this will require significant redesign of the TestAdapter as it stands? I see that each test is run by a separate node process and the problems are apparently stemming from this.

Would an approach to fixing this be as simple as running the tests with the same instance of node or does it run deeper than that? (I'm assuming the latter...)

Any pointers as to where to start/ general issues to take into account would be greatly appreciated. At this point I'm trying to plan an approach and estimate the amount of development time required for a fix.

Thanks

[mjbvz]

Very exciting to have someone interested in looking into this!

You are correct that the core problem is that a new Node process is launched for each test case (see TestExecutor::RunTestCase). The fix is to run all tests of a single batch (such as 'run all' or 'run failed') using a single node instance. Besides improving performance, this would fix test class setup and teardown methods being run for each test.

---

Let me start by explaining some of the high level concepts of the NTVS test pipeline:

1. The main Visual Studio test explorer integration comes from the NTVS test adapter. This project defines a test discoverer for finding tests and a test executor for running them. For your change, you are mainly interested in the executor component.
2. The test adapter supports multiple Javascript test frameworks (export, mocha, and tape). To support multiple frameworks, we implement a simple interface for each one. Here's the export runner javascript code for example. The function you are interested in is called run_tests. This takes testName, testFile, workingFolder, projectFolder and, despite its name, only run a single test.
3. The TestAdapter code invokes the individual test framework implementations through another Javascript file called run_tests.js. This is the file that is actually being launched with node from the tests adapter. It just finds and invokes run_tests for the target framework (which is provided as a command line option).

The entire flow of running a test therefore looks like this:

1. In VS Test Explorer, user selects tests to run.
2. The VS Test Explorer invokes the NTVS Test adapter test executor.
3. The test executor does some setup, including determining which Javascript framework to use.
4. For each test, the test executor then launches a new node instance from the script run_tests.js, passing all info to run the test as command line arguments
5. run_tests.js loads the correct framework code (such as export_runner.js) and invokes run_tests on the framework for the requested test.
6. Test failures are reported through node process exit code, with error messages coming from stderr.

---

There are many ways to fix the problem, and I'd be happy to discuss or review any of your ideas. My initial thought to minimize code change is the following:

1. Update run_tests.js to act more like a function and return a structured result instead of relying using exit codes and stdout and stderr. This will make the next steps easier. Update the TestAdapter to use this results.

   You can even use process in the first pass to make this easier. Just move the processing the test adapter does on the node process result in RunTestCase, into the Javascript. The result object could look something like this:

   {
     "test_name": "MyTestClass:this_should_pass",
     "outcome": "passed",
     "stdout": "optional message from test",
     "stderr": "optional error message from test"
   }
   

2. Update the run_tests.js script to run a list of tests instead of a single test.

   A first pass may literally have the script take a json array of tests and return a json result, but a better solution would be to have the run_tests.js script open a communication channel that the C# test adapter code can send tests info to, and get test results back on.

3. Update the test adapter to talk with the updated run_tests.js in RunTestCase. Then update it to run multiple tests with a single node instance using the new run_tests.js logic.

This alone would fix many of the perf issues, but would not fix mocha setup and teardown being run multiple times for each test. To fix that, I believe we would have to update the test framework run_test function interface to take a list of tests. This could be handled in another pass.

Other things to consider after the initial prototype:

• Test timeouts.
• Exceptions.
• Test setup and teardown.
• Projects that use multiple test frameworks.

---

Let me know if that gives you an idea of the problem and some initial direction. Again, none of the ideas I've thrown out are set in stone, and I'd be happy to discuss any ideas you have further. And please let me know if you have any other questions or get blocked by anything.

[ozyx]

Thanks so much for your helpful and detailed response.

I'm currently in the process of planning a fix for this. Just to give some context, I'm on a development team and we have integration testing set up using the NTVS plug-in. The tests require a server to be launched/killed during the before() and after() hooks of the tests, so getting these running in the proper order will reduce the time spent running our tests by one-half.

Currently I'm trying to estimate the amount of development time needed to fix this and weigh it against our other options. From reading your response and looking at the code it seems to me this fix should take a reasonable amount of time (~ 20 days), so I should be able to get started in the coming weeks. Please let me know if you disagree.

I do have one question, though. When I start working, which branch should I work from? We are using NTVS 1.1 but this seems like an issue that would be best resolved for all versions of NTVS. If I work from master, how long could it take to merge the fixes into 1.1?

Thanks again!

[mjbvz]

I think that sounds reasonable.

For the development, feel free to work however you feel would be best, but I would encourage you to submit incremental we can have more discussion during the development and hopefully catch potential issues early on. You could either open a WIP PR against NTVS and update that as you work, or you could create a separate branch on your fork that you submit changes against before submitting the entire branch against NTVS.

It sounds like you are proposing that we take this change into 1.1, 1.2, and the next release of NTVS. The test adapter code is fairly static, so you should be able to work from master, or the v1.1.x branch if needed. master would be preferable, because we can merge the work in more quickly and start getting it out in dev builds. The bigger challenge is that since 1.2 and 1.1 have already been released and stabilized, we need additional testing to ensure this change is stable and will not regress functionality. Do you absolutely need this in NTVS 1.1, or could your team migrate to a newer release?

[ozyx]

I'll have to check with the team, but I'm almost certain we should be able to migrate to a newer release. If it means it will be easier to merge / get builds out then it's in both of our best interests as we are looking for a fix ASAP.

I'll get back to you if the story changes.

[ozyx]

Update run_tests.js to act more like a function and return a structured result instead of relying using exit codes and stdout and stderr. This will make the next steps easier. Update the TestAdapter to use this results.

Could you elaborate on this a bit? Would this redesign mean abandoning the use of the ProcessOutput object's attributes as a means of determining a test's success or failure? I guess I'm just confused as to the details of the "communication" between run_tests.js and TestAdapter. Is there a way to return the JSON result from run_tests.js to TestAdapter aside from simply writing / reading a json file? Should I look into something like edge.js to solve this? Sorry if this is a dumb question.

[mjbvz]

Once we start running multiple tests in a single node session, we can no longer use the exit code of the node process itself to determine whether an individual test succeed or failed. Using stdout and stderr also becomes more complicated, since we have to map any text back to a specific unit test somehow.

That's why I suggest making the tests return a structured "Result" object instead. The result object would just map the old process output to a json object with the following fields:

• Which test the result is for
• Did the test succeed or fail (old exit code)
• Log output (old stdout)
• Error output (old stderror)

The results could be returned over stdout or a similar IO channel, so we could keep using ProcessOutput to collect the json string and then parse it in our test adapter code.

For the first step, you would only update run_tests to return a structured result. The new flow for handling test output would look something like this (based on the existing executor code):

// Launch node
_nodeProcess = ProcessOutput.Run(...);

...

 WaitHandle.WaitAll(new WaitHandle[] { _nodeProcess.WaitHandle });

bool runCancelled = _cancelRequested.WaitOne(0);

// Parse result from stdout
// Should handle case where runCancelled too
var resultObject = ParseTestResult(_nodeProcess.StandardOutputLines);

// Use result object instead of process outputs directly
RecordEnd(frameworkHandle, test, testResult,
               resultObject.stdOut,
               resultObject.stdError,
               !runCancelled && resultObject.passed ? TestOutcome.Passed : TestOutcome.Failed);
...

Then you could later change run_tests to return multiple test results, perhaps first in a json array of results, but we could later update the results to be returned incrementally. edge.js seems like it may be overkill for this

Let me know if that clears things up. Again, there are other approaches here that would work great as well and I'd be happy to discuss any ideas more.

[ozyx]

Oh! I suppose I was overthinking everything. This clears it up quite a bit-- thank you!

[kant2002]

@mjbvz What if we create custom test runner for the Mocha which will produce output in the machine readable way so we could parse stdout/stderr reliable?

[ozyx]

Hi again,
I've created a PR on my fork with some initial changes relating to returning/using structured results for tests. I've also begun looking into how run_tests.js can run a list of tests as noted here:

Update the run_tests.js script to run a list of tests instead of a single test.
A first pass may literally have the script take a json array of tests and return a json result, but a better solution would be to have the run_tests.js script open a communication channel that the C# test adapter code can send tests info to, and get test results back on.

If taking the JSON Array route for this problem, run_tests.js could just take the JSON array as a command-line argument? or is there a better/cleaner way to accomplish this?

I am particularly interested in the 'communication channel' solution, as it seems it would be much easier in the long run but, full disclosure, I'm not sure I would even know where to start with something like that. If it's not too much trouble, could you elaborate a bit?

Please let me know if I'm on the right track with the PR or if there is anything I can improve.

Thanks again

[mjbvz]

@ozyx If you use the array approach, passing the json using the command line may work, but larger projects may start hitting command line length limits. In this case, it would be better to read the json array from stdin by doing something like this on the Node side:

process.stdin.resume();
process.stdin.setEncoding('utf8');

var data = '';

process.stdin.on('data', function(chunk) { data += chunk; });

process.stdin.on('end', function() {
    var data = JSON.parse(data);
    // process test result array
});

You could then return all the results over stdout in another json array, which would be parsed on the C# side.

---

Using a communications channel, instead of sending all the data to the test adapter in a single array and returning it back in another giant array, the Javascript code would create something like a server that the C# could send requests to and get results back from. The main benefit is that you are able to receive test results back as they are completed, instead of blocking until all tests are run.

How this is accomplished is pretty open ended, but here are some thoughts:

• Easiest solution: Pass test data over stdin. Pass a single json object on each line, and parse the line data in runtest.js (readline may help). Then in runtest.js, send JSON test result objects over stdout. Send one object per line.

  On the C# side, you could use a method like: ReadLine https://msdn.microsoft.com/en-us/library/system.io.streamreader.readline(v=vs.110).aspx on the StandardOutput of the process. You may have to chaange to use Process directly instead of our wrapper, or at least expose the StandardOutput property on process

• As an alternative, in runtests.js, open a normal http server on a local port: https://nodejs.org/api/http.html#http_class_http_server Send json test data to the server from C# and wait for responses. Using http is unnecessary, but c# does have some nice helper methods for sending and receiving http: http://stackoverflow.com/a/4988809/306149

• Same general idea as above, but use tcp sockets directly instead of http. I believe C# should also have higher level helpers for working with these, but I'm not as familiar with them. On the node side, this npm library may be helpful: http://www.sebastianseilund.com/json-socket-sending-json-over-tcp-in-node.js-using-sockets

Let me know if that provides some good starting points and if you have any other thoughts or ideas here.

Thanks.

[ozyx]

Thanks again for your guidance.
I've created another PR with the second batch of changes for when you have time to review.

[fforjan]

@mjbvz any status on the merge on the remaining pull request ?

My understanding is that the merge windows for the next version is closed ? Am I correct ?

[mjbvz]

@fforjan I no longer work on NTVS. Please try following up with @billti to see if this can be merged in

Thanks

[fforjan]

thanks for the info !!!! @billti any info ?

[billti]

Thanks for your help to improve the Test Explorer perf @fforjan . At this point, we've heads down trying to meet other requirements to include NTVS components in the next Visual Studio release (e.g. localization, setup changes, etc.), so unfortunately this isn't something we're going to have time to review and merge before that ships. It is definitely high on our priority list to cover once we get that behind us though.

Sorry for the delay, and thanks again.

[ozyx]

@billti Thanks for the info. I'll be available to answer any questions / make any necessary changes when that time comes-- just ping me 👍

@mjbvz Thanks for your help throughout!

[fforjan]

@billti We've allocated some time to one member of our team last year to participate in this fix and we only want to claim victory when the code will be merge. Any timeline for accepting this merge ?

[billti]

We've had someone looking at what it would mean to make the Test Adapter more generally useful across VS (i.e. not just Node.js projects). I'm meeting with the Dev who's been prototyping some stuff early next week, so I'll put this on the agenda and circle back after that.

[paulvanbrenk]

Closing as this has been merged and shipped.