`EngineDiscoveryOrchestrator#applyPostDiscoveryFilters` should probably do depth-first processing

[Vampire]

In platform 1.9.0, the EngineDiscoveryOrchestrator#applyPostDiscoveryFilters calls acceptInAllTestEngines which iterates over the test engine descriptors and for each engine descriptor calls accept with a visitor that checks all post discovery filters and removes nodes from the hierarchy eventually.

This removing is done, if the descriptor is not the root, and any of the filters excludes it, and the descriptor has no children.

Gradle, Maven, and IntelliJ for example use post discovery filters to match test method names with own supported pattern syntaxes.

So if you now have a hierarchy like

a
  b
    c

and the post discovery filter would filter out b and c, the end result would be

a
  b

because when the filter is applied to b, it has children and thus is not excluded. When it then checks c, it is removed because it does not have children.

Shouldn't the children be checked first, so that the filter can remove c first and then remove b as it no longer has children?

[marcphilipp]

In case b is a container it gets pruned automatically so this usually isn't a problem. I don't think we can change the behavior without breaking existing use cases but we could introduce another method that is called after all children have been visited.

@Vampire Is there a concrete use case you can't solve with the current behavior?

[Vampire]

In case b is a container it gets pruned automatically so this usually isn't a problem.

Well, the point is that b is not CONTAINER, but ' CONTAINER_AND_TEST`, that's why it actually is a problem.

I don't think we can change the behavior without breaking existing use cases but we could introduce another method that is called after all children have been visited.

Of course it is at your discretion, but imho the use-cases that depend on this behavior depend on a bug and so you should maybe just don't care too much? :-)

@Vampire Is there a concrete use case you can't solve with the current behavior?

As I said, the IDE and build tools use this method to do test filtering and by using that method, their implementation is buggy and all these tools would need to work-around the issue the same way if possible at all. They register a filter that says "no" for b and c, but only c gets removed and b stays around.

Of course you could provide a fixed variant of the method, but then you would also need to make all current users use the new version, while imho the vast majority of the callers - if not all - would need and expect the fixed behavior. So maybe you could also fix this method and provide one with the old behavior, so that if there is really a need for the current behavior, it could then be used instead.

[marcphilipp]

We could change the default implementation of prune() or you could switch from CONTAINER_AND_TEST to just CONTAINER. 🤔

On a related note, I just introduced TestPlan.Visitor that could be used to implement filtering. It would allow filtering before or after visiting a container. It might need a few changes to avoid ConcurrentModificationExceptions, though.

[Vampire]

or you could switch from CONTAINER_AND_TEST to just CONTAINER

Not really. I switched it to CONTAINER_AND_TEST because you told me to use it in that case. ;-) And well, if that would be the solution, then JUnit should remove CONTAINER_AND_TEST completely, because this surfaces in Spock, but it will happen exactly the same with any JUnit platform engine that does not only have CONTAINER hierarchy with TEST at the leafs.

We could change the default implementation of prune()

So that the parent is rechecked when it actually should have been removed whether it can be removed now as the childs are finally gone? That might work too, it is then similar to the implied nodes work-around I did in Spock. But a depth-first traversal might be easier and cleaner, but whatever you like. :-)

On a related note, I just introduced TestPlan.Visitor that could be used to implement filtering. It would allow filtering before or after visiting a container. It might need a few changes to avoid ConcurrentModificationExceptions, though.

Well sure, if that is the "fixed method", you can also make Maven, Gradle, IntelliJ, and whatever else to use that one instead. But imho this doesn't change that the logic of applyPostDiscoveryFilters is broken as long as it is not depth-first traversal. :-)

[marcphilipp]

I can see your point but stand my statement regarding backwards compatibility. I think we could add another method to PostDiscoveryFilter though or allow it to opt-in to a depth-first traversal via an annotation or a default method returning an enum.