wcm-io / io.wcm.testing.aem-mock

Mock implementation of selected AEM APIs.
Apache License 2.0
1 stars 11 forks source link

Add support for @Nested tests #21

Open dennisfisch opened 11 months ago

dennisfisch commented 11 months ago

I have several tests that would really benefit from nesting to separate them logically and for improved maintainability. I have a static @BeforeAll method in the enclosing class which initializes the context. The context is stored in a static class field and used by the nested classes to retrieve resource and model under test.

` @ExtendWith({AemContextExtension.class, MockitoExtension.class}) @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) class ModelTest {

public static final String PAGE_PATH = "/content/test/testpage";

private static final AemContext context = new AemContextBuilder()
        .beforeTearDown(context1 -> System.out.println("beforeTeardown"))
        .build();

@BeforeAll
static void beforeAll() {
    //registerService / create test content, etc
}

@Nested
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
class WithResults {

    private SlingModelClass modelWithResults;

    @BeforeAll
    void beforeAll() {
        final MockRequestPathInfo requestPathInfo = (MockRequestPathInfo) context.request().getRequestPathInfo();
        requestPathInfo.setSuffix("/TEST_EXISTS");

        final Resource pageResource = context.resourceResolver().getResource(PAGE_PATH);
        context.currentPage(pageResource.adaptTo(Page.class));

        modelWithResults = context.request().adaptTo(SlingModelClass.class);
    }

    @Test
    void Test_Model() {
        assertTrue(modelWithResults.hasResults()));
    }
}

@Nested
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
class WithoutResults {

    private SlingModelClass modelWithoutResults;

    @BeforeAll
    void beforeAll() {
        final MockRequestPathInfo requestPathInfo = (MockRequestPathInfo) context.request().getRequestPathInfo();
        requestPathInfo.setSuffix("/NON_EXISTANT");

        final Resource pageResource = context.resourceResolver().getResource(PAGE_PATH);
        context.currentPage(pageResource.adaptTo(Page.class));

        modelWithoutResults = context.request().adaptTo(SlingModelClass.class);
    }

    @Test
    void Test_Model() {
        assertFalse(modelWithoutResults.hasResults()));
    }
}

`

The first nested test runs just fine, but then the context is torn down before the second nested class is executed. Obviously, this will then fail...

The problem lies in the isBeforeAllContext() method. The method works fine for the enclosing class.

On the nested classes, it iterates the methods via ReflectUtil->getAnnotatedMethod()->testClass.getDeclaredMethods(). This actually sees the method annotated with @BeforeAll, but for some reason its declaredAnnotations() is empty, so skips over it. The method contains a reference to itself via getRoot(), which does contain the @BeforeAll annotation. But even if this method were found, the surrounding code in isBeforeAllContext() would ignore it because it checks if the method is actually static: Modifier.isStatic(method.getModifiers())

IMHO, the method should be changed to check whether the enclosing class already has a beforeAllState set: AemContextStore.getBeforeAllState(extensionContext);

stefanseifert commented 10 months ago

hmm, in general we do not really support static instances of AemContext with the BeforeAll pattern. a Aem/SlingContext object setup is not an immutable thing, its very mutable with the repository, OSGi services coming and going etc. so, how do you make sure you have a clean context for each unit test you are running? i think very few people are using it like this, and we have no unit tests to coverage scenarios like this.

did you try using the @Nested approach without the static context instance and without using BeforeAll? if this is possible i would be happy to look into ways of making AEM mocks compatible with this, if that's not already the case. but i'm doubting if it's worth to fully support the static context/BeforeAll approach (or that might be a very big effort to get that hassle-free).