-
Notifications
You must be signed in to change notification settings - Fork 163
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
Exclude Feature or Scenario by Tag #433
Exclude Feature or Scenario by Tag #433
Conversation
Thanks for this contribution! I will do my best to review it tomorrow. |
@dirkrombauts thank's for the quick reply, we waiting your returns. |
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 your contribution. It's looking quite good already!
One thing that's missing is support for the exclusion by tags in the GUI. Did you forget about it, or do you perhaps no experience with WPF?
@@ -51,6 +51,8 @@ public class Pickles : Task | |||
|
|||
public string EnableComments { get; set; } | |||
|
|||
public string IgnoreTag { get; set; } |
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.
In terms of the intent of the API, I think that "ExcludeTags" is a better name for the property. Right now we support only one tag but I still want to call the property with the plural "Tags" to ensure forward compatibility.
Please rename the property to ExcludeTags (and adapt the rest of the code).
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.
Ok, done
[Test] | ||
public void ThenCanParseIgnoreTagSuccessfully() | ||
{ | ||
var ignoreTag = "ignore-tag"; |
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.
I'm happy that you include tests on your own initiative. I'm also happy about the style.
One detail I'd like you to change: I prefer Roy Osherhove's unit testing style of making values explicit and not using parameters.
So instead of (for example) Check.That(configuration.IgnoreTag).IsEqualTo(ignoreTag)
I prefer Check.That(configuration.IgnoreTag).IsEqualTo("ignore-tag")
.
The reason is that the second style makes the test easier to read: I instantly know what value we are talking about, and I don't need to check the value of some variable.
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.
As you prefer, it's also ok for us
@@ -40,6 +40,7 @@ public class CommandLineArgumentParser | |||
public const string HelpTestResultsFormat = "the format of the linked test results (nunit|nunit3|xunit|xunit2|mstest |cucumberjson|specrun|vstest)"; | |||
public const string HelpIncludeExperimentalFeatures = "whether to include experimental features"; | |||
public const string HelpEnableComments = "whether to enable comments in the output"; | |||
public const string HelpIgnoreTag = "tag used for ignore feature or scenario"; |
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.
please use this text: exclude scenarios that match this tag
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.
Ok, done
@@ -106,7 +106,8 @@ private bool CollectFiles(DirectoryInfoBase directory, INode rootNode, Tree tree | |||
foreach (FileInfoBase file in directory.GetFiles().Where(file => this.relevantFileDetector.IsRelevant(file))) | |||
{ | |||
INode node = this.featureNodeFactory.Create(rootNode.OriginalLocation, file); | |||
collectedNodes.Add(node); | |||
if(node != null) |
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.
Would it be possible to create a unit test that shows the need for this check?
} | ||
|
||
throw new InvalidOperationException("This feature file could not be read and will be excluded"); | ||
return feature != null ? new FeatureNode(file, relativePathFromRoot, feature) : null; |
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.
Why did you remove the invalidoperationexception? I know that a feature can be null if it's excluded by the tags, but malformed files will fail silently now ...
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.
Do you prefer a default value check for preserve the InvalidOperationException ?
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.
Yes, I think I do.
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.
Ok, i do that. If feature is excluded i return default value
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.
Sorry, but default value check is not the solution.
I think InvalidOperationException is useless because if we have an parsing error we already have an exception for malformed file in function Parse(string filename)
of FeatureParser.cs
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.
You're right ... let's keep your version.
src/Pickles/Pickles/FeatureParser.cs
Outdated
if (result.Tags.Any(t => t == $"@{configuration.IgnoreTag}")) | ||
return null; | ||
|
||
var wantedFeatures = result.FeatureElements.Where(fe => fe.Tags.All(t => t != $"@{configuration.IgnoreTag}")).ToList(); |
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.
This makes the tag exclusion case sensitive. Do we want that? I'm not sure ... what do you think?
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.
We don't want or want that, we can check the tag without sensitivity.
Thanks continuing to work on this. I'm fully booked tomorrow but I set aside some time on Friday to review your changes. |
Ok, for WPF I am not an expert on this technology. If you want do this part you can help me a lot. |
Ok I'll do the wpf part tomorrow. |
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 your updates! There are a couple more things I'd like you to take a look at.
@@ -43,9 +43,12 @@ protected IContainer Container | |||
if (this.container == null) | |||
{ | |||
var builder = new ContainerBuilder(); | |||
|
|||
var configuration = new Configuration() { ExcludeTags = "ignore-tag" }; |
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.
I'd prefer it if you also renamed the ignore-tag in the test data to exclude-tag - here and in the other test cases
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.
Ok, I do that
@@ -35,6 +35,7 @@ public void ReportOn(IConfiguration configuration, Action<string> writeToLog) | |||
writeToLog($"Language : {configuration.Language}"); | |||
writeToLog($"Incorporate Test Results? : {(configuration.HasTestResults ? "Yes" : "No")}"); | |||
writeToLog($"Include Experimental Features? : {(configuration.ShouldIncludeExperimentalFeatures ? "Yes" : "No")}"); | |||
writeToLog($"Ignore Tag : {configuration.ExcludeTags}"); |
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.
Please also change this instance of "Ignore" to Exclude" - and make sure the :
-signs in the lines are aligned with each other.
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.
Ok, I do that
} | ||
|
||
throw new InvalidOperationException("This feature file could not be read and will be excluded"); | ||
return feature != null ? new FeatureNode(file, relativePathFromRoot, feature) : null; |
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.
You're right ... let's keep your version.
if (result.Tags.Any(t => t.Equals($"@{configuration.ExcludeTags}", StringComparison.InvariantCultureIgnoreCase))) | ||
return null; | ||
|
||
var wantedFeatures = result.FeatureElements.Where(fe => fe.Tags.All(t => !t.Equals($"@{configuration.ExcludeTags}", StringComparison.InvariantCultureIgnoreCase))).ToList(); |
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.
Could you perhaps add a test that shows that both scenarios with lowercase and mixed case versions of the exclude tag are being excluded?
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.
We have already a test case for that, but i can do a Special test for zoom on it "feature".
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.
We do have a test case for it? Sorry, I missed it - could you tell me where it is?
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.
In Then_can_parse_and_ignore_scenario_with_tag_in_configuration_ignore_tag_and_keep_feature
@scenario-tag-1 @scenario-tag-2 @Ignore-Tag
and @scenario-tag-1 @scenario-tag-2 @ignore-tag
But if you have missed it, it isn't enough clear and a test is needed
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.
You're right - I prefer to have an explicit test case for this scenario.
I added the UI part. |
…kles into feature/add-ignore-tag
Thanks again for your contribution! I'll release a new version next week with your changes. |
Hi Dirk,
Thanks for the new release :-)
We will have a look and update the documentation this week.
Best Regards,
Wilfried
… Le 17 févr. 2017 à 20:15, Dirk Rombauts ***@***.***> a écrit :
@Wil75, @Riges would you be willing to also extend the documentation with the new argument? It's in the docs repository.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
* Feature/add ignore tag (#433) * add .vs to gitignore * add IgnoreTag Feature * add ignoreTag parameter in any Runner * Fix after test with real case * Implement requests * Fix whitespace * Add properties and handling to model and viewmodel * Insert a row and move all rows down. * Add label and textbox for exclude tags * Reorganize UI * Rename Test Explude Tag "ignore-tag" to "exclude-tag" and add test for no sensitivity * Second try of wildcard support for testresult files (#435) * Added semicolon and wildcard support for all call types (MsBuild, CommandLine, PowerShell, UI-api). Folders and no match will deliver no result file. Multiple matches on the same file will distinct into a single result. * Improved design of tests for TestResultFiles in order to reflect their intention * Changed add file into add directory to the filesystem mock. Pickles throws an exception when a folder does not exist for a resultfile. The about selecting a non existing file was a victim of this exception making it fail. By ensuring the directory exists, the testcase passes. * Release 2.14.0 (#437) * Adapt change log * Version Bump (2.14.0) * Enable creation of outputs in deploy script * Version 2.14.0
Released in version 2.14.0. |
Nice, thank's |
I use Pickles in order to generate Living documentation. I faced the same issue described in: Idea: Exclude Scenarios or Features by tags #18