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

Plugin causes exceptions when HibernateTestMixin is also used on a test #33

Merged
merged 2 commits into from
Jul 13, 2014

Conversation

gregopet
Copy link
Contributor

This is (a probably naive) attempt to enable using both the build-test-data plugin as well as the new HibernateTestMixin mixin for Grails.

I love the plugin's functionality but some tests do not work properly with Grails' mocked ORM (a named query in my case). That's why I tried using the new HibernateTestMixin annotation (at least it is new to me..) and my test was now working - but the ones using the @Build annotation suddenly failed with the following exception under Grails 2.4.2:

|  org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type [org.grails.datastore.mapping.simple.SimpleMapDatastore] is defined
        at grails.test.mixin.domain.DomainClassUnitTestMixin.getSimpleDatastore(DomainClassUnitTestMixin.groovy:71)
        at grails.test.mixin.domain.DomainClassUnitTestMixin.mockDomains(DomainClassUnitTestMixin.groovy:81)
        at grails.buildtestdata.mixin.BuildTestDataUnitTestMixin.mockGrailsDomainClasses(BuildTestDataUnitTestMixin.groovy:133)
        at grails.buildtestdata.mixin.BuildTestDataUnitTestMixin.mockForBuild(BuildTestDataUnitTestMixin.groovy:75)
        at org.spockframework.util.ReflectionUtil.invokeMethod(ReflectionUtil.java:138)
        at org.spockframework.runtime.extension.builtin.JUnitFixtureMethodsExtension$FixtureType$FixtureMethodInterceptor.intercept(JUnitFixtureMethodsExtension.java:145)
        at org.spockframework.runtime.extension.MethodInvocation.proceed(MethodInvocation.java:84)
        at org.spockframework.util.ReflectionUtil.invokeMethod(ReflectionUtil.java:138)
        at org.spockframework.runtime.extension.MethodInvocation.invokeTargetMethod(MethodInvocation.java:91)
        at org.spockframework.runtime.extension.MethodInvocation.proceed(MethodInvocation.java:85)
        at org.spockframework.runtime.extension.builtin.AbstractRuleInterceptor$1.evaluate(AbstractRuleInterceptor.java:37)
        at grails.test.runtime.TestRuntimeJunitAdapter$1$2.evaluate(TestRuntimeJunitAdapter.groovy:48)
        at org.spockframework.runtime.extension.builtin.TestRuleInterceptor.intercept(TestRuleInterceptor.java:38)
        at org.spockframework.runtime.extension.MethodInvocation.proceed(MethodInvocation.java:84)
        at org.spockframework.util.ReflectionUtil.invokeMethod(ReflectionUtil.java:138)
        at org.spockframework.util.ReflectionUtil.invokeMethod(ReflectionUtil.java:138)
        at org.spockframework.runtime.extension.MethodInvocation.invokeTargetMethod(MethodInvocation.java:91)
        at org.spockframework.runtime.extension.MethodInvocation.proceed(MethodInvocation.java:85)
        at org.spockframework.runtime.extension.builtin.AbstractRuleInterceptor$1.evaluate(AbstractRuleInterceptor.java:37)
        at grails.test.runtime.TestRuntimeJunitAdapter$3$4.evaluate(TestRuntimeJunitAdapter.groovy:74)
        at org.spockframework.runtime.extension.builtin.ClassRuleInterceptor.intercept(ClassRuleInterceptor.java:38)
        at org.spockframework.runtime.extension.MethodInvocation.proceed(MethodInvocation.java:84)

The patch is my attempt at solving this issue (though I'm not intimately familiar with Grails' internals so a better solution may well exist).

…n the test

Trying to mock domain classes when HibernateTestMixin was also used on a
test results in an exception. This patch tries to detect
HibernateTestMixin and stops the domain class mocking if it did.
@wuestenfuchs
Copy link

We run into the same problem on try the "new" hibernate unit tests. Using grails 2.4.2 and hibernate4:4.3.5.4

I tried the pull request, but had the following problems.

  • We have TestClass1 (with HibernateTestMixin annotation). TestClass2 extends TestClass1. HibernateTestMixin annotation must be on TestClass1, don't know why. I think this looking at HibernateTestMixin will not work in such cases.
  • Even when I try at BuildTransformation.java Boolean doGrailsMocking = false; still not working.
    org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type [org.grails.datastore.mapping.simple.SimpleMapDatastore] is defined
    at grails.test.mixin.domain.DomainClassUnitTestMixin.getSimpleDatastore(DomainClassUnitTestMixin.groovy:71)
    at grails.test.mixin.domain.DomainClassUnitTestMixin.mockDomains(DomainClassUnitTestMixin.groovy:81)
    at grails.buildtestdata.mixin.BuildTestDataUnitTestMixin.mockGrailsDomainClasses(BuildTestDataUnitTestMixin.groovy:132)
    at grails.buildtestdata.mixin.BuildTestDataUnitTestMixin.mockForBuild(BuildTestDataUnitTestMixin.groovy:74)

Build-test-data is awesome and the bread and butter of our grails test based development! Any idea how to fix it?

@tednaleid
Copy link
Collaborator

Thanks for the PR, from what @wuestenfuchs is saying, it sounds like this isn't quite fixing the error.

The projects that I'm on right now are still using grails 2.3.X and hibernate 3 so I haven't gotten a chance to work with the new mixins yet. Do either of you have a simple test project that uses hibernate 4 and demonstrates the problem? I should probably add another test project to the root of the build-test-data repo (like the other projects I have there) that uses hibernate 4 as well as the hibernate annotation that you mention.

From a quick look at the docs, it sounds like the want some sort of @Domain annotation, but I'm not sure yet how that plays with the @Build and @Mock annotations or if it's redundant...

If you don't have a test case, I'll probably try to throw one together in the next week or so and take a closer look at grails 2.4/hibernate 4

@gregopet
Copy link
Contributor Author

I'll take a look at inherited tests, we just started a new client project so I haven't arrived at anything 'complex' yet (so I do have a simple test case, I am just not allowed to share it :) ). But a word of warning, @wuestenfuchs: I had to clean the entire Grails project whenever I made any changes whatsoever in BuildTransformation.java or they would simply get ignored! Can you please do this and try doGrailsMocking = false again?

@gregopet
Copy link
Contributor Author

Here's a quick test case for @tednaleid. The master branch contains the Grails 2.4.2 app with your plugin while the with-fix branch contains the plugin with my patch applied. If you test-app unit: the first you will get an exception while the second will pass.

Now I'll try creating test classes that extend each other and see where that goes.

@gregopet
Copy link
Contributor Author

I tried detecting if HibernateTestMixin is present on a parent class and now my spec passes... but only after a few grails clean invocations and seemingly random code changes - I can't seem to figure out why my spec is passing and why it did not before :(

I guess I am trying to say I would appreciate a test after the new commit, but please grails clean-app first, @wuestenfuchs :)

@gregopet
Copy link
Contributor Author

Hmm, on the other hand, don't bother: I can still make it fail, it was just too early in the morning to see that before that last comment :)

@tednaleid: the test case branch now contains a with-fix-inheriting-spec branch as well. I'll try taking another look at the problem myself, later.

EDIT: huh, now my tests fail even if I remove @Build from all specs.. I sense a Grails bug here, somewhere.

@gregopet
Copy link
Contributor Author

Grails bug reported.

(hmm, JIRA isn't exactly Spock-friendly :P)

@wuestenfuchs
Copy link

Hello. Thank you very much for reporting the grails bug. I wasn't sure on that. I experienced the same thing as you, randomly strange behavior, unable to identify. I spent 1 days developing with rm -r target && grails... trying to isolate something. All our domains, around 100, all related to each other, and @Domain requires all related domains to be mocked as well, so it took some time to get it to compile.

You are right, in this case I didn't even try a clean, sorry for that. changes to BuildTransformation.java really require each time a clean. Now (doGrailsMocking = false) I'm having different issue:

Without @build(User) everything runs, User.count works. If I add @build(User) (without calling User.build() yet)I get on User.count:

java.lang.IllegalStateException: Method on class [com.chat.authentication.User] was used outside of a Grails application. If running in the context of a test using the mocking API or bootstrap Grails correctly.
at com.chat.command.MyTestUserTest.testAnything(MyTestUserTest.groovy:14)
at junit.framework.TestCase.runTest(TestCase.java:176)
at junit.framework.TestCase.runBare(TestCase.java:141)

without any other useful information.

@gregopet
Copy link
Contributor Author

Huh, that seems like a different problem, maybe you can open another issue?

One difference that I see is that you seem to be using JUnit tests while I'm working with Spock.. I can try making a JUnit test to see what happens but it definitely looks like a separate issue.

Perhaps Grails isn't always picking up the HibernateTestMixin and Domain when they are defined on the super class and the implicit @Mock (caused by @build) was actually required here (although you are then using @Mock instead of the HibernateTestMixin..)? It's a Grails bug/quirk in that case.

Can you try using both @Build (with doGrailsMocking = false) and @Mock - if that makes the tests work again, that could confirm the theory from the above paragraph?

@wuestenfuchs
Copy link

Yes seems you are right. If I replace @Domain and @TestMixin(HibernateTestMixin) with @mock it works.

There are strange things going on, I just reported https://jira.grails.org/browse/GRAILS-11575 produces the same java.lang.IllegalStateException, just without any use of build-test-data plugin.

@tednaleid
Copy link
Collaborator

Following along with the stuff you guys tried, it seems like you both concluded that these are grails bugs and not build-test-data bugs. Right? Should we close this issue (and reopen if it does end up being a BTD thing)?

If not, let me know what testcase I should be using to repeat what you guys are seeing with BTD.

@gregopet
Copy link
Contributor Author

Well the original reason I wrote this patch is not a Grails bug by itself: by using both @Build (which is kind of similar to using their @Mock) and @TestMixin(HibernateTestMixin) we are effectively telling Grails that for this test it should be using both an actual Hibernate environment as well as the in-memory simulation of it. Grails may include mitigating code in future versions and decide on one or the other when both annotations are present but personally, I would just make the error message clearer.

What my patch does is disable @Build annotation's @Mock behaviour when used on a unit test that has had a real Hibernate instance injected. There are other ways this could be achieved also - through configuration, another annotation (e.g. @BuildNoMock) or another annotation parameter (e.g. @Build(value=Author, mock=false)) and this is for you to decide as the plugin author.

As for the test case, you can clone the repository I posted a few comments above and test-app the master branch (it should fail). The with-fix branch is there as a convenience to see how my patch handles the problem (it contains the patch, although without the second commit, in a plugins subfolder).

The inherited test/spec class problems are indeed a Grails bug.

@wuestenfuchs
Copy link

yes, I agree. Getting the plugin to work together with @TestMixin(HibernateTestMixin) I would consider very helpful.

@tednaleid
Copy link
Collaborator

@gregopet got it thanks for the explanation. I wasn't previously understanding that patching in the equivalent of @Mock in the @Build annotation wasn't compatible with the "real" hibernate instances that come with the HibernateTestMixin.

Thanks for the patch and the sample test project. I'm taking a closer look at it this weekend and want to have some tests in the build-test-data project that exercise the functionality, so I'll probably upgrade the whole thing to 2.4 to get that working.

@tednaleid
Copy link
Collaborator

I've got a working test locally in build-test-data in a new test project called hibernate4. Your fix does fix it and I'm going to merge it in.

I'm not releasing it yet though as I want to make a few changes to it so that the @Domain annotation isn't needed, in much the same way that the @Mock annotation isn't needed with the current @Build annotation.

I need to do some more investigation, but my hope is that if we detect that the user is using the HibernateTestMixin that we apply whichever is appropriate, @Domain or @Mock. This is important as to build a valid object, BTD might need to build a related domain that you didn't explicitly put in the list.

Thanks again for the pull request, and your work to dig into what's going on with the new hibernate annotations in 2.4.

tednaleid added a commit that referenced this pull request Jul 13, 2014
Plugin causes exceptions when HibernateTestMixin is also used on a test
@tednaleid tednaleid merged commit c28f4d3 into longwa:master Jul 13, 2014
@tednaleid
Copy link
Collaborator

Looks like the HibernateTestMixin has even more problems that cause it to be unusable with build-test-data for anything but the most trivial domain models.

It doesn't create addTo* methods on domain classes with hasMany relationships. BTD relies on addTo* methods when creating the object graph.

I've opened up GRAILS-11579 to report the issue. When they get that fixed, I should be able to make BTD work with what you've done, but until that's in the plugin is pretty much useless in HibernateTestMixin test classes.

@gregopet
Copy link
Contributor Author

Glad to have been of help!

I know about the addTo* problem and it's very annoying - I filed a bug report as well, albeit to the Github page and not their JIRA and I didn't include a package test case - good that it's now in both locations.

@lhotari
Copy link
Contributor

lhotari commented Jul 17, 2014

@gregopet @tednaleid Besides HibernateTestMixin, the next version of grails-datastore-test-support will also include a similar feature for using real Mongo DB in unit tests. Please see https://jira.grails.org/browse/GRAILS-11533 for details of MongoDbTestMixin .

As you have probably noticed, the unit test runtime API (including the TestMixin) in Grails 2.4 has been heavily refactored as part of https://jira.grails.org/browse/GRAILS-11001 . There is some explanation in the "What's new in 2.4" guide http://grails.org/doc/latest/guide/introduction.html#whatsNew24 under "Unit Testing improvements". The static fields for keeping state in TestMixin classes has been removed and all state is kept in a separate TestRuntime instance. Because of this change we had to move HibernateTestMixin and MongoDbTestMixin classes to the https://github.com/grails/grails-datastore-test-support module. The master branch currently has support for Grails 2.4.x and the 2.3.x branch of this module is for Grails 2.3.x. . (I hope you don't have to do something similar for build-test-data) .

build-test-data is a very useful plugin. I'd like to help you in getting build-test-data compatible with HibernateTestMixin and MongoDbTestMixin . You can ping me directly on Skype , my nick is lhotari there.

@tednaleid
Copy link
Collaborator

@lhotari thanks for the links and the work you did to fix the defect we uncovered (as well as all the other unit test improvements that you've done)!

That's interesting info about the MongoDbTestMixin. From a quick look, it appears that it is also using the @Domain annotation. I think that I might be able to detect either the Mongo or Hibernate mixins and automatically apply that annotation to the objects that BTD detects need it.

I think you're going to be at gr8conf.us in a week or so. If you have time, we should talk there about your plans for the grails-datastore-test-support and whether you plan on adding a lot more mixins. Maybe there's a better way that I can be detecting that I should be applying the @Domain annotation instead of @Build.

The alternative is to not have my annotation be that smart and instead have an alternate @BuildDomain annotation that's for those types of tests, but I'm betting that people will be confused by that, and I'd rather avoid it if possible...

I'm going to open up another issue (as this one is marked as closed) to track the ideas/work on getting this implemented. It'll be linked here shortly.

@tednaleid
Copy link
Collaborator

What I've decided to do is that instead of looking for @HibernateTextMixin annotation, I'm instead looking for the @Domain annotation. This will make it work for the @MongoDbTestMixin as well as any other mixin that comes along as I believe that they'll all use the @Domain annotation.

It will also make it easier in the future to split out Build Test Data from grails and make it support stand-alone apps that just use GORM (like Ratpack apps). That's a longer term goal, but something I'm thinking about.

This fix will be released shortly. Thanks again for the patch, sorry it took so long to release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants