jqwik-team / jqwik

Property-Based Testing on the JUnit Platform
http://jqwik.net
Eclipse Public License 2.0
575 stars 64 forks source link

Support property testing with Micronaut #419

Closed Nahuel92 closed 1 year ago

Nahuel92 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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() andtestInstance()` 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 commented 1 year ago

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

Nahuel92 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

What jumps into the eye at first glance:

Nahuel92 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/jqwik-team/jqwik/issues/261

Nahuel92 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Creating an instance just for the properties looks like a hack. I checked the Jupiter extension and it only works there if lifecycle is set to class-based - not possible in jqwik. I see two options:

I'd probably go with the latter, comment it really well and wait for problems.

jlink commented 1 year ago

I noticed you're using Java language features beyond Java 8. Since jqwik 1.x has Java 8 as base version, please switch to Java 8 features. E.g.:

@Override
protected void resolveTestProperties(LifecycleContext context, MicronautTestValue testAnnotationValue, Map<String, Object> testProperties) {
    context.optionalContainerClass().ifPresent(testContainerClass -> {
        final Object testClassInstance = context.newInstance(testContainerClass);

        if (testClassInstance instanceof TestPropertyProvider) {
            TestPropertyProvider propertyProvider = (TestPropertyProvider) testClassInstance;
            final Map<String, String> dynamicPropertiesToAdd = propertyProvider.getProperties();
            testProperties.putAll(dynamicPropertiesToAdd);
        }
    });
}
Nahuel92 commented 1 year ago

Creating an instance just for the properties looks like a hack. I checked the Jupiter extension and it only works there if lifecycle is set to class-based - not possible in jqwik. I see two options:

* Don't support that feature

* Restructure code so that the properties are resolved once per test method instead of once per class.

* Go with the hack and wait for problems to show up - which might be never - but there is the potential that some people will access state in their `getProperties()` implementation, which could be different for each jqwik property or even for each try.

I'd probably go with the latter, comment it really well and wait for problems.

According to the TestPropertyProvider documentation (please search for TestPropertyProvider as there is no anchor I can link to), when implemented, test class should be annotated with @TestInstance(TestInstance.Lifecycle.PER_CLASS). To me, that sounds that we are kind of covered in that regard. If someone breaks the TestPropertyProvider contract, the resulting behavior can be unexpected. I agree that my solution is hacky, but I couldn't find a better way of implementing that feature. If something better is available in the future, of course we can switch to that.

I noticed you're using Java language features beyond Java 8. Since jqwik 1.x has Java 8 as base version, please switch to Java 8 features. E.g.:

@Override
protected void resolveTestProperties(LifecycleContext context, MicronautTestValue testAnnotationValue, Map<String, Object> testProperties) {
    context.optionalContainerClass().ifPresent(testContainerClass -> {
        final Object testClassInstance = context.newInstance(testContainerClass);

        if (testClassInstance instanceof TestPropertyProvider) {
            TestPropertyProvider propertyProvider = (TestPropertyProvider) testClassInstance;
            final Map<String, String> dynamicPropertiesToAdd = propertyProvider.getProperties();
            testProperties.putAll(dynamicPropertiesToAdd);
        }
    });
}

That's true, I am using Java 17 which is the latest LTS version. My idea is, when we are all happy with the features implemented, to configure Gradle to have a multi-release JAR build. This way, we can still support old Java versions and, at the same time, support current Java versions with only one codebase. But if you believe that is not going to work, I can convert the code to Java 8.

jlink commented 1 year ago

You can certainly make it work that way. I just think that a multi-release jar is more effort and Java 17 features don’t seem to be crucial for the micronaut extension. I may be wrong, though.

Java 17 will probably be the base for jqwik 2; until then I decided to stick with 8 for my jqwik-related code.

Nahuel92 commented 1 year ago

Yeah, I agree that creating a multi-release JAR is kind of overkill. Allow me to finish it using Java 17 and I promise I will port it back to Java 8 immediately after. That way, we can have the extension ready for both Java versions and I should be able to port it back in a couple of hours as it's a small project.

I just added support for constructor injection. The code is ugly and I basically copied, pasted and added changes from the MicronautJunit5Extension, but I will clean it up later. On the upside, I feel the extension is taking shape and is usable at this point, at least for a basic/common usage.

Nahuel92 commented 1 year ago

@jlink I just pushed the last test classes I took from the MicronautJunit5Extension project. Those ones are quite tough and I can't make them to pass. If you have some time (and will), could you give it a look to see if you can find a clue on what's going on please? I added a // FIXME to facilitate identifying the failing classes.

To my knowledge, that's the only thing that interposes between us and having a working extension (besides of code refactoring + migrating it back to Java 8, but that's an easy cake).

jlink commented 1 year ago

@Nahuel92 Are you seeing 8 failing tests as well? Just checking that no one's running into local probs...

Nahuel92 commented 1 year ago

@Nahuel92 Are you seeing 8 failing tests as well? Just checking that no one's running into local probs...

Yes, those 8 failing tests are the ones I need help with.I couldn't find why they fail and I have no clue on where to look at.

I had no problems bringing the rest of the MicronautJunit5Extension tests to this extension, but those failing 8 are a pain in the neck since the errors don't give any good clue on what is happening.

That's why I asked if you could give them a look to see if you can see anything that I may have missed. If we fix them, then I believe we are good to go to release it (actually, I will need to migrate the extension back to Java 8, but that's super easy).

Nahuel92 commented 1 year ago

I just created a new branch called java-8 that contains the extension migrated back to Java 8. I will keep spending time on trying to get those tests to pass. I have no idea on what may be the reason why they fail, so I will continue investigating to see if I can figure out.

jlink commented 1 year ago

I’ll probably have time later this week to look a bit more closely. Which is the branch I should be looking at then?

Nahuel92 commented 1 year ago

master is the branch that contains my latest changes and the one I use as default, please take a look at that one. I will port back the changes needed to pass the failing tests to the java-8 branch as soon as have them available. Thank you :)

P.S.: I just fixed 3 tests, only 5 left to go.

jlink commented 1 year ago

@Nahuel92 I'll drop whatever I think can help you here.

For @Requires to work, you'll have to implement and register a SkipExecutionHook. Maybe look at the implementation of DisabledHook in the jqwik engine.

jlink commented 1 year ago

As for the failing PropertySourceTest, the test works as soon as I move the file myprops.properties to the folder resources/net/jqwik/micronaut. Micronaut seems to look for properties files in the same package as the test class. If that's intended behaviour, I cannot say.

jlink commented 1 year ago

Can you explain how TransactionalTest is supposed to work?

jlink commented 1 year ago

JpaNoRollbackTest could have a thread-related problem. Do you know in which method of the AbstractMicronautExtension the entity manager is being initialized?

jlink commented 1 year ago

One general thing: AroundPropertyMicronaut should probably override aroundPropertyProximity() in order to allow life cycle methods to access fields injected by Mirconaut:

public class AroundPropertyMicronaut implements AroundPropertyHook {
    @Override
    public PropertyExecutionResult aroundProperty(final PropertyLifecycleContext context,
                                                  final PropertyExecutor property) {
        ....
    }

    @Override
    public int aroundPropertyProximity() {
        // Property lifecycle methods (@BeforeProperty, @AfterProperty) use -10. 
        // Smaller numbers means "further away" from actual invocation of property method.
        // -20 is therefore around the lifecycle methods.
        return -20;
    }
}

Using more hooks with different proximity may also be a way for invoking all the different intercept and before/after methods in AbstractMicronautExtension, in case these do anything useful, which I cannot judge.

Nahuel92 commented 1 year ago

Thank you, now PropertySourceTest and RequiresTest work like a charm. The former I wasn't sure if I had to go that way (implementing SkipExecutionHook) and the latter was my fault (when I first copied the test from the MicronautJunit5Extension, I skimmed through the documentation to understand that feature a bit better but didn't realize that the properties file must be on the same package as the test class).

Regarding TransactionalTest, after implementing aroundPropertyProximity() in AroundPropertyMicronaut the applicationContext is no longer null. It still fails, though, but I am not quite sure why since I literally copied and pasted the same code as it is in the MicronautJunit5Extension.

To my knowledge, Micronaut should create a transaction before executing methods annotated with either @Transactional or @TransactionalAdvice (I have seen the later being recomended over the former, not sure why to be honest). The test class doesn't have any of those annotations so far, but I even have tried adding them without success. On the other hand, the MicronautJunit5Extension doesn't have any special code to handle transactions (at least I haven't seen anything).

Last but not least, for JpaNoRollbackTest, I believe the EntityManager creation is done through either micronaut-data-hibernate or micronaut-hibernate-jpa dependencies via a factory class (couldn't find the actual place yet). I will look into those failing tests to see what the heck is happening, I really want to see them passing.

jlink commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.