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

Extensions for AroundAll, AroundEach and AroundTest #3221

Closed
2 tasks
hiddewie opened this issue Apr 1, 2023 · 11 comments
Closed
2 tasks

Extensions for AroundAll, AroundEach and AroundTest #3221

hiddewie opened this issue Apr 1, 2023 · 11 comments

Comments

@hiddewie
Copy link

hiddewie commented Apr 1, 2023

When writing a JUnit extension, I encountered problems when using the Junit Extension API that required me to change the applications in undesirable ways.

Code example below (heavily simplified) (Kotlin):

class Context {

  // thread local state

  fun executeWithSomeContext(action: Context -> Unit) =
    try {
       setup()
       action(context)
    } finally {
       cleanup()
    }
  
  internal fun setup() { } // < -- This had to be made non-internal for the extension
  internal fun cleanup() { } // < -- This had to be made non-internal for the extension
}

The use of this class is to ensure clients always use the executeWithSomeContext to use the context, and cannot use the setup and cleanup methods.

We have built a JUnit extension that is used automatically set up this context for JUnit tests, using an annotation. The JUnit extension reads the annotation, sets up the correct context before the test, runs the test and clears the context.

Currently, the extension looks like

class ContextExtension : BeforeEach, AfterEach {
  val context = // ... 
  override fun beforeEach(...) {
    context.setup()
  }
  override fun afterEach(...) {
    context.cleanup()
  }
}

Instead, it would be useful to be able to build the extension like

class ContextExtension : AroundEach {
  val context = // ... 
  override fun aroundEach(context: ExtensionContext, next: TestContinuation) {
    context.executeWithSomeContext(...) {
      next(context)
    }
  }
}

To support this use case of the JUnit extension calling the Context setup/clear methods, they had to be made public and accessible outside the defining package. This allows other (user) code to circumvent the executeWithSomeContext method and call the setup or cleanup methods directly.

My use case is about setting up a context and automatically clearing it after the test, but this could be extended to other types of resources can be used, but not set up, stored or manually cleared by user code.

Even a common guide to JUnit extensions, https://www.baeldung.com/junit-5-extensions#3-lifecycle-callbacks, lists a database connection with a manual cleanup that could benefit from an Around* type extension, with a try/finally block to support correct cleaning up of resources.

Other languages and test frameworks have similar around extension APIs, for example around in RSpec (https://rubydoc.info/gems/rspec-core/RSpec%2FCore%2FHooks:around).

Another benefit of the Around* extension API is a clear stacktrace showing the call stack of each extension running around the actual JUnit test.

Some semantics that these interfaces should expose:

  • The next argument must be invoked exactly once by the extension.
  • The name and type of the next argument to the aroundEach method of the AroundEach interface could be improved.

Deliverables

  • Addition of AroundAll, AroundEach, AroundTest interfaces which will be handled in the test extension lifecycle by JUnit.
  • Optionally deprecate the Before* and After*, because they could be replaced by the Around* extension lifecycles.

Thanks in advance.

@jbduncan
Copy link
Contributor

jbduncan commented Apr 1, 2023

@hiddewie This sounds a lot like InvocationInterceptor . Have you tried it yet, by any chance?

Alternatively, I recently authored a resources extension for junit-pioneer which sounds like it does something similar to what you want.

I hope this helps.

@hiddewie
Copy link
Author

hiddewie commented Apr 1, 2023

@jbduncan Thanks for the suggestion. I tried the InvocationInterceptor (some time ago, but I just tried again to verify):

@ExtendWith(ContextExtension::class, ContextInterceptor::class)
class ContextTest {

    companion object {
        @BeforeAll
        @JvmStatic
        fun ba() {
            println("in before all")
        }
    }

    @Test
    fun test() {
        println("running test")
    }
}

class ContextExtension : BeforeAllCallback {
    override fun beforeAll(context: ExtensionContext?) {
        println("Before all execution")
    }
}

class ContextInterceptor : InvocationInterceptor {
    override fun interceptBeforeAllMethod(invocation: InvocationInterceptor.Invocation<Void>, invocationContext: ReflectiveInvocationContext<Method>?, extensionContext: ExtensionContext?) {
        println("before all proceed")
        invocation.proceed()
        println("after all proceed")
    }
}

This wraps the beforeAll method invocation, but the wrapping is not called if there is no @BeforeAll defined, and the interceptor just wraps the @BeforeAll method itself, not the test in the sequence around the beforeAll and afterAll lifecycle (like the table described in https://junit.org/junit5/docs/current/user-guide/#extensions-execution-order).

The code prints

Before all execution
before all proceed
in before all
after all proceed
running test

But I require something like

before all proceed
running test
after all proceed

irrespective if there are zero, one or multiple @BeforeAll or @AfterAll defined.

Using the interceptTestMethod could work, but it is 'too late', executed much later in the test execution lifecycle than @BeforeAll or @BeforeEach. For my example with the context setup/cleanup, the execution order in the test lifecycle is important.


I did find some historical issues/PRs with similar requests:

Especially #1442 seems similar to this issue.


Alternatively, I recently authored a resources extension for junit-pioneer which sounds like it does something similar to what you want.

Interesting. This will unfortunately not work for my use case with thread locals as 'resources' that need to be kept internal and have no create/get/set access outside the package. But for most other types of 'external' resources it would probably work nicely.

@jbduncan
Copy link
Contributor

jbduncan commented Apr 1, 2023

@hiddewie Oh, I see, thanks for clarifying! Indeed, intercepting just the @BeforeAll (or @BeforeEach for that matter) wouldn't allow this "around" behaviour you're after. Also, my understanding of thread locals is limited, so I don't entirely see why my extension wouldn't work, but I'm more than happy to trust you on this.

Have you tried overriding InvocationInterceptor::interceptTestMethod​? It should work similarly to your AroundEach idea or implementing (Before|After)EachCallback.

Otherwise, I think (Before|After)EachCallback, as you already use them, is the best solution. If I were a JUnit 5 maintainer, I'd hesitate to add AroundEachCallback because I'd value having one way of doing things to reduce cognitive load for users, and to stop users getting confused between AroundEachCallback and InvocationInterceptor::interceptTest. But I'll leave this to the JUnit folks to decide.

@jbduncan
Copy link
Contributor

@hiddewie I don't suppose you've found the time to try InvocationInterceptor::interceptTestMethod yet, have you? I admit I'm very curious to see if it works or not. :)

@marcphilipp
Copy link
Member

My use case is about setting up a context and automatically clearing it after the test, but this could be extended to other types of resources can be used, but not set up, stored or manually cleared by user code.

@hiddewie For setting up a resource and clearing it, there's CloseableResource which is yet to be documented in more detail (see #1555).

@hiddewie
Copy link
Author

@jbduncan

Yes I have tried InvocationInterceptor::interceptTestMethod, but it does not intercept at the stage of the test setup/teardown where I would need it. Specifically in the beforeAll/afterAll stage, and in the beforeEach/afterEach stage.

I made another fake test to explain my thinking:

@ExtendWith(ContextExtension::class, ContextInterceptor::class)
class ContextTest {
    @Test fun test1() = println("running test 1")
    @Test fun test2() = println("running test 2")
    @Test fun test3() = println("running test 3")
}

class ContextExtension : BeforeAllCallback, AfterAllCallback {
    override fun beforeAll(context: ExtensionContext?) {
        println("Before all execution")
    }
    override fun afterAll(context: ExtensionContext?) {
        println("After all execution")
    }
}

class Context {
    fun use(value: String, action: () -> Unit) =
        try {
            println("Setting context for $value")
            action()
        } finally {
            println("Clearing context for $value")
        }
}

class ContextInterceptor : InvocationInterceptor {
    override fun interceptTestMethod(invocation: InvocationInterceptor.Invocation<Void>, invocationContext: ReflectiveInvocationContext<Method>?, extensionContext: ExtensionContext?) {
        Context().use("test-context") {
            invocation.proceed()
        }
    }
}

The output is

Before all execution
Setting context for test-context
running test 1
Clearing context for test-context
Setting context for test-context
running test 2
Clearing context for test-context
Setting context for test-context
running test 3
Clearing context for test-context
After all execution

while I would need something like

Setting context for test-context
Before all execution
running test 1
running test 2
running test 3
After all execution
Clearing context for test-context

(the order of the beforeAll/afterAll interceptors does not much matter, as long as it only runs once at the correct test stage)


@marcphilipp

Thanks for the suggestion.

Using a CloseableResource cannot work as far as I can see, because it needs a way to 'set' and 'clear' the resource.

In the issue description, the setting and cleaning methods are the 2 context methods that I do not want to expose publicly. Only the executeWithSomeContext method from my example should be used, which executes some code (the test) with some context.

@hiddewie
Copy link
Author

To give my use case a bit more context. A similar Java API is proposed for the Scoped Variables, JEP 429, https://openjdk.org/jeps/429.

var result = ScopedValue.where(Server.PRINCIPAL, guest)
              .call { test.run() }

The construct to manipulate ScopedValue within a piece of code, but without setting or clearing it manually, is similar to my example about context.

The JEP reasons about the Java API:

The syntactic structure of the code delineates the period of time when a thread can read its incarnation of a scoped value. This bounded lifetime, combined with immutability, greatly simplifies reasoning about thread behavior. The one-way transmission of data from caller to callees — both direct and indirect — is obvious at a glance. There is no set(...) method that lets faraway code change the scoped value at any time. Immutability also helps performance: Reading a scoped value with get() is often as fast as reading a local variable, regardless of the stack distance between caller and callee.

(emphasis mine)

@marcphilipp
Copy link
Member

@hiddewie Thanks for providing more detail! I agree that there's no way to do this with the current extension APIs. While JUnit 4's extension model works like that, we opted for more fine-granular but composable extension points.

the setting and cleaning methods are the 2 context methods that I do not want to expose publicly

Would it be possible to put the Extension into the same package but in a different source folder (e.g. test or testFixtures if it's a Gradle project) so that they can be package-private?

@hiddewie
Copy link
Author

Thanks for the comment.

Would it be possible to put the Extension into the same package but in a different source folder (e.g. test or testFixtures if it's a Gradle project) so that they can be package-private?

It might be possible, but I think this is discouraged and not conform the idea of one-package-belongs-to-one-module when using Jigsaw on the JVM 9+. When fully using modules, the classes in a package must all be part of the same module.

I have to experiment a bit with my code to see how large a blocker it is to put test code in the same package as the infrastructure code.

@marcphilipp
Copy link
Member

Team decision: For the reasons stated in #3221 (comment) we won't add support for "around" type of extensions.

@marcphilipp marcphilipp closed this as not planned Won't fix, can't repro, duplicate, stale May 5, 2023
@hiddewie
Copy link
Author

hiddewie commented May 5, 2023

Thanks for looking into it, I understand your decision.

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