-
Notifications
You must be signed in to change notification settings - Fork 752
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
Improve test coverage #204
Conversation
There are two tests that are marked to be skipped for now in order to not break the build. Both don't really uncover bugs but rather counterintuitive behaviour regarding the special treatment of the help and version option that probably isnt't problematic in most cases.
@Brar, |
Assert.True(optVerbose.HasValue()); | ||
} | ||
|
||
[Fact(Skip = "As of version 1.1.0 giving the help option aborts processing without settig it")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, did this behavior change between 1.0.0 and 1.1.0? This is probably a bug we should fix.
@natemcmaster I thought it was working before but I now checked it back to 1.0.0-rc2 when the code was released publicly for the first time and it has always been this way. The relevant code is the following (twice, once for short options and once for long options): if (command.OptionHelp == option)
{
command.ShowHelp();
return 0;
}
else if (command.OptionVersion == option)
{
command.ShowVersion();
return 0;
} The reason for the current behavior is because we return before reaching the following code: else if (option.OptionType == CommandOptionType.NoValue)
{
// No value is needed for this option
option.TryParse(null);
option = null;
} Anyways. It has been consistent in previous versions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing the tests. As noted below, there are just a code style changes to make. Once those are fixed, I have no problem merging this. cc @muratg
Assert.True(optHelp.HasValue()); | ||
} | ||
|
||
[Fact(Skip = "Until now giving the version option aborts processing without settig it")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these tests don't run and we don't plan to change this, can you remove the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
[InlineData(new[] { "-h", "-f" }, "some flag")] | ||
public void HelpAndVersionOptionStopProcessing(string[] input, string expectedOutData) | ||
{ | ||
using (StringWriter outWriter = new StringWriter()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code style: can you change to use using (var outWriter...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
app.Execute(input); | ||
|
||
outWriter.Flush(); | ||
string outData = outWriter.ToString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise...we prefer to use var
where possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
[Fact] | ||
public void ThrowsExceptionOnEmptyCommandOrArgument() | ||
{ | ||
string inputOption = String.Empty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code style: var inputOption = string.Empty;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Make changes requested by @natemcmaster ind #204
Thanks @Brar ! |
There are two tests that are marked to be skipped for now in order to
not break the build.
Both don't really uncover bugs but rather counterintuitive behaviour
regarding the special treatment of the help and version option that
probably isnt't problematic in most cases.