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

Support property testing with Micronaut #419

Closed
Nahuel92 opened this issue Nov 28, 2022 · 75 comments
Closed

Support property testing with Micronaut #419

Nahuel92 opened this issue Nov 28, 2022 · 75 comments

Comments

@Nahuel92
Copy link

Testing Problem

Hi there!
I would like to use property-based tests with Micronaut (via the @MicronautTest annotation) to create a much more meaningful test suite, but as of now it doesn't seem to be possible.

Suggested Solution

Please consider creating an extension for Micronaut in the same way the Jqwik team created one for Spring in the past.

Discussion

Advantages: More meaningful/interesting tests suite.
Disadvantages: An extension needs to be developed for this to work.

@jlink
Copy link
Collaborator

jlink commented Nov 29, 2022

@Nahuel92 Since no one in the jqwik team is a Micronaut user, would you be willing to take a stab at implementing such a module? Could then be delivered as a jqwik artifact through Maven.

@jlink
Copy link
Collaborator

jlink commented Nov 29, 2022

@Nahuel92 Also: Have you asked the Micronaut team if they want to support jqwik testing? Since they are already supporting a Jupiter plugin (I assume), doing the same for jqwik would be straightforward for them.

@Nahuel92
Copy link
Author

@Nahuel92 Also: Have you asked the Micronaut team if they want to support jqwik testing? Since they are already supporting a Jupiter plugin (I assume), doing the same for jqwik would be straightforward for them.

Sure, I will reach out to them to see if they are interested in adding support for Jqwik.

@Nahuel92 Since no one in the jqwik team is a Micronaut user, would you be willing to take a stab at implementing such a module? Could then be delivered as a jqwik artifact through Maven.

I can give it a try. Do you have some pointers to me on how to start developing that module? (In case the Micronaut team is not willing to support Jqwik out-of-the-box).

@adam-waldenberg
Copy link

adam-waldenberg commented Dec 1, 2022

@Nahuel92 Take a look at;

https://github.com/jlink/jqwik-spring
https://github.com/jlink/jqwik-testcontainers
https://github.com/Befrish/jqwik-vavr

Supporting Micronaut probably also means supporting Weld? There is a in-the-works plugin made by me that is discussed in #386 that is used in several live test suites. With the lifecycle hooks, extending JQwik is actually very easy to do.

While there is still room for improvement, the hook that adds support for CDI (together with support for a @WeldSetup) annotation only needed ~100 lines of code for the actual implementation. Here it is for reference:

https://github.com/unigrid-project/janus-java/blob/master/fx/src/test/java/org/unigrid/janus/jqwik/WeldHook.java

This hook also has code to suport stuff like:

@Inject @Instances(20)
private List<InjectedClass> injected;

... which actually initializes the list and injects 20 instances, each from a separate CDI container - great for tests where you want to run multiple containers. So strip that away and the hook becomes even shorter.

@jlink-workshop
Copy link

@Nahuel92 @adam-waldenberg So it would probably make sense to accelerate the creation of a jqwik organization on GitHub. If I only had a bit more time... :-/

@jlink
Copy link
Collaborator

jlink commented Feb 9, 2023

I just created an organisation for all jqwik-related development: https://github.com/jqwik-team/

@Nahuel92 If you want to start work on micronaut extension I can create a repo and add you as collaborator

@Nahuel92
Copy link
Author

I just created an organisation for all jqwik-related development: https://github.com/jqwik-team/

@Nahuel92 If you want to start work on micronaut extension I can create a repo and add you as collaborator

Hi, sorry for the delay. I can give it a try on my spare time.

@Nahuel92
Copy link
Author

Nahuel92 commented Mar 9, 2023

Hi there. I started working on the Micronaut extension for Jqwik but I am not quite sure about how to integrate the Micronaut's TestContext class with the Jqwik's RegistrarHook interface (actually, the custom implementation class I am developing that implements that interface).
I tried following the JqwikSpringExtension as part of jqwik-spring and, even though I made some progress, I am currently blocked. Not sure where to ask for help or guidance (or any idea that can help me to continue progressing).
Any help or suggestion is welcomed.

@jlink
Copy link
Collaborator

jlink commented Mar 10, 2023

@Nahuel92 Can you point to your repo? Or you can bring it over to a repo within jqwik-team. Then we can all have a look.

@Nahuel92
Copy link
Author

Nahuel92 commented Mar 10, 2023

I don't think I have permissions to create repos within jqwik-team but I have created my own repo here: https://github.com/Nahuel92/jqwik-micronaut-extension
Once this works and reaches an acceptable quality (i.e. production-ready) you can bring it over to this organization where it belongs to.

What I have so far compiles but doesn't work but I will spend some time today to see if I can find where the problem is.
I suspect it is because I am creating a new ApplicationContext in the method getOrCreateTestContextStore from the class JqwikMicronautExtension.
I still don't understand how things integrate with each other and there are classes not being used, but I expect to have a first working version before cleaning up everything.

