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.

Sign up for GitHub

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

Jump to bottom

Allow users of nunit3 set environment variables in the runner #2543

Merged
merged 3 commits into from
Sep 9, 2020
Merged

Allow users of nunit3 set environment variables in the runner #2543

merged 3 commits into from
Sep 9, 2020

Conversation

robertpi
Copy link
Contributor

I need this for an obscure reason, maybe other people will too?

@robertpi
Allow users of nunit3 set environment variables in the runner
e9cc785 
I need this for an obscure reason, maybe other people will too?
@matthid
Copy link
Member

matthid commented Sep 3, 2020

Hey, thanks for sending in a PR :)
To be honest I'm a bit skeptical, because when this is needed people should fall back to the lower level APIs (ideally we return the CreateProcess instance somewhere). I quickly tried to find some other place we have this and found none. Do we have this somewhere else (Because if we decide to add this it should work similar to existing APIs)?

@robertpi
Copy link
Contributor Author

robertpi commented Sep 3, 2020

Seems the Fsi module is the thing to copy, there's some clever use of inlining in Process.setEnvironmentVariable that allows it operate on any type that has a Environment and WithEnvironment member.

I think it should be easy to change the PR to copy that pattern.

@robertpi
Copy link
Contributor Author

robertpi commented Sep 9, 2020

As the commit message says, we now do exactly what other task such as DotNet and Fsi do.

matthid
matthid reviewed Sep 9, 2020
View reviewed changes
src/app/Fake.DotNet.Testing.NUnit/NUnit3.fs
@@ -330,7 +338,7 @@ let internal createProcess createTempFile (setParams : NUnit3Params -> NUnit3Par
let argLine = Args.toWindowsCommandLine [ (sprintf "@%s" path) ]
CreateProcess.fromRawCommandLine tool argLine
|> CreateProcess.withFramework
|> CreateProcess.withWorkingDirectory (getWorkingDir parameters)
|> CreateProcess.withEnvironment (parameters.Environment |> Map.toList)
Copy link
Member

Choose a reason for hiding this comment

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

We sill want to set the working directory, correct?

@matthid matthid merged commit 514bb27 into fsprojects:release/next Sep 9, 2020
@matthid
Copy link
Member

matthid commented Sep 9, 2020

Thanks! Will probably take me a bit to release, but it will be part of the next one.

@matthid matthid mentioned this pull request Mar 31, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthid matthid matthid left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robertpi @matthid