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

Support JUnit 5 #11

Closed
jbduncan opened this issue Jul 24, 2021 · 27 comments
Closed

Support JUnit 5 #11

jbduncan opened this issue Jul 24, 2021 · 27 comments

Comments

@jbduncan
Copy link

I quite like the look of this API (well done, @nymanjens!). But I'm a JUnit 5 user, so if I wanted to use this library, I'd have to port it to a JUnit 5 extension, or use a potentially more verbose alternative.

Is this something that's been considered already, by any chance?

@nymanjens
Copy link
Collaborator

Hi Jonathan,

At Google, we almost exclusively use JUnit4, which is why we haven't felt this need ourselves yet. But we did anticipate this being requested once we open source the project.

Looks like you are the first one requesting this :-). I'll look into how to do this when I get some time.

If anyone has pointers on how this is best done, that would be appreciated :-).

@jbduncan
Copy link
Author

@nymanjens Thanks for acknowledging this so quickly!

Re. pointers, JUnit 5 provides individual extension points for various parts of the test lifecycle, rather than a super-flexible test runner or rule like JUnit 4 does. There is an official User Guide with a comprehensive reference on the various APIs. But I've found it more helpful to use baeldung's guide first, and then read the User Guide and javadocs or study an existing extension like those in junit-pioneer to get a better feel on what can be done and how to do things.

I hope this helps!

@jbduncan
Copy link
Author

For those of you looking for a library like TestParameterInjector for JUnit 5 in the meantime, consider using JUnit 5's parameterized tests, junit-pioneer's @CartesianProductTest , or both.

@nymanjens nymanjens changed the title Consider porting to JUnit 5 Support JUnit 5 Jul 26, 2021
@nymanjens
Copy link
Collaborator

nymanjens commented Aug 9, 2021

I've investigated this a bit. My ideal scenario would be to have something like this:

// Old code
@RunWith(TestParameterInjector.class)
public class MyTest {

  @TestParameter boolean isDryRun;

  @Test public void test1(@TestParameter boolean enableFlag) {
    // ...
  }
}

// New code
@ExtendWith(TestParameterInjector.class)
public class MyTest {

  @TestParameter boolean isDryRun;

  @Test public void test1(@TestParameter boolean enableFlag) {
    // ...
  }
}

i.e. replacing @RunWith by @ExtendWith, keeping everything else the same.

It looks like that's not possible because the only extension interface I could find that allows multiple runs is TestTemplateInvocationContextProvider, which seems to need a custom annotation.

So the next best API I can think of would look like this:

@ExtendWith(TestParameterInjector.class)
public class MyTest {

  @TestParameter boolean isDryRun;

  @TestParameterInjector.Test
  public void test1(@TestParameter boolean enableFlag) {
    // ...
  }


  @TestParameterInjector.Test
  public void test2() {
    // ...
  }
}

@jbduncan
Copy link
Author

jbduncan commented Aug 9, 2021

@nymanjens Ah, that's a pain.

Alternatively you may be able to make a custom "source" for @ParameterizedTest as per this guide.

public class MyTest {

  @TestParameter boolean isDryRun;

  @ParameterizedTest
  @TestParameterInjectorSource
  public void test1(@TestParameter boolean enableFlag) {
    // ...
  }

  @ParameterizedTest
  @TestParameterInjectorSource
  public void test2() {
    // ...
  }
}

On its own, this wouldn't any better. But you may be able to create a new annotation like @TestParameterInjector.Test that is meta-annotated with @ParameterizedTest and @TestParameterInjectorSource. I've never tried this before, so I don't know if JUnit 5 would recognise it, but if it does, then you could do something like this:

public class MyTest {

  @TestParameter boolean isDryRun;

  @TestParameterInjector.Test
  public void test1(@TestParameter boolean enableFlag) {
    // ...
  }

  @TestParameterInjector.Test
  public void test2() {
    // ...
  }
}

...which looks more similar to the JUnit 4-based API to me.

@jbduncan
Copy link
Author

jbduncan commented Aug 9, 2021

The JUnit 5 maintainers will have more insight on this than me, so try raising an issue on the JUnit 5 issue tracker to see if using @ExtendWith and @Test like your ideal scenario is possible, or could be made possible. They're very receptive, in my experience.

@nymanjens
Copy link
Collaborator

Good point. Actually, by adding @ExtendWith(...) to @TestParameterInjector.Test, we can avoid a class-level extension altogether.

So this is the new proposed API then:

public class MyTest {

  @TestParameter boolean isDryRun;

  @TestParameterInjector.Test
  public void test1(@TestParameter boolean enableFlag) {
    // ...
  }

  @TestParameterInjector.Test
  public void test2() {
    // ...
  }
}

@jbduncan
Copy link
Author

@nymanjens Actually, it looks like we might be able to make this even more concise.

In the next release of JUnit 5 - 5.8 - @ExtendWith will be able to be applied to fields and parameters. (See their ongoing feature branch for more technical info if you're interested.)

So the following annotation will be possible:

@Target({ ElementType.FIELD, ElementType.PARAMETER })
@Retention(RetentionPolicy.RUNTIME)
@ExtendWith(TestParameterInjectorExtension.class)
public @interface TestParameter {
}

And in turn your example API usage can be shortened to:

public class MyTest {

  @TestParameter boolean isDryRun;

  @Test
  public void test1(@TestParameter boolean enableFlag) {
    // ...
  }

  @Test
  public void test2() {
    // ...
  }
}

This makes it just like TestParameterInjector's JUnit 4-based API, minus a @RunWith(TestParameterInjector.class) usage! So I'd say waiting for JUnit 5.8 is the best move.

@nymanjens
Copy link
Collaborator

Cool, thanks for the info. That sounds ideal :-D

@TWiStErRob
Copy link

https://junit.org/junit5/docs/snapshot/release-notes/#release-notes-5.8.0

5.8 has landed the feature necessary, is there ongoing work?

@nymanjens
Copy link
Collaborator

This is on my TODO list, but I haven't gotten around to this yet.

@nymanjens
Copy link
Collaborator

Also, thanks for letting me know about the feature landing :-)

@nymanjens
Copy link
Collaborator

I've investigated this some more. My conclusions:

  • The following JUnit5 extension interfaces are relevant for this project:
    • TestTemplateInvocationContextProvider: For creating multiple tests for a single method. This is key because it's the only extension mechanism that I could find that can increase the amount of test invocations. So this has to be called for every test method, even if the method itself isn't parameterized (because there might be field/constructor parameterization)
    • TestInstanceFactory: For calling a constructor with parameters
    • TestInstancePostProcessor: For populating parameterized fields (can be omitted if TestInstanceFactory is also used for the default constructor)
  • TestTemplateInvocationContextProvider is only called when the test method is annotated with a custom annotation (with the @TestTemplate annotation). This means that the solution proposed in an earlier comment (Support JUnit 5 #11 (comment)) won't work because it uses the generic @Test. Instead, we'll need an @Test variant for this library, e.g. @TestParameterInjectorTest
    • I tested this on version 5.8.1. I think the new feature request doesn't help here because it doesn't seem to apply to TestTemplateInvocationContextProvider (the examples are usually about ParameterResolver)
  • TestInstanceFactory and TestInstancePostProcessor only work if the class is annotated with @ExtendWith(TestParameterInjectorExtension.class) (it doesn't matter if all tests are annotated with @TestParameterInjectorTest or fields/parameters/constructors are annotated with @TestParameter)
  • It's likely going to be a requirement that the JUnit5 tests don't depend on JUnit4 and vice versa (example issue).
    • So any JUnit5 solution won't be able to use TestParameterInjector because it extends from the JUnit4 runner.
    • Likewise, @TestParameter[s] shouldn't be annotated with @ExtendWith, because that's a JUnit5 dependency. Given the above, that's not really needed anyway.

So given the constraints above, this is the new proposed API:

@ExtendWith(TestParameterInjectorExtension.class)
class MyTest {

    @TestParameter boolean field;

    @TestParameterInjectorTest
    void myTest() { ... }

    @TestParameterInjectorTest
    @TestParameters2({"{a: 1}", "{a: 2}"})
    void withParameters_success(int a) { ... }

    @TestParameterInjectorTest
    void withParameter_success(@TestParameter boolean b, @TestParameter boolean c) { ... }
}

@TWiStErRob
Copy link

Nice investigation!

Mind you that "extendswith" is additive and there could be other ParameterResolvers (e.g. get a spring context, or create a DB session). With that, @TestParameterInjectorTest should make sure to work with other extensions.

@ExtendWith(TestParameterInjectorExtension.class)
@ExtendWith(OtherExtension.class)
class MyTest {

    @TestParameterInjectorTest
    @TestParameters2({"{a: 1}", "{a: 2}"})
    void withParameters_success(int a, @Something Type injected) { ... }

(Quick note: TPIT is pretty a mouthful, imagine every developer having to write that every time they write a test.)

@TWiStErRob
Copy link

re JUnit 4 v 5, I totally agree testing helpers extending JUnit rules is a plague when it comes to pure JUnit 5 tests.
I think what the approach should be is to have a test-framework agnostic clear API and then a separate junit4 and a jupiter artifact adding the wiring on top of that (this opens the possibility of using the a library with other test frameworks too), an example of such a refactor cashapp/paparazzi#293, sadly usually these are breaking changes because LibraryMainEntry and LibraryMainEntryRule are the same class, and they need to split. I guess in the case of this project, it's quite coupled with JUnit 4 even apart from the API surface?

@nymanjens
Copy link
Collaborator

I think what the approach should be is to have a test-framework agnostic clear API and then a separate junit4 and a jupiter artifact

Yep, that's the plan. I'm currently refactoring the implementation to make it JUnit4-agnostic. The public types @TestParameter and @TestParameters can be shared because they don't reference any framework. The public type TestParameterInjector will remain tied to JUnit4 without a breaking change that would have too big of an impact I think.

(Quick note: TPIT is pretty a mouthful, imagine every developer having to write that every time they write a test.)

I agree it's a mouthful, and I'm open to alternative suggestions. I've considered @TpiTest, but we're usually no fans of acronyms and strong abbreviations.

@TWiStErRob
Copy link

Yeah, not a fan of abbreviations either, I had some thinking and couldn't come up with any better name 😢.

Just found a new thing, regarding @ExtendWith(TestParameterInjectorExtension.class) on the class. I think that might not be necessary in some cases, for example @ParameterizedTest is annotated with @ExtendsWith and a single annotation carries the necessary setup.

@nymanjens
Copy link
Collaborator

nymanjens commented Nov 29, 2021

I think that might not be necessary in some cases

As I understand it, if you don't add @ExtendWith, you can't support constructor or field parameterization, only method parameterization. I think that's OK for @ParameterizedTest because it only supports method parameters.

Method parameterization is probably the most commonly used one, but field parameterization is definitely occasionally useful, and used enough to be confusing if it weren't supported. It could work by only requiring @ExtendWith when using field/constructor parameterization, but that just feels like it's going to lead to a lot of NullPointerException confusion.

@TWiStErRob
Copy link

TWiStErRob commented Nov 29, 2021

I had a play and this is what I learned:

  • The meta-annotated @TPIT's @ExtendWith propagates to class from test method, so it actually works without any @ExtendWith. (you said that for templating all test methods have to be annotated.)
  • TestInstancePostProcessor works based on @TPIT on method, but not with @Test (good)
  • Constructor parameter injection is handled by ParameterResolver if the parameters are annotated rather than the constructor. This means the TestInstanceFactory might not be necessary, unless it's a requirement for templating.
  • During implementation mind @Nested and @TestFactory (dynamicTest) features. Also can it be compatible with @RetryingTest (pioneer) and @RepeatedTest (jupiter).
  • Have a look at JUnit Pioneers' @CartesianProductTest.
  • Jupiter extension APIs are awesome!
annotation class TP

@ExtendWith(TPIE::class)
@Test
annotation class TPIT
TestParameterInjectorExtension impl
import org.junit.jupiter.api.extension.BeforeEachCallback
import org.junit.jupiter.api.extension.ExtensionContext
import org.junit.jupiter.api.extension.ParameterContext
import org.junit.jupiter.api.extension.ParameterResolver
import org.junit.jupiter.api.extension.TestInstanceFactory
import org.junit.jupiter.api.extension.TestInstanceFactoryContext
import org.junit.jupiter.api.extension.TestInstancePostProcessor

class TPIE : BeforeEachCallback, TestInstancePostProcessor, ParameterResolver/*, TestInstanceFactory*/ {
//	override fun createTestInstance(factoryContext: TestInstanceFactoryContext, extensionContext: ExtensionContext): Any? {
//		val ctor = factoryContext.testClass.constructors.single()
//		val outer = factoryContext.outerInstance.map { listOf(it) }.orElse(emptyList())
//		val args = ctor.parameters.drop(outer.size).map { "ctor arg from ext for ${it.name}" }
//		return ctor.newInstance(*(outer + args).toTypedArray())
//	}

	override fun postProcessTestInstance(testInstance: Any, context: ExtensionContext) {
		println("postProcessTestInstance ${testInstance}")
		testInstance::class.java.declaredFields
			.filter { it.isAnnotationPresent(TP::class.java) }
			.forEach { it.set(testInstance, "field from ext") }
	}

	override fun beforeEach(context: ExtensionContext) {
		println("beforeEach ${context.displayName}")
	}

	override fun supportsParameter(parameterContext: ParameterContext, extensionContext: ExtensionContext): Boolean =
		parameterContext.isAnnotated(TP::class.java)

	override fun resolveParameter(parameterContext: ParameterContext, extensionContext: ExtensionContext): Any =
		"param from ext"
}
Test examples

(ignore the @Nested inner class part, it was easier to hack it together self-enclosed)

import org.junit.jupiter.api.Nested

class TPITTests {
	@Nested	inner class OnParam {
		@TPIT fun test(@TP param: String) {
			println(param) // param from ext
		}
	}

	@Nested	inner class OnField {
		@field:TP lateinit var field: String

		@TPIT fun test() {
			println(field) // field from ext
		}
	}

	@Nested inner class OnCtor constructor(
		@param:TP private val ctor: String
	) {
		@TPIT fun test() {
			println(ctor) // param from ext
		}
	}

	@Nested inner class Combo constructor(
		@param:TP private val ctor: String
	) {
		@field:TP lateinit var field: String

		@TPIT fun test(@TP param: String) {
			println("ctor=$ctor field=$field param=$param") // ctor=param from ext field=field from ext param=param from ext
		}
	}

	@Nested inner class Mixed {
		@field:TP lateinit var field: String

		@TPIT fun test(@TP param: String) {
			println("field=$field param=$param") // field=field from ext param=param from ext
		}

		@Test fun normal() {
			println(::field.isInitialized) // false
		}

		@Test fun mismatch(@TP param: String) {
			// error: No ParameterResolver registered for parameter
		}
	}
}

@jbduncan
Copy link
Author

Excellent investigations, both! It's a bit gutting to see that we cannot use @Test directly, but it's good to know.

And hopefully there's a way we can avoid forcing users to use @ExtendsWith(...)... Fingers crossed.🤞 Worst case scenario, we could make TestParameterInjectorExtension it's own meta-annotation annotated with @ExtendsWith(TPIE.class), perhaps. Or even introduce another TestParameterInjector class under the JUnit 5 artifact, separate from the JUnit 4 one.

@jbduncan
Copy link
Author

jbduncan commented Nov 29, 2021

As for acronyms for @TestParameterInjectorTest, if we went for my idea of a separate, JUnit 5-specific TestParameterInjector type, we could then introduce @TestParameterInjector.Test (note the dot), so that people can explicitly import it like so:

import com.google.testing.junit.jupiter.testparameterinjector.TestParameterInjector
import com.google.testing.junit.jupiter.testparameterinjector.TestParameterInjector.Test
...

@TestParameterInjector
class MyTest {

    @TestParameter boolean field;

    @Test
    void myTest() { ... }

    @Test
    @TestParameters2({"{a: 1}", "{a: 2}"})
    void withParameters_success(int a) { ... }

    @Test
    void withParameter_success(@TestParameter boolean b, @TestParameter boolean c) { ... }
}

A disadvantage is users could easily get @TestParameterInjector.Test confused with JUnit 5's @Test, I suppose.

@TWiStErRob
Copy link

TWiStErRob commented Nov 29, 2021

Careful, remember that code is read more than written. It can be easily confused, agreed; also you would get into problems with automatic formatters (forcing qualified usages), or wanting to use parameterized and non-paramterized tests in the same class.

The same applies for a meta-annotated extension annotation. Does it actually bring benefits? Having 4 ExtendsWith on the class is cleaner than 4 random annotations that might or might not extend.

@jbduncan
Copy link
Author

I think the @ExtendsWith is where we'll have to agree to disagree, as I'm using a meta-annotation to hide my extension class name in my new extension for JUnit Pioneer, which IMO looks neater.

But I agree with everything else you've said. :)

@TWiStErRob
Copy link

TWiStErRob commented Nov 29, 2021

Might be an unrelated conversation here, @nymanjens feel free to collapse this.

At https://github.com/junit-pioneer/junit-pioneer/pull/491/files#diff-7b1c59658c736ae3c2131a2425f2f8c88f264a605dd0c0a14dee256d665860c9R18-R21 @jbduncan is double-indirecting the @ExtendWith which is perfectly fine (nice contrib BTW!), because this annotation is meant for a specific parameter (supportsParameter). What I was saying is that one should not just introduce:

@ExtendsWith(SomethingExtension::class)
annotation class Something

for the sake of it, so that you can write

@Something
class MyTest

unless it adds extra value, for example parameters, or more meta-annotations; before then, it's yagni.

Note that TPIT is there to stay (whatever its name) because of the templating constraints (see point 2 in #11 (comment))

@jbduncan
Copy link
Author

@TWiStErRob Good argument re. my meta-annotation idea not adding anything extra behaviour-wise, I'm convinced!

@nymanjens
Copy link
Collaborator

@TWiStErRob Thanks for your investigation! I confirm your conclusions: The ParameterResolver is indeed also used for constructor parameters, which makes things a lot easier.

TestTemplateInvocationContextProvider and TestInstancePostProcessor seem to be sufficient to cover the following use cases:

Proposed API
class Junit5Test {

    private final boolean constructorParam;

    @TestParameter boolean field;

    public Junit5Test(@TestParameter boolean x){
        this.constructorParam = x;
    }

    @TestParameterInjectorTest
    void withoutParameters() {
        System.out.printf(">>>> JUnit5Test(%s).withoutParameters()\n", constructorParam);
    }

    @TestParameterInjectorTest
    @TestParameters({"{s: false}", "{s: true}"})
    void withParameters_success(boolean s) {
        System.out.printf(">>>> JUnit5Test(%s).withParameters_success(%s)\n", constructorParam, s);
    }

    @TestParameterInjectorTest
    void withParameter_success(@TestParameter boolean b) {
        System.out.printf(">>>> JUnit5Test(%s).withParameter_success(%s)\n", constructorParam, b);
    }
}

Questions I'm still investigating:

  • How is this going to work with nested classes? It feels like some API decisions around field parameters and constructors is going to have to be made here. Maybe for a first version this should all be unsupported to keep things manageable.
  • How do I set a custom test name? Currently the test names are in the following form, which is terrible:
withParameter_success{boolean}[1]
withParameter_success{boolean}[2]
withParameter_success{boolean}[3]
withParameter_success{boolean}[4]
  • What safeguards should I add to reduce the conflicts with other extensions to a minimum?
    • In my local test, I'm always returning true on TestTemplateInvocationContextProvider.supportsTestTemplate() and ParameterResolver.supportsParameter() (see TestParameterInjectorExtension implementation below). It feels like fully claiming TestTemplateInvocationContextProvider for methods annotated with @TestParameterInjectorTest makes sense though.
TestParameterInjectorExtension high level implementation
class TestParameterInjectorExtension implements TestTemplateInvocationContextProvider, TestInstancePostProcessor {

    @Override
    public void postProcessTestInstance(Object testInstance, ExtensionContext context) throws Exception {
        // TODO
    }

    @Override
    public boolean supportsTestTemplate(ExtensionContext context) {
        return true;
    }

    @Override
    public Stream<TestTemplateInvocationContext> provideTestTemplateInvocationContexts(ExtensionContext context) {
        // Returns a stream of TpiTestInvocationContexts, based on the annotated methods
    }


    private static class TpiTestInvocationContext implements TestTemplateInvocationContext {

        private final TestInfo testInfo; // package-private type that holds the parameters and the test name

        @Override
        public String getDisplayName(int invocationIndex) {
            // TODO
        }

        @Override
        public List<Extension> getAdditionalExtensions() {
            return ImmutableList.of(new TpiResolver());
        }

        class TpiResolver implements ParameterResolver {
            @Override
            public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext) {
                return true;
            }

            @Override
            public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext) {
                // TODO
            }
        }
    }

}

nymanjens added a commit that referenced this issue Dec 8, 2021
nymanjens added a commit that referenced this issue Dec 10, 2021
nymanjens added a commit that referenced this issue Dec 15, 2021
nymanjens added a commit that referenced this issue Dec 21, 2021
…combining abstraction of all processors.

See #11.
nymanjens added a commit that referenced this issue Jan 5, 2022
… that don't depend on JUnit4.

This is a prerequisite for #11.
nymanjens added a commit that referenced this issue Jan 14, 2022
See #11

Notable things that are missing in this first version:
- @nested support
- Support for injecting other types, such as JUnit5's TestInfo
@nymanjens
Copy link
Collaborator

I've just pushed a commit tha adds basic support for JUnit5 in new Maven target.

Notable things that are missing in this first version:

  • @nested support
  • Support for injecting other types, such as JUnit5's TestInfo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants