-
Notifications
You must be signed in to change notification settings - Fork 33
Added support for multiple test frameworks #31
Conversation
File.WriteAllText(glueFile.FullName, content); | ||
} | ||
} | ||
|
||
private static string FixMsTest(string content) | ||
{ | ||
content = Regex.Replace(content, ".*Microsoft.VisualStudio.TestTools.UnitTesting.Description.*", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't seem to be any support for this or a similar attribute in latest for netcore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's the cause of the ugly mstest names, but i dont know how to fix it or if a fix exist as I found no similar attribute.
content = content.Replace(" : Xunit.IUseFixture<", " : Xunit.IClassFixture<"); | ||
content = content.Replace("[Xunit.Extensions", "[Xunit"); | ||
content = FixXUnit(content); | ||
content = FixMsTest(content); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Micro-optimisation: could hold on to the provider and only fix tests for that provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the mstest one is a regex so might be appropriate. I'll let you decide on argument/guess first and I'll fix this once we have a decision there.
Is this PR waiting on anything else or can it be merged? |
Unfortunately there are some conflicts now. Is this worth trying to merge? |
Hey Stajs, I'll try to fix this during the weekend. I'd like to put in a commandline argument as well before we merge it if you don't mind. |
Sounds good, should be relatively easy to support arguments now! |
WriteLine(Content); | ||
WriteLine("Saving: " + config.Path); | ||
File.WriteAllText(config.Path, Content); | ||
|
||
return config; | ||
} | ||
|
||
public void Validate() | ||
public void ChangeTestProviderIfNeeded(string testRunner) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty wordy and brittle method (if it doesn't find the exact app config stuff it does nothing silently). It also means it is no longer possible to change the app.config testRunner as we will overwrite it. Probably not a big deal as test runner is now specified using the arguments instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think I prefer considering this as part of the validation rather than modifying the app.config
which will probably go unnoticed. Will let it settle to see how it feels...
Also added a feature to always replace test runner part of app.config if app.config exists. Also fixed a small bug if there are spaces in the project path
private void RunSpecFlow(string csproj) | ||
{ | ||
// Credit: http://www.marcusoft.net/2010/12/specflowexe-and-mstest.html | ||
var arguments = $"generateall {csproj} /force /verbose"; | ||
var arguments = $@"generateall ""{csproj}"" /force /verbose"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes a bug where if you try to generate with a space in path it breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, thanks!
@@ -1,5 +1,5 @@ | |||
{ | |||
"version": "1.0.0-rc5", | |||
"version": "1.0.0-rc4", // TODO: update version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this to you, @stajs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
If you feel it's ready, you can merge this now :) |
} | ||
|
||
public static AppConfig CreateIn(DirectoryInfo directory) | ||
public static AppConfig CreateIn(DirectoryInfo directory, out string usedTestRunner, string testRunner = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get away without an out
parameter, and just use the TestFramework
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that too, nice solution
@@ -56,6 +62,7 @@ public Args(string[] args) | |||
|
|||
WriteLine("SpecFlowPath: " + SpecFlowPath); | |||
WriteLine("WorkingDirectory: " + WorkingDirectory.FullName); | |||
WriteLine("Test runner: " + WorkingDirectory.FullName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this.
@erik-lundgren Thanks for work on this! |
With this we can support MSTest and NUnit by just looking at the
testRunner
entry in project.json since the moniker there will be identical to the one used in app.config.I haven't tested this extensively and MSTest produces fairly ugly test names (see code comment) but the sample builds and tests fine on both those test frameworks.
Also editorconfig seems to be doing its job. Yay pretty diffs!