-
Notifications
You must be signed in to change notification settings - Fork 1.6k
GH-3873: Deprecate JUnit 4 utilities in the project #3878
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
base: main
Are you sure you want to change the base?
GH-3873: Deprecate JUnit 4 utilities in the project #3878
Conversation
Signed-off-by: chickenchickenlove <[email protected]>
* | ||
*/ | ||
public class Log4j2LevelAdjuster implements MethodRule { | ||
public class Log4j2LevelAdjuster implements InvocationInterceptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this class used with Junit5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct.
The class is public and for end-user consumption.
It is totally wrong to change the class contract.
We develop here a library for target projects, so whatever is public
cannot be changed like this.
We have to revert the change here and deprecate it.
We may come up with JUnit 5 variant. But that is a different story.
You may take a inspiration from Spring AMQP: https://github.com/spring-projects/spring-amqp/blob/main/spring-rabbit-junit/src/main/java/org/springframework/amqp/rabbit/junit/LogLevels.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can deprecate it on the 3.3.x
line and then still remove it in 4.0.0. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... That would be inconsistent with Spring Framework plans.
They do deprecate JUnit 4 in version 7.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @artembilan mentioned before, this class is to configure log levels.
Also, It can be replace with @LogLevels
in Junit5.
* | ||
*/ | ||
public class Log4j2LevelAdjuster implements MethodRule { | ||
public class Log4j2LevelAdjuster implements InvocationInterceptor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct.
The class is public and for end-user consumption.
It is totally wrong to change the class contract.
We develop here a library for target projects, so whatever is public
cannot be changed like this.
We have to revert the change here and deprecate it.
We may come up with JUnit 5 variant. But that is a different story.
You may take a inspiration from Spring AMQP: https://github.com/spring-projects/spring-amqp/blob/main/spring-rabbit-junit/src/main/java/org/springframework/amqp/rabbit/junit/LogLevels.java
build.gradle
Outdated
@@ -378,9 +377,6 @@ project ('spring-kafka-test') { | |||
api 'org.junit.platform:junit-platform-launcher' | |||
optionalApi "org.hamcrest:hamcrest-core:$hamcrestVersion" | |||
optionalApi "org.mockito:mockito-core:$mockitoVersion" | |||
optionalApi ("junit:junit:$junit4Version") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct.
We are talking about deprecation of the utility, but not total removal.
Therefore dependency and deprecated classes must still be here.
Again: we develop library, so whatever we do here might break target projects using our library.
Removing it temporary locally is a good way to check if we still have any JUnit 4 tests left. 😄
@@ -14,7 +14,7 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
package org.springframework.kafka.test.rule; | |||
package org.springframework.kafka.test.extensions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like this class logic does something with log adjustment.
Might be better to move it to some other package.
Probably to the root one alongside with the EmbeddedKafkaKraftBrokerTests
…oject" This reverts commit 2da4d5d. Signed-off-by: chickenchickenlove <[email protected]>
Signed-off-by: chickenchickenlove <[email protected]>
0bc99b7
to
c8839a5
Compare
Signed-off-by: chickenchickenlove <[email protected]>
I’m not sure if I’m on the right track with my work.
Is my understanding correct? Sorry again and Thanks for your time. 🙇♂️ |
@@ -121,15 +121,19 @@ The following example configuration creates topics called `cat` and `hat` with f | |||
|
|||
[source, java] | |||
---- | |||
@ExtendWith(SpringExtension.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why @SpringJUnitConfig
doesn't work for us here?
@@ -350,6 +354,38 @@ They include: | |||
* xref:testing.adoc#kafka-testing-junit4-class-rule[JUnit4 Class Rule] | |||
* xref:testing.adoc#kafka-testing-embeddedkafka-annotation[`@EmbeddedKafka` Annotation or `EmbeddedKafkaBroker` Bean] | |||
|
|||
[[kafka-testing-junit4-embedded-broker]] | |||
=== Junit4 Embedded Broker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we deprecate JUnit 4, we just should mention that we have done that and recommend to migrate to JUnit 5 without any further advises how to configure JUnit 4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed all contents about JUnit4 and left comment for asking migration to JUnit5.
Thanks a lot 🙇♂️
@@ -43,7 +43,7 @@ | |||
* <p> | |||
* The typical usage of this annotation is like: | |||
* <pre class="code"> | |||
* @RunWith(SpringRunner.class) | |||
* @ExtendWith(SpringExtension.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DITTO about @SpringJUnitConfig
.
It is very rare situation when we need to use @ExtendWith(SpringExtension.class)
.
*/ | ||
@Deprecated(since = "4.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed forRemoval = true
.
We really are going to remove it in the next 4.1
or 4.2
@@ -37,8 +37,11 @@ | |||
* @author Dave Syer | |||
* @author Artem Bilan | |||
* @author Gary Russell | |||
* | |||
* @author Sanghyeok An | |||
* @deprecated since Spring for Apache Kafka 4.0 in favor of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not good style since the @author
section becomes not visible and we lose a credit for those who contributed to the class.
You use too many words for the @deprecated
tag. It is really obvious if we use version without project mentioning. If that is not a case, we would mention the target library name.
So, for me this sentence could be like this:
@deprecated since 4.0 in favor of {@link org.springframework.kafka.test.condition.LogLevels}.
That is also obvious by LogLevels
design that it is for JUnit 5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not good style since the @author section becomes not visible and we lose a credit for those who contributed to the class.
Sorry to say that I don't understand your point. 😓
other spring projects do same things. for examples,
I built the java docs in my local and I can't see @author in all java docs.
However, spring-kafka
java docs don't display @author tags at all as well.(https://javadoc.io/doc/org.springframework.kafka/spring-kafka/latest/org/springframework/kafka/listener/CommonErrorHandler.html)
Could you let me know about not visiable @author tag...? 😢
Thanks for your time... 🙇♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Different projects may have different styles or just don't care about readability.
I try to be with my code as friendly as possible.
That style:
makes it hard for me to read the code, so I imaging that not only me, therefore in my code I try to separate those sections for readers of my code.
If you use @
here in GH comments, please, wrap that into code snippet.
Otherwise GH treats it as mentioning of that user here on GH.
Just try to follow that @author
link. Since there that GH user you have just called him into our discussion 🤷
I'm not sure what Javadoc link you show there, but the one we provide on our site has @author
list: https://docs.spring.io/spring-kafka/docs/3.3.5/api/org/springframework/kafka/listener/AbstractMessageListenerContainer.html.
Either way, my point is not about Javadoc tool, but rather source code to be readable clearly.
In the end that's why it is an Open Source!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comments!!
I totally misunderstood your points. 😅
I thought you were letting me know that the author would not be visible in the JavaDoc.
|
||
} | ||
---- | ||
|
||
[[kafka-testing-junit4-class-rule]] | ||
=== JUnit4 Class Rule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to have all the JUnit 4 text removed in favor of a single sentence that its support is deprecated for removal in the next minor version.
Signed-off-by: chickenchickenlove <[email protected]>
@@ -56,8 +56,7 @@ If this is not possible for some reason, note that the `consumeFromEmbeddedTopic | |||
Since it does not have access to the consumer properties, you must use the overloaded method that takes a `seekToEnd` boolean parameter to seek to the end instead of the beginning. | |||
==== | |||
|
|||
NOTE: The `EmbeddedKafkaRule` JUnit 4 rule has been removed in version 4.0. | |||
For JUnit 4, you should use the `EmbeddedKafkaKraftBroker` directly or migrate to JUnit 5 with the `@EmbeddedKafka` annotation. | |||
NOTE: `Spring Kafka` no longer supports JUnit4. Please consider migrating from JUnit4 to JUnit5. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our rule is One sentence per line
: https://asciidoctor.org/docs/asciidoc-recommended-practices/
And another nit-pick.
Since this is a technical documentation it has to be in a business language and impersonal.
Therefore words, like you
, we
, please
have to be avoided.
I know we have a lot like this in our doc, but that doesn't mean that we have to pollute more.
@@ -121,15 +120,19 @@ The following example configuration creates topics called `cat` and `hat` with f | |||
|
|||
[source, java] | |||
---- | |||
@SpringJUnitConfig | |||
@EmbeddedKafka( | |||
partitions = 5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear from this view, but can we be sure that we don't use tabs for indents?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -56,8 +56,7 @@ If this is not possible for some reason, note that the `consumeFromEmbeddedTopic | |||
Since it does not have access to the consumer properties, you must use the overloaded method that takes a `seekToEnd` boolean parameter to seek to the end instead of the beginning. | |||
==== | |||
|
|||
NOTE: The `EmbeddedKafkaRule` JUnit 4 rule has been removed in version 4.0. | |||
For JUnit 4, you should use the `EmbeddedKafkaKraftBroker` directly or migrate to JUnit 5 with the `@EmbeddedKafka` annotation. | |||
NOTE: `Spring Kafka` no longer supports JUnit4. Please consider migrating from JUnit4 to JUnit5. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not refer to JUnit Jupiter as "JUnit 5", especially since we (the JUnit team) will be releasing JUnit Jupiter 6 as part of "JUnit 6" later this year.
In other words, I think it is better to refer to JUnit Jupiter as "JUnit Jupiter" to avoid any confusion and unnecessary updates to documentation in the future.
I would also suggest that "JUnit Jupiter" be used consistently across the documentation.
Signed-off-by: chickenchickenlove <[email protected]>
[NOTE] | ||
==== | ||
`Spring Kafka` no longer supports `JUnit4`. | ||
Migration to `JUnit Jupiter` is recommended. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those words don’t justify to be a code.
Plus, why stick 4
to JUnit
word?
Also, the official project name is Spring for Apache Kafka
and since we don’t use code wrapping here why do that for JUnit projects?
Signed-off-by: chickenchickenlove <[email protected]>
The class that implements
MethodRule
in Junit4 is called whenever each test method is executed.Before executing the test method, it configures the log level.
Then, after executing the test method, it returns the log level back.
Like
MethodRule
in Junit4, the interfaceInvocationInterceptor
in Junit5 providesinterceptTestMethod(...)
.So, I migrated
MethodRule
toInvocationInterceptor
.