Fix #216 and add additional mocha configuration options

[lukedgr]

#216

This change adds the ability to configure mocha by adding a config file to the project located at $(ProjectDirectory)\test\mocha.json. The relative path under the test directory was chosen because this is where mocha looks for a configuration by default when run from the command line (looks for mocha.opts). Adding the mocha.json file will result in the mocha tests running with the options specified.

An example of a valid mocha.json file is:

{
"ui": "tdd",
"timeout": 300000,
"reporter": "xunit"
}

Also, note that if the config file does not specify a ui, timeout or reporter the default will be chosen automatically.

[msftclas]

Hi @lukedgr, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

[mousetraps]

This should be console.error("NTVS_ERROR:", blah), otherwise the test reporter won't pick it up and display it i nthe output window (it's a bit confusing because you see it printed out in debug mode.)

[mousetraps]

I saw your comment fly by in email, but I can't see it on github for some reason. Anyways to elaborate on this... as the code is written now, the user will not see it. "NTVS_ERROR" is a string that we search for and strip out of the final output when we display it in the output window.

IEnumerable<String> stdoutLines = stdout.Split(new string[] {Environment.NewLine},
    StringSplitOptions.RemoveEmptyEntries).Where(s => s.StartsWith("NTVS_ERROR:")).Select(s => s.Trim().Remove(0,11));

https://github.com/Microsoft/nodejstools/blob/35817ffdaa1797f95628a22f588fc3ed885f884b/Nodejs/Product/TestAdapter/TestFrameworks/TestFramework.cs#L53-L54

You'll only see the full output printed out when the product is built with the "Debug" configuration, but not the "Release" config. So using logError is the way to go here.

[mousetraps]

Updated the comment/title to point to the right issue (just created a feature for mocha config options). Separate issues helps us track and verify everything is working before a release. I also just submitted a fix to the defaults on #217, which would be good to merge with these changes as well. So we can also consider adding another timeout for debug mode.

[mousetraps]

console.error instead of console.log. Also see recent changes (very minor), so adding another argument here would be good too.

[mousetraps]

Hey, thanks so much for the contribution! 😃 I left some more specific notes on the code, but some more general comments below.

I've actually been toying around with this change in my mind, and it would be great to hear your thoughts.

I'm not entirely sure about the configuration discovery aspect (projectFolder\tests), but I also think it's a big step in the right direction 😃. So I'm in favor of taking this change, and then adapting it over time as we get feedback on it and understand people's preferences. Ultimately if people like this approach, and they start requesting it for more features, we might want to start including many more settings in a single file (sort of like VSCode's settings.json). But this is a good litmus test.

The other interesting note is that... when we get the chance, we'll want to consider moving to the Common Project System (rather than maintaining VisualStudio-SharedProject, as we do now), and CPS is a more folder/file based system, so we'll probably have to revisit this issue then.- and not just go for mocha tests, but really settings in general.

On the technicalities end...

• legal stuff - your profile says MSFT org, but for some reason it's marked as CLA-required. Not sure what's up there... did you join the github msft org after you submitted the PR?
• as an FYI, there will be some merge conflicts with recent changes / PRs (we were both working on test stuff at the same time and I didn't get a chance to look through this PR until now). They're pretty minor, and I'll resolve them at merge-time, but worth taking a look at.

[mousetraps]

with logError, there is no need to prepend NTVS_ERROR here anymore.

[mousetraps]

Thx for the new iteration. Added some super minor comments, and lgtm after that

[lukedgr]

I am happy to help improve NTVS!

... On the mocha options json path, I agree that it is not ideal to have the relative path hard-coded, and I think it would be great if each .njsproj had it's own settings file. The reason I decided to use projectFolder\test\mocha.json is that when running from the command line mocha will look for options at .\test\mocha.opts (http://mochajs.org/#mocha.opts).

[mousetraps]

Ahh, that makes much more sense now, and thanks for the link - I wasn't familiar with mocha.opts

So eventually... what if instead of specifying a separate json file, we could just pick up the options from the mocha.opts? That way it "just works" with an existing setup.

[lukedgr]

I see your point of it just working with the existing set up, however the downside to that is you are unable to specify different options for the CL and VS.

For example, my project uses a different reporter on the CL than when running inside VS.

[mousetraps]

Gotcha, good call

[mousetraps]

👍 with the new changes btw. I'll merge it in when I get the chance.

[landonpoch]

Any plans to support more options (like the --require flag)? Also, it might be good to document which flags don't currently work. I'm tempted to hack around in the mocha.js file to get things working but I don't want to break my upgrade path.

[mjbvz]

@landonpoch I don't think we have any plans to do this at the moment, but feel free to look into adding additional options and submit a PR (see #940.)

However, I don't think we want to encourage the use of the --require flag. In my understanding, it messes up intellisense since an analyzer cannot know about modules listed in --require.