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

Exclude Feature or Scenario by Tag #433

Merged
merged 12 commits into from
Feb 17, 2017
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*.DS_Store

#Visual Studio files
.vs
*.[Oo]bj
*.user
*.aps
Expand Down
7 changes: 7 additions & 0 deletions src/Pickles/Pickles.MSBuild/Pickles.cs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ public class Pickles : Task

public string EnableComments { get; set; }

public string IgnoreTag { get; set; }
Copy link
Member

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).

Copy link

Choose a reason for hiding this comment

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

Ok, done


public override bool Execute()
{
try
Expand Down Expand Up @@ -113,6 +115,11 @@ private void CaptureConfiguration(IConfiguration configuration, IFileSystem file
{
configuration.DocumentationFormat = (DocumentationFormat)Enum.Parse(typeof(DocumentationFormat), this.DocumentationFormat, true);
}

if (!string.IsNullOrEmpty(this.IgnoreTag))
{
configuration.IgnoreTag = this.IgnoreTag;
}

bool shouldEnableExperimentalFeatures;

Expand Down
2 changes: 2 additions & 0 deletions src/Pickles/Pickles.ObjectModel/IConfiguration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ public interface IConfiguration

bool ShouldIncludeExperimentalFeatures { get; }

string IgnoreTag { get; set; }

void AddTestResultFile(FileInfoBase fileInfoBase);

void AddTestResultFiles(IEnumerable<FileInfoBase> fileInfoBases);
Expand Down
8 changes: 8 additions & 0 deletions src/Pickles/Pickles.PowerShell/Pickle_Features.cs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ public class Pickle_Features : PSCmdlet
[Parameter(HelpMessage = CommandLineArgumentParser.HelpEnableComments, Mandatory = false)]
public string EnableComments { get; set; }

[Parameter(HelpMessage = CommandLineArgumentParser.HelpIgnoreTag, Mandatory = false)]
public string IgnoreTag { get; set; }

protected override void ProcessRecord()
{
var builder = new ContainerBuilder();
Expand Down Expand Up @@ -120,6 +123,11 @@ private void ParseParameters(IConfiguration configuration, IFileSystem fileSyste
configuration.EnableExperimentalFeatures();
}

if (!string.IsNullOrEmpty(this.IgnoreTag))
{
configuration.IgnoreTag = this.IgnoreTag;
}

bool shouldEnableComments;

if (bool.TryParse(this.EnableComments, out shouldEnableComments))
Expand Down
14 changes: 14 additions & 0 deletions src/Pickles/Pickles.Test/WhenParsingCommandLineArguments.cs
Original file line number Diff line number Diff line change
Expand Up @@ -487,5 +487,19 @@ public void ThenSetsLanguageToEnglishByDefault()

Check.That(configuration.Language).IsEqualTo("en");
}

[Test]
public void ThenCanParseIgnoreTagSuccessfully()
{
var ignoreTag = "ignore-tag";
Copy link
Member

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.

Copy link

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

var args = new[] { $@"-ignoreTag={ignoreTag}" };

var configuration = new Configuration();
var commandLineArgumentParser = new CommandLineArgumentParser(FileSystem);
bool shouldContinue = commandLineArgumentParser.Parse(args, configuration, TextWriter.Null);

Check.That(shouldContinue).IsTrue();
Check.That(configuration.IgnoreTag).IsEqualTo(ignoreTag);
}
}
}
96 changes: 96 additions & 0 deletions src/Pickles/Pickles.Test/WhenParsingFeatureFiles.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
// --------------------------------------------------------------------------------------------------------------------

using System;
using System.IO.Abstractions;
using System.Linq;

using Autofac;
Expand Down Expand Up @@ -395,5 +396,100 @@ Then I should see that this thing happens
}



[Test]
public void Then_can_parse_and_ignore_feature_with_tag_in_configuration_ignore_tag()
{
var ignoreTag = "ignore-tag";
string featureText =
$@"# ignore this comment
@feature-tag @{ignoreTag}
Feature: Test
In order to do something
As a user
I want to run this scenario

@scenario-tag-1 @scenario-tag-2
Scenario: A scenario
Given some feature
When it runs
Then I should see that this thing happens";

var parser = new FeatureParser(Container.Resolve<IFileSystem>(), new Configuration { IgnoreTag = ignoreTag });
var feature = parser.Parse(new StringReader(featureText));

Check.That(feature).IsNull();
}

[Test]
public void Then_can_parse_and_ignore_scenario_with_tag_in_configuration_ignore_tag()
{
var ignoreTag = "ignore-tag";
string featureText =
$@"# ignore this comment
@feature-tag
Feature: Test
In order to do something
As a user
I want to run this scenario

@scenario-tag-1 @scenario-tag-2
Scenario: A scenario
Given some feature
When it runs
Then I should see that this thing happens

@scenario-tag-1 @scenario-tag-2 @{ignoreTag}
Scenario: B scenario
Given some feature
When it runs
Then I should see that this thing happens

@scenario-tag-1 @scenario-tag-2
Scenario: C scenario
Given some feature
When it runs
Then I should see that this thing happens";

var parser = new FeatureParser(Container.Resolve<IFileSystem>(), new Configuration { IgnoreTag = ignoreTag });
var feature = parser.Parse(new StringReader(featureText));

Check.That(feature.FeatureElements.Count).IsEqualTo(2);
Check.That(feature.FeatureElements.FirstOrDefault(fe => fe.Name == "A scenario")).IsNotNull();
Check.That(feature.FeatureElements.FirstOrDefault(fe => fe.Name == "B scenario")).IsNull();
Check.That(feature.FeatureElements.FirstOrDefault(fe => fe.Name == "C scenario")).IsNotNull();
}

[Test]
public void Then_can_parse_and_ignore_scenario_with_tag_in_configuration_ignore_tag_and_keep_feature()
{
var ignoreTag = "ignore-tag";
string featureText =
$@"# ignore this comment
@feature-tag
Feature: Test
In order to do something
As a user
I want to run this scenario

@scenario-tag-1 @scenario-tag-2 @{ignoreTag}
Scenario: A scenario
Given some feature
When it runs
Then I should see that this thing happens

@scenario-tag-1 @scenario-tag-2 @{ignoreTag}
Scenario: B scenario
Given some feature
When it runs
Then I should see that this thing happens";

var parser = new FeatureParser(Container.Resolve<IFileSystem>(), new Configuration { IgnoreTag = ignoreTag });
var feature = parser.Parse(new StringReader(featureText));

Check.That(feature).IsNotNull();
Check.That(feature.FeatureElements).IsEmpty();
}
}
}

10 changes: 9 additions & 1 deletion src/Pickles/Pickles/CommandLineArgumentParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Member

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

Copy link

Choose a reason for hiding this comment

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

Ok, done


public const string HelpTestResultsFile =
"the path to the linked test results file (can be a semicolon-separated list of files)";
Expand All @@ -58,6 +59,7 @@ public class CommandLineArgumentParser
private bool versionRequested;
private bool includeExperimentalFeatures;
private string enableCommentsValue;
private string ignoreTag;

public CommandLineArgumentParser(IFileSystem fileSystem)
{
Expand All @@ -75,7 +77,8 @@ public CommandLineArgumentParser(IFileSystem fileSystem)
{ "v|version", v => this.versionRequested = v != null },
{ "h|?|help", v => this.helpRequested = v != null },
{ "exp|include-experimental-features", HelpIncludeExperimentalFeatures, v => this.includeExperimentalFeatures = v != null },
{ "cmt|enableComments=", HelpEnableComments, v => this.enableCommentsValue = v }
{ "cmt|enableComments=", HelpEnableComments, v => this.enableCommentsValue = v },
{ "it|ignoreTag=", HelpIgnoreTag, v => this.ignoreTag = v }
};
}

Expand Down Expand Up @@ -148,6 +151,11 @@ public bool Parse(string[] args, IConfiguration configuration, TextWriter stdout
configuration.EnableExperimentalFeatures();
}

if (!string.IsNullOrEmpty(this.ignoreTag))
{
configuration.IgnoreTag = this.ignoreTag;
}

bool enableComments;

if (bool.TryParse(this.enableCommentsValue, out enableComments) && enableComments == false)
Expand Down
2 changes: 2 additions & 0 deletions src/Pickles/Pickles/Configuration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ public void AddTestResultFiles(IEnumerable<FileInfoBase> fileInfoBases)
}
}

public string IgnoreTag { get; set; }

private void AddTestResultFileIfItExists(FileInfoBase fileInfoBase)
{
if (fileInfoBase.Exists)
Expand Down
1 change: 1 addition & 0 deletions src/Pickles/Pickles/ConfigurationReporter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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.IgnoreTag}");

if (configuration.HasTestResults)
{
Expand Down
3 changes: 2 additions & 1 deletion src/Pickles/Pickles/DirectoryCrawler/DirectoryTreeCrawler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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?

collectedNodes.Add(node);
}

foreach (var node in OrderFileNodes(collectedNodes))
Expand Down
8 changes: 2 additions & 6 deletions src/Pickles/Pickles/DirectoryCrawler/FeatureNodeFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
using System.Xml.Linq;

using PicklesDoc.Pickles.DocumentationBuilders.Html;
using PicklesDoc.Pickles.DocumentationBuilders.Word.TableOfContentsAdder;
using PicklesDoc.Pickles.Extensions;
using PicklesDoc.Pickles.ObjectModel;

Expand Down Expand Up @@ -57,12 +58,7 @@ public INode Create(FileSystemInfoBase root, FileSystemInfoBase location)
if (this.relevantFileDetector.IsFeatureFile(file))
{
Feature feature = this.featureParser.Parse(file.FullName);
if (feature != null)
{
return new FeatureNode(file, relativePathFromRoot, feature);
}

throw new InvalidOperationException("This feature file could not be read and will be excluded");
return feature != null ? new FeatureNode(file, relativePathFromRoot, feature) : null;
Copy link
Member

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 ...

Copy link

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 ?

Copy link
Member

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.

Copy link

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

Copy link

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

Copy link
Member

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.

}
else if (this.relevantFileDetector.IsMarkdownFile(file))
{
Expand Down
19 changes: 17 additions & 2 deletions src/Pickles/Pickles/FeatureParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

using System;
using System.IO.Abstractions;

using System.Linq;
using PicklesDoc.Pickles.ObjectModel;

using TextReader = System.IO.TextReader;
Expand Down Expand Up @@ -74,8 +74,10 @@ public Feature Parse(TextReader featureFileReader)
new Gherkin.TokenMatcher(new CultureAwareDialectProvider(language)));

Feature result = new Mapper(this.configuration, gherkinDocument.Feature.Language).MapToFeature(gherkinDocument);
result = this.RemoveFeatureWithIgnoreTag(result);

this.descriptionProcessor.Process(result);
if (result != null)
this.descriptionProcessor.Process(result);

return result;
}
Expand All @@ -90,5 +92,18 @@ private string DetermineLanguage()
}
return language;
}

private Feature RemoveFeatureWithIgnoreTag(Feature result)
{
if (result.Tags.Any(t => t == $"@{configuration.IgnoreTag}"))
return null;

var wantedFeatures = result.FeatureElements.Where(fe => fe.Tags.All(t => t != $"@{configuration.IgnoreTag}")).ToList();
Copy link
Member

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?

Copy link

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.


result.FeatureElements.Clear();
result.FeatureElements.AddRange(wantedFeatures);

return result;
}
}
}