Test adapter improvements #1524
Conversation
* have run_tests return structured result used by TestExecutor * have run_tests accept test data from stdin * run_tests receives test data over stdin * expose StandardOutput and disable async readline * TestExecutor receives test results over process StandardOutput * include testing framework in testInfo object sent to run_tests.js * comments * close readline interface after running test * revert ProcessOutput * use JsonConvert.DeserializeObject<TestResult>() instead * create GetTestResultFromProcess() method * use Process, copy over helper methods from ProcessOutput * close StandardInput after sending test info * include dependency for copied ProcessOutput method * launch node inside of RunTestCases instead * remove some debugging stuff * handle case if result is null * pass in streams as parameters to allow continual reading/writing * move setup logic to RunTests * only remove double quotes from test case info * move Process launch logic into function * mocha uses runner events to determine test pass/fail * get TestExecutor in working state, still launching one process per one test * get TextExecutor code into working state * fixes from PR feedback
* prepare run_tests to run multiple tests * have tape.js run_tests use callback to return result * launch node if test run from 'run selected tests'
…1383) * run_tests accepts a list of tests * mocha.js runs a list of tests and records duration * use full title for tests and initialize time to 0 * run ExportRunner tests with single node instance * close readline interface after tests are done * have ExportRunner tests record test duration * tape framework runs multiple tests with one node instance * enable TestExecutor to run multiple tests with one instance of node * add comment
This will make the branch build on appveyor.
…st-adapter-improvements
|
Hmmmm..... trying to debug the test I'm getting the below.... Looks like there's still some work to do here. |
|
Using fuslogvw I can see it wasn't looking in PublicAssemblies (which is where that DLL lives), but even copying it over, I'm still getting other errors. Can't see anything similar in older issues, so looks like this is a regression we'll need to figure out (I'm not sure its related to this pull request - I'll need to roll back and test even before these changes that everything was working fine on the latest bits). |
|
Slightly more progress... looks like the Node.js process is being launched with I'd also note that with the deprecation of the old debug protocol and |
|
Digging through the code, I can see @ozyx commented out the attach in one of his commits. (See ozyx@97a5d9e#diff-e3f74ce6d8921c9c68f663ff5523a243R260 ) Any reason for this? Is this just work in progress? I'll see if I can get this hooked up again. |
|
FWIW: The error regarding loading So seems like debugging is already broken there, and I'll need to open a separate issue for that - though this pull request still likely breaks debugging tests on VS 2015. |
|
Commenting out the attach was a mistake on my part and replacing it shouldn't affect the functionality of my changes-- not sure why debugging is broken here. I'm currently getting my environment set-up and will be taking a look at this some time this week. |
|
Thanks @ozyx . You can't just uncomment it, as you've replaced the Also FYI, I got the debugger working (before this change) on VS 2017 by adding the below entries to the file at <dependentAssembly>
<assemblyIdentity name="Microsoft.VisualStudio.OLE.Interop" publicKeyToken="b03f5f7f11d50a3a" culture="neutral"/>
<codeBase version="7.1.40304.0" href="..\..\..\PublicAssemblies\Microsoft.VisualStudio.OLE.Interop.dll" />
</dependentAssembly>
<dependentAssembly>
<assemblyIdentity name="EnvDTE90" publicKeyToken="b03f5f7f11d50a3a" culture="neutral"/>
<codeBase version="9.0.0.0" href="..\..\..\PublicAssemblies\envdte90.dll" />
</dependentAssembly>
<dependentAssembly>
<assemblyIdentity name="EnvDTE80" publicKeyToken="b03f5f7f11d50a3a" culture="neutral"/>
<codeBase version="8.0.0.0" href="..\..\..\PublicAssemblies\envdte80.dll" />
</dependentAssembly>
<dependentAssembly>
<assemblyIdentity name="EnvDTE" publicKeyToken="b03f5f7f11d50a3a" culture="neutral"/>
<codeBase version="8.0.0.0" href="..\..\..\PublicAssemblies\envdte.dll" />
</dependentAssembly>The problem is that these assemblies are no longer in the GAC in VS 2017 (see https://docs.microsoft.com/en-us/visualstudio/extensibility/breaking-changes-2017) |
|
Now I've got test debugging working again for VS 2017 in the original code (see #1527), I'm digging into this code a bit more. I think this is going to take a bit of refactoring and cleaning up still. I'm seeing the same code blocks repeated 3 times (the 2 RunTests overloads now effectively contain copies of the code the was, and still is, in RunTestCases - and as the RunTests methods call in to RunTestCases, its repeating operations it doesn't need to). I see a Node.js process is still being launched for each test file. Is that necessary? It would be a perf win if not. Also, the launching logic needs to change back (or other changes made) so that debugging can attach again with this model. I'll try and step through this a little more tomorrow and see what I come up with. |
|
Prior to these changes, the node process would be launched per test instead of per test file. Mocha tests were run by calling There's some dialogue between me and @mjbvz on issue #518 that might be helpful. |
|
I spent quite a bit of time on this today. I've cleaned up the code to remove some repeated blocks, and to move operations that only needed to be done once outside of loops. On my laptop, with 60 unit tests spread across 2 files I see: So running all of them in under a second... not bad! I also fixed the test debugging on VS 2017 with a slighter cleaner approach than I checked in to 'master' recently. This should also improve perf and make the code simpler. |
| if (!DTELoaded) | ||
| { | ||
| string currentProc = Process.GetCurrentProcess().MainModule.FileName; | ||
| if (Path.GetFileName(currentProc).ToLowerInvariant().Equals("vstest.executionengine.x86.exe")) |
There was a problem hiding this comment.
prefer StringComparer.OrdinalIgnoreCase.Equals since this deals with nulls cleanly. (i.e. doesn't throw if either argument is null)
| // .ts file path -> project settings | ||
| var fileToTests = new Dictionary<string, List<TestCase>>(); | ||
| var sourceToSettings = new Dictionary<string, NodejsProjectSettings>(); | ||
| NodejsProjectSettings settings = null; |
|
|
||
| if (!File.Exists(settings.NodeExePath)) | ||
| { | ||
| frameworkHandle.SendMessage(TestMessageLevel.Error, "Interpreter path does not exist: " + settings.NodeExePath); |
There was a problem hiding this comment.
We should probably grab this message from the resource bundle
There was a problem hiding this comment.
Agreed... but this assembly isn't localized at all currently, so that's a bigger effort.
| string.Join(Environment.NewLine, _nodeProcess.StandardErrorLines), | ||
| (!runCancelled && _nodeProcess.ExitCode == 0) ? TestOutcome.Passed : TestOutcome.Failed); | ||
| _nodeProcess.Dispose(); | ||
| private static int GetFreePort() { |
There was a problem hiding this comment.
Can you add a note where you copied this from ( I've seen something similar in the debugger code).. I think we want to refactor in the future to clean that up.
There was a problem hiding this comment.
I didn't copy this from anywhere - I just moved it.
There was a problem hiding this comment.
We should probably still refactor though
|
Was hoping to get this in today... but on testing the various frameworks, looks like the changes totally broke Tape. I think I can see why, as @ozyx , did you try these changes after making them? I can only assume it can't of been working for you either - unless I'm doing something wrong on my end. I'll be on vacation for a couple days, so will need to come back to this later. |
|
That last commit (5cfb3e2) seems to mostly have Tape working also now. Should be nearly good to go! |
|
Ugggh. Spent forever tracking down those last couple of issues. One was in Tape itself (see referenced issue above), and the other was as a VS Preview update reset some of my files without me knowing it - so I was back running on old code I was sure I'd already fixed 😩 Good times... These last changes (and the Tape fix) seem to have things running well. Maybe just some final clean up after another set of eyes to sign-off before checking in. Thanks. |
|
LGTM |
|
Yaaaaaaaaaaaaaaaaaaaaaaaaaay! Thanks @ozyx for all the good work to get this improvement in. |
|
Awesome, thanks! Glad to be able to contribute. |
The test-adapter-improvements branch was fairly out of date, as master has had a lot of work recently for VS 2017 build, string localization, Node.js debugger updates, etc... This is effectively that branch with master merged in, then the merge conflicts fixed, then I fetch the work in pull request #1430 from @ozyx, merged that and fixed the merge conflicts also.
This builds clean, but I haven't done much testing on it yet (or even code reviewed the existing changes in depth). Consider this the new baseline branch to get that work reviewed, tested, completed, and checked in.