Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify implementation of self-contained dynamic tests #3261

Open
odrotbohm opened this issue Apr 19, 2023 · 9 comments · May be fixed by #3720
Open

Simplify implementation of self-contained dynamic tests #3261

odrotbohm opened this issue Apr 19, 2023 · 9 comments · May be fixed by #3720

Comments

@odrotbohm
Copy link

@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
Copy link
Member

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
Copy link
Author

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

@sormuras
Copy link
Member

@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
Copy link
Author

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
Copy link
Member

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

@sbrannen
Copy link
Member

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
Copy link
Member

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
Copy link
Member

marcphilipp commented Jun 7, 2023

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
Copy link

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

mobounya added a commit to mobounya/junit5 that referenced this issue Jan 20, 2024
Introduce new interface NamedExecutable that associates a name, a
payload and an Executable.

This was introduced to help colocate test's diplayName, payload, and
testExecutor in one place. Concrete usage of this interface is in the
next commit.

Issue: junit-team#3261
mobounya added a commit to mobounya/junit5 that referenced this issue Jan 20, 2024
Introduce two methods in the class DynamicTest to generate a stream of
DynamicTests from a Stream/Iterator of NamedExecutables.

Instead of passing an inputStream, a displayNameGenerator and a
testExecutor separately, these two additional methods will allow you
to pass one Iterator/Stream of a NamedExecutable.

Unlike the Named interface, the NamedExecutable can colocate assertion
callbacks in one container using the execute method.

Issue: junit-team#3261
mobounya added a commit to mobounya/junit5 that referenced this issue Jan 20, 2024
Write new changes to the release notes for 5.11M1 and add a simple demo
in DynamicTestsDemo.

Issue: junit-team#3261
mobounya added a commit to mobounya/junit5 that referenced this issue Jan 20, 2024
Introduce new interface NamedExecutable that associates a name, a
payload and an Executable.

This was introduced to help colocate test's diplayName, payload, and
testExecutor in one place. Concrete usage of this interface is in the
next commit.

Issue: junit-team#3261
mobounya added a commit to mobounya/junit5 that referenced this issue Jan 20, 2024
Introduce two methods in the class DynamicTest to generate a stream of
DynamicTests from a Stream/Iterator of NamedExecutables.

Instead of passing an inputStream, a displayNameGenerator and a
testExecutor separately, these two additional methods will allow you
to pass one Iterator/Stream of a NamedExecutable.

Unlike the Named interface, the NamedExecutable can colocate assertion
callbacks in one container using the execute method.

Issue: junit-team#3261
mobounya added a commit to mobounya/junit5 that referenced this issue Jan 20, 2024
Write new changes to the release notes for 5.11M1 and add a simple demo
in DynamicTestsDemo.

Issue: junit-team#3261
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment