Write the NUnit arguements to a temp file and pass that to NUnit console #2114
Conversation
|
Not sure why this or the other fail .. |
|
I guess this (and the other one) wants pull from release/next as well |
|
Ok I'll sort that all tomorrow. Laptop is off for the night now 👍 |
BlythMeister
force-pushed
the
nunitFileBasedAlways
branch
from
643ed6a to
f5ef276
Compare
|
Still don't get why this is failing...I included the latest... |
| @@ -325,6 +329,9 @@ let run (setParams : NUnit3Params -> NUnit3Params) (assemblies : string seq) = | |||
| FileName = tool | |||
| WorkingDirectory = getWorkingDir parameters | |||
| Arguments = args }) >> Process.withFramework) processTimeout | |||
|
|
|||
| File.Delete(path) | |||
There was a problem hiding this comment.
Agreed, will update
| @@ -316,7 +316,11 @@ let run (setParams : NUnit3Params -> NUnit3Params) (assemblies : string seq) = | |||
| let assemblies = assemblies |> Seq.toArray | |||
| if Array.isEmpty assemblies then failwith "NUnit: cannot run tests (the assembly list is empty)." | |||
| let tool = parameters.ToolPath | |||
| let args = buildArgs parameters assemblies | |||
| let generatedArgs = buildArgs parameters assemblies | |||
| let path = Path.Combine(Path.GetTempPath(), (sprintf "%s.txt" (Guid.NewGuid().ToString()))) | |||
There was a problem hiding this comment.
What about Path.GetTempFileName()?
There was a problem hiding this comment.
I didn't know that was a thing. Will update (and update a lot of other code too 😂)
|
nevermind the failures, mono is stupid as usually. Added a new test and after fixing it on windows it still fails on mono. Should work soon |
|
@matthid i think this is just that build error you told me to ignore :) |
|
@matthid any chance of getting this into 5.8.2? |
|
@BlythMeister Yes let me first try to get stuff green again then I'll look over everything again. Maybe it will be 5.8.0 or some other random patch version :) |
|
Honestly I want to get some other smaller fixes in as well, but I don't want to be held back into feature creep :) So I guess we probably should release something and then try to get the rest in and release again... |
|
Could we add a small integration test, or would that add to much headache (I'd be fine with restricting the test to CIs where .NET or mono is available)? |
|
I'm not really sure how to do this. |
I'd just add a new test to the "Fake.Core.IntegrationTests" project, add a project reference to |
|
@matthid i started looking at this and got lost in the whole getting the nunit console runner in place. |
|
@BlythMeister Yes generally I feel like we need an easy way to mock the |
|
@matthid would you be ok to accept this without that test...or do i need to work out how to get this functioning :( |
|
@BlythMeister I'm ok if you say you tested it manually :) Problem is that new features like this might break people and having at least some rudimentary test-infrastructure in place makes it much easier for me and others to add regression tests if needed. I'm just in the process of trying to improve azure devops integration into PRs (which is red at the moment), which might make it easier to deploy on staging from a PR to test stuff, but its a lot of overhead for stuff which I think could/should be tested more "locally" |
|
I've run this in a mock rig without full FAKE running (linqpad script) to make sure it calls nunit ok. But i agree, there should be a nicer way to test that the arguments being run in a process are correct without actually needing the process...but that feels really hard with the way Fake.Core.Process works,. |
|
The other thing making this tricky is that the file created has a temporary name...and is deleted once finished... |
|
@BlythMeister I think the following could work (will try to get some POC working locally later, maybe even with this PR): Add another (internal) overload which gets some "process starter interface", this overload could be used for unit testing and the public one just calls this internal method with the default process api. |
|
@matthid i've never done that sort of thing in F# before, would be interested to see it in action if you could add to this PR :D |
|
@BlythMeister It would look like #2131 |
|
Ooo nice, does that pr mean this one is redundant? Both can go in together? |
|
@BlythMeister Yes I think github will mark this one as merged when I merge the other one. Can you test if the stuff I did over there still works (I have not added a full integration test)? |
|
@matthid the stuff over there looks good. I think the test you added does show the expected behaviour. |
Description
Another option for #2113 to always write NUnit args to a file and call nunit with the path to the args.