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

TestInstanceFactory on enclosing class is not called for @Nested test class #1567

Closed
seanf opened this issue Aug 30, 2018 · 14 comments
Closed

Comments

@seanf
Copy link
Contributor

seanf commented Aug 30, 2018

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

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

Thanks for raising the issue, @seanf.

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

@sbrannen
Copy link
Member

in progress

@sbrannen
Copy link
Member

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:

private TestInstanceFactory resolveTestInstanceFactory(ExtensionRegistry registry) {
List<TestInstanceFactory> localFactories = registry.getLocalExtensions(TestInstanceFactory.class);
if (localFactories.isEmpty()) {
return null;
}
ExtensionRegistry parentRegistry = registry.getParent();
if (parentRegistry != null) {
List<TestInstanceFactory> parentFactories = parentRegistry.getExtensions(TestInstanceFactory.class);
localFactories.removeAll(parentFactories);
if (localFactories.isEmpty()) {
return null;
}
}
if (localFactories.size() > 1) {
String factoryNames = localFactories.stream().map(factory -> factory.getClass().getName()).collect(
joining(", "));
String errorMessage = String.format(
"The following TestInstanceFactory extensions were registered for test class [%s], but only one is permitted: %s",
testClass.getName(), factoryNames);
throw new ExtensionConfigurationException(errorMessage);
}
return localFactories.get(0);
}

@sbrannen sbrannen added this to the 5.3 GA milestone Aug 30, 2018
@aschoerk
Copy link

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

@sbrannen
Copy link
Member

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

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

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

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

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

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

@ghost ghost removed the status: in progress label Aug 31, 2018
@sbrannen sbrannen changed the title TestInstanceFactory on outer class is not called for inner class instances TestInstanceFactory on enclosing class is not called for @Nested test class Aug 31, 2018
@sbrannen
Copy link
Member

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

@sbrannen sbrannen reopened this Aug 31, 2018
@sbrannen
Copy link
Member

sbrannen commented Aug 31, 2018

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

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

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

No branches or pull requests

3 participants