@jlink
Copy link
Collaborator

jlink commented Mar 11, 2023

@Nahuel92 I invited you as a maintainer to https://github.com/jqwik-team/jqwik-micronaut

I skimmed very shortly through your repo. What I noticed is that there still seems to be a dependency on Jupiter. I'd hope one can get rid of it, but I haven't dived deep enough yet.

Feel free to ask questions if you have any. We could also do an online session if you need more information on the hooks mechanism - it's not really documented well enough.

@adam-waldenberg
Copy link

@Nahuel92 What you want to do here is look at the code from the Junit extension and then move it into a JQwik hook. I would start by getting rid of any dependencies on Junit/Jupiter.

@Nahuel92
Copy link
Author

I accepted the invitation. Totally, we can have an online session but we need to coordinate the date and time (my timezone is GMT-3, or Buenos Aires time).

In the meanwhile, I will do my best to make some progress integrating Micronaut with the Jqwik hooks.
As of now, the only blocker I think I have is the BeforeContainerHook interface not exposing targetMethod() and testInstance()` required to initialize the Micronaut container before tests are executed.

Maybe I am misunderstanding a few things here and the approach I'm taking is incorrect, so I will continue reading the code and trying to figure out how things should be connected together.

@jlink
Copy link
Collaborator

jlink commented Mar 12, 2023

BeforeContainerHook cannot know the method, because a container (class) can have many. You probably want to use AroundPropertyHook.

@Nahuel92
Copy link
Author

I have spent some time playing around AroundPropertyHook but I am doing something wrong because I can't make a simple test to pass.

This one:

@JqwikMicronautTest
class JqwikMicronautExtensionTest {
    @Inject
    EmbeddedApplication<?> application;

    @Property(tries = 1)
    void testItWorks() {
        assertThat(application.isRunning()).isTrue();
    }
}

Fails because application is null. Not sure how/when to create a TestContext to initialize and inject class fields. I will spend some more time on this on next weekend to see if I can figure it out.

@jlink
Copy link
Collaborator

jlink commented Mar 13, 2023

If you push the actual code to https://github.com/Nahuel92/jqwik-micronaut-extension I can take a look.

Some probable sources of confusion:

  1. A hook is not applied to a property method because the hook's appliesTo method excludes it.
  2. A hook is applied at the wrong time in relation to other lifecycle hooks.
    This can be controlled by overriding the various ???Proximity methods, eg aroundPropertyProximity() for the AroundPropertyHook.

@Nahuel92
Copy link
Author

Ups, my bad. Yesterday I spent some time on this but I forgot to commit and push my changes.
I just pushed all my current changes and I will play around with your suggestions to see if I can identify what is happening. Thanks!

@jlink
Copy link
Collaborator

jlink commented Mar 14, 2023

What jumps into the eye at first glance:

  • AbstractMicronautExtension.beforeClass(..) should probably be called in a BeforeContainerHook not in AroundPropertyMicronaut
  • AbstractMicronautExtension.beforeEach(..) is never called, so dependency injection won't take place. This one should probably happen in AroundPropertyMicronaut.aroundProperty(..) { ... }

@Nahuel92
Copy link
Author

Nahuel92 commented Mar 14, 2023

That partially worked like a charm. Unfortunately, if I don't call beforeClass() in the aroundProperty() method the applicationContext (among other stuff) is not initialized.

The beforeClass() method is run as part of the beforeContainer() but it seems that the initialized objects are lost at some point.

But at least we made one test to pass. Now we can start building on top of this code.

I will see if I can figure out why the beforeContext() initialization is ignored.

P.S.: My latest changes are pushed.

@jlink
Copy link
Collaborator

jlink commented Mar 15, 2023

The problem as I see it is that the jqwik hooks use extends JqwikMicronautExtension. This leads to several instances of JqwikMicronautExtension but you always need the same instances since the micronaut context objects are saved as members.

The jqwik way of tackling this is to use the Store mechanism to create and retrieve a singleton instance of JqwikMicronautExtension.

Here's a patch of how'd I tackle it (I'm currently too lazy to fork the repo for a PR branch):

Subject: [PATCH] Move JqwikMicronautExtension instance to store
---
Index: src/main/java/net/jqwik/micronaut/extension/JqwikMicronautExtension.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/net/jqwik/micronaut/extension/JqwikMicronautExtension.java b/src/main/java/net/jqwik/micronaut/extension/JqwikMicronautExtension.java
--- a/src/main/java/net/jqwik/micronaut/extension/JqwikMicronautExtension.java	(revision b3e674a83b127a032988626749ce0406446c11b1)
+++ b/src/main/java/net/jqwik/micronaut/extension/JqwikMicronautExtension.java	(revision 3259cf129030835c9dd65d246012e610ccb01f6f)
@@ -2,19 +2,34 @@
 
 import io.micronaut.aop.InterceptedProxy;
 import io.micronaut.context.ApplicationContext;
+import io.micronaut.context.annotation.*;
 import io.micronaut.core.util.CollectionUtils;
 import io.micronaut.inject.FieldInjectionPoint;
 import io.micronaut.test.annotation.MicronautTestValue;
 import io.micronaut.test.annotation.MockBean;
 import io.micronaut.test.extensions.AbstractMicronautExtension;
 import io.micronaut.test.support.TestPropertyProvider;
-import net.jqwik.api.lifecycle.LifecycleContext;
 
-import java.lang.reflect.Field;
-import java.util.Map;
-import java.util.Optional;
+import net.jqwik.api.lifecycle.*;
+
+import java.lang.reflect.*;
+import java.util.*;
 
 public class JqwikMicronautExtension extends AbstractMicronautExtension<LifecycleContext> {
+
+    public static Store<JqwikMicronautExtension> extensionStore =
+        Store.getOrCreate(JqwikMicronautExtension.class, Lifespan.RUN, () -> new JqwikMicronautExtension());
+
+    @Override
+    public void beforeEach(LifecycleContext context, Object testInstance, AnnotatedElement method, List<Property> propertyAnnotations) {
+        super.beforeEach(context, testInstance, method, propertyAnnotations);
+    }
+
+    @Override
+    public void beforeClass(LifecycleContext context, Class<?> testClass, MicronautTestValue testAnnotationValue) {
+        super.beforeClass(context, testClass, testAnnotationValue);
+    }
+
     @Override
     protected void resolveTestProperties(final LifecycleContext context, final MicronautTestValue testAnnotationValue,
                                          final Map<String, Object> testProperties) {
Index: src/main/java/net/jqwik/micronaut/hook/AroundPropertyMicronaut.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/net/jqwik/micronaut/hook/AroundPropertyMicronaut.java b/src/main/java/net/jqwik/micronaut/hook/AroundPropertyMicronaut.java
--- a/src/main/java/net/jqwik/micronaut/hook/AroundPropertyMicronaut.java	(revision b3e674a83b127a032988626749ce0406446c11b1)
+++ b/src/main/java/net/jqwik/micronaut/hook/AroundPropertyMicronaut.java	(revision 3259cf129030835c9dd65d246012e610ccb01f6f)
@@ -1,10 +1,8 @@
 package net.jqwik.micronaut.hook;
 
 import io.micronaut.test.annotation.MicronautTestValue;
-import net.jqwik.api.lifecycle.AroundPropertyHook;
-import net.jqwik.api.lifecycle.PropertyExecutionResult;
-import net.jqwik.api.lifecycle.PropertyExecutor;
-import net.jqwik.api.lifecycle.PropertyLifecycleContext;
+
+import net.jqwik.api.lifecycle.*;
 import net.jqwik.micronaut.annotation.JqwikMicronautTest;
 import net.jqwik.micronaut.extension.JqwikMicronautExtension;
 import org.junit.platform.commons.support.AnnotationSupport;
@@ -12,17 +10,12 @@
 import java.util.List;
 
 public class AroundPropertyMicronaut extends JqwikMicronautExtension implements AroundPropertyHook {
+
     @Override
     public PropertyExecutionResult aroundProperty(final PropertyLifecycleContext context,
                                                   final PropertyExecutor property) {
-        // Without this `beforeClass` here, the test fails. It seems that the initialization done
-        // in the `BeforeContextHook` is lost at this point.
-        beforeClass(
-                context,
-                context.optionalContainerClass().orElse(null),
-                buildMicronautTestValue(context.optionalContainerClass().orElse(null))
-        );
-        beforeEach(
+
+        JqwikMicronautExtension.extensionStore.get().beforeEach(
                 context,
                 context.testInstance(),
                 context.targetMethod(),
Index: src/main/java/net/jqwik/micronaut/hook/BeforeMicronautContainerHook.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/net/jqwik/micronaut/hook/BeforeMicronautContainerHook.java b/src/main/java/net/jqwik/micronaut/hook/BeforeMicronautContainerHook.java
--- a/src/main/java/net/jqwik/micronaut/hook/BeforeMicronautContainerHook.java	(revision b3e674a83b127a032988626749ce0406446c11b1)
+++ b/src/main/java/net/jqwik/micronaut/hook/BeforeMicronautContainerHook.java	(revision 3259cf129030835c9dd65d246012e610ccb01f6f)
@@ -10,9 +10,7 @@
 public class BeforeMicronautContainerHook extends JqwikMicronautExtension implements BeforeContainerHook {
     @Override
     public void beforeContainer(final ContainerLifecycleContext context) {
-        // This is called before `aroundProperty` but, when at the time that method executes,
-        // the `applicationContext` (among other stuff) is lost.
-        beforeClass(
+        JqwikMicronautExtension.extensionStore.get().beforeClass(
                 context,
                 context.optionalContainerClass().orElse(null),
                 buildMicronautTestValue(context.optionalContainerClass().orElse(null))



@jlink
Copy link
Collaborator

jlink commented Mar 15, 2023

BTW, the approach above use a single instance of JqwikMicronautExtension for the whole test run by using Lifecycle.RUN.
This might be wrong depending on how the extension caches values per test run / class / or method.

@Nahuel92
Copy link
Author

Nahuel92 commented Mar 15, 2023

Thank you.
I wasn't 100% sure how to use that Store class and, in fact, I suspected it was a way to represent a single instance of the applicationContext but I couldn't figure out the missing pieces.
That worked, now I will be working to provide support for mocks.
I added Mockito as a dependency but not sure if that is a good first step (considering that there are multiple mocking libraries).
In any case, the code I have so far fails to initialize mocks and I will investigate why that is happening.

@jlink
Copy link
Collaborator

jlink commented Mar 16, 2023

I'd try to stay as close to the Jupiter extension as possible. If the Jupiter extension comes with Mockito support by default I'd do it here as well. This issue may be interesting since it shows a straightforward implementation of a MockitoHook: #261

@Nahuel92
Copy link
Author

I will take a look at that code but, correct me if I'm wrong, I believe it will help in unit tests rather than tests that require an applicationContext.
The code Micronaut has for the Jupiter extension uses a method called alignMocks which I implemented here but, for some reason, the annotation @MockBean is not being processed (isMock is always false).

On the other hand, I am also struggling with dynamic (application) properties injection.
My understanding is that it should happen in a beforeClass method, but at that point the Jqwik context (ContainerLifecycleContext) doesn't know/has the testInstance object (only the Class<?> of it).

This is important since the Micronaut's way of injecting dynamic properties is via implementing an interface (TestPropertyProvider) that exposes a method that returns a map of properties to be added to the applicationContext.
This leads me to think that I should grab the testInstance object, make sure it implements the aforementioned interface, call that method and add all the resulting properties into the applicationContext.
Maybe my approach is wrong, I will keep investigating.

Apologies for the million questions I keep asking :)

P.S. I also added a few more tests.

@jlink
Copy link
Collaborator

jlink commented Mar 17, 2023

The code Micronaut has for the Jupiter extension uses a method called alignMocks which I implemented here but, for some reason, the annotation @MockBean is not being processed (isMock is always false).

The only place in JqwikMicronautExtension where @MockBean is referenced is in line 63. There it's only about injected fields, never about annotated methods. So I assume, some code is missing to handle annotated methods.

@jlink
Copy link
Collaborator

jlink commented Mar 17, 2023

On the other hand, I am also struggling with dynamic (application) properties injection.
My understanding is that it should happen in a beforeClass method, but at that point the Jqwik context (ContainerLifecycleContext) doesn't know/has the testInstance object (only the Class<?> of it).

The Jupiter Micronaut extension should have the same problem - unless they are enforcing @TestInstance(Lifecycle.PER_CLASS) somehow. How are they dealing with it there?

@Nahuel92
Copy link
Author

The code Micronaut has for the Jupiter extension uses a method called alignMocks which I implemented here but, for some reason, the annotation @MockBean is not being processed (isMock is always false).

The only place in JqwikMicronautExtension where @MockBean is referenced is in line 63. There it's only about injected fields, never about annotated methods. So I assume, some code is missing to handle annotated methods.

That worked, though my first approach was the same that the Micronaut team took in the
alignMocks method for jUnit. I ended up writting some logic that seems to be working fine, at least for the test I wrote to validate that feature. Thanks for the advice.

On the other hand, I am also struggling with dynamic (application) properties injection.
My understanding is that it should happen in a beforeClass method, but at that point the Jqwik context (ContainerLifecycleContext) doesn't know/has the testInstance object (only the Class<?> of it).

The Jupiter Micronaut extension should have the same problem - unless they are enforcing @TestInstance(Lifecycle.PER_CLASS) somehow. How are they dealing with it there?

I am not 100% sure but, based on the beforeAll method, it seems that there is something like you mentioned happening at some point.

On the other hand, I noticed jUnit has only one context interface and a couple of implementing classes.
In my humble opinion, that makes it easier to work with it but I am aware I feel that way because of my lack of knowledge, not only with Jqwik's internals, but also with testing frameworks' internals in general also.

Anyway, I will continue working on that dynamic properties feature to see what I can do. In the future, I guess I will do good by copying the test battery used in the jUnit extension to make sure this one has all, or at least most of, the features that that one has.

If you feel that is not good or there is a better approach, by all means please let me know. This is the very first project I contribute to (or try to do so) so any suggestion is more than welcomed. Thanks :)

@jlink
Copy link
Collaborator

jlink commented Mar 18, 2023

The only place in JqwikMicronautExtension where @MockBean is referenced is in line 63. There it's only about injected fields, never about annotated methods. So I assume, some code is missing to handle annotated methods.

That worked, though my first approach was the same that the Micronaut team took in the
alignMocks method for jUnit. I ended up writting some logic that seems to be working fine, at least for the test I wrote to validate that feature. Thanks for the advice.

On second thought, I don't know why the following lines, that you had in before, don't work for jqwik but do for Jupiter:

 boolean isMock = applicationContext.resolveMetadata(injectedField.getType())
                                .isAnnotationPresent(MockBean.class);

At first, it looked like it was about fields, but it actually is about a field's metadata, so it probably determines if there is a mock method for a field's type. First assumption, the code in beforeEach is important.

I am not 100% sure but, based on the beforeAll method, it seems that there is something like you mentioned happening at some point.

Well, they have special handling for the case of Lifecycle.PER_CLASS. Since jqwik does not have that feature you should be able to safely ignore it.

If you feel that is not good or there is a better approach, by all means please let me know. This is the very first project I contribute to (or try to do so) so any suggestion is more than welcomed. Thanks :)

No worries, your effort is appreciated. In the end, the code in JqwikMicronautExtension should almost look like MicronautJunit5Extension - except for the features that are not provided by jqwik - like a class-based test lifecycle. As for the rest, I'd assume the writers of that extension are more experts than we are - that's definitely true for me.

@Nahuel92
Copy link
Author

I just pushed code to support dynamic properties injection. I have concerns regarding the creation of a test instance solely for the sake of getting the properties to inject via its getProperties() method but at least it is working now.

I believe we are getting close to have something functional, so I will spend some time trying to bring tests from the MicronautJupiterExtension to this one. I want to make sure we are covering at least the main use cases before considering it "production-ready".

Please feel free to add any suggestion/advice, I really appreciate your feedback on this. Thanks!

@jlink
Copy link
Collaborator

jlink commented Apr 6, 2023

I'm quite certain that the transaction handling problem is due to missing calls to interceptBeforeEach(..).
In the Junit5 extension this is where the TransactionSynchronisationManger is being initialized.

So my suggestion is that you call all intercept hooks with AroundPropertyHook implementations and fitting proximity values:

  • interceptBefore/AfterEach: proximity -15
    Alternatively, those could be invoked in the existing AroundPropertyMicronaut but that might prevent you from making the micronaut lifecycle configurable "per property" or "per try". But I'm just guessing here.

  • interceptTest: proximity -5
    I wonder if this should be called in a AroundTryHook instead. But again, I know too little about Micronaut to really say.

Have a look at MicronautJunit5Extension.interceptBefore/AfterEachMethod(..) which handles the creation of the required TestMethodInvocationContext.

Maybe doing this will also resolve the entity manager issues.

@Nahuel92
Copy link
Author

Nahuel92 commented Apr 7, 2023

Thanks, that partialy worked for the JpaNoRollbackTest class.
A few JPA tests are fixed but others are still failing (just in case, I pushed the latest changes). BTW I added the remaining JPA tests from the MicronautJunit5Extension (this means more tests than yesterday are failing, all of them from JPA).

I added the interceptors you mentioned, but not sure if I added them in the right place.
Perhaps I need to play around those interceptTest and the AroundTryHook you also suggested.
In that regard, Micronaut shouldn't be too diferent to Spring to be honest.

@Nahuel92
Copy link
Author

Nahuel92 commented Apr 7, 2023

Well, I made most tests to pass but I cheated. I added hacky code to begin/rollback a tx accordingly in the @BeforeProperty/@AfterProperty methods. The only one refusing to pass is the one from TransactionalTest.

You are absolutely right regarding the need to call the intercept methods from the extension, but it seems I still can't make them being invoked the right way.

I will revisit the AroundPropertyMicronaut code to see if I can find where is the mistake. I believe I haven't implemented your second suggestion so far (calling interceptTest with proximity -5 from an AroundTryHook) and I will see if that is the missing piece.

@Nahuel92
Copy link
Author

Nahuel92 commented Apr 7, 2023

I just pushed my latest changes including those two new hooks you mentioned.
As of now, only one test fails (actually, it's a flaky test) from TransactionalTest.
Also, my hacky code is still there. We shouldn't need to manually control transactions in tests so I will spend some more time trying to figure out why automatic transaction handling is failing to happen even though we are calling those interceptMethod and interceptBefore and interceptAfter methods.

@jlink
Copy link
Collaborator

jlink commented Apr 11, 2023

At first glance the new hooks look alright. One might argue if TestInterceptor should be around property or around try - probably this should be configurable in the long run. I changed it to around property but that didn't make a difference as for the failing test. I can try to dig deeper down today or later this week.

@jlink
Copy link
Collaborator

jlink commented Apr 11, 2023

What I noticed is that the JUnitExtension is doing a bit more than just calling the intercept methods.
E.g:

@Override
public void interceptTestMethod(Invocation<Void> invocation, ReflectiveInvocationContext<Method> invocationContext, ExtensionContext extensionContext) throws Throwable {
    interceptTest(new TestMethodInvocationContext<Object>() {
        TestContext testContext;

        @Override
        public TestContext getTestContext() {
            if (testContext == null) {
                testContext = buildContext(extensionContext);
            }
            return testContext;
        }

        @Override
        public Object proceed() throws Throwable {
            return invocation.proceed();
        }
    });
}

Maybe invocation.proceed() is critical for some micronaut functionality? Just guessing, though.

@jlink
Copy link
Collaborator

jlink commented Apr 11, 2023

There's more code missing in JqwikMicronautExtension that's being called in MicronautJUnit5Extension, e.g.
JqwikMicronautExtension.beforeEach should probably also do the injectEnclosingTestInstances:

@Override
public void beforeEach(final LifecycleContext context, final Object testInstance,
                       final AnnotatedElement method, final List<Property> propertyAnnotations) {
    injectEnclosingTestInstances(context);
    super.beforeEach(context, testInstance, method, propertyAnnotations);
}

private void injectEnclosingTestInstances(LifecycleContext lifecycleContext) {
    if (lifecycleContext instanceof PropertyLifecycleContext) {
        PropertyLifecycleContext propertyLifecycleContext = (PropertyLifecycleContext) lifecycleContext;
        propertyLifecycleContext.testInstances().forEach(applicationContext::inject);
    }
}

All in all I suggest you look at all relevant lifecycle hooks in MicronautJUnit5Extension and make the ones in JqwikMicronautExtension to behave as similar as possible. Otherwise, each failing test and each diverging behaviour will require to check if another piece of it is missing.

Some are not relevant, though. I could identify: interceptTestTemplateMethod and interceptTestFactoryMethod just because jqwik does provide neither template methods nor factory methods.

@Nahuel92
Copy link
Author

That helped a lot, thanks a lot for the suggestion. I really missed those methods you mentioned, I guess I'm kind of blind these days :)

Now I was able to get rid of the flaky test and I also could remove most of the hacky code except from 3 test classes. The weird thing is, if I remove the hacky code from those 3 remaining test classes, they not only fail but also the test from TransactionalTest fails again.

I will revisit the extension code to see if there is more code missing from the MicronautJunit5Extension that is causing this misbehavior.
One concern I have is about screwing up with the hooks I have created and their proximity. Not quite sure if they are ok or need to be tuned.

But, for now, I will go through the MicronautJunit5Extension code searching for any piece of code missing from our extension.

@jlink
Copy link
Collaborator

jlink commented Apr 12, 2023

The hooks and their proximity values look good to me.
You probably need tests with more than a single try to check if resetting behaviour is triggered correctly between tries.

Which code do you consider "hacky"? I could see if I have less hacky ideas for it.

@Nahuel92
Copy link
Author

Nahuel92 commented Apr 12, 2023

Basically I passed broken tests by cheating and doing manual transaction control.

Please take a look at:

And search for my comments // FIXME: This should be done automatically (which are sprinkled throughout those classes).
If we get them to pass without cheating, (I believe) we will be ready to release it. At least I included most of the MicronautJunit5Extension tests. But we can add our own if you think so.
I wonder if that is due to the fact that multiple hooks call property.execute(), causing the test method to be executed more than once. If that is the case, I have no clue on how to work around it.

One thing that I am not sure about is if Jqwik supports nested tests (I skimmed the docs and I haven't seen it). I'm mentioning this because I haven't brought the nested tests to this extension.

@jlink
Copy link
Collaborator

jlink commented Apr 13, 2023

One thing that I am not sure about is if Jqwik supports nested tests (I skimmed the docs and I haven't seen it). I'm mentioning this because I haven't brought the nested tests to this extension.

https://jqwik.net/docs/current/user-guide.html#grouping-tests

@jlink
Copy link
Collaborator

jlink commented Apr 13, 2023

Basically I passed broken tests by cheating and doing manual transaction control.

If you look at https://micronaut-projects.github.io/micronaut-test/latest/guide/#_transaction_semantics, they write that org.springframework:spring-tx is required in the classpath. I couldn't find it in the list of dependencies, maybe that's the problem?

@Nahuel92
Copy link
Author

I tried adding that dependency but unfortunately it didn't work.
I also bumped-up dependencies versions in the hope that it might have been caused by a dependency bug and, finally, added a missing afterClass method call that broke even more tests than before (related to NPE, maybe I added the call in the wrong hook).
Those changes were pushed. including the hacky code removal so it's more clear which tests fail.

Not sure where to keep looking at, but I will compare again the MicronautJunit5Extension code to ours to see if I can find anything that may be causing this misbehavior.

The only thing I am aware of is that for the TestMethodInvocationContext object, we are returning null. otherwise more tests break because of extra mock calls made.
I'm not quite sure about how the right approach is here because the jUnit extension uses Invocation<Void> whilst Jqwik offers executing the property/try directly (thus, increasing the amount of times test is executed, altering the amount of times a mock is called). I will leave it as null for now.

@jlink
Copy link
Collaborator

jlink commented Apr 14, 2023

also bumped-up dependencies versions in the hope that it might have been caused by a dependency bug and, finally, added a missing afterClass method call that broke even more tests than before (related to NPE, maybe I added the call in the wrong hook).

AfterMicronautContainer must set a suitable proximity. I suggest -20:

public class AfterMicronautContainer implements AfterContainerHook {
    ...
    @Override
    public int afterContainerProximity() {
        // Run it before @BeforeContainer and after @AfterContainer
        return -20;
    }
}

This will get rid of the error in JpaNoRollbackTest.

@jlink
Copy link
Collaborator

jlink commented Apr 14, 2023

I traced the behaviour of one of the Jpa tests, first with Jupiter then with jqwik:

Jupiter Trace

MicronautJunit5Extension.beforeAll
MicronautJunit5Extension.beforeEach
MicronautJunit5Extension.interceptBeforeEachMethod
  JpaSingleTransactionNoSetupTest.beforeEachMethod
MicronautJunit5Extension.beforeTestExecution
MicronautJunit5Extension.interceptTestMethod
  JpaSingleTransactionNoSetupTest.testPersistOne
MicronautJunit5Extension.afterTestExecution
MicronautJunit5Extension.interceptAfterEachMethod
  JpaSingleTransactionNoSetupTest.afterEachMethod
MicronautJunit5Extension.afterEach
MicronautJunit5Extension.beforeEach
MicronautJunit5Extension.interceptBeforeEachMethod
  JpaSingleTransactionNoSetupTest.beforeEachMethod
MicronautJunit5Extension.beforeTestExecution
MicronautJunit5Extension.interceptTestMethod
  JpaSingleTransactionNoSetupTest.testPersistTwo
MicronautJunit5Extension.afterTestExecution
MicronautJunit5Extension.interceptAfterEachMethod
  JpaSingleTransactionNoSetupTest.afterEachMethod
MicronautJunit5Extension.afterEach
MicronautJunit5Extension.afterAll

jqwik Trace

Jqwik.beforeContainer
Jqwik.beforeEach
Jqwik.beforeTestExecution
Jqwik.interceptBeforeEachMethod
  JpaSingleTransactionNoSetupTest.beforePropertyMethod
  JpaSingleTransactionNoSetupTest.testPersistOne
  JpaSingleTransactionNoSetupTest.afterPropertyMethod --> throws AssertionError
Jqwik.beforeEach
Jqwik.interceptBeforeEachMethod
  JpaSingleTransactionNoSetupTest.beforePropertyMethod
  JpaSingleTransactionNoSetupTest.testPersistTwo
  JpaSingleTransactionNoSetupTest.afterPropertyMethod --> throws AssertionError
Jqwik.afterContainer

What can be seen is that with Jupiter the BeforeEach- and AfterEach methods in the test class are separated from the test method by beforeTestExecution, interceptTestMethod, afterTestExecution and interceptAfterEachMethod.
This code is either completely missing in the jqwik extension or executed at the wrong time.
Something in-between the actual test method and the after-property method must be called to roll back the JPA transaction. But there is no call there.

Sadly, the trace also shows that there's a bug in jqwik's extension mechanism, because the around property hook won't finish with a return but end with an AssertionError instead. This is not the cause of the failing test, but it's a bug all the same: #473. I'll fix it in 1.7.4

Same recommendation as before: Make JqwikMicronautExtension fully symmetrical to MicronautJunit5Extension and then check that the proximities of jqwik hooks are configured so that the order of Jupiter calls is preserved.

@jlink
Copy link
Collaborator

jlink commented Apr 14, 2023

You should use version 1.7.4-SNAPSHOT of jqwik now. 1.7.4 will be released in time for this extension.

Your Gradle file requires another repository then:

repositories {
    ...
    # For snapshot releases only:
    maven { url 'https://s01.oss.sonatype.org/content/repositories/snapshots' }
}

@Nahuel92
Copy link
Author

Thanks for sharing that :)

I upgraded the Jqwik dependency, fixed the hook proximity (which, in turn fixed that test you mentioned) and checked the hooks execution and you were absolutely right but, after fixing the execution order tests keeps failing (I even tried changing the AroundTryHook to a AroundPropertyHook with no luck).

In the same vein, I added a few primitive logs to track the hooks execution (not sure what technique you have used, but I tried using Intellij's CPU profiler and I got tons of method calls that I couldn't filter properly and I am not sure how to use VisualVM when running unit tests). Code was rearranged too, in an attempt to improve legibility/maintainability.

On the other hand, I checked again the code and we are doing roughly the same MicronautJunit5Extension is doing. The only difference I can think of is the use we are doing of the TestMethodInvocationContext class (specifically, the proceed method). They pass an Invocation<Void> parameter and call it in that method, but we don't have such thing.

Anyway, I will continue looking into this so see what I can find.

@jlink
Copy link
Collaborator

jlink commented Apr 15, 2023

The order of calls is still not as it is in the JUnit5 extension.

If you don't mind, I'll try a pull request to show you what I mean.

@jlink
Copy link
Collaborator

jlink commented Apr 15, 2023

I created a pull request that seems to fix all failing tests: Nahuel92/jqwik-micronaut-extension#1

The decisive problem was that Jupiter's intercept mechanism just works differently than I had thought.
I kind of simulated that using jqwik hooks. There are still some TODOs which should be discussed.

@jlink
Copy link
Collaborator

jlink commented Apr 15, 2023

I think there should be a way to configure if things like transactions will be rolled-back per try or per property.
Currently, everything is done per property, ie having more than one try in those tests will fail:

@JqwikMicronautTest(transactionMode = TransactionMode.SINGLE_TRANSACTION)
@DbProperties
class JpaSingleTransactionNoSetupTest {
    @Inject
    private EntityManager entityManager;

    @AfterProperty
    void tearDown() {
        // check test was rolled back
        final CriteriaQuery<Book> query = entityManager.getCriteriaBuilder().createQuery(Book.class);
        query.from(Book.class);
        assertThat(entityManager.createQuery(query).getResultList()).isEmpty();
    }

    @Property(tries = 10)
    void testPersistOne() {
        System.out.println("#### testPersistOne");
        final Book book = new Book();
        book.setTitle("The Stand");
        entityManager.persist(book);

        final CriteriaQuery<Book> query = entityManager.getCriteriaBuilder().createQuery(Book.class);
        query.from(Book.class);
        assertThat(entityManager.createQuery(query).getResultList().size()).isEqualTo(1);
    }
}

This fails with

JpaSingleTransactionNoSetupTest:testPersistOne = 
  org.opentest4j.AssertionFailedError:
    expected: 1
     but was: 2

in the second try.

@Nahuel92
Copy link
Author

Thank you very much, that worked like a charm. I did a few minor changes and now I am in condition to start addressing your suggestions regarding what you mentioned about rolling-back txs per property or per try.

@jlink
Copy link
Collaborator

jlink commented Apr 16, 2023

I suggest this is the right time to do the downgrade to Java 8 and move it over to https://github.com/jqwik-team/jqwik-micronaut.
This will allow easier collaboration and use GH issues to manage bugs and missing features etc.

@Nahuel92
Copy link
Author

Yes, I will move the extension to the repo it belongs to. I also updated the java-8 branch with the latest changes, so we are covered there.

Unfortunately, tests now fail when run together but they all work when run each in isolation.
According to my investigation, it started happening after jqwik:1.7.4-SNAPSHOT was introduced in the project. I need to double-check this, though.

@Nahuel92
Copy link
Author

I have been delving into a bit into the code to see why tests fail when I run them together but pass when run individually and I suspect that something might be happening in Jqwik (but I can't confirm it yet).

I couldn't test it enough due to a lack of time, but here are my findings on the subject.

Using the debugger, I came to this point in Jqwik's code.
Note how there is only one hook in the resolveParameterHooks collection (no signs of our ParameterResolver). This code will resolve to Optional.empty and, thus, exception will be thrown:

Test run together

Test run together error

Notice in the previous screenshot how the app flow goes into that .orElseThrow() method.

Now, same test that failed before but this time ran in isolation:

Test run isolated

It includes our ParameterResolver and the parameter is resolved as expected, passing the test without issues.

Again, not sure yet if this is a Jqwik issue or an issue in the extension itself. I need to keep looking into this to figure out.

P.S.: We can close this issue and move the conversation to the extension repo if you prefer so.

@jlink
Copy link
Collaborator

jlink commented Apr 19, 2023

Thanks for diving in. I will have a look.

@jlink
Copy link
Collaborator

jlink commented Apr 19, 2023

It's definitely a bug I introduced in 1.7.4-SNAPSHOT. I'll track it down and tell you when it's solved. Sorry for that.

@jlink
Copy link
Collaborator

jlink commented Apr 19, 2023

If you refresh 1.7.4-SNAPSHOT dependency everything should be fine now.

@Nahuel92
Copy link
Author

Worked like a charm now, thank you. I will go ahead and close this issue since we have the extension created in its own repo. If anything pops up, we can address it there.

Thanks a lot for your help, suggestions, effort and time :)

@Nahuel92
Copy link
Author

Closing issue as we have a Micronaut extension for Jqwik here.

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

4 participants