Modifies SpringListener to include Spring's Test Listeners #887
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm not completely sure this will solve all the issues, but I know that now the listeners are being executed. I'll ask my peer (he reported that this wasn't working) to take a look and see if this is likely to solve his bug |
sksamuel
requested changes
Jul 16, 2019
...tensions/kotlintest-extensions-spring/src/main/kotlin/io/kotlintest/spring/SpringListener.kt
Outdated
Show resolved
Hide resolved
|
|
||
| override fun afterTestClass(testContext: TestContext) { | ||
| count++ | ||
| } |
There was a problem hiding this comment.
Each of these counts should be separate otherwise we don't know if the correct ones were invoked just that the correct quantity was.
There was a problem hiding this comment.
Good one. I made the change and found a bug because of it.
...tensions/kotlintest-extensions-spring/src/main/kotlin/io/kotlintest/spring/SpringListener.kt
Outdated
Show resolved
Hide resolved
LeoColman
force-pushed
the
feature/886-spring-test-listener
branch
from
736a670
to
016d4db
Compare
Originally, our SpringLister would only execute one part of the Spring Test Context's API. This was insufficient for some of Spring's implementations, such as the TestExecutionListener api. This commit changes the way the listener works to enable that kind of listener. It was very similar to our listener execution, so we just had to hook the executions. Users that didn't use these features before won't be impacted in any way. Implements #886
LeoColman
force-pushed
the
feature/886-spring-test-listener
branch
from
016d4db
to
419ae8c
Compare
|
I think this is ready, @sksamuel |
|
Lgtm
…On Tue, 16 Jul 2019, 08:57 Leonardo Colman Lopes, ***@***.***> wrote:
@Kerooker <https://github.com/Kerooker> requested your review on: #887
<#887> Modifies
SpringListener to include Spring's Test Listeners.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#887>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFVSGRLTUQK26UDMUIYFCLP7XHUJANCNFSM4ICWELZA>
.
|
LeoColman
added a commit
that referenced
this pull request
Aug 23, 2019
More information at #950 Spring executes transactions (and this include anything inside @DataJpaTest) through a Listener called TransactionalTestExecutionListener. This comes pre-registered with a SpringBootTest, and KotlinTest was executing this listener correctly as per #887. However, only executing the Listener wasn't enough, as it couldn't autowire the EntityManager. Initially, this was happening because our SpringListener had a fake "test method" that was a pointer to java.lang.Object.hashcode method. We made this hack to allow us to execute code inside Spring Test Framework, and it seemed fine on our tests. Due to hashcode being a java.lang.Object method, Spring couldn't find the transaction attribute, as it verified directly that the method wasn't from Java Object. With this in mind, we must use an object from another class. My first attempt was to use a dummy method, declared in the SpringListener itself. But this also wouldn't work, as the listener would never find the TransactionAttribute, as the method doesn't (and shouldn't) declare a @transaction, and SpringListener also didn't declare that annotation. The only way to have a method with that annotation would be inside user's Spec definition, where the user would choose it. We don't have methods, as we define everything through a DSL. But that didn't seem an issue, as Spring's Listener would get the Transaction Attribute from the method's class if it doesn't declare it. @DataJpaTest already declare transaction attributes, so if we managed to get a method inside it, it would work without having to declare @transaction. If we could guarantee that our users were overriding a method, we could get its reference, but we couldn't force that, and referecing a method from Spec wouldn't work (as Spec doesn't declare the attributes). The only way to solve this issue that I found was to inject a dummy method into the class, and get it through reflection, so that Spring Listener would correctly find the Transaction Attributes, and that's what I've done. Fixes #950
LeoColman
added a commit
that referenced
this pull request
Aug 23, 2019
More information at #950 Spring executes transactions (and this include anything inside @DataJpaTest) through a Listener called TransactionalTestExecutionListener. This comes pre-registered with a SpringBootTest, and KotlinTest was executing this listener correctly as per #887. However, only executing the Listener wasn't enough, as it couldn't autowire the EntityManager. Initially, this was happening because our SpringListener had a fake "test method" that was a pointer to java.lang.Object.hashcode method. We made this hack to allow us to execute code inside Spring Test Framework, and it seemed fine on our tests. Due to hashcode being a java.lang.Object method, Spring couldn't find the transaction attribute, as it verified directly that the method wasn't from Java Object. With this in mind, we must use an object from another class. My first attempt was to use a dummy method, declared in the SpringListener itself. But this also wouldn't work, as the listener would never find the TransactionAttribute, as the method doesn't (and shouldn't) declare a @transaction, and SpringListener also didn't declare that annotation. The only way to have a method with that annotation would be inside user's Spec definition, where the user would choose it. We don't have methods, as we define everything through a DSL. But that didn't seem an issue, as Spring's Listener would get the Transaction Attribute from the method's class if it doesn't declare it. @DataJpaTest already declare transaction attributes, so if we managed to get a method inside it, it would work without having to declare @transaction. If we could guarantee that our users were overriding a method, we could get its reference, but we couldn't force that, and referecing a method from Spec wouldn't work (as Spec doesn't declare the attributes). The only way to solve this issue that I found was to inject a dummy method into the class, and get it through reflection, so that Spring Listener would correctly find the Transaction Attributes, and that's what I've done. Fixes #950
sksamuel
pushed a commit
that referenced
this pull request
Aug 25, 2019
…#953) More information at #950 Spring executes transactions (and this include anything inside @DataJpaTest) through a Listener called TransactionalTestExecutionListener. This comes pre-registered with a SpringBootTest, and KotlinTest was executing this listener correctly as per #887. However, only executing the Listener wasn't enough, as it couldn't autowire the EntityManager. Initially, this was happening because our SpringListener had a fake "test method" that was a pointer to java.lang.Object.hashcode method. We made this hack to allow us to execute code inside Spring Test Framework, and it seemed fine on our tests. Due to hashcode being a java.lang.Object method, Spring couldn't find the transaction attribute, as it verified directly that the method wasn't from Java Object. With this in mind, we must use an object from another class. My first attempt was to use a dummy method, declared in the SpringListener itself. But this also wouldn't work, as the listener would never find the TransactionAttribute, as the method doesn't (and shouldn't) declare a @transaction, and SpringListener also didn't declare that annotation. The only way to have a method with that annotation would be inside user's Spec definition, where the user would choose it. We don't have methods, as we define everything through a DSL. But that didn't seem an issue, as Spring's Listener would get the Transaction Attribute from the method's class if it doesn't declare it. @DataJpaTest already declare transaction attributes, so if we managed to get a method inside it, it would work without having to declare @transaction. If we could guarantee that our users were overriding a method, we could get its reference, but we couldn't force that, and referecing a method from Spec wouldn't work (as Spec doesn't declare the attributes). The only way to solve this issue that I found was to inject a dummy method into the class, and get it through reflection, so that Spring Listener would correctly find the Transaction Attributes, and that's what I've done. Fixes #950
LeoColman
added a commit
that referenced
this pull request
Sep 20, 2019
…#953) More information at #950 Spring executes transactions (and this include anything inside @DataJpaTest) through a Listener called TransactionalTestExecutionListener. This comes pre-registered with a SpringBootTest, and KotlinTest was executing this listener correctly as per #887. However, only executing the Listener wasn't enough, as it couldn't autowire the EntityManager. Initially, this was happening because our SpringListener had a fake "test method" that was a pointer to java.lang.Object.hashcode method. We made this hack to allow us to execute code inside Spring Test Framework, and it seemed fine on our tests. Due to hashcode being a java.lang.Object method, Spring couldn't find the transaction attribute, as it verified directly that the method wasn't from Java Object. With this in mind, we must use an object from another class. My first attempt was to use a dummy method, declared in the SpringListener itself. But this also wouldn't work, as the listener would never find the TransactionAttribute, as the method doesn't (and shouldn't) declare a @transaction, and SpringListener also didn't declare that annotation. The only way to have a method with that annotation would be inside user's Spec definition, where the user would choose it. We don't have methods, as we define everything through a DSL. But that didn't seem an issue, as Spring's Listener would get the Transaction Attribute from the method's class if it doesn't declare it. @DataJpaTest already declare transaction attributes, so if we managed to get a method inside it, it would work without having to declare @transaction. If we could guarantee that our users were overriding a method, we could get its reference, but we couldn't force that, and referecing a method from Spec wouldn't work (as Spec doesn't declare the attributes). The only way to solve this issue that I found was to inject a dummy method into the class, and get it through reflection, so that Spring Listener would correctly find the Transaction Attributes, and that's what I've done. Fixes #950
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Originally, our SpringLister would only execute one part of the Spring Test Context's API. This was insufficient for some of Spring's implementations, such as the TestExecutionListener api.
This commit changes the way the listener works to enable that kind of listener. It was very similar to our listener execution, so we just had to hook the executions.
Users that didn't use these features before won't be impacted in any way.
Implements #886