Junit5: TestInstanceFactory on enclosing class is not called for @Nested test class

[seanf]

Overview

If I use nested classes together with a TestIntanceFactory on the outer class, the instance factory is not called for the inner class, unless a different factory class is used.

I would expect that I can use the same factory on the outer class and the inner class, and have it called both for outer and inner instances. Ideally the factory on the outer class would be "inherited" by the inner class automatically. (It's not clear from the docs if extensions are meant to be propagated to nested classes the way other things are.)

Note that the existing unit test org.junit.jupiter.engine.extension.TestInstanceFactoryTests#instanceFactoriesInNestedClassHierarchy (org.junit.jupiter.engine.extension.TestInstanceFactoryTests.OuterTestCase) uses a different factory class for the outer class and the inner class.

JUnit version: 5.3.0-RC1

Example Test

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.util.EmptyStackException;
import java.util.Stack;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

@DisplayName("A stack")
// NB if this is enabled, the TestInstanceFactory is never called for inner class instances
@ExtendWith(JUnit5ExtensionJava.class)
//@TestInstance(org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS)
//@TestInstance(org.junit.jupiter.api.TestInstance.Lifecycle.PER_METHOD)
class TestingAStackJavaTest {

    Stack<Object> stack;

    @Test
    @DisplayName("is instantiated with new Stack()")
    void isInstantiatedWithNew() {
        new Stack<>();
    }

    @Nested
    @DisplayName("when new")
    // TODO if specified on outer class, are extensions inherited?
    @ExtendWith(JUnit5ExtensionJava.class)
    class WhenNew {

        @BeforeEach
        void createNewStack() {
            stack = new Stack<>();
        }

        @Test
        @DisplayName("is empty")
        void isEmpty() {
            assertTrue(stack.isEmpty());
        }

        @Test
        @DisplayName("throws EmptyStackException when popped")
        void throwsExceptionWhenPopped() {
            assertThrows(EmptyStackException.class, () -> stack.pop());
        }

        @Test
        @DisplayName("throws EmptyStackException when peeked")
        void throwsExceptionWhenPeeked() {
            assertThrows(EmptyStackException.class, () -> stack.peek());
        }

        @Nested
        @DisplayName("after pushing an element")
        @ExtendWith(JUnit5ExtensionJava.class)
        class AfterPushing {

            String anElement = "an element";

            @BeforeEach
            void pushAnElement() {
                stack.push(anElement);
            }

            @Test
            @DisplayName("it is no longer empty")
            void isNotEmpty() {
                assertFalse(stack.isEmpty());
            }

            @Test
            @DisplayName("returns the element when popped and is empty")
            void returnElementWhenPopped() {
                assertEquals(anElement, stack.pop());
                assertTrue(stack.isEmpty());
            }

            @Test
            @DisplayName("returns the element when peeked but remains not empty")
            void returnElementWhenPeeked() {
                assertEquals(anElement, stack.peek());
                assertFalse(stack.isEmpty());
            }
        }
    }
}

import java.util.Optional;

import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.api.extension.TestInstanceFactory;
import org.junit.jupiter.api.extension.TestInstanceFactoryContext;
import org.junit.jupiter.api.extension.TestInstantiationException;

import static org.junit.platform.commons.util.ReflectionUtils.newInstance;

public class JUnit5ExtensionJava implements TestInstanceFactory {

    @Override
    public Object createTestInstance(TestInstanceFactoryContext factoryContext,
            ExtensionContext extensionContext)
            throws TestInstantiationException {
        try {
            Optional<Object> outerInstance = factoryContext.getOuterInstance();
            Class<?> testClass = factoryContext.getTestClass();
            if (outerInstance.isPresent()) {
                System.out.println("TestInstanceFactory.createTestInstance() called for inner class");
                return newInstance(testClass, outerInstance.get());
            } else {
                System.out.println("TestInstanceFactory.createTestInstance() called for outer class");
                return newInstance(testClass);
            }
        } catch (Exception e) {
            throw new TestInstantiationException(e.getMessage(), e);
        }
    }
}

The logging shows that createTestInstance() is never called for inner class instances, unless you comment out @ExtendWith(JUnit5ExtensionJava.class) on the outer class.

Note that if I simply clone the extension class and use one factory class for the outer class and the other factory class for the inner class, both factories are called as expected. It's only when the same extension is specified on both classes, or perhaps "inherited" from the outer to the inner class, that the inner instances are skipped.

I haven't checked whether this applies to other extension types or just TestInstanceFactory.

[aschoerk]

Just a comment: Since callbacks of the Extension (i.e. BeforeEachCallback, AfterEachCallback) normally also work for methods in nested Innerclasses, imO it should not be necessary to annotate those inner classes to make the TestInstanceFactory work.

[sbrannen]

Thanks for raising the issue, @seanf.

This sounds like it in fact might be a _bug_. So we'll look into it ASAP.

[sbrannen]

_in progress_

[sbrannen]

I haven't checked whether this applies to other extension types or just TestInstanceFactory.

It's very likely specific to TestInstanceFactory due to the custom lookup code we have only for that type of extension:

https://github.com/junit-team/junit5/blob/740af363a49b7b195a1e678dee9914082ca7b9a1/junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/ClassTestDescriptor.java#L233-L261

[aschoerk]

This change might brake existing code based on a release candidate. Perhaps it is worth, to mention that anywhere.

[sbrannen]

I'm not concerned so much about a breaking change in a release candidate.

The most important thing is that this has been brought to our attention before the GA release.

In any case, it is in fact a _bug_, since the current behavior contradicts the documentation in the User Guide. The User Guide specifically states the following:

A factory registered on a @Nested test class will override any factories registered on enclosing classes.

[sbrannen]

Just a comment: Since callbacks of the Extension (i.e. BeforeEachCallback, AfterEachCallback) normally also work for methods in nested Innerclasses, imO it should not be necessary to annotate those inner classes to make the TestInstanceFactory work.

You're right: that's part of the same bug in the code.

I'm adding a test for that particular use case as well.

[sbrannen]

FYI: I improved the logging the example extension.

public class CustomTestInstanceFactory implements TestInstanceFactory {

    @Override
    public Object createTestInstance(TestInstanceFactoryContext factoryContext, ExtensionContext extensionContext) {
        try {
            Optional<Object> outerInstance = factoryContext.getOuterInstance();
            Class<?> testClass = factoryContext.getTestClass();
            if (outerInstance.isPresent()) {
                System.out.println("createTestInstance() called for inner class: " + testClass.getSimpleName());
                return newInstance(testClass, outerInstance.get());
            }
            else {
                System.out.println("createTestInstance() called for outer class: " + testClass.getSimpleName());
                return newInstance(testClass);
            }
        }
        catch (Exception e) {
            throw new TestInstantiationException(e.getMessage(), e);
        }
    }

}

[sbrannen]

And I simplified the example test class:

// NB if this is enabled, the TestInstanceFactory is never called for inner class instances
@ExtendWith(CustomTestInstanceFactory.class)
// @TestInstance(TestInstance.Lifecycle.PER_CLASS)
// @TestInstance(TestInstance.Lifecycle.PER_METHOD)
class Outer {

    @Test
    void outer() {
    }

    @Nested
    // @ExtendWith(CustomTestInstanceFactory.class)
    class Inner {

        @Test
        void inner() {
        }

        @Nested
        // @ExtendWith(CustomTestInstanceFactory.class)
        class InnerInner {

            @Test
            void innerInner() {
            }

        }
    }
}

[sbrannen]

With my current changes on my machine, that now outputs the following

createTestInstance() called for outer class: Outer
createTestInstance() called for outer class: Outer
createTestInstance() called for inner class: Inner
createTestInstance() called for outer class: Outer
createTestInstance() called for inner class: Inner
createTestInstance() called for inner class: InnerInner

[sbrannen]

And uncommenting the duplicated registrations for the custom factory on the @Nested test classes results in the same output as above.

[sbrannen]

This issue has been addressed in 87bda94; however, I am reopening this issue in order to ensure that the documentation is consistent.

[sbrannen]

Team Decision:

Align the behavior between conventional class hierarchies and nested class structures with regard to disallowing multiple registrations of TestInstanceFactory extensions. Consequently, it should not be possible to _override_ a TestInstanceFactory registered on an enclosing class for an enclosed @Nested test class.

[sbrannen]

I am closing this issue now, since the aforementioned team decision will now be addressed in #1573.