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

Passing runsettings as command line arguments. #297

Merged
merged 19 commits into from
Dec 27, 2016
Merged

Passing runsettings as command line arguments. #297

merged 19 commits into from
Dec 27, 2016

Conversation

harshjain2
Copy link
Contributor

@harshjain2
Copy link
Contributor Author

dotnet vstest.console.dll -h

image

image

@@ -152,13 +152,27 @@ private int GetArgumentProcessors(string[] args, out List<IArgumentProcessor> pr
int result = 0;
var processorFactory = ArgumentProcessorFactory.Create();

foreach (string arg in args)
for (var index = 0; index < args.Length; index++)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test in Acceptance Tests project.

/// <summary>
/// The name of the command line argument that the PortArgumentExecutor handles.
/// </summary>
public const string CommandName = "--";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try passing --- for CLIRunSettingsArgument, I think it will work two. Because at the time of creating BuildCommandMaps( https://github.com/Microsoft/vstest/blob/master/src/vstest.console/Processors/Utilities/ArgumentProcessorFactory.cs#L226) we replace first character of CommandName with "--" because to create xplat command. All the CommandName is starting with "/", shouldn't we have something similar??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having "/" is good to have, but I am not sure if we need it or not.
@codito Any thought on this?

Also, as mentioned by Faizan, we are also getting "---" by default, do we need to remove it?

Copy link
Contributor

@codito codito Dec 23, 2016

Choose a reason for hiding this comment

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

/ will create confusion with root path on unix. -- looks good to me.

All the CommandName is starting with "/",

We can keep supporting it for back compact but, (IMO) we should remove this from help text for simplicity.

@harshjain2
Copy link
Contributor Author

@pvlakshm Please review the documentation that will appear in console.

@pvlakshm
Copy link
Contributor

pvlakshm commented Dec 22, 2016

Using msbuild /? and dotnet run /h as reference.

Here is what we can have:
Usage: vstest.console.exe [Arguments] [Options] [[--] <args>...]]
...
Args:
Any extra arguments that should be passed to adapter. Arguments may be specified as name-value pair of the form <n>=<v>, where <n> is the argument name, and <v> is the argument value. Use a space to separate multiple arguments.

@@ -43,6 +43,11 @@ internal enum ArgumentProcessorPriority
RunSettings = 5,

/// <summary>
/// Priority of processors related to CLI Run Settings.
/// </summary>
CLIRunSettings = 6,
Copy link
Contributor

Choose a reason for hiding this comment

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

So we are going to entertain commandline arg over runsetting if same arg is getting passed in through commandline and run setting, right?


public void Initialize(string[] arguments)
{
// if argument is null or white space, don't do anything.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think white space from comment should be removed.

node = CreateNode(xmlDoc, key.Split('.'));
}

node.InnerText = WebUtility.HtmlEncode(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not encode value in HTML as our reader is not aware of that. We need this encoder just to pass value from dotnet test to vstest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codito : all the runsettings reader are not aware that xml values could be encoded.
IMO as well, we should not encode values for now.
Any thoughts on this?

@harshjain2 harshjain2 merged commit 77f351f into microsoft:master Dec 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants