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

Introduce extension API for executing test in user-defined thread #157

Closed
schauder opened this issue Feb 15, 2016 · 31 comments · Fixed by #1852
Closed

Introduce extension API for executing test in user-defined thread #157

schauder opened this issue Feb 15, 2016 · 31 comments · Fixed by #1852

Comments

@schauder
Copy link
Contributor

schauder commented Feb 15, 2016

Overview

I couldn't find a way to write an extension that changes the behavior of test execution.

Examples of what I would like to do are:

  • let each test run 100x in 5 threads to check for race conditions
  • let a test execute in the Swing EDT Thread.

Such use cases were easy to implement with JUnit 4 Rules, and I used especially the second a lot. So I think it would be bad to lose this option in JUnit Jupiter.

Proposals

Related Issues

@jlink
Copy link
Contributor

jlink commented Feb 15, 2016

Your first point falls into the broader category of dynamic test creation
(see #58) and is the next bigger feature on our list.

As for test execution in a different thread, we haven't discussed that yet

  • as far as I can remember. It could be handled as a special case of
    asynchronous testing and I personally never felt the need to have the whole
    test run in the SWT thread; wouldn't it suffice to have some asserts use
    that thread?

2016-02-15 14:23 GMT+01:00 Jens Schauder [email protected]:

I couldn't find a way to write an extension that changes the behavior of
test execution.

Examples of what I would like to do are:

  • let each test run 100x in 5 threads to check for race conditions
  • let a test execute in the Swing EDT Thread.

These where easy to implement with JUnit Rules and I used especially the
second a lot. So I think it would be bad to loose this option.


Reply to this email directly or view it on GitHub
#157.

@schauder
Copy link
Contributor Author

wouldn't it suffice to have some asserts use
that thread?

Not really, because all Swing Component manipulation must be in the EDT. Anyway these are just two examples where the approach of Rules came in really handy: you get a Statement, you return a Statement, and that gets executed as a Test.

I'd really like to have a similar abstraction right in the heart of JUnit 5. Right now Extensions seem to be either rather limited, or bound to the JUnit 5 Engine JUnit 5 Execution Context, because they depend on things being there like TestClasses / or methods, which might not exist for other engines.

@jlink
Copy link
Contributor

jlink commented Feb 15, 2016

The current design tries to balance the power of extensions, composability and the guarantees the JUnit 5 engine can give. JUnit4's statement approach was one of the things that made composition difficult.

I'm not saying we made the best calls or that we won't change it. But I do believe there's no solution that will give us only benefits and no drawbacks.

Am 15.02.2016 um 16:21 schrieb Jens Schauder [email protected]:

wouldn't it suffice to have some asserts use
that thread?

Not really, because all Swing Component manipulation must be in the EDT. Anyway these are just two examples where the approach of Rules came in really handy: you get a Statement, you return a Statement, and that gets executed as a Test.

I'd really like to have a similar abstraction right in the heart of JUnit 5. Right now Extensions seem to be either rather limited, or bound to the JUnit 5 Engine JUnit 5 Execution Context, because they depend on things being there like TestClasses / or methods, which might not exist for other engines.


Reply to this email directly or view it on GitHub.

@bechte
Copy link
Contributor

bechte commented Feb 15, 2016

The JUnit5 engine was designed in that way, that the test execution is always guaranteed by the engine and cannot be changed. In this way, we can guarantee the execution flow and allow extension providers to build extension upon this contract.

For altering the test execution we do not yet provide a mechanism. One would be the resolver based discovery approach (see pull request #134) that would allow extension writers to contribute they own TestDescriptor logic to the test plan. It would require to use another annotation like @RaceConditionTest but actually, that would absolutely fit your needs for the race condition testing. One could also add another TestDescriptor for running EDT tests. If they require a very specific environment, then one might want to declare the test as @SwingComponentTest or whatever you name it, and let it be resolved to another specific TestDescriptor that handles this special case.

To me, all of your examples seem to be very specific use-cases which might not need to be composable and, therefore, are perfect candidates for TestResolvers.

@schauder : Please have a look and contact me if you have further questions. I am looking forward to getting your feedback. 😃

@schauder
Copy link
Contributor Author

@jlink can you elaborate, how the statement approach makes composition difficult? I seem to experience the exactly opposite effect.

@bechte I read through the description of the pull request. Sounds great. I might find time tomorrow evening to actually look at the code.

@jlink
Copy link
Contributor

jlink commented Feb 16, 2016

Composing (nesting) statements only works if all statements do the exact right thing as of exception handling, ordering, calling before/after behaviour etc.; the difference between correct and incorrect can be subtle. The framework has no means to influence things from the outside. Thus it's easy to compose statements if you control all of them, but it becomes erratic as you compose unknown ones from different sources.

One could of course introduce an extension point that takes an Executable of the test method only and returns a different Executable. My fear is that it would be abused for all kind of before/after/exception-handling stuff - just because it's so easy to do. And then we'd be in the same place as we were with JUnit4.

Am 16.02.2016 um 08:26 schrieb Jens Schauder [email protected]:

@jlink can you elaborate, how the statement approach makes composition difficult? I seem to experience the exactly opposite effect.

@bechte I read through the description of the pull request. Sounds great. I might find time tomorrow evening to actually look at the code.


Reply to this email directly or view it on GitHub.

@schauder
Copy link
Contributor Author

@jlink I'm probably missing something doing the "right thing" shouldn't be to difficult. The contract of statement in JUnit4 seemed to be rather simple and easy to follow.

When filtering and ordering enter the mix I can understand your concerns, but as long as we follow a modified Mad Max principle: One statement goes in one statement goes out everything should be fine.

Filtering and ordering should be done by a separate part of the system to avoid any problems.

My fear is that it would be abused for all kind of before/after/exception-handling stuff - just because it's so easy to do.

That was actually the reason why Rules are so important to me: I can put all my reusable before/after/stuff in single code point and get rid of tons of ugly reuse-by-inheritance abdominations.

An appropriate substitute for this is essential for adoption of JUnit5 for me.

@jlink
Copy link
Contributor

jlink commented Feb 22, 2016

I don't think that the Mad Max principle is enough since you can do so many
(too many IMO) things with a statement that will break reasonable
assumptions about the normal behaviour.

I'm absolutely willing though, to rethink out extension mechanism. Let's
see what the others think.

2016-02-22 8:26 GMT+01:00 Jens Schauder [email protected]:

@jlink https://github.com/jlink I'm probably missing something doing
the "right thing" shouldn't be to difficult. The contract of statement in
JUnit4 seemed to be rather simple and easy to follow.

When filtering and ordering enter the mix I can understand your concerns,
but as long as we follow a modified Mad Max principle: One statement goes
in one statement goes out everything should be fine.

Filtering and ordering should be done by a separate part of the system to
avoid any problems.


Reply to this email directly or view it on GitHub
#157 (comment).

@marcphilipp marcphilipp added this to the 5.0 M1 milestone Feb 29, 2016
@marcphilipp
Copy link
Member

Regarding execution in a different thread, we've discussed two alternatives this morning in a team call:

(1) Add a TestExecutor extension point

interface TestExecutor extends ExtensionPoint {
    void execute(TestExtensionContext context, Executable executable);
}

(2) Add a ExecutorProvider extension point

interface ExecutorProvider extends ExtensionPoint {
    Executor getExecutor(TestExtensionContext context);
}
interface Executor {
    void execute(Runnable runnable);
}

Either could be used to execute test methods in a different thread. However, in both cases @BeforeEach and @AfterEach methods and BeforeEach-/AfterEachExtensionPoints would still be executed in the "main" thread. Is that what you would want?


Regarding repeating tests, we think we should first get a grip on dynamic tests. Maybe repeating a test is just a dynamic test invocation.

@schauder
Copy link
Contributor Author

As far as I can tell right now both variants should work for what I do with Rules.

I kind of agree with your statement about dynamic tests.

@jlink
Copy link
Contributor

jlink commented Feb 29, 2016

Running @Before/AfterEach (and other extension points) in a different
thread than the test itself can lead to subtle bugs concerning
synchronization of member variables and other shared state. If we want to
prevent all instance-based extension points would have to be covered by
this new one. This will make ordering of individual pieces of an extension
more difficult because there are no longer names for things like
InstancePostProcessor and ExceptionHandler etc.

Moreover, a working contract would require that returning from execute
must be synchronous and all exceptions thrown therein be rethrown during
exception point invocation.

2016-02-29 13:21 GMT+01:00 Jens Schauder [email protected]:

As far as I can tell right now both variants should work for what I do
with Rules.

I kind of agree with your statement about dynamic tests.


Reply to this email directly or view it on GitHub
#157 (comment).

@sbrannen
Copy link
Member

Composing (nesting) statements only works if all statements do the exact right thing as of exception handling, ordering, calling before/after behaviour etc.; the difference between correct and incorrect can be subtle. The framework has no means to influence things from the outside. Thus it's easy to compose statements if you control all of them, but it becomes erratic as you compose unknown ones from different sources.

One could of course introduce an extension point that takes an Executable of the test method only and returns a different Executable. My fear is that it would be abused for all kind of before/after/exception-handling stuff - just because it's so easy to do. And then we'd be in the same place as we were with JUnit4.

@jlink, I couldn't have phrased it better myself!

I share the exact same concerns and have for a long time: these are in fact the reasons that we opted not to implement extensions in JUnit 5 using the Chain of Responsibility pattern like JUnit 4's rules.

However, having said that, I do understand that some extensions may need to control/spawn the thread in which a test executes. To that end, something like a TestExecutor or ExecutorFactory extension could suffice.

But... at the very least we need to guide developers to use the standard extensions and not abuse these executor extensions, and as Johannes pointed out: we need to be very careful about the ramifications of introducing such an extension.

@marcphilipp
Copy link
Member

Alternative design:

interface UserCodeExecutor {
    default void execute(ExtensionContext context, UserCodeType userCodeType, Executable executable) throws Exception {
        executable.execute();
    }
}
enum UserCodeType {
    TEST_CLASS_CONSTRUCTOR,
    BEFORE_ALL_METHOD,
    ...
}

@awegmueller
Copy link

Hello Marc

Thanks a lot for the detailed analysis and your proposals! I am sorry my reply comes a bit late, but I was on holiday until today. However, we really like your first proposal (TestExecutor interface) and prefer that solution to the version that uses enums. This allows us to wrap each executable in a Scout RunContext/Subject again, as we have done it with JUnit 4. I guess this solution would also work for thread-starter @schauder who wanted to execute a test in the Swing thread.

Can you make any predictions in which JUnit release this will be implemented (if at all)?

@akozlova
Copy link

The suggested proposal would work for us as well, I like TestExecutor proposal more but one with enum would work as well. Any ETA when we can try it? Thanks

@marcphilipp marcphilipp modified the milestones: 5.4 M1, 5.4 M2 Nov 23, 2018
@marcphilipp
Copy link
Member

marcphilipp commented Dec 19, 2018

Slightly modified proposal to allow multiple registered ExecutionInterceptor extensions:

interface ExecutionInterceptor extends Extension {
    void createTestInstance(ExecutionContext context, Executable executable);
    void executeBeforeAllMethod(ExecutionContext context, Executable executable);
    void executeBeforeEachMethod(ExecutionContext context, Executable executable);
    void executeExtensionMethod(ExecutionContext context, Executable executable);
    void executeTestMethod(ExecutionContext context, Executable executable);
    void executeAfterEachMethod(ExecutionContext context, Executable executable);
    void executeAfterAllMethod(ExecutionContext context, Executable executable);
}
interface ExecutionContext {
    ExtensionContext getExtensionContext();
    Optional<Object> getTarget(); // empty for static methods
    Method getMethod();
    List<Object> getArguments();
}

@marcphilipp
Copy link
Member

Another iteration that allows to inspect invocation results:

interface InvocationInterceptor extends Extension {

    default <T> T createTestInstance(Invocation<T> invocation) throws Throwable {
        return invocation.proceed();
    }

    default void executeBeforeAllMethod(Invocation<Void> invocation) throws Throwable {
        invocation.proceed();
    }

    default void executeBeforeEachMethod(Invocation<Void> invocation) throws Throwable {
        invocation.proceed();
    }

    default <T> T executeExtensionMethod(Invocation<T> invocation) throws Throwable {
        return invocation.proceed();
    }

    default void executeTestMethod(Invocation<Void> invocation) throws Throwable {
        invocation.proceed();
    }

    default <T> T executeTestFactoryMethod(Invocation<T> invocation) throws Throwable {
        return invocation.proceed();
    }

    default void executeTestTemplateMethod(Invocation<Void> invocation) throws Throwable {
        invocation.proceed();
    }

    default void executeAfterEachMethod(Invocation<Void> invocation) throws Throwable {
        invocation.proceed();
    }

    default void executeAfterAllMethod(Invocation<Void> invocation) throws Throwable {
        invocation.proceed();
    }

    interface Invocation<T> {
        T proceed() throws Throwable;
        ExtensionContext getExtensionContext();
        Class<?> getTargetClass();
        Optional<Object> getTarget(); // empty for static methods
        Executable getExecutable();
        List<Object> getArguments();
    }
}

@marcphilipp
Copy link
Member

Updated once again to add support for intercepting dynamic tests and to increase consistency with other Extension subinterfaces:

import java.lang.reflect.Executable;
import java.util.List;
import java.util.Optional;

interface InvocationInterceptor extends Extension {

    default <T> T createTestInstance(ReflectiveInvocation<T> invocation, ExtensionContext extensionContext) throws Throwable {
        return invocation.proceed();
    }

    default void executeBeforeAllMethod(ReflectiveInvocation<Void> invocation, ExtensionContext extensionContext) throws Throwable {
        invocation.proceed();
    }

    default void executeBeforeEachMethod(ReflectiveInvocation<Void> invocation, ExtensionContext extensionContext) throws Throwable {
        invocation.proceed();
    }

    default <T> T executeExtensionMethod(ReflectiveInvocation<T> invocation, ExtensionContext extensionContext) throws Throwable {
        return invocation.proceed();
    }

    default void executeTestMethod(ReflectiveInvocation<Void> invocation, ExtensionContext extensionContext) throws Throwable {
        invocation.proceed();
    }

    default <T> T executeTestFactoryMethod(ReflectiveInvocation<T> invocation, ExtensionContext extensionContext) throws Throwable {
        return invocation.proceed();
    }

    default void executeTestTemplateMethod(ReflectiveInvocation<Void> invocation, ExtensionContext extensionContext) throws Throwable {
        invocation.proceed();
    }

    default void executeDynamicTest(Invocation<Void> invocation, ExtensionContext extensionContext) throws Throwable {
        invocation.proceed();
    }

    default void executeAfterEachMethod(ReflectiveInvocation<Void> invocation, ExtensionContext extensionContext) throws Throwable {
        invocation.proceed();
    }

    default void executeAfterAllMethod(ReflectiveInvocation<Void> invocation, ExtensionContext extensionContext) throws Throwable {
        invocation.proceed();
    }

    interface Invocation<T> {
        T proceed() throws Throwable;
    }

    interface ReflectiveInvocation<T> extends Invocation<T> {
        Class<?> getTargetClass();
        Optional<Object> getTarget(); // empty for static methods
        Executable getExecutable();
        List<Object> getArguments();
    }
}

@sbrannen
Copy link
Member

Starting to remind me of AspectJ's ProceedingJoinPoint, with proceed, getArgs, getSignature, getTarget, etc. 😉

@marcphilipp
Copy link
Member

WIP: https://github.com/junit-team/junit5/compare/issues/157-invocation-interceptor

marcphilipp added a commit that referenced this issue Apr 8, 2019
The new extension API allows intercepting the invocation of test class
constructors, lifecycle methods, testable methods, and dynamic tests.
It validates that an invocation is asked to proceed exactly once. The
user guide is updated with an example that executes all test methods in
Swing's EDT.

Resolves #157.
marcphilipp added a commit that referenced this issue Apr 14, 2019
The new extension API allows intercepting the invocation of test class
constructors, lifecycle methods, testable methods, and dynamic tests.
It validates that an invocation is asked to proceed exactly once. The
user guide is updated with an example that executes all test methods in
Swing's EDT.

Resolves #157.
@sbrannen sbrannen modified the milestones: 5.5 Backlog, 5.5 M2 May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment