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 providing a custom ClassLoader (e.g., for Powermock) #201

Closed
mepeisen opened this issue Mar 13, 2016 · 123 comments
Closed

Comments

@mepeisen
Copy link

mepeisen commented Mar 13, 2016

Overview

I tried to build a Powermock Extension similar to the Mockito example. I have to test some classes that create new objects within their constructors. In JUnit 4 I simply use the PowerkmockRunner and the whenNew from Powermock.

At least I ended with many problems and gave up. However in JUnit 4 Powermock creates an instrumented class loader and even duplicates everything (the test instance etc.) by using the new class loader. It is simple because the loader/rule are aware of executing the test itself. In JUnit Jupiter an Extension is not able to execute the test by itself.

I am wondering how this will be done in JUnit Jupiter. Are you planning to introduce some CreateTestInstance extension API? Something that is aware of manipulating class loaders. Or at least an AroundAll extension API to instrument the whole test class?

Related Issues

@jlink
Copy link
Contributor

jlink commented Mar 14, 2016

What we have is the InstancePostProcessor extension point. Currently, it does not allow to return a different test instance but we have been discussing to allow that, e.g.:

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

Would that be sufficien for your purposes? I am curious though, why PowerMock has to change the test instance (which can easily lead to incompatibilities with other extensions). Maybe there's another way to go about it?

As for an extension point that wraps test execution itself, we are currently shying away from it for reasons discussed (among other places) in #157.

@mepeisen
Copy link
Author

As far as I can see returning a different instance should fit the needs.
I do not know why powermock needs it but I will ask the authors. All I can see is that mocking objects and classes already works.

Actually in my Scenario i need to mock things happening in constructors.

class ToTest { public ToTest() { this.aObj = new A(); doSomeThingWithAObj(); } }

And I need to inject a mock for "aObj". Actually aObj is a console reader to receive commands.

Mabye exactly this scenario need playing around with class loaders and replacing the test instance. I will invite the powermock guys to discuss this in this issue :-)

@jlink
Copy link
Contributor

jlink commented Mar 14, 2016

2016-03-14 8:43 GMT+01:00 Martin Eisengardt [email protected]:

Mabye exactly this scenario need playing around with class loaders and
replacing the test instance. I will invite the powermock guys to discuss
this in this issue :-)

Thanks. This should certainly help :)

@thekingn0thing
Copy link

@mepeisen, thanks for raising the problem. We ware going to look to jUnit5 and implementing supporting of jUnit5 after we finish preparing current release. And maybe it could be to late :)

PowerMock uses custom class loader to modified loaded class and either remove private/final modified or modify body of static classes to add call interceptors. As result instance which created via mock @Mock could have classes which is loaded by custom class loader and cannot be casted to classes which are loaded by test class loader. What's why PowerMock has to change the test instance and reload it via custom class loader.

By the way, PowerMock creates a new instance of PowerMock class loader for test chunk. Test chunk is a set of tests which have to be run with same set of modified classes and declared via @PrepareForTest annotation. This annotation could be used either on class or on method level.

Example:

TestClass{

     @PrepareForTest(SomeStaticClass.class)
     public void testA()
     }

     @PrepareForTest(SomePrivateClass.class)
     public void testB()
     }

     public void testC()
     }
}

In this case PowerMock creates two test chunk and two class loader: one for running test testA and another to run TestB. The TestC is run with either first class loader or second depends on order in array which is returned by testClass.getMethods().

When a test is run with jUnit4 Runner then some jUnit logic is duplicated in our code and it's not very nice and I prefer to avoid it.

@thekingn0thing
Copy link

Good example, how we can avoid over controlling - TestNG ObjectFactory. This class has a method to create a new instance of class. PowerMock create a new instance, which is loaded via PowerMock class loader and wrapped by proxy, to clear PowerMock internals after each test.

We also try to use Java Agent to get off necessity using custom class loader, but Java Instrumentation limits us by allowing only modify body of method.

@thekingn0thing
Copy link

@johanhaleby what do think?

@mepeisen
Copy link
Author

Clearing internal states can already be done via AfterEach-ExtensionPoint or am I wrong?

@thekingn0thing
Copy link

