… rather than just a synchronization. Otherwise, at least SharedEntityManagerCreator will create a 'lingering' EM inside NEVER/NOT_SUPPORTED propagation, which is closed only after exiting from inside that boundary.
I did look at some other usage of isSynchronizationActive, and I'm not sure if it is more proper to use isActualTransactionActive there. I suspect this might be true for a couple of places.
isActualTransactionActive and doGetTransactionalEntityManager seem to be around here since before the recorded history, so we can't say if isActualTransactionActive was added after or before doGetTransactionalEntityManager.
Comment From: ettavolt
@sdeleuze, may I please have your opinion on this? ☺
Comment From: sdeleuze
@ettavolt Not my area of expertise, so no specific feedback to share.
Comment From: snicoll
can you please rebase and squash to a single commit, removing all the unrelated changes that you've done in test assertions? It makes it harder to understand what changes you've actually introduced in unit tests.
Comment From: ettavolt
can you please rebase
Sure!
and squash to a single commit, removing all the unrelated changes that you've done in test assertions? It makes it harder to understand what changes you've actually introduced in unit tests.
That's why I've split the changes in three separate commits: 1) auto-conversion (1beb7068f62d0d75683e2d701d5cbfa9c2459ee8) clean up, 2) actual changes, 3) additional test. I think the latter two could be merged together without damaging comprehensibility, but I'd prefer them being swapped, so that one can see the topic test failing before the fix.
What do you think?
Comment From: snicoll
I’d like the commit that updates the assertions to be removed please. It’s unrelated to this PR and, as such, should not be handled here.
Comment From: ettavolt
@snicoll , I've restarted the PR as #31364, so that when I update it next time it does create close-reopen noise.
Comment From: snicoll
I don't know what you mean by create-close. We would have preferred the history to be in a single place but that's too late now. In the future, please push force on your existing branch and that'll update the existing PR.