Filter.matchMethodDescription(Description) has confusing behavior

[Reissner]

I think the documentation of this method is misleadinig.
What it really does is:
If desiredDescription does not describe a single test, no test is executed at all.
If desiredDescription describes a single test, then each description is run containing this single test.

I think this is not what is documented and also it is not what is wanted:
it is used in

Request.filterWith(final Description desiredDescription)

[kcooney]

@Reissner would you be willing to send us a pull request that improves the Javadoc?

[kcooney]

Also your description of the current behavior doesn't match the code (see https://github.com/junit-team/junit4/blob/master/src/main/java/org/junit/runner/manipulation/Filter.java). Does the test you are trying to filter use a Runner that is Filterable?

[kcooney]

Nevermind, I see your point about the behavior.

[Reissner]

I think, it is not the point to improve the documentation,
but to change the behavior to make it useful.
The documentation for this new behavior could be something like

If you agree with documentation
'Returns a Request that only runs tests in this request contained in desiredDescription '
for
Request.filterWith(final Description desiredDescription)
with current implementation based on
Filter.matchMethodDescription(desiredDescription)

the according documentation should be something like
'Returns a filter running only tests contained in desiredDescription. '
But I am not sure, how to treat suites containing testcases in desiredDescription.

Filter.matchMethodDescription(desiredDescription)

[kcooney]

It's unclear to me what this should do if you pass a suite Description. Firstly, they are hard to build (it needs to include all children and grandchildren, etc). Secondly, if the request was previously filtered, then the suite you pass in may no longer match anything.

I can think of two options:

a. If you pass in a suite it should throw IllegalArguementExxeption
b. When comparing suites to the passed-in description, it should remove children before comparing

[Reissner]

I digged a little bit more into and now I think, the problem is elsewhere:
Filter has a method
public abstract boolean shouldRun(Description description)
with almost no javadoc comments.
The contract should be:
For atomic descriptions, i.e. if description.isTest() holds,
shouldRun(description) determines whether description passes the filter.

Otherwise, if description contains at least one leaf description descL which passes the filter,
then shouldRun(description) must be true also.
This means nothing but that the runner has to go into.
If description contains no atomic test which passes the filter,
then shouldRun(description) may be either true or false!

This interpretation is the only possible one
if Filter.intersect(Filter) is implemented correctly:
It is possible that the resulting filter returns shouldRun(...) true for suites,
although the intersection is empty.

Also in method ParentRunner.filter(Filter)
shouldRun==false implies dropping the suite,
but still shouldRun==true may result in dropping also if no atomic test is left.

Summarizing, Filter.matchMethodDescription(final Description) is perfectly ok
but documentation of Filter.shouldRun shall be extended
and so shall be the documentation of Filter.matchMethodDescription(final Description),
mentioning the case that the given description is not atomic but a suite
in which case the filter is empty.

[kcooney]

I think it's perfectly fine for shouldRun() to return false for a suite description without looking at the children

[Reissner]

Oh, no! not at all. It is possible to return true on any suite but it makes bad performance...
also scalability.
So I suggest to return false if one can be sure that no atomic test shall be run in the suite.
If not sure, one may return true.

[kcooney]

I am also not sure that matchMethodDescription is okay. See my comment starting with "It's unclear to me"

As for the Javadoc suggestion, I have a counter proposal: create a subclass of Filter named TestFilter that implements the algorithm you described.

[Reissner]

Now that i understood the principle, i think the implementation of
Filter.matchMethodDescription(final Description) is perfectly ok.

My problem was that I misunderstood the intention behind Filter.shouldRun.
This in turn comes from a lack of documentation.
Thus I think we shall add something:
clear what to do for atomic tests.
For suits: true only if not excluded that at least one atomic test in the suite passes the filter.
Ideally no more than those.
Additional true's result in loss of performance, nothing else.

There are several places in the code supporting this view:
intersection of filters and application of filters to runners.

[kcooney]

Again, instead of updating the Filter Javadoc to describe a complicated algorithm for creating a filter that matches on leaf nodes, why don't we add to JUnit an abstract Filter implementation that implements that algorithm?