Clarify and test tag filters

[sbrannen]

Status Quo

There are currently no tests for filtering based on tags. Consequently, we have no regression tests for the factory methods in TagFilter.

Deliverables

• Introduce unit and/or integration tests for required filters in TagFilter.
• Introduce unit and/or integration tests for exclude filters in TagFilter.
• Rename DiscoveryFilter#combine and write tests for it

[jlink]

Team discussion:

• TagFiler.includes should probably be renamed to includesOnly.
• Combination of filters should differentiate between and() and or().

[sbrannen]

I've been thinking about the wording with regard to tags that are included and excluded.

By default, all tags are included. So, when a user specifies one or more tags for a particular test plan, what the user is actually saying is something along the lines of "require these tags". One could also phrase that as, "include only these tags". However, I think the word require is more straightforward.

This would lead to terminology such as "required tags" and "excluded tags" for filter names, annotation names (for the JUnit5 runner), etc.

Thoughts?

[jlink]

"Required" sounds more precise. And it clarifies that "include" and
"exclude" are not symetrical here.

2016-01-27 21:34 GMT+01:00 Sam Brannen notifications@github.com:

I've been thinking about the wording with regard to tags that are
included and excluded.

By default, all tags are included. So, when a user specifies one or more
tags for a particular test plan, what the user is actually saying is
something along the lines of "require these tags". One could also phrase
that as, "include only these tags". However, I think the word require
is more straightforward.

This would lead to terminology such as "required tags" and "excluded tags"
for filter names, annotation names (for the JUnit5 runner), etc.

Thoughts?

—
Reply to this email directly or view it on GitHub
#131 (comment).

[jlink]

@sbrannen what would be a more suitable name for DiscoveryFilter.combine()? and ?
And what does it have to do with TagFilters which are PostDiscoveryFilters?

[sbrannen]

Well, it's a static factory method. Thus and would not be suitable.

Basically, it's a factory for a CompositeFilter.

With that in mind, I would suggest renaming CombinedDiscoveryFilter to CompositeDiscoveryFilter.

That would then imply that combine() should be renamed to something along the lines of:

• from(): create a composite filter from the supplied filters.
• createCompositeFilter()
• buildCompositeFilter()

[jlink]

What about compose() then?

And it should be usable for PostDiscoveryFilter instances as well, since the same logic is used in Root.isIncluded.

[sbrannen]

I kinda like the simplicity of from(). 😉

But when it's used as static import (like in JUnit4DiscoveryRequestResolver), the meaning of from is lost in the (absent) context.

compose() would also be OK, but I think I might prefer compositeFilter() over just compose(). Plus, compositeFilter() would also work nicely when statically imported.

In the end it's not really that big a deal either way.

And it should be usable for PostDiscoveryFilter instances as well, since the same logic is used in Root.isIncluded.

Well, PostDiscoveryFilter is not an instance of DiscoveryFilter. So that would require more work regarding CombinedDiscoveryFilter. If you want a single, generic composite filter, that would lead toward CompositeFilter<T> as I hinted at before.

[jlink]

I'm going for an integration of both ideas: composeFilters()

[jlink]

Fixed in master.

[sbrannen]

@jlink, nice work on the CompositeFilter. 👍

Thanks!