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

Fix Console runner to support space in temp path #2180

Closed
wants to merge 1 commit into from
Closed

Fix Console runner to support space in temp path #2180

wants to merge 1 commit into from

Conversation

pchinery
Copy link
Contributor

Description

If the user profile and therefore the temp folder contained spaces, they would be passed to the NUnit runner as two separate arguments, so running unit tests would not work

If the user profile and therefore the temp folder contained spaces, they would be passed to the NUnit runner as two separate arguments, so running unit tests would not work
@@ -317,7 +317,7 @@ let internal createProcess (setParams : NUnit3Params -> NUnit3Params) (assemblie
//let processTimeout = TimeSpan.MaxValue // Don't set a process timeout. The timeout is per test.

let path = Path.GetTempFileName()
let args = (sprintf "@%s" path)
let args = (sprintf "\"@%s\"" path)
Copy link
Member

Choose a reason for hiding this comment

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

Hm, can you instead use the Args class, this way it should work with other special cases of windows escaping horror as well. Also we might want to add a simple test to check that it is properly escaped.

@matthid matthid added bug needs-tests This PR needs tests in order to be accepted labels Nov 1, 2018
@matthid matthid closed this in d6d596f Nov 10, 2018
@matthid
Copy link
Member

matthid commented Nov 10, 2018

Thanks for reporting and sending a PR. I added a test and a slightly different fix in order to handle some other edge cases as well (as indicated by my comment above), see d6d596f

@pchinery pchinery deleted the nunit-temp-space-patch branch November 12, 2018 07:46
@pchinery
Copy link
Contributor Author

Thanks for doing the changes, the last days have been so creazy that it did not find the time to make the changes you had proposed myself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs-tests This PR needs tests in order to be accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants