modifications to mocha.js and some TestExecutor logic #1334
Conversation
|
Hi @ozyx, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! The agreement was validated by Microsoft and real humans are currently evaluating your PR. TTYL, MSBOT; |
There was a problem hiding this comment.
I think the approach looks good overall.
To help support the other test framework, you may want to have run_tests for each framework return a result object, and let run_test.js handle actual communication with NTVS. Then just update the other test implementations to return a result object instead of logging their results. Tape should provide an API that is somewhat similar to Mocha, while the export runner is simple enough that it shouldn't be too challenging (it just invokes a test function and catches any error). Did you have any specific concerns with the current approach?
Other than that, just a few minor comments and suggestions. I can merge this in to the working branch as is or you can submit another update to the change before the merge. Let me know either way
| @@ -244,6 +354,52 @@ select connection.LocalEndPoint.Port | |||
| return projSettings; | |||
| } | |||
|
|
|||
| private static string GetArguments(IEnumerable<string> arguments, bool quoteArgs) { | |||
There was a problem hiding this comment.
For these, instead of duplicating the code, just mark them as public static in ProcessOutput. They really should be extracted to their own helper class, but just making them accessible is fine for now.
| function hook_stdout(callback) { | ||
| var old_write = process.stdout.write; | ||
|
|
||
| process.stdout.write = (function (write) { |
There was a problem hiding this comment.
Just use process.stdout.write = callback here
| @@ -100,7 +162,8 @@ class TestExecutor : ITestExecutor { | |||
|
|
|||
| try { | |||
| RunTestCase(app, frameworkHandle, runContext, test, sourceToSettings); | |||
| } catch (Exception ex) { | |||
| } | |||
There was a problem hiding this comment.
[nit] Keep these on the same line for formatting consistency. Same in other locations.
| frameworkHandle.SendMessage(TestMessageLevel.Error, "Error occurred connecting to debuggee."); | ||
| KillNodeProcess(); | ||
| } | ||
| } catch (COMException) { |
There was a problem hiding this comment.
[nit] Looks like some weird indentation was added here
|
Made the appropriate fixes as per your feedback and this PR should be good to merge now. |
Hi again.
This is kind of a big, messy PR but I'd like to get your feedback on the changes before I continue further. I've moved the logic for determining test pass/failure and sending results to
mocha.js. It is much easier to determine if we are using mocha API'srunnerevents. This works really nicely if we are running mocha tests, but my main concern is how to makerun_testswork among all frameworks with this new logic. I'd like your feedback on these changes and if they are viable.I've also changed some logic in
TestExecutor. The code is currently in a working state, so only running one test per node process, but I've moved much of the setup logic toRunTests, and eventually launching/disposing the process logic will be moved there, as well. I've added some comments with my thought process-- if you need more info I can elaborate.My biggest concern would be making
run_testswork among all testing frameworks, any info on how I can make this work would be greatly appreciated. Using mocha'srunnerevents is really great because I would be able to determine pass/fail easily, record start and end times and include much more data in the result than was available before. IftapeandExportRunnerdon't have a similar feature (I am unfamiliar with these frameworks) then I'm not sure how viable these changes will be.Either way, when you have time to review I'd love to hear your opinion.
Thanks!