Grails Unit Tests using the new Annotation framework is slow

I think one of the biggest culprits is the new @Mock annotation for domain classes. It adds anywhere from 3-5s to the start up of a unit test class.

I think the biggest hitter in the implementation is in AbstractMappingContext.addPersistentEntityInternal(Class, boolean).
At line 167, it calls entity.initialize() which recurses through the associations of the entity.
For domain graphs that have a large relational graph, this could end up recursing through many domain objects that are not being mocked for the unit test that is being executed.

Not sure what the overall impact would be to having this method only initialize domain classes that are being mocked for the test. We'd have to somehow pass the list of all mocked domains to the collaborator which would be tricky since mockDomain could be called directly at any point in the test.

There is a related issue GRAILS-9480 that was a bug in Spock (pull request merged in Spock git: https://github.com/spockframework/spock/pull/10 )

I'm thinking about this one and there are a number of options that I can see.

1) Change @Mock to call mockDomain in an @BeforeClass annotated method so that setup is done once per class instead of once per test method. This is possibly a breaking change, but given the @Mock is applied to the class probably makes sense
2) Cache calls to mockDomain so a previous initialised domain is returned if it exists in some cache. This has risks of memory leakage, as we will need some post suite cleanup logic, but may provide a bigger win than 1)
3) I think trying to partially initialize a domain model by modifying AbstractMappingContext / entity.initialize() has problems and would result in unexpected behavior differing across tests. It could be investigated, but a combination of 1) and 2) are probably preferable.

Thoughts?

Option #1 may have breaking side-effects indeed. Or actually, will have. One thing you expect from tests is to have a clean context before each method. Even if the annotation is at class level, its action is expected at method level. @Mock is an elegant shortcut for repeated mockDomain in each individual method and there is no use of a @Mock that simply does mockDomain once in BeforeClass IMO.

The last thing you want is accidental pollution. In that respect, option #2 presents a risk if the cached instance is not correctly cleaned up in time. If you cache an instance but perform a cleanup operation that can potentially be costly, will the performance really improve?

Avoid recursion when mocking an instance as proposed by John Engelman sounds sane from a functional PoV. If I have A<-B<-C with @Mock(A), I don't expect B or C to be mocked.

Correctness, predicability and reasonable expectations over performance in any case. And of course, better performance would be great - 1000 units tests take some time even on a Core i7.

My two cents, only from a user perspective, without prejudice of the practicability of the discussed options.

The trouble with @Mock(A) only mocking A and not B or C is that you end up with behavioural differences. Since you are saying that @Mock(A) should not mock any associated instances. It is also a breaking change, since it means you will no longer be able to save associations of A unless they are also added to the list of mocked domains. So for example new A(b:new B()).save() will no longer work, unless B is also explicitly mocked. It is doable, but I thought I should point out the downsides of this option as well.

What about if when mocking an entity, we mock 1 level deep? That way, if you have something like

class A {
  List<B> children
}
class B {
   A parent
   List<C> children
}
class C {
   B parent
}

Then doing @Mock(A), it would persist A and B, but not C. If you wanted to persist C in your test, then you would have to do either @Mock(B) or @Mock(C)

Also, I tend to find that if I expect to access or persist any of the associated domains from the domain under test in a unit test, that I @Mock it that domain as well. I understand it's a breaking change, but I actually think that's a bit more clear on the intended behavior of the mock.

Right now, I believe, if you do @Mock(A) and then A has some B elements in it, then the B instances will persist to the datastore, but you won't have any GORM methods on B because you didn't call @Mock(B). So you're actually getting a mix of behaviors: it persists through a cascade, but you can't call new B().save()

• Mocking one-level deep is still a breaking change and seems like a weird in-between solution.

• Explicit @Mock for each class (with a meaningful error if you attempt to access unmocked children) won't hurt if you always @Mock all your classes. But it's actually a breaking change so maybe not desirable.

• Optimising the way instances are mocked, lazily mocking a child when necessary, would be ideal but may prove difficult to implement.

"So you're actually getting a mix of behaviors: it persists through a cascade, but you can't call new B().save()"

If this is the case, then introducing a non-compatible change is less of a issue if you actually make things more consistent and therefore predictable in return while improving performance. It's a win-win-lose so kind of a win As a framework developer, I'd be more inclined to change a component behavior rather than dragging an inefficient (and maybe inconsistent in that respect) mechanism for the sake of backward-compatibility. But I don't have to deal with possibly upset users neither!

If the backward non-compatible change is not acceptable, you're left with the optimisation path, lazy mocking and/or instance caching, with a strong constraint on correctness and predictability - at the end, that's what matters most.

I would prefer explicitly adding @Mock for each class. It is a breaking change, but everything will become much more consistent.
Mocking everything one-level-deep only causes confusion, because it will work in simple cases, but as soon as your domain model gets more complex, it breaks.

Ok I will go for the explicit approach of requiring a @Mock for each domain used

Could somebody please test the build of Grails 2.1.4 at

http://hudson.grails.org/view/Grails%202.1.x/job/grails_core_2.1.x/lastSuccessfulBuild/artifact/build/distributions/grails-2.1.4.BUILD-SNAPSHOT.zip

This includes changes that only partially mock a domain which I hope will improve performance. However, I don't have a huge domain model lying around that I can fully verify against. Feedback appreciated.

Getting an error on test-app with a gigantic stacktrace

Compiling 176 source files.
Error Compilation error compiling [unit] tests: startup failed:
General error during canonicalization: String index out of range: -1

java.lang.StringIndexOutOfBoundsException: String index out of range: -1

Apps runs on 2.1.3 w/o issues however. Ideas?

A bug in the AST transform somewhere, can you attach the full stacktrace? (run with --verbose --stacktrace)

Attached: General error during canonicalization: String index out of range: -1

Managed to run 2.1.4-SNAPSHOT by not cleaning existing 2.1.3. I suppose that does not count as a valid test.

Could you please try the latest build http://hudson.grails.org/job/grails_core_2.1.x/lastSuccessfulBuild/artifact/build/distributions/grails-2.1.4.BUILD-SNAPSHOT.zip

It does require a recompile, so running without a clean is not a valid test

OK, good news: tremendous improvement. 1015 unit tests running in 2m30s i.o. 5m30s! Got 40 in error:

1°- Explicit @Mock missing, as expected. No signature of method 'addTo***' error -> ok.

2°- Enum in domain class although not mapped. Still allowed in 2.1.3. Breaking change in 2.1.4 like in 2.2.0?
Cannot add Domain class [class myclient.Country]. It is not a Domain!

3°- Attempt to test for errors (contract.hasErrors()) although the test just deleted the contract -> incorrect test, improved behavior although error message not obvious

4°- Flush on save: attempt to save an instance with (failOnError: true, flush: true), then modify it, then save again with flush: NPE.

Does not occur if I don't use flush: true, which is probably useless in a unit test anyway -> ignore flush during test execution?

null
java.lang.NullPointerException
at org.grails.datastore.mapping.engine.NativeEntryEntityPersister$NativeEntryModifyingEntityAccess.setProperty(NativeEntryEntityPersister.java:1577)
at org.grails.datastore.mapping.engine.EntityAccess.refresh(EntityAccess.java:129)

5°- Test controller for optimistic locking failures:

// Test with older version
employee.save(failOnError: true) // increments the version to 2
params.version = 1
controller.updateEmployeeDetails()

assert employee.hasErrors()
assert employee.errors['version'] == 'default.optimistic.locking.failure'

Test passes with 2.1.3, fails with 2.1.4 -> employee has no errors

Nicely done!

Ok that is good news. For the enum issue (no.2) and the optimistic locking failure (no.5) can you provide an example in a separate JIRA?

For no 4. I think this is a side effect of the change, since it is trying to persist an association which is not persistable. I have added a null check to avoid this problem https://github.com/SpringSource/grails-data-mapping/commit/64f387b34921e8b89d55795ba7377c1f8ffef5e7

I'll mark this issue as resolved, if anyone else has more feedback / data points please commend here

I'm trying to get some data points for you Graeme, but I can't just run the nightly build. Apparently the bug in Groovy where ConfigObject has inadvertently started throwing a CloneNotSupportedException has worked it's way back to Groovy 1.8.9.

I'm going to try and back the Groovy version to 1.8.8 and run with your fixes and let you know.

#2 could it be related to GRAILS-9627 ?

#4 most likely

@Youri - that issue is unrelated and effects only 2.2.x

@John - I will back out the upgrade to Groovy 1.8.9 in that case

I'm seeing this error when applying the Build Test Data plugin to a project with the fixes:

/Users/jengelman/workspace/bloom/webapp_bhbo/bhbo/test/unit/com/bloomhealthco/bhbo/service/invoice/InvoiceGenerationServiceUnitTests.groovy: -1: Repetitive method name/signature for method 'java.lang.Object mockDomains([Ljava.lang.Object;)' in class 'com.bloomhealthco.bhbo.service.invoice.InvoiceGenerationServiceUnitTests'.
 @ line -1, column -1.

Build Test Data contains an AST for applying it's own stuff in the Unit Tests, but I checked and it doesn't define a 'mockDomains' method anywhere. However, it does extend the DomainClassUnitTestMixin, so it looks like maybe that method is getting mixed in twice (once when the @Build annotation is applied and a second time when @Mock is applied).

John - you couldn't attach the test that causes this? Or send it to me directly off list? I need to know how to reproduce

I attached a sample application that exhibits the compiler failure...uses the latest build-test-data plugin release (2.0.4)
This could be an issue with the plugin itself, if it is, I can tackle that and get a PR through to Ted to fix it.

Thanks John

John - could you try the latest build. I believe the problem is solved

Raised GRAILS-9861 and GRAILS-9862 for #2 and #5

Graeme -
The only thing that I saw that wasn't obvious was a domain object that extends from another domain object.
If you don't @Mock the parent object, then you get a NPE when trying to save the child object instance. Adding the @Mock resolved the error, but the error message isn't obvious:

| Failure:  verify_buildMemberPurchaseInformation(com.bloomhealthco.application.MemberPurchaseInformationBuilderUnitTests)
|  java.lang.NullPointerException: Cannot get property 'type' on null object
	at org.grails.datastore.mapping.simple.engine.SimpleMapEntityPersister.generateIdentifier(SimpleMapEntityPersister.groovy:237)
	at org.grails.datastore.mapping.engine.NativeEntryEntityPersister.persistEntity(NativeEntryEntityPersister.java:811)
	at org.grails.datastore.mapping.engine.EntityPersister.persist(EntityPersister.java:132)
	at org.grails.datastore.mapping.core.AbstractSession.persist(AbstractSession.java:477)
	at org.grails.datastore.gorm.GormInstanceApi.doSave(GormInstanceApi.groovy:166)
	at org.grails.datastore.gorm.GormInstanceApi$_save_closure4.doCall(GormInstanceApi.groovy:143)
	at org.grails.datastore.mapping.core.DatastoreUtils.execute(DatastoreUtils.java:302)
	at org.grails.datastore.gorm.AbstractDatastoreApi.execute(AbstractDatastoreApi.groovy:34)
	at org.grails.datastore.gorm.GormInstanceApi.save(GormInstanceApi.groovy:142)

In this case we have the following structure

class FamilyMember {
}

class Member extends FamilyMember {
}

Every other errors appears to be missing @Mocks -> "no method for addTo...", things like that.

John - did you see a performance improvement? The "no method for addTo.." is due to the change to require declaring all classes to be mocked, same with the NPE for parent/child. I will look at fixing the NPE for parent child though, because you are right that is not a good error.

Yeah - sorry I didn't mention that.
In our suite of ~2500 units test, we saw a drop from ~500ms/test to ~192ms/test. We have about 10% of our test cases failing for the reason you mentioned above, so once I can get a CI job set up, we'll start trying to fix various tests.

I plan on testing this change today with our 1500 unit tests to see what kind of improvement it makes. I was playing with a change in our test run that caches the PersistentEntity which is created by the addPersistentEntity call on the MappingContext. Profiling showed that 66% of our time is spent in that method and it seems like it might be something that could be cached in a simple map by class name. It wouldn't help with a single run, but it seems to help when you are mocking the same classes over and over again across all the tests. Obviously it could have implications of allowing more test pollution.

I'll report back if we have any issues or metrics with this change. Very nice work, good to see some love in the Unit testing area.

Ok, I can say one thing for sure. This change and the build-test-data plugin hate each other. build-test-data wants to build the graph and now I have to basically tell it every single object that will be mocked.

Yup, as mentioned in the release notes, you have to specify all domains to be mocked to @Mock or @Build

Yeah, I read those but I missed John's comments in this ticket about build-test-data on the first read. The problem, as he mentioned, is that BTD wraps the mockDomain call and will need some updates in order to work with this change in 2.2.1.

Definitely much faster though, good work.

@Aaron - hopefully John can ask Ted to release an update to build-test-data

I'm making the changes to BTD and working with Ted to get them in the next release. The main changes is just getting the BTD mixin to use the new mockDomains() method which takes all of the classes at once. Also, since Graeme split out the mockDomain stuff into some separate helper methods, it makes overriding the DomainClassUnitTestMixin much cleaner.

I hope to have the BTD changes done in the next few days.

@Aaron, if you need help testing it, let me know

@Alvaro, absolutely. We have a pretty large and complex domain model and about 3000 tests so I get pretty good coverage from our test run but nothing beats having more testing. Right now I'm chasing down 12 test failures but I think a few are 2.2.1 related and not BTD.

@Aaron
I'm working through one of our code bases now too (where we don't use BTD) and I'm creating some tickets along the way.
GRAILS-9882 I already have and I'm trying to isolate an issue where it appears the order that you declare classes in @Mock matters. Not sure on this one yet.

Interesting. I have some situations where I'm getting "No signature of method: com.triu.location.Subsidiary.addToEmployees()" but I have both Subsidiary and Employee in the @Mock line. If I can work up a simple example I'll open a ticket.

@Aaron, I faced a similar situation. In my case, both domains were in the @Build annotation, and the solution was to also add them to the @Mock one

Has this really been fixed? I haven't encountered a reduction in unit test execution time.
250 tests ran about 15s in Grails 1.3.7 and but take about 50s in Grails 2.1.5.

It will never be as fast as 1.3.7 because 1.3.7 didn't include a complete mocking API for GORM and the testing APIs were very basic