Simplify implementation of self-contained dynamic tests

[odrotbohm]

@TestFactorys and DynamicTests provide API to implement multiple test probes that will become individual tests. They're centered around the idea that one would create test fixtures as instances of types and provide API to then define the naming of the test and the actual assertion:

class MyTests

  @TestFactory
  Stream<DynamicTest> foo() {

    var fixture = Stream.of(new Foo(1, 1, 2), new Foo(2, 3, 5));

    return DynamicTest.stream(fixture, it -> assertThat(it.left() + it.right()).isEqualTo(it.result()));
  }

  record Foo(int left, int right, int result) implements Named<Foo> {

    @Override
    public String getName() {
      return left + " + " + right + " = " + result;
    }

    @Override
    public Foo getPayload() {
      return this;
    }
  }
}

There are a few things to note here:

• We need to implement getPayload() to return the instance itself to satisfy Named although that method doesn't play into the signature of DynamicTest.stream(…) as the elements of the stream are handed into the callable.
• It would be nice if we were able to colocate the assertion callback with the fixture type so that the implementation of the test factory would only have to return a stream of fixtures.

Assuming an extension of Named looking like this existed:

interface NamedExecutable<T extends NamedExecutable<T>> extends Named<T>, Executable {

  @Override
  public default T getPayload() {
    return (T) this;
  }
}

a bit of syntactical sugar on DynamicTest:

@SafeVarargs
public static <T extends NamedExecutable<T>> Stream<DynamicTest> stream(
    NamedExecutable<T>... inputGenerator) {
  return stream(Arrays.stream(inputGenerator));
}

public static <T extends NamedExecutable<T>> Stream<DynamicTest> stream(
    Stream<? extends NamedExecutable<T>> inputGenerator) {
  Preconditions.notNull(inputGenerator, "inputGenerator must not be null");

  return DynamicTest.stream(inputGenerator, it -> it.execute());
}

Would allow us to reduce the very first code sample to:

public class DummyTest {

  @TestFactory
  Stream<DynamicTest> foo() {
    return DynamicTest.stream(new AdderTest(1, 1, 2), new AdderTest(2, 3, 5));
  }

  record AdderTest(int left, int right, int result) implements NamedExecutable<AdderTest> {

    @Override
    public String getName() {
      return left + " + " + right + " = " + result;
    }

    @Override
    public void execute() throws Throwable {
      assertThat(left + right).isEqualTo(result);
    }
  }
}

This allows to define parameterize test fixtures nicely and colocate the assertions within a type and the test factory method's responsibility is reduced to set up the individual probes.

[sormuras]

Executable in interface NamedExecutable<T extends NamedExecutable<T>> extends Named<T>, Executable is https://junit.org/junit5/docs/current/api/org.junit.jupiter.api/org/junit/jupiter/api/function/Executable.html - right?

[odrotbohm]

Yes, @sbrannen and me brainstormed the interface and additional methods in a 1:1 yesterday.

[sormuras]

@FunctionalInterface
interface NamedExecutable<T extends NamedExecutable<T>> extends Named<T>, Executable {
  @Override
  public String getName() {
    return this instanceof Record ? toString() : "Override getName() to generate a better name for " + this;
  }

  @Override
  public default T getPayload() {
    return (T) this;
  }
}

🤔

[odrotbohm]

Great idea. I usually define an explicit name as I value properly readable test names, but I agree that a record's toString() would be a sensible default.

[sbrannen]

this instanceof Record would have to be replaced with some reflective hacks since we have a Java 8 baseline.

[sbrannen]

Please note that the name of the NamedExecutable interface is just an example.

We may well want to come up with a more descriptive name if we implement this feature.

[sormuras]

this instanceof Record would have to be replaced with some reflective hacks since we have a Java 8 baseline.

✨Multi-release JAR magic to the rescue.✨

Or lifting the baseline to 17...

[marcphilipp]

Team decision: Add <T extends NamedExecutable<T>> Stream<DynamicTest> stream methods along with NamedExecutable interface with a default implementation of getName() that returns toString().

[etrandafir93]

Hello @marcphilipp , @odrotbohm, I would enjoy contributing here if it is needed.

I'll try to summarize the discussion and ask a short question at the end. At the moment there is a static factory method for building a stream of DynamicTests, overloaded 4 times:

public static <T> Stream<DynamicTest> stream(
        Iterator<T> inputGenerator,
        Function<? super T, String> displayNameGenerator, 
        ThrowingConsumer<? super T> testExecutor
    ) { ... }

public static <T> Stream<DynamicTest> stream(
        Stream<T> inputStream,
        Function<? super T, String> displayNameGenerator, 
        ThrowingConsumer<? super T> testExecutor
    ) { ... }   

public static <T> Stream<DynamicTest> stream(
        Iterator<? extends Named<T>> inputGenerator,
        ThrowingConsumer<? super T> testExecutor
    ) { ... }

public static <T> Stream<DynamicTest> stream(
        Stream<? extends Named<T>> inputStream,
        ThrowingConsumer<? super T> testExecutor
    ) { ... }

As I understand, for this issue, the idea would be to add two more methods here. They will be using the new interface NamedExecutable that encapsulates all 3 parameters (the data, the names and the test executor functions):

public static <T extends NamedExecutable<T>> Stream<DynamicTest> stream(
        NamedExecutable<T>... inputGenerator
    ) { ... }

public static <T extends NamedExecutable<T>> Stream<DynamicTest> stream(
        Stream<? extends NamedExecutable<T>> inputGenerator
    ) { ... }

Since the current api exposes methods accepting Stream and Interator, should it be the same for the new ones for consistency reasons? I mean having Iterable<? extends NamedExecutable<T> instead of the var args? or having all 3 options?

Let me know what you think and if I can try looking into it. Cheers, Emanuel

[marcphilipp]

I've simplified the proposed implementation in #3720 to remove the self type parameter.

Instead of

interface NamedExecutable<T extends NamedExecutable<T>> extends Named<T>, Executable {
  @SuppressWarnings("unchecked")
  @Override
  public default T getPayload() {
    return (T) this;
  }
}

it's now

interface NamedExecutable extends Named<Executable>, Executable {
  @Override
  public default Executable getPayload() {
    return this;
  }
}

which is simpler to implement.

Moreover, I've changed the signature of the new DynamicTest.stream method to be more generic so it allows for other Named<E> implementations where E extends Executable:

<T extends Named<E>, E extends Executable> Stream<DynamicTest> stream(
            Stream<? extends T> inputStream) {...}

@odrotbohm @sbrannen @sormuras @juliette-derancourt @mmerdes @mobounya Any objections?

[odrotbohm]

Is that something I would be able to find in some snapshots already? I see a few commits related to the ticket but can't find the described code in 5.11 snapshots. 🤔

[marcphilipp]

@odrotbohm The latest 5.11.0-SNAPSHOT versions in https://oss.sonatype.org/content/repositories/snapshots now have the changes. Looking forward to your feedback! :slightly_smiling_face:

[odrotbohm]

Congrats on the release. The additions work nicely! 🙇