EngineDiscoveryRequestResolver API overly complicated?

[JojOatXGME]

I was recently creating a small test engine for a rather specific use case. I was using EngineDiscoveryRequestResolver, but I got the impression that the API is overly complicated for no apparent reason. I haven't identified any reason at least. Unfortunately, I am a bit late as the feature was recently promoted to stable, but I still wanted to provide my feedback. (I am not sure if this is actually the right platform for feedback, though.)

Unnecessary redundancy and nesting into callbacks

Here is the simplest example I could think of on how to use the API.

@Override
public Resolution resolve(UniqueIdSelector selector, Context context) {
  UniqueId uniqueId = selector.getUniqueId();
  return context.addToParent(__ -> Optional.of(new MyTestDescriptor(uniqueId)))
                .map(testDescriptor -> Resolution.match(Match.exact(testDescriptor)))
                .orElse(Resolution.unresolved());
}

If Context would instead provide the method void addMatch(TestDescriptor) and TestDescriptor getParent(), we could rewrite such methods into the following.

@Override
public Resolution resolve(UniqueIdSelector selector, Context context) {
  UniqueId uniqueId = selector.getUniqueId();
  context.addMatch(new MyTestDescriptor(uniqueId));
  return Resolution.unresolved(); // We could now change the return type to void
}

We could also use context.getParent().addChild(testDescriptor) instead of providing the addMatch method. However, I will extend the method later.

Note that this method is not only shorter and easier to read, it is also less error-prone. The previous implementation above provides the test descriptor to context.addToParent and returns it as part of the Resolution. This redundancy can cause confusing errors when you miss either of these steps. Also note that my example is very simple and doesn't include any hierarchy. In practice, the callback within addToParent would often become noticeably larger, which makes this API even more frustrating to use.

Match.Type

There are other aspects of the API which feel overcomplicated to me. Let's consider the Match class. The purpose of this class seems to distinguish between exact matches and partial matches, and to optionally add a childSelectorsSupplier. There are four factory methods which can be used to construct a Match. (The constructor is private.)

Match exact(TestDescriptor testDescriptor) // 1
Match exact(TestDescriptor testDescriptor, Supplier<Set<? extends DiscoverySelector>> childSelectorsSupplier) // 2
Match partial(TestDescriptor testDescriptor) // 3
Match partial(TestDescriptor testDescriptor, Supplier<Set<? extends DiscoverySelector>> childSelectorsSupplier) // 4

The EngineDiscoveryRequestResolver calls childSelectorsSupplier only if the match is exact. This is the only usage of this property. This means that the factory methods (1), (3), and (4) are effectively identical. The second parameter of (4) is always ignored. This also means that Match.Type is effectively redundant.

childSelectorsSupplier

The childSelectorsSupplier is called when a descriptor shall be expanded. A descriptor is expanded if and only if the user supplied a selector which points to the descriptor or to a parent of the descriptor. Specifically, a descriptor is not expanded if the selector was only resolved over Context.resolve. Context.resolve is usually used to resolve a parent node of another node which was selected by the user.

We can now provide the same functionality by overloading the addMatch method I introduced above.

interface Context {
  ...
  void addMatch(TestDescriptor testDescriptor, Supplier<Set<? extends DiscoverySelector>> childSelectorsSupplier);
  ...
}

However, there is another point I find a bit irritating. Why do I have to use selectors here to identify the children? When you try to resolve the children, I think it is usually quite natural to also create the descriptors of the children. In fact, in my case I couldn't think of a sensible implementation which wouldn't follow the following steps:

1. Create the descriptor of the child node
2. Call getUniquieId() on the child node
3. Create a UniqueIdSelector and trow away the descriptor
4. Return the UniqueIdSelector back to JUnit
5. JUnit gives the UniqueIdSelector back to me by calling SelectorResolver.resolve
6. I re-create the descriptor from step 1 based on the unique ID

Why couldn't I just provide the descriptor to JUnit in step one? For example, the API could instead look like this:

interface Context {
  ...
  void addMatch(TestDescriptor testDescriptor, Consumer<Context> childResolver);
  ...
}

[mpkorstanje]

I had the same thoughts/problems when I looked using the EngineDiscoveryRequestResolver in Cucumber. And though I didn't get around to implementing using it yet (lack of time), I do think the API does make sense.

What you think of as your simplest example is the most complicated usecase. To resolve a unique id, each of the segments has to be resolved in turn. And this has to be done working backwards. However building the test descriptor tree has to be done working forwards.

For example suppose we have the uniqueId (paraphrased) OuterClass -> InnerClass -> Parameterized method -> 3rd value.

Then we need to do

1. Resolve 3rd value of a patermized method.
2. Resolve the method
3. Resolve the Inner class
4. Resolve the outer class
5. Build the descriptor for the OuterClass (if not already build) and add it to its parent (test engine)
6. Build the descriptor for the InnerClass (if not already build) and add it to its parent
7. Build the descriptor for the parameterized method (if not already build) and add it to its parent
8. Build the descriptor for the 3rd value and add it to its parent

This could be simplified by working forwards only, but that would require that each resolver knows about its possible descendants.

[mpkorstanje]

So the parent might not be resolvable. This means that you'd have to write something like:

@Override
public void resolve(UniqueIdSelector selector, Context context) {
  UniqueId uniqueId = selector.getUniqueId();
  Context parentContext = context.resolveContext(uniqueId.removeLastSegment());
  if (parentContext != null)
      parentContext.addMatch(new MyTestDescriptor(uniqueId), this::resolveChildren);
}

And that gets error prone quickly. So you'd use an optional.

@Override
public void resolve(UniqueIdSelector selector, Context context) {
  UniqueId uniqueId = selector.getUniqueId();
  Optional<Context> parentContext = context.resolveContext(uniqueId.removeLastSegment());
  parentContext.ifPresent(p -> p.addMatch(new MyTestDescriptor(uniqueId), this::resolveChildren));
}

But that gets repetitive too. So then you may as well write:

@Override
public void resolve(UniqueIdSelector selector, Context context) {
  UniqueId uniqueId = selector.getUniqueId();
  context.addToParent( ...how to resolve the parent...., parent -> .... how to create the child... )
}

And I believe similar arguments can be made for other aspects of the APIs complexity. Now you may again argue these features are not relevant for your example. And that is probably true. But then you may also not need this API for the use case expressed in that example.

You have the impression that the API is overly complicated for no apparent reason. Having implemented discovery selector resolution in Cucumber prior to the introduction of the EngineDiscoveryRequestResolver I went more or less down the path your example proposes. After having done so I can only say that I now see plenty of good reasons this API exists.

But I'm happy to agree to disagree on that.

[JojOatXGME]

So the parent might not be resolvable. This means that you'd have to write something like:

Technically, resolveContext(…) could also throw an exception. However, handling this case over the return value might be cleaner.

And that gets error prone quickly. So you'd use an optional.

That would be fine,

But that gets repetitive too. So then you may as well write:

Well, so far, I would still be mostly happy with this code. (This code still avoids 2.5 of the 3 points I complained about.)

And I believe similar arguments can be made for other aspects of the APIs complexity.

Well, I cannot argue with that. Maybe you are right, maybe not. For me, the API still feels overly complicated. I guess the only thing I could do is to provide a full API proposal. Not sure if I want to do that right now.

Now you may again argue these features are not relevant for your example. And that is probably true. But then you may also not need this API for the use case expressed in that example.

I think you are right that my initial example was inappropriate. It was ignoring most of the inherent complexity of the problem. The test engine I was working on is also more complex. I just took this very simple example to avoid making the post even longer. Not sure if there were a good other solution except creating a concrete API proposal with a long example.

PS: I wouldn't agree that EngineDiscoveryRequestResolver would not be needed in such simple cases. Such a simple engine without a hierarchy might still need to discover test classes. I think discovering the test classes from ModuleSelector, ClasspathRootSelector and PackageSelector is actually one of the main features of the API.

[mpkorstanje]

So I've found some time to somewhat finish cucumber/cucumber-jvm#2835 and use the EngineDiscoveryRequestResolver in Cucumber. If you're interested, the meaty bits are in: https://github.com/cucumber/cucumber-jvm/blob/dd135fa6554e213a23304e2287a3522db5229198/cucumber-junit-platform-engine/src/main/java/io/cucumber/junit/platform/engine/FeatureResolver.java

With everything fresh in my head, I think I can give you a demonstration of why this API works. Though I won't do a point by point rebuttal.

To start, I'm willing to grant that the API is complicated. But that's because the underlying goal is complicated.

And I think that can be illustrated with the following code:

Feature: A feature with scenario outlines

  Scenario: A scenario
    Given a scenario
    When it is executed
    Then is only runs once

  Scenario Outline: A scenario outline
    Given a scenario outline
    When it is executed
    Then <example> is used

    Examples: With some text
      | example |
      | A       |
      | B       |
      
    Examples: With some numbers
      | example |
      | 1       |
      | 2       |      

This is a feature file. It has some hierarchy in it. When the Feature is selected, everything should be executed. When the Scenario is the selected only that scenario should be selected. When the Scenario Outline is selected both Examples (groups) are executed and when Example is selected, the individual examples are executed. Ect, ect.

So suppose we want to execute Examples: With some text. This means we need to build a test descriptor hierarchy that covers:

Feature: A feature with scenario outlines
  Scenario Outline: A scenario outline
    Examples: With some text
      | A       |
      | B       |

And to target Examples: With some text Cucumber internally uses a FeatureElementSelector to do this. Which we resolve by using:

    public Resolution resolve(FeatureElementSelector selector, Context context) {
        Feature feature = selector.getFeature();
        Node selected = selector.getElement();
        return selected.getParent()
                .map(parent -> context.addToParent(() -> selectElement(feature, parent), createTestDescriptor(feature, selected)))
                .orElseGet(() -> context.addToParent(createTestDescriptor(feature, selected)))
                .map(descriptor -> Match.exact(descriptor, () -> selectElementsOf(feature, selected)))
                .map(Resolution::match)
                .orElseGet(Resolution::unresolved);
    }

So this element it turned into a test descriptor using createTestDescriptor(feature, selected) and Match.exact(descriptor, () -> selectElementsOf(feature, selected)) creates its children and recursively its descendants.

So far, I agree, this is on the face of it, overly complicated. Why separate all those things? Why not create the test descriptor and descendants at once?

But. We now create the test descriptors for ancestors of the element using:

context.addToParent(() -> selectElement(feature, parent), createTestDescriptor(feature, selected)))

Where selectElement creates a FeatureElementSelector for the parent of the element we selected. So we now recursively attempt to create the parent test descriptor. The difference here is that we now do not want to include the children of the parent, because we only want the particular one we have been resolving already. And because the match includes all call back to create the children of the parent, rather then the children itself, we can avoid doing so.

So to summarize, the EngineDiscoveryRequestResolver can be used to select and create certain nodes test descriptor hierarchy. To do so it must know for each node how to create its parents, and it must know how to create its children. It can then recursively create ancestors of the targeted node without creating any of those ancestors children, and it can recursively create any descendants of the target node.

All by invoking the same block of code. But this means that the functions to select the parent, create the test descriptor, and create its children must be provided separately.

Hence the complexity.

Additionally, when we want to select multiple elements in a feature file. The EngineDiscoveryRequestResolver can reuse work done by one selector and use it for another. And looking back at my original implementation prior to using the EngineDiscoveryRequestResolver I had some really ugly code that would for a given selector create the entire tree, filter it, and recursively merge test descriptors. This is definitely the better solution.

I hope that helps you understand the API better.

[mpkorstanje]

I realize this didn't really cover partial matches. But there is a use-case for them in dynamic tests. The JUnit code that uses is probably worth looking at.

[JojOatXGME]

Thanks for your explanation. I still think an API could archive all of that while being simpler. I actually spend a bit of time to draft an alternative API two days ago. I added it to the bottom of this post. However, since I don't think that the API will actually change, and because a meaningful example would easily take 100 to 200 lines, I did not have the motivation to demonstrate its usage in comparison to the current API. However, I think it captures all the use cases.

I realize this didn't really cover partial matches. But there is a use-case for them in dynamic tests. The JUnit code that uses is probably worth looking at.

I had looked at the source code. As far as I could see, the only difference is that partial matches will not be expanded. If a user supplies a ClassSelector and you have a partial match on the class, EngineDiscoveryRequestResolver will ignore the children you supplied. In other words, a partial match is equal to an exact match which does not have an childSelectorsSupplier. In other words, partial matches are completely redundant. This was point 2 in my initial post.

Anyway, here is my draft of an API proposal. However, without a demonstration, it is probably not really that convincing.

Click here to show the proposed API…

EngineDiscoveryRequestResolver.java

Replace the following methods in EngineDiscoveryRequestResolver.Builder

public Builder<T> addSelectorResolver(SelectorResolver resolver) {
  ...
}

public Builder<T> addSelectorResolver(Function<InitializationContext<T>, SelectorResolver> resolverCreator) {
  ...
}

with

public Builder<T> addSelectorResolver(SelectorResolver<? super T> resolver) {
  ...
}

public Builder<T> addSelectorResolver(Function<InitializationContext<? extends T>, SelectorResolver<? super T>> resolverCreator) {
  ...
}

SelectorResolver.java

Replace the whole class with the following:

/** @param <T> the type of the engine's descriptor */
public interface SelectorResolver<T extends TestDescriptor> {

  default void resolve(ClassSelector selector, Context<T> context) {
    resolve((DiscoverySelector) selector, context);
  }

  default void resolve(MethodSelector selector, Context<T> context) {
    resolve((DiscoverySelector) selector, context);
  }

  default void resolve(UniqueIdSelector selector, Context<T> context) {
    resolve((DiscoverySelector) selector, context);
  }

  // ... and all the other `resolve` methods ...

  default void resolve(DiscoverySelector selector, Context<T> context) {
  }

  interface Context<T extends TestDescriptor> {
    /** Returns the parent to which you may add a child.
     * The engine's descriptor for the resolve-methods in {@link SelectorResolver}. */
    T getParent();

    /** Adds the given descriptor as a child to {@link #getParent()}. */
    void add(TestDescriptor descriptor);

    /** Adds the given descriptor as a child to {@link #getParent()}.
     * {@code childResolver} is called with the given descriptor as the parent
     * if the descriptor shall be expanded. */
    <U extends TestDescriptor> void add(U descriptor, Consumer<Context<U>> childResolver);

    /** Resolves {@code parentSelector} and calls {@code childResolver} with the
     * resolved descriptor as the parent. If {@code parentSelector} cannot be
     * resolved, {@code childResolver} will be ignored. */
    void addToParent(DiscoverySelector parentSelector, Consumer<Context<TestDescriptor>> childResolver);

    /** Same as in the actual API. */
    Optional<TestDescriptor> resolve(DiscoverySelector selector);
  }
}

By the way, just in case someone is interested. In my project, I ended up designing a more high-level API and some clue-code. There is one method which defines the hierarchy for a test class using a builder. (It gets the data from the test class and returns a structure which is then used by my clue code to resolve the specific tests lazily.) My hierarchy was rather complex. Code to manually resolve all the different cases would have been thousands of lines. So, I actually reduced the amount of code significantly by building this “framework”.

See my simplified example of how I define the hierarchy right now…

This code is a simplified example. I could not provide you my actual code. A short explanation. The first parameter of child or children is the segment type used in the unique ID. The segment type used by a children method must be unique. Multiple child calls can use the same segment type, but then they must use different segment values. For child, the second parameter is the segment value. For children, the second parameter is a ResolvableCollection. A resolvable collection is a collection which allows selecting individual items using selectors. (However, in my case, I only care about UniqueIdSelector on a level below classes.)

private DemoTestDescriptor resolve(DemoTestData test, UniqueId parentId) {
  return DemoTestDescriptor
      .container(test.getTestClass().getSimpleName())
      .source(ClassSource.from(test.getTestClass()))
      .child("static", "static", () -> DemoTestDescriptor
          .container("Static")
          .source(ClassSource.from(test.getTestClass()))
          .children("validation", test.getValidations(), validation -> DemoTestDescriptor
              .test(validation.method().getName())
              .source(MethodSource.from(validation.method()))
              .run(() -> runTest(validation))
          )
      )
      .children("setting", test.getSettings(), setting -> DemoTestDescriptor
          .container("Change " + setting.clazz().getSimpleName())
          .source(ClassSource.from(setting.clazz()))
          .children("change", ResolvableCollection.crossProduct(setting.options(), setting.options()), change -> DemoTestDescriptor
              .container(change.get(0).method().getName() + " -> " + change.get(1).method().getName())
              .source(CompositeTestSource.from(List.of(MethodSource.from(change.get(0).method()), MethodSource.from(change.get(1).method()))))
              .children("validation", test.getValidations(), validation -> DemoTestDescriptor
                  .test(validation.method().getName())
                  .source(MethodSource.from(validation.method()))
                  .run(() -> runTest(change, validation))
              )
          )
      )
      .build(parentId.append(SEGMENT_TYPE_CLASS, test.getTestClass().getName()));
}