Refactor to Eliminate Repetitive Mock Object Creation(more exmple with #9271) #9282
Conversation
There was a problem hiding this comment.
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-jms/src/test/java/org/springframework/integration/jms/OutboundGatewayConnectionTests.java:36:8: Unused import - org.springframework.integration.context.IntegrationContextUtils. [UnusedImports]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-jms/src/test/java/org/springframework/integration/jms/OutboundGatewayConnectionTests.java:41:8: Unused import - org.springframework.scheduling.TaskScheduler. [UnusedImports]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-jms/src/test/java/org/springframework/integration/jms/OutboundGatewayConnectionTests.java:45:15: Unused import - org.mockito.Mockito.mock. [UnusedImports]
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-jms/src/test/java/org/springframework/integration/jms/OutboundGatewayConnectionTests.java:46:15: Unused import - org.mockito.Mockito.when. [UnusedImports]
Please, consider to run ./gradlew check locally before pushing to pull request.
| TcpConnectionSupport mockConn1 = makeMockConnection(); | ||
| TcpConnectionSupport mockConn2 = makeMockConnection(); | ||
| AbstractClientConnectionFactory factory1 = FailoverClientConnectionFactoryTests.makeMockactory(mockConn1); | ||
| AbstractClientConnectionFactory factory2 = FailoverClientConnectionFactoryTests.makeMockactory(mockConn2); |
There was a problem hiding this comment.
First of all there is a typo in the method name.
Secondly I'd prefer to mix test classes with each other.
Even if right now they have similar logic for mock it might be hard in the future to refactor them.
My point is that those tests are essentially closed boxed and it is better to design them without tangling with other test classes.
It might look right now as a readability improvement, but it will be harder to support them individually in the future when the logic one of them might change.
In the end these are just tests, so might not worse pursuing such a refactoring if you cannot find a solution to not tangle test classes with each other.
|
FYI, we prefer to use Will review the rest on Monday. |
There was a problem hiding this comment.
Error: eckstyle] [ERROR] /home/runner/work/spring-integration/spring-integration/spring-integration-ip/src/test/java/org/springframework/integration/ip/tcp/connection/MockFactory.java:33:17: 'METHOD_DEF' should be separated from previous line. [EmptyLineSeparator]
Please, use gradlew check command locally before pushing changes to PR.
| /** | ||
| * @author Gengwu Zhao | ||
| * | ||
| * @since 6.3.1 |
There was a problem hiding this comment.
This version has been release already.
And we are now in the 6.4 phase.
Since the change you are doing is just plain cosmetic refactoring, we are not going to back-port it into 6.3.x branch.
| * | ||
| */ | ||
|
|
||
| public class MockFactory { |
There was a problem hiding this comment.
The name for the class is too generic to be as a public API.
Plus it really does not reflect what is going on in the class.
The code style in the class does not follow our rules.
Please, use check Gradle command locally to catch all the code style problems before pushing.
I commented before that I prefer to not have a mixes between test classes since their logic might diverse in the future.
So, while the change you are doing is OK now and makes less code lines, it can become burden in the future to support such a generic code.
Therefore I'm OK with extracting common code into static methods of the same test class.
This will ensure that change into one test class won't effect others.
Plus it is better to be more specific with names.
This MockFactory class name is not about what it does.
The name of the makeMockFactory() method is also not fully about what it is doing.
| * | ||
| */ | ||
|
|
||
| public class MockBeanFactory { |
There was a problem hiding this comment.
This name is also wrong for public API.
And it also does not explain what it does to let other classes to use it.
I also wonder if we can use TestUtils.createTestApplicationContext() instead which has an expected bean registered.
Right, we might need to think about closing that ctx in the end of test, but that is indeed an existing API for the goal of your change: refactor code to minimize duplication.
gzhao9
force-pushed
the
refactor-CachingClientConnectionFactoryTests
branch
from
9f5406f to
a3fb68a
Compare
| @@ -26,12 +26,12 @@ | |||
| * | |||
| */ | |||
|
|
|||
| public class MockFactory { | |||
| private MockFactory() { | |||
| public class MockClientConnectionFactoryBuilder { | |||
There was a problem hiding this comment.
I am still against these new classes.
Please, make the logic as private static in those test classes affected.
Thanks
I told you to use |
gzhao9
force-pushed
the
refactor-CachingClientConnectionFactoryTests
branch
from
9a5e245 to
1856546
Compare
gzhao9
force-pushed
the
refactor-CachingClientConnectionFactoryTests
branch
from
4971574 to
d5c3904
Compare
Pull Request Description:
This pull request addresses issue #9283 , which is essentially doing the same thing as issue #9271. It refactors the 4 classes to eliminate redundant mock creations and simplify the testing code. Specifically, it introduces helper methods to centralize the creation and configuration of mocks for
AbstractClientConnectionFactory, andBeanFactory, reducing code duplication and enhancing readability.