Fix auditing when running through DatastoreTemplate.performTransaction #2604
Conversation
| @@ -82,6 +84,20 @@ public void testModifiedPrevProperties() { | |||
| this.datastoreTemplate.saveAll(Collections.singletonList(testEntity)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testInTransaction() { | |||
| when(datastore.runInTransaction(any())) | |||
There was a problem hiding this comment.
I see mock setup all done in datastoreTemplate(). Would it make sense to move it there?
There was a problem hiding this comment.
Personally, I prefer keeping the specific setup where it's actually used, and Mockito could complain about unnecessary setup in the other tests, depending on its configuration. However, I've tested moving the code and it works, so Mockito is not in strict mode and the code can be arranged however you prefer.
There was a problem hiding this comment.
I don't feel strongly either way. I'm also not sure it's good that the verification is outside the test method, but that's a problem for another time. :)
There was a problem hiding this comment.
OK, I think I'll leave it that way then, not specifying every possible behavior for all the tests.
I agree the verification in an Answer is brittle, which is when I first ran the test, without the added behavior on the mock, it passed: the Datastore.put() method was never called! We could extract an assertion method which would use an ArgumentCaptor to verify the call and its parameter, instead. However it's not the only occurence in the project of assertions in Answers, so just fixing this one instance probably doesn't really make sense.
There was a problem hiding this comment.
Yeah, no need to address that here.
Codecov Report
@@ Coverage Diff @@
## master #2604 +/- ##
============================================
- Coverage 81.29% 74.06% -7.23%
+ Complexity 2396 2168 -228
============================================
Files 267 267
Lines 7798 7800 +2
Branches 808 808
============================================
- Hits 6339 5777 -562
- Misses 1113 1644 +531
- Partials 346 379 +33
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
The new `DatastoreTemplate` instance should have the same `ApplicationEventPublisher` as the original one, so that `AuditingHandler` can be called.
fpavageau
force-pushed
the
bugfix-auditing-transaction
branch
from
fb40156 to
278d09c
Compare
|
@fpavageau BTW, no need to squash or force-push in branches/PRs because we always squash commits on merge to master. Do you want to add your name to |
|
I added my name to the test class, I guess that's enough. |
#2604) (#157) The new `DatastoreTemplate` instance should have the same `ApplicationEventPublisher` as the original one, so that `AuditingHandler` can be called. Port of spring-attic/spring-cloud-gcp#2604. Co-authored-by: Frank Pavageau <[email protected]>
The new
DatastoreTemplateinstance should have the sameApplicationEventPublisheras the original one, so thatAuditingHandlercan be called.
Fixes #2603