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

Specifying environment variables in RunSettings file #2128

Merged
merged 5 commits into from
Aug 20, 2019
Merged

Specifying environment variables in RunSettings file #2128

merged 5 commits into from
Aug 20, 2019

Conversation

vagisha-nidhi
Copy link
Contributor

Description

Option to specify environment variables in run settings file.
RFC: microsoft/vstest-docs#202

For example:

<?xml version="1.0" encoding="utf-8"?>
<!-- File name extension must be .runsettings -->
<RunSettings>
  <RunConfiguration>
    <EnvironmentVariables>
      <!-- List of environment variables we want to set-->
      <DOTNET_ROOT>C:\ProgramFiles\dotnet</DOTNET_ROOT>
      <SDK_PATH>C:\Codebase\Sdk</SDK_PATH>
    </EnvironmentVariables>
  </RunConfiguration>
</RunSettings>

These are set when spawning the test host process.

Related issue

#2100

singhsarab
singhsarab reviewed Aug 13, 2019
View reviewed changes
src/vstest.console/Processors/RunSettingsArgumentProcessor.cs
@@ -137,6 +138,12 @@ public void Initialize(string argument)
this.runSettingsManager.AddDefaultRunSettings();

this.commandLineOptions.SettingsFile = argument;

if (this.runSettingsManager.QueryRunSettingsNode("RunConfiguration.EnvironmentVariables") != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

"RunConfiguration.EnvironmentVariables" [](start = 65, length = 39)

Constants ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not available

singhsarab
singhsarab reviewed Aug 13, 2019
View reviewed changes
src/vstest.console/Processors/RunSettingsArgumentProcessor.cs
if (this.runSettingsManager.QueryRunSettingsNode("RunConfiguration.EnvironmentVariables") != null)
{
this.commandLineOptions.InIsolation = true;
this.runSettingsManager.UpdateRunSettingsNode(InIsolationArgumentExecutor.RunSettingsPath, "true");
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why is this required if the CLI option is already set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/microsoft/vstest/blob/master/src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs#L244 This checks inisolation node from runsettings.

@singhsarab
Copy link
Contributor 

    private class TestableRunSettingsArgumentExecutor : RunSettingsArgumentExecutor

FileHelper please

Refers to: test/vstest.console.UnitTests/Processors/RunSettingsArgumentProcessorTests.cs:354 in 9ac75ee. [](commit_id = 9ac75ee, deletion_comment = False)

singhsarab
singhsarab reviewed Aug 13, 2019
View reviewed changes
src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs Outdated

while (childNodes.MoveNext())
{
environmentVariables.Add(childNodes.Current.Name, childNodes.Current.Value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Checks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes out to be always empty and never null.
The following is invalid and document fails to load.

<RunConfiguration>
 <EnvironmentVariables>
   <Foo>
 </EnvironmentVariables>
</RunConfiguration>

singhsarab
singhsarab reviewed Aug 16, 2019
View reviewed changes
test/Microsoft.TestPlatform.Utilities.UnitTests/InferRunSettingsHelperTests.cs
<RunConfiguration>
<EnvironmentVariables>
<RANDOM_PATH>C:\temp</RANDOM_PATH>
<RANDOM_PATH2>C:\temp2</RANDOM_PATH2>
Copy link
Contributor

Choose a reason for hiding this comment

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

Check for the case when user tried to add the same key twice

mayankbansal018
mayankbansal018 reviewed Aug 19, 2019
View reviewed changes
src/Microsoft.TestPlatform.CrossPlatEngine/DataCollection/ProxyDataCollectionManager.cs Outdated
@@ -226,7 +226,7 @@ public void Initialize()
this.dataCollectionPort = this.dataCollectionRequestSender.InitializeCommunication();

// Warn the user that execution will wait for debugger attach.
this.dataCollectionProcessId = this.dataCollectionLauncher.LaunchDataCollector(null, this.GetCommandLineArguments(this.dataCollectionPort));
this.dataCollectionProcessId = this.dataCollectionLauncher.LaunchDataCollector(InferRunSettingsHelper.GetEnvironmentVariables(SettingsXml), this.GetCommandLineArguments(this.dataCollectionPort));
EqtTrace.Info("ProxyDataCollectionManager.Initialize: Launched datacollector processId: {0} port: {1}", this.dataCollectionProcessId, this.dataCollectionPort);
Copy link
Contributor

Choose a reason for hiding this comment

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

acollector processId: {0} port: {1}", this.dataCollectionProcessId, this.dataColl [](start = 78, length = 81)

Log env vars used to launch process

mayankbansal018
mayankbansal018 reviewed Aug 19, 2019
View reviewed changes
src/Microsoft.TestPlatform.Utilities/InferRunSettingsHelper.cs

while (childNodes.MoveNext())
{
if (!environmentVariables.ContainsKey(childNodes.Current.Name))
Copy link
Contributor

Choose a reason for hiding this comment

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

ContainsKey [](start = 50, length = 11)

You can use TryAdd, instead of checking and then adding.

mayankbansal018
mayankbansal018 approved these changes Aug 19, 2019
View reviewed changes
Copy link
Contributor

@mayankbansal018 mayankbansal018 left a comment

Choose a reason for hiding this comment

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

:shipit:

@vagisha-nidhi vagisha-nidhi merged commit 485de78 into microsoft:master Aug 20, 2019
@ViktorHofer
Copy link
Member

Very happy to see this implemented. Please cc me the next time in the implementation PR as well. Were you able to address my comment (microsoft/vstest-docs#202 (comment)) as well?

@singhsarab
Copy link
Contributor

@ViktorHofer There is no scenario where we would now be running in vstest.console, wherenever there are environment variables specified, it runs in isolation which means testhost will be spawned.

@tonyhallett tonyhallett mentioned this pull request Feb 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@singhsarab singhsarab singhsarab left review comments

@mayankbansal018 mayankbansal018 mayankbansal018 approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@vagisha-nidhi @singhsarab @ViktorHofer @mayankbansal018