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

Test run parameter added as part of CLI runsettings args #2251

Merged
merged 9 commits into from
Dec 27, 2019

Conversation

hvinett
Copy link
Contributor

@hvinett hvinett commented Nov 14, 2019

The following error is shown when TestRunParameter Argument is invalid.
testrunparameter

@singhsarab
Copy link
Contributor

Resources files need to be added

@singhsarab
Copy link
Contributor

singhsarab commented Nov 18, 2019

Update the docs, probably add a one pager on the vstest-docs
And also update this PR with screenshots

@singhsarab singhsarab changed the title test run parameter added as second class argument Test run parameter added as part of CLI runsettings args Nov 28, 2019
Copy link
Contributor

@singhsarab singhsarab left a comment

Choose a reason for hiding this comment

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

With suggestions

@singhsarab
Copy link
Contributor

@mayankbansal018 Are you fine with the changes ? Just wanted to get a second opinion regarding the format expected from the user

var attrName = $"(?<{AttributeNameString}>\\w+)";
var attrValue = $"(?<{AttributeValueString}>.+)";
Regex regex = new Regex($"{Constants.TestRunParametersName}.{ParameterString}\\(name\\s*=\\s*\"{attrName}\"\\s*,\\s*value\\s*=\\s*\"{attrValue}\"\\)");
Match match = regex.Match(node);
Copy link
Contributor

Choose a reason for hiding this comment

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

this can throw exception, either catch here, or in the caller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are four argument exceptions possible
1.ArgumentOutOfRangeException : This comes if options is not a valid bitwise combination, but we have not given options in match
2.ArgumentNullException : This commes if input or pattern is null, but input is always not null and pattern is not null.
3.RegexMatchTimeoutException :This commes if timout occured,but as Pattern is fixed and we have not set timeout in match. We won't get this exceptions
4.ArgumentException : This comes if regular expression is not syntactically correct,This too not valid in our case..
so catching an exception is not that reasonable


var match = runSettingsProvider.GetTestRunParameterNodeMatch(node);

if (string.Compare(match.Value, node) == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Compare [](start = 23, length = 7)

is this case insensitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it is case sensitive.

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.

🕐

@singhsarab
Copy link
Contributor

Can we add acceptance tests for this?

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:

@singhsarab singhsarab merged commit 33531d4 into microsoft:master Dec 27, 2019
@cslutgen
Copy link

cslutgen commented Jan 2, 2020

d a

Are there doc updates to describe how to use this new capability? Or screenshots for this PR?

@AbhitejJohn
Copy link
Contributor

@hvinett : would you be able to point folks to docs for this in the main issue(#862) and close it out please?
/cc: @singhsarab

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