@mepeisen, I don't know how it could be done in jUnit5. I spoke about jUnit4 (where when we use runner we have to control test execution or if use Rule then reload all classes with PowerMock class loader) and about TestNG (where not way to programatically register listeners, so we emulate listeners by using cglib).

It'll be nice if jUnit5 give us ability to control test instance creating and programatically add ExtensionPoint to avoid asking a user add some additional annotation or make any additional action.

For example. We ask a user register somehow a PowerMock implementation of ExtensionPoint and all over stuff will be done by PowerMock.

@mepeisen
Copy link
Author

I will create an example by trying to rebuild the powermock runner. I already did it locally but ran into multiple problems trying to replace the test instance. Maybe I did something illegal with powermock :-)

@jlink
Copy link
Contributor

jlink commented Mar 14, 2016

As already said. There is currently no way to replace test instance in
JUnit5.

2016-03-14 14:30 GMT+01:00 Martin Eisengardt [email protected]:

I will create an example by trying to rebuild the powermock runner. I
already did it locally but ran into multiple problems trying to replace the
test instance. Maybe I did something illegal with powermock :-)


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

@thekingn0thing
Copy link

@jlink, I think, that suggested approach with be sufficient for PowerMock, in case if there will be a way to reloaded test Instance via PowerMock class loader, for example by making deep copy or make a new instance by using `Class.forName. I mean approach with:

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

@jlink
Copy link
Contributor

jlink commented Mar 14, 2016

I created an issue to implement that (#203) and blocked this issue until #203 will be resolved.

@johanhaleby
Copy link

@thekingnothing describes what PowerMock is doing today and it gets complex because of the so called "test chunking" feature. Just like @thekingnothing says you have the ability to execute an individual test method with a new classloader. What actually happens under the cover is that PowerMock creates a new JUnit test for each test chunk (loaded by an individual classloader) and creates an "uber test" that calls all child tests. To the end user it will look like only one test is executed but it may actually be many tests making up the "uber test".

But this is (as you can imagine) very complex and it's a feature that I don't think is used very much (and I think that actual use case for it is very slim). So I don't mind dropping this feature from PowerMock when it comes to JUnit 5.

What we do need though is a way to hook in our classloader. Doing a deep copy is very slow (and error prone, at least when it comes to copy static final fields between classloaders) so if we can avoid doing that it would be nice. If I remember things correctly (I think @thekingnothing is more up to date on this) PowerMock currently loads JUnit from our classloader (i.e. PowerMock is driving the test execution) but from what I understand JUnit 5 will drive PowerMock? I wonder if this will work with classloaders?

PowerMock also has an agent which might work for this but the problem with agents are that you need to start them explicit (well, without hacks anyways) which defines the purpose of PowerMock. Agents are also more limited than classloaders and you can't do things like suppress static initializers etc afaik.

@mepeisen
Copy link
Author

@mepeisen
Copy link
Author

This lead to a new question: I now have an Extension (NukkitExtension) that depends on another Extension (PowermockExtension). Currently the test code must ensure that both extensions are loaded and that they even are in correct order. That's ok for me because I do not expect to Play around with multiple extensions by myself.

Are there any plans to describe such dependencies and sort orders?

@jlink
Copy link
Contributor

jlink commented Mar 15, 2016

@mepeisen

To make sure that two extensions are loaded together, just introduce a meta annotation that loads both (see http://junit-team.github.io/junit5/#annotations section 3.1.1). I'm not sure that's a good approach for most cases. If two extensions must go together, just make it one - maybe through using a registrar (see http://junit-team.github.io/junit5/#extension-registration). Individual extensions should be independent from each other.

Ordering of extensions is possible (also using a registrar); It's badly documented though (on purpose), since this mechanism will probably change fundamentally till milestone 1 or 2.

As for your sample code: Why do you need two extensions in the first place? I don't really understand your example fully, but what you're doing in NukkitExtension seems to be test-case related, right? In that case I think it should rather be done in a before-each-method or the test method itself. Maybe PowermockExtension could provide the PowermockState object via the parameter resolution extension point (MethodParameterResolver)?

@jlink
Copy link
Contributor

jlink commented Mar 15, 2016

@johanhaleby Let's see if I understand you correctly...

You see two alternative (?) ways to implement PowerMock support:

  1. Via deep copying the test instance using a factory as described in InstancePostProcessor should return test instance #203. You don't like this approach because it's bad performance-wise and error-prone.
  2. Using a specialized class loader for ... yes for what parts of the test lifecycle exactly?
  • Creating the test instance
  • Running the test?
  • Running all (or some) extension points? Creating the extension points?
  • Would it be the same class loader for all tests or a tailor-made one for every test case?

Have I understood what you meant?

And yes, usually JUnit5 would drive PowerMock. That said, we could make the default class loader globally configurable. But I fear we'd be opening many cans of worms with that.

@mepeisen
Copy link
Author

Why do you need two extensions in the first place?

I personally do not need both. It is only a show case. I am fine with a single extension that does everything internally or with a meta extension that loads both in correct order. However this will be solved in final release it will be fine for me as Long as there is any solution for powermock :-)

but what you're doing in NukkitExtension seems to be test-case related, right?

The Registration of instrumented classes (the lines 58 to 63) must be done before the new test instance is created. So that the class loader knows what to do. I decided to put it into postProcess because of your answer above that it may be changed for returning a new test instance. Maybe the powermock guys find another solution for it. There are already other annotations in powermock to specify instrumented classes on test method level. Simply do not know the powermock internals :-)

@johanhaleby
Copy link

@jlink:

  1. Yes
  2. Historically PowerMock has used either:
    1. One classloader to create the test instance and run all test cases in a class
    2. One classloader to create the test instance and run all test cases excepted those explicitly defined with @PrepareForTest that are executed in another classloader

Personally I would be fine with only supporting (i). What do you think @thekingnothing?

And yes, usually JUnit5 would drive PowerMock. That said, we could make the default class loader globally configurable. But I fear we'd be opening many cans of worms with that.

I understand your concerns, but if not I guess PowerMock would have to do some hacks to try to launch JUnit with its classloader.

@mepeisen
Copy link
Author

Instead opening class loader configuration there may be another smart solution: What about delegating not only the test instance creation but the extension creation to a meta-Extension?
What I mean is a two phase initialization for junit:

  1. Look if there is a meta extension. If there is a meta extension ask it for test instance and for the Extension instances (maybe giving the classes to the meta extension).
  2. If there is no meta extension create the test instance by itself and the extension instances by searching for @ExtendWith

This willl open a whole new bunch of use cases. For example a meta Extension that filters the extensions because of annotations. For example one might disable an Extension within Hudson (=env variable) but enable it within local eclipse.

And for Pockermock it enables nice Features, for example powermock can look for @PrepareForTest even on other Extension classes and not only on the test class itself.

And yes, there should only be one meta Extension on a specific test class.

@thekingn0thing
Copy link

I think, ideally, it will be awesome if PowerMock could register anyhow global class loader (or class loader factory, which will be better, because PowerMock will still be able control class loader creating). The class loader will be used to create test instance, run test, load extension and so on.

This approach will give a chance to us to discard checking if class should be deferred to another class loader (test/application class loader) to avoid class cast exception and code will be more simple and bug-free.

By the way, @mepeisen it's an answer why we don't include "Mockito" classes to classes which always should be ignored by PowerMock class loader: some times it should be loaded via PowerMock class loader, because some internal classes is loaded only in case when you call Mockito.mock. In this case PowerMock class loader will be used to create these classes and if other Mockito classes is loaded via not PowerMock class loader then classes get incompatible.

As I mentioned now, PowerMock has an option which lets a user to specify which classes should not be loaded by PowerMock classloader. It's useful for case, when classes have been loaded before PowerMock takes control test run process.

@thekingn0thing
Copy link

@jlink,

I also understand your doubts about giving such Powerful option which could breaks all in case improper using. But what do you think to allowed register class loader or class loader factory only from trusted source which directly allowed in code. Seems to me not a lot of test frameworks need such feature. Another way will be create a one new module junit5-powermock, which could be a bridge between jUnit and PowerMock and could be delivered with jUnit5. What do you think?

@thekingn0thing
Copy link

@mepeisen,

PowerMock have to control all pointed stages:

  • Creating the test instance
  • Running the test
  • Running all extension points

to avoid class of problems which PowerMock has now with TestNG, when we can only control creating the test instance and partially controls running the test via setting context class loader. Some of them I mentioned above.

@thekingn0thing
Copy link

@johanhaleby,

Personally I would be fine with only supporting (i). What do you think @thekingnothing?

I think we can limit to use one class loader, but there are boundary cases when it could be useful. I mean load each test by a new classloader.

For example: test uses a class which has a static (may even final field). The field is filled during static initialisation with using a mocked class which generate return value base on test conditionals. After first run, the class will be initialised and field will have correct mock value, when a second test runs field will still have value from previous test. The user cannot do anything to solve the problem without changing production code.

Code:

public class ClassA{

    private static final ClassB CLASS_B_INSTANCE = ClassBFactory.create();

    public String doSomething(){
       return  doSomethingOnBResult(CLASS_B_INSTANCE.message());
    }

    private String doSomethingOnBResult(String message){
      return "_" + message + "_";
    }
} 
public class ClassB {

  public String message(){
    return "Hello";
  }

}
public class ClassBFactory{

   private static ClassBFactoryImpl factoryImpl;

   public static void setImplementer(ClassBFactoryImpl factoryImpl){
     this.factoryImpl = factoryImpl;
   }

   public static ClassB create(){
     return factoryImpl.create();
   }
}
public class TestClassA{

   private ClassA instance;
   private ClassB clessbmock; 

  @Before
  public void setUp(){
     ClassBFactoryImpl factoryImpl = mock(ClassBFactoryImpl.class);
     ClassBFactory.setImplementer(factoryImpl);

     clessbmock = mock(ClassB.class);
     when(factoryImpl.create).thenReturn(clessbmock);

     instance = new ClassA();
  }

  @Test
  public void testCaseA(){
    when(clessbmock.message()).thenReturn("Welcome"); 

    String result = instance.doSomething(); // result will be correct "_ Welcome_"

  }

  @Test
  public void testCaseB(){
    when(clessbmock.message()).thenReturn("Hi"); 

    String result = instance.doSomething(); // result will be incorrect "_ Welcome_", because  instance still use old mock and mock cannot be replaced 
  }
}

I don't see a way to handle this case without reloading class with new class loader.

@jlink
Copy link
Contributor

jlink commented Mar 15, 2016

@thekingnothing @johanhaleby
So do you think an extension point like that could suffice:

public interface TestInstanceClassLoaderProvider extends ExtensionPoint {
    ClassLoader provide(ContainerExtensionContext context);
}

The provided class loader would then be used to create a test instance and set as context class loader for the thread that is running test execution and all instance related extension points like InstancePostProcessor, Before/AfterEachExtensionPoint and ExceptionHandlerExceptionPoint.

And if that suited your needs, would the instance creation extension point from #203 still be needed?

@thekingn0thing
Copy link

@jlink

I think it's what we need and it will suffice most cases. Will the TestInstanceClassLoaderProvider be called once?

@thekingn0thing
Copy link

And yes, in case if TestInstanceClassLoaderProvider is added, then #203 will be redundant.

@jlink
Copy link
Contributor

jlink commented Mar 15, 2016

@thekingnothing The way I imagined it, it would be called once per "container" i.e. test class or nested test class. But it could also be called freshly for each test instance.

@marcphilipp
Copy link
Member

+1 for a more focused issue with a concrete use case and suggestion how to achieve it.

I'm not sure how much clearer I can express why this issue isn't resolved after "half a decade". I haven't given up hope that someone will come along with a concrete proposal instead of just complaining, yet. As for the many watchers getting notified unnecessarily, they can always unsubscribe should they no longer be interested.

marcphilipp added a commit that referenced this issue Dec 21, 2020
`LauncherDiscoveryListeners` can now be registered via Java's
`ServiceLoader` mechanism in addition to those passed as part of each
`LauncherDiscoveryRequest` and the default one.

Whether discovery listeners are picked up via ServiceLoader is
configurable via `LauncherConfig` (defaults to true).

Closes #2457. Related to #201.

Co-authored-by: Marc Philipp <[email protected]>
@demiurg906
Copy link

One more use case: in Kotlin compiler we have a lot of independent tests which may be perfectly parallelized besides one fact: when compiler initializes it changes some static state and returns it back after test is finished. So if I just enable parallel test run than this state would be in inconsistent state. Perfect solution for this problem would be have separate classloaders for each thread which used for running tests

@LunNova
Copy link

LunNova commented Sep 24, 2021

Here is a concrete example of a test which worked on JUnit 4 and doesn't work with 5 and is due to classloading changes. I am not confident that it is a central example of the overall type of issue in this thread, but it seemed worth noting here due to earlier comments about a lack of a concrete use case.

MinimallyCorrect/JavaTransformer#59

@Frontrider
Copy link

We have concrete use cases, but I'm not legally allowed to share them.

@ledoyen
Copy link
Contributor

ledoyen commented Sep 25, 2021

@TransLunarInjection the example you linked concerns the support of distinct classloaders for tests in junit-vintage-engine.

I think this is a different topic (linked but different), as the current issue concerns the existence of an API for extensions (such as a powermock one), ergo more in the junit-jupiter part.

On the orther hand, it could influence the design of a solution by moving its central parts from jupiter engine to the platform.

@gabizou
Copy link

gabizou commented Jan 15, 2022

A very unique use-case is being able to unit test that relies on class transformations via a custom class loader.

Some background: Sponge is an implementation of a Java API for Minecraft that uses its own tool (Mixin) to apply transformations on target classes (whether to implement the API interfaces, or add behaviors enabling event-based systems the API exposes), and while it'd be considered a massive integration test to "run various behaviors", we'd sooner wish to have better JUnit-like tests that could be run and verified for various API implementations that we'd better catch common bugs, and these would be better tested without having a full integration test being run.

A fair bit of the details about how the classes are transformed, we use ModLauncher as our transformative classloader to apply such large sweeping changes.

@TWiStErRob
Copy link

:( sad to see this, I'm all for using JUnit 5, but until this is done, it's not viable to set up a fully Jupiter-based Android project.

@shaburov
Copy link

opened this issue on 13 Mar 2016 · 118 comments
now - 25 Aug 2022

I wonder how many years to wait for a decision...

The author of the related problem already has grandchildren going to school...

@atsiporu
Copy link

atsiporu commented Nov 9, 2022

Hello all,
I just want to add my usecase to this discussion.

So with Junit4 I was able to run tests with my own runner via @RunWith annotation. This was super powerful and allowed me to use my own "special" class loader for each test. This "special" class loader job was to reload a subset of classes when those were accessed (actually it could implement different reloading policies but that's beside the point). What this effectively allowed me to achieve is running each test in a "sandbox".

I had multiple tests that were setting/requiring different values of static class variables running in parallel without stepping on each other toes.

My question is whether it's possible to achieve the same state of nirvana :) with a new Junit5?

Thank you very much for taking time to look and answer this.

My brute force attempt to use @ExtendWith along with custom implementation of TestInstanceFactory that was reloading class and returning instance of "reloaded" class failed miserably with the following exception:

org.junit.jupiter.api.extension.TestInstantiationException message: TestInstanceFactory [<my implementation of TestInstanceFactory class name>] failed to return an instance of [<my-test-class>@<hash as loaded by original loader>] and instead returned an instance of [<my-test-class>@<hash as loaded by my special loader>].

@atsiporu
Copy link

Here is the link to the answer I got from @ledoyen
#3028 (comment)

@marcphilipp
Copy link
Member

As it turns out, supporting PowerMock does not require any changes in JUnit if its existing Java agent is used along with a relatively simple Jupiter extension: powermock/powermock#1146. To be frank, no new APIs were needed and this could have written years ago if someone had taken the time to give it a shot.

Besides the Java agent approach which allows transforming classes as necessary, the upcoming 5.10 release will introduce LauncherInterceptor (see #3091 for details) and there's an example for replacing the class loader for all tests in the User Guide.

In #3028, we'll explore how to run tests with different classpaths. If you have a specific use case that you'd like to see supported that isn't covered by any of the above, please raise a new issue.

@jlink
Copy link
Contributor

jlink commented Feb 4, 2023

Good riddance

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