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

Add support for @Nested tests #21

Open
dennisfisch opened this issue Sep 15, 2023 · 1 comment
Open

Add support for @Nested tests #21

dennisfisch opened this issue Sep 15, 2023 · 1 comment

Comments

@dennisfisch
Copy link

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

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).

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

No branches or pull requests

2 participants