Remove tag test block filter from core #127
Conversation
|
Let me know when this is ready for review and I'll take a look. |
phillcunnington
force-pushed
the
remove_tag_test_block_filter_from_core
branch
from
f140779
to
d53a88d
Compare
|
|
||
| Configuration() { | ||
| Configuration(Options runState) { |
There was a problem hiding this comment.
Im thinking it would be nice for the test transform to know what state it expects and for the ConfigurationProvider just to state that the test transform is to be used in a specified order rather than expect the ConfigurationProvider to know what state the test transform needs...
There was a problem hiding this comment.
Yes, this decouples things nicely. Good thinking!
There was a problem hiding this comment.
In general I'm in support of this change. However, it looks to me like this change will require users to make code changes in order to continue to use tag filtering. If that's the case, then this is a quasi API-breaking change and we should probably bump the major version number. To compound the problem, in some cases the code change required would be to introduce a ConfigurationProvider, which I've always considered a more advanced use of Cuppa.
I think the changes you've made around runtime-provided options is good and should stay.
Is there a different approach that would allow us to maintain tag filtering as an out of the box feature?
For instance, perhaps we could add a method to Configuration that lets the user opt-out of the default transforms. For instance, in your ConfigurationProvider:
configuration.disableBuiltinTransforms();
Another approach could be to say that the use of registerTestTreeTransform implicitly disables built-in transforms. This is still an API-breaking change, but it would only affect users who are already using their own transforms, which I hope is a minority of users.
If we go down this path, I'd prefer to only allow the user to disable all built-in transforms rather than making a special case for the tag filtering.
WDYT?
| */ | ||
|
|
||
| /** | ||
| * Test block filters. |
There was a problem hiding this comment.
Consider expanding on this javadoc a little. Perhaps:
This package contains common transforms that you can to use by adding them in your ConfigurationProvider.
| /** | ||
| * Tag run state to perform tag based filtering with. | ||
| */ | ||
| public static final class RunState extends Option<Tags> { |
There was a problem hiding this comment.
This name seems a little generic to me. Why did you go for RunState over something like FilteredTagsOption?
|
|
||
| private final List<Function<TestBlock, TestBlock>> coreTestTransforms; | ||
| private final Configuration configuration; | ||
| private final Options runState; |
There was a problem hiding this comment.
I think I would prefer to call this runOptions or runParameters, but I'm fine with the current name if you like it.
|
|
||
| Configuration() { | ||
| Configuration(Options runState) { |
There was a problem hiding this comment.
Yes, this decouples things nicely. Good thinking!
mrpotes
force-pushed
the
remove_tag_test_block_filter_from_core
branch
from
d53a88d
to
6fc60fb
Compare
| * <li>{@link org.forgerock.cuppa.internal.filters.EmptyTestBlockFilter}</li> | ||
| * </ul> | ||
| */ | ||
| public void removeCoreTestTransforms() { |
There was a problem hiding this comment.
What about withoutCoreTransforms which returns a new Configuration instance?
There was a problem hiding this comment.
To do that would still involve a change to the public API of ConfigurationProvider, so I would prefer not to...
When trying to implement a "chunking" test block transform we realised that the tag filtering was always done by Cuppa afterwards so the chunks of tests where not evenly disributed, dependent on the tags being run.
To allow the "chunking" to be done after the tag filtering this change pulls the
TagTestBlockFilterout of the list of core transforms so that it can be applied by theCuppaConfigurationProviderin the desired order.