-
Notifications
You must be signed in to change notification settings - Fork 585
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
Port VSTest to FAKE 5 #2008
Port VSTest to FAKE 5 #2008
Conversation
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.
Thanks for looking into that. Some fake 5 Api guidelines are missing and we probably want to fix/change the warnings in https://github.com/fsharp/FAKE/blob/release/next/src/legacy/FakeLib/UnitTest/VSTest.fs
@@ -0,0 +1,150 @@ | |||
/// Contains tasks to run [VSTest](https://msdn.microsoft.com/en-us/library/ms182486.aspx) unit tests. | |||
module Fake.DotNet.Testing.VSTest |
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.
please add [<RequireQualifiedAccess>]
open System.Text | ||
|
||
/// [omit] | ||
let vsTestPaths = |
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.
internal?
type ErrorLevel = TestRunnerErrorLevel | ||
|
||
/// Parameter type to configure [VSTest.Console.exe](https://msdn.microsoft.com/en-us/library/jj155800.aspx) | ||
[<CLIMutable>] |
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.
remove CliMutable
TestAdapterPath: string} | ||
|
||
/// VSTest default parameters. | ||
let VSTestDefaults = |
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.
internal
|
||
/// Builds the command line arguments from the given parameter record and the given assemblies. | ||
/// [omit] | ||
let buildVSTestArgs (parameters : VSTestParams) assembly = |
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.
internal?
/// !! (testDir + @"\*.Tests.dll") | ||
/// |> VSTest.VSTest (fun p -> { p with SettingsPath = "Local.RunSettings" }) | ||
/// ) | ||
let VSTest (setParams : VSTestParams -> VSTestParams) (assemblies : string seq) = |
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.
Maybe rename that such that it looks like VSTest.run
for example (look how other runners call it).
I made the suggested changes and I propose adding refactoring the remaining test runners into the backlog, because none of the following are currently in line with these changes:
|
Still missing is the updates for the warnings in https://github.com/fsharp/FAKE/blob/release/next/src/legacy/FakeLib/UnitTest/VSTest.fs to point to the new module. And ideally an update to the documentation template to add it to me menu (https://github.com/fsharp/FAKE/blob/release/next/help/templates/template.cshtml). Thanks!
That is one way, and yes it should be generated once the new module is added to the |
I've added the |
I don't quite know what isn't working with the CI... on my machine the build.fsx was green. Any idea? |
Maybe our fake-templates broke, lets see if restarting works. |
Thanks looks good! We figured out the test-error. |
I copied the MSTest module and made the appropriate changes for VSTest. Please take a first look at the code.
So far, I haven't been able to test it. To do so, I understand, I need to build the nuget locally. Do I have to run the
build.fsx
with some particular target or something?