Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSTest: add Tests parameter to specify list of tests #1615

Merged
merged 5 commits into from Jul 28, 2017
Merged

MSTest: add Tests parameter to specify list of tests #1615

merged 5 commits into from Jul 28, 2017

Conversation

alex-bogomaz
Copy link
Contributor

Hello, please review this PR.
This is a fix for #1601 (MSTest /test support).
Sample usage:

Target "Test" (fun _ ->
    !! (@"UnitTestProject1.dll") 
      |> MSTest (fun p -> { p with Tests = ["UnitTestProject1.TestClass1.TestMethod1"; "TestMethodXX"]})
)

@matthid
Copy link
Member

matthid commented Jul 26, 2017

Sorry for the delay. The change looks good, thanks!

Can we migrate the MSTest module to FAKE 5 as suggested in https://fake.build/contributing.html.
There is a guide at the end (https://fake.build/contributing.html#Porting-a-module-to-FAKE-5).

Let me know if you need help.

@matthid matthid added the waiting for author Some information or action was requested and it needs to be addressed. Or a response from author label Jul 26, 2017
@alex-bogomaz
Copy link
Contributor Author

no problem. gimme couple of days

@matthid matthid removed the waiting for author Some information or action was requested and it needs to be addressed. Or a response from author label Jul 27, 2017
Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I can merge and release this once the build is green. Let me know if there are further bootstrapping problems.

build.fsx Outdated
@@ -34,6 +34,7 @@ open Fake.DotNet.Cli
open Fake.Testing.Common
open Fake.DotNet.Testing.MSpec
open Fake.DotNet.Testing.XUnit2
open Fake.DotNet.Testing.MSTest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a small bootstrapping problem with this ;). I think we can remove those lines for now.

@@ -306,6 +306,9 @@
<Compile Include="UnitTest\MSTest.fs" />
<Compile Include="UnitTest\ProcessTestRunner.fs" />
<Compile Include="UnitTest\VSTest.fs" />
<Compile Include="..\Fake.DotNet.Testing.MSTest\MSTest.fs">
<Link>Fake.DotNet.Testing.MSTest/MSTest.fs</Link>
</Compile>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move that up to the other "Modules"? This prevents accidental usage of "old" members (and makes the IDE help you)

@matthid
Copy link
Member

matthid commented Jul 28, 2017

OK Travis is still failing:

MSTest runner argument construction, When using Category parameter
» should include the expected category argument (FAIL)
System.TypeInitializationException: The type initializer for '<StartupCode$FakeLib>.$Fake.MSTest' threw an exception. ---> System.Exception: MSTest is not supported on mono platform
  at <StartupCode$FakeLib>.$Fake.MSTest..cctor () [0x00043] in <597af65a7c20dbeda74503835af67a59>:0 
   --- End of inner exception stack trace ---
  at <0x00000 + 0x00000> <unknown method>
  at Test.FAKECore.Testing.MSTestSpecs.BuildArgumentsSpecsBase+<>c.<.ctor>b__5_0 () [0x0000a] in <52dbdee1df254ed68681ec573823124d>:0 
  at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00032] in <a07d6bf484a54da2861691df910339b1>:0 

I think we need to disable the tests on mono.
Now thinking about it, we might need to change the check a bit, because isMono will be false when we switch to dotnetcore as well. So we might need to change that to isUnix.

@alex-bogomaz
Copy link
Contributor Author

  1. can we instead check if current platform is Windows ?
let mstestexe =
   if Environment.isWindows then "mstest.exe" 
   else failwith "MSTest is only supported on Windows platform"
  1. what is your preferred way to exclude tests for Mono - use ExcludeTags ?

@matthid
Copy link
Member

matthid commented Jul 28, 2017

  1. can we instead check if current platform is Windows ?

absolutely :)

  1. what is your preferred way to exclude tests for Mono - use ExcludeTags ?

Personally I'm ok with everything that works ... (Don't know MSpec good enough tbh.)

@matthid matthid merged commit 3f8a075 into fsprojects:master Jul 28, 2017
@matthid
Copy link
Member

matthid commented Jul 28, 2017

@walliski
Copy link
Contributor

walliski commented Aug 1, 2017

Hey! Thanks for fixing this, really looking forward to testing it (pun intended -_-')!

I noted when creating the mstest argument string manually that it is fairly simple to reach the maximum allowed length of windows command line, somewhere around 8000 characters. Our tests can have names closer to 200 chars long. :<

Does FAKE handle this, or is it something that should be included here?

@matthid
Copy link
Member

matthid commented Aug 1, 2017

@walliski How could FAKE handle this? To be honest I'm not even sure what would happen if you reach the limit. Maybe windows will strip the commandline or will yield an error?

@walliski
Copy link
Contributor

walliski commented Aug 1, 2017

FAKE could either warn/throw error about it, or then split the execution into multiple parts with a set of tests in each. Alternatively I've understood, though not tested, that you should be able to split the command into multiple lines and this might mitigate the problem. Although this seems to be a bit of a fuzzy area of Windows.

Windows throws an error and wont run the command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants