InstancePostProcessor should return test instance

[jlink]

public interface InstancePostProcessor extends ExtensionPoint {
        /** Return the original or a wrapped instance of the test object */
    Object postProcessTestInstance(TestExtensionContext context) throws Exception;
}

This seems to be necessary for PowerMock extension (see #201).

@junit-team Any objections?

[sbrannen]

I don't think it is a wise idea to repurpose InstancePostProcessor for extensions that wish to replace the test instance.

For example, let's assume we have two registered extensions that implement InstancePostProcessor. For the sake of argument, let's call them S and P (for Spring and PowerMock).

If S injects dependencies into the test instance and P then completely replaces the test instance, any tests relying on dependency injection from Spring will fail even though Spring's log message will state that dependency injection in fact occurred.

At the very least, there is an issue with regard to ordering of such extensions, but I would argue that is only an indicator of the true problem:

When does it ever make sense for more than one extension to replace the test instance?

In my opinion, the answer to the above question is never.

With that in mind, I would propose a new extension -- call it InstanceFactory or similar -- that can only be registered once per test container. Any attempt for a second extension to register itself as such a factory would then result in an exception.

[sbrannen]

Of course, the potential problem I outlined above does not come into effect if instances of InstancePostProcessor merely wrap the passed in instance and don't actually replace it. So in that regard, the proposal to return Object from postProcessTestInstance() should be fine. I guess it boils down to whether the framework should somehow attempt to warn developers (via a log message) if multiple extensions have either replaced or wrapped the test instance.

[thekingn0thing]

For PowerMock main goal- it's to be able to load test class and all other classes with PowerMock class loader.

Approach with InstanceFactory very attractive for us, it's how TestNG works now. And of course, only one InstanceFactory could be declared.

But, it also could be a point of fail, for example assertion exception or any other exception witch test could throw and jUnit engine is expected. In this class casting issues moved forward. I mean that trying to cast exception to expected class could throw `ClassCastException' or instanceOf would return incorrect result.

[jlink]

@sbrannen It's impossible to enforce wrapping, or is it. It might not even be practical for PowerMock's purposes when using a different class loader to create basically the same object.

[mepeisen]

In my Showcase I got another Problem. Playing around with class loader results in other extensions not working as expected.
See https://github.com/mepeisen/xw-nukkit-test/blob/master/src/main/java/eu/xworlds/nukkit/test/NukkitExtension.java#L98-L102 and https://github.com/mepeisen/xw-nukkit-test/blob/master/src/main/java/eu/xworlds/nukkit/test/NukkitExtension.java#L133-L136
Other extensions need to be aware that they are loaded by a different class loader and that their annotations and classes can only be used by name. Meaning: Extensions must be aware that powermock is present. And that's not very clever for extensions that even do not know what powermock is.

Actually I do not know much about the powermock internals. Maybe this problem can be solved to let the powermock class loader ignore the packages of other extensions. I already added mockito for ignore: https://github.com/mepeisen/xw-nukkit-test/blob/master/src/main/java/eu/xworlds/nukkit/test/NukkitExtension.java#L63 and I do not know why i have to do this manually :-)

[jlink]

@sbrannen I currently prefer a new extension point. Although I'm not sure it will really solve PowerMock's needs (see discussion in #201).

What we would need then, is a way to have different ordering and uniqueness constraints per extension point type (e.g. "There must be only one test instance factory"). I already implemented something like that in PR #135.

[jlink]

@mepeisen Yes, that's the can of worms I was talking about :-/

[marcphilipp]

I'm closing this issue because this would not solve PowerMock's problem (as discussed in #201).