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

Add support for intersection groups #131

Conversation

alx75
Copy link

@alx75 alx75 commented Jan 29, 2018

This feature modifies the TagTestBlockFilter to handle the intersection groups. It also modifies the ConfigurationProvider to pass the plugin properties as a parameter.

That intersection group could have been added as a custom testTransforms from the configuration of my own project but it was a bit hacky because the current TagTestBlockFilter would have been applied to the test blocks after my own filter. Plus, in the transformer, it wasn't possible to get the properties defined in the surefire plugin.

I ended up creating that PR because I think other cuppa users could benefit it.

Example :
mvn verify -Dgroups=smoke -DintersectGroups=group1,group2
Will return all the smoke tests plus the intersection of group1 and group2

Signed-off-by: Alex ROBUCHON alex.robuchon@forgerock.com

@coveralls
Copy link

coveralls commented Jan 29, 2018

Coverage Status

Coverage increased (+0.7%) to 90.057% when pulling 87cd3f2 on alx75:add_support_for_intersection_groups into a3f9d90 on cuppa-framework:master.

@alx75 alx75 force-pushed the add_support_for_intersection_groups branch from 32c43f2 to d908889 Compare February 1, 2018 17:33
Copy link
Member

@mrpotes mrpotes left a comment

Choose a reason for hiding this comment

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

I'm not massively keen on adding another property for a specific use case. I would rather add something like -DgroupsExpression=and(group1,group2) for which the grammar would support and(GROUP...), or(GROUP...) and not(GROUP).

*/
void configure(Configuration configuration);
void configure(Configuration configuration, Map<String, String> properties);
Copy link
Member

Choose a reason for hiding this comment

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

This appears to be a breaking change - is there anything we can do to avoid that?

Copy link
Member

Choose a reason for hiding this comment

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

Looking through the changes, I can't actually see why this change is necessary for the described feature, anyway... Have I missed something?

Copy link
Author

Choose a reason for hiding this comment

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

It's not necessary for the intersection group indeed.
It's useful though if you want to register a TestTreeTransform that needs to be aware of the provider configuration.

I'm not keen to pass it in the Configuration because it's not the cuppa configuration. I can extract that change to another PR

@alx75
Copy link
Author

alx75 commented Feb 5, 2018

I like the -DgroupsExpression idea. Will update my PR.

@alx75 alx75 force-pushed the add_support_for_intersection_groups branch 2 times, most recently from e68d01f to 6d50414 Compare February 7, 2018 16:07
/**
* A condition used by {@link ExpressionTagTestBlockFilter}.
*/
public interface Condition {
Copy link
Member

Choose a reason for hiding this comment

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

@FunctionalInterface?

@@ -1,5 +1,5 @@
/*
* Copyright 2015-2016 ForgeRock AS.
* Copyright 2015-2018 ForgeRock AS.
Copy link
Member

Choose a reason for hiding this comment

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

No change to file so revert

@@ -1,5 +1,5 @@
/*
* Copyright 2015-2016 ForgeRock AS.
* Copyright 2015-2018 ForgeRock AS.
Copy link
Member

Choose a reason for hiding this comment

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

revert

@@ -35,14 +35,28 @@ For example, to run all tests except tests tagged with `slow`:
mvn -DexcludedTags=slow test
```

If you want more flexibility wou can use an expression.
Copy link
Member

Choose a reason for hiding this comment

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

wou -> you

@@ -35,14 +35,28 @@ For example, to run all tests except tests tagged with `slow`:
mvn -DexcludedTags=slow test
```

If you want more flexibility wou can use an expression.
For example, to run all the tests with`fast` tag or with `smoke` and `ui` tags excluding all `slow` tags :
Copy link
Member

Choose a reason for hiding this comment

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

withfast -> with fast

<div class="alert alert-info" role="alert">
#### Note

When running with a combination of TestNG or JUnit along side Cuppa, you can use the TestNG/JUnit way of
specifying/excluding groups so that you can run a sub-set of tests across both TestNG/JUnit and Cuppa.

It's important to note that you cannot use both `-Dgroups=` and `-Dtags=` or `-DexcludedGroups=` and `-DexcludedTags=`
at the same time.
It's important to note that you cannot use `-Dgroups=` and `-Dtags=` , `-DexcludedGroups=` and `-DexcludedTags=`
Copy link
Member

Choose a reason for hiding this comment

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

-Dtags= , -DexcludedGroups= -> -Dtags=, -DexcludedGroups=

Copy link
Member

Choose a reason for hiding this comment

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

sorry nit picking :)

return union;
}

private static class ExpressionParser {
Copy link
Member

Choose a reason for hiding this comment

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

Extract out to package private class as will keep visibility hidden due to all expression classes being in sub-package already and have the added benefit to see which of these methods are called by the outer class?

@@ -77,8 +78,8 @@ public Runner(Tags runTags) {
* @param configuration Cuppa configuration to control the behaviour of the runner.
*/
public Runner(Tags runTags, Configuration configuration) {
coreTestTransforms = Arrays.asList(new OnlyTestBlockFilter(), new TagTestBlockFilter(runTags),
new EmptyTestBlockFilter());
coreTestTransforms = Arrays.asList(new OnlyTestBlockFilter(), new ExpressionTagTestBlockFilter(runTags),
Copy link
Member

Choose a reason for hiding this comment

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

I feel like there shouldn't be a need for 2 tag block filters. But saying that i'd like to look at trying to make changes to allow the groups expression to be an extension to Cuppa core. I will look at that separate to this PR though.

The provider can be called with -DgroupsExpression=and(group1,group2) for which the grammar would support and(GROUP...), or(GROUP...) and not(GROUP)

Signed-off-by: Alex ROBUCHON <alex.robuchon@forgerock.com>
@alx75 alx75 force-pushed the add_support_for_intersection_groups branch from 165b64b to 87cd3f2 Compare February 16, 2018 13:59
@phillcunnington phillcunnington merged commit 87cd3f2 into cuppa-framework:master Feb 16, 2018
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.

None yet

4 participants