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.