Details
-
Type:
Task
-
Status: Closed
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: None
-
Fix Version/s: 4.0.0-incubating
-
Component/s: None
-
Labels:None
Description
There are lots of System.out.println and the try...catch exception code in the test of rocketmq-store module. It's better to avoid the usage of System.out.println and just through out the exception to fail the test.
Issue Links
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
GitHub user dongeforever opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/7
ROCKETMQ-3 Clean up and perfect the unit test of rocketmq-broker
Clean up and perfect the unit test of rocketmq-broker.
Add BrokerTestHarness as a base class to create temporary BrokerController.
Flush test has three steps:
1 persist to the disk
2 clear the in-memory values
3 load from the disk.
Used for both testFlushConsumerOffset and testFlushTopicConfig
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dongeforever/incubator-rocketmq master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/7.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #7
commit 4a35f9575100ca04b379f7903f7134a406c3cd01
Author: zhendongliu.lzd <zhendongliu.lzd@alibaba-inc.com>
Date: 2016-12-23T06:49:29Z
ROCKETMQ-3 Clean up and perfect the unit test of rocketmq-broker
commit b82d36f977dbfa6d966efc3a33d05e50a49c3b7c
Author: zhendongliu.lzd <zhendongliu.lzd@alibaba-inc.com>
Date: 2016-12-23T07:14:15Z
Modify configs in BrokerTestHarness to avoid conflict
Github user dongeforever commented on the issue:
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/7
That's ok. please @lollipopjin @zhouxinyu review it
Github user lollipopjin commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/7
OK
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/7
Good job!
Github user asfgit closed the pull request at:
Github user WillemJiang commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/7#discussion_r93738207
— Diff: rocketmq-broker/src/test/java/com/alibaba/rocketmq/broker/topic/TopicConfigManagerTest.java —
@@ -20,52 +20,43 @@
*/
package com.alibaba.rocketmq.broker.topic;
-import com.alibaba.rocketmq.broker.BrokerController;
-import com.alibaba.rocketmq.common.BrokerConfig;
+import com.alibaba.rocketmq.broker.BrokerTestHarness;
import com.alibaba.rocketmq.common.MixAll;
import com.alibaba.rocketmq.common.TopicConfig;
-import com.alibaba.rocketmq.remoting.netty.NettyClientConfig;
-import com.alibaba.rocketmq.remoting.netty.NettyServerConfig;
-import com.alibaba.rocketmq.store.config.MessageStoreConfig;
import org.junit.Test;
import static org.junit.Assert.assertTrue;
/**
- * @author shijia.wxr
+ * @author zander
*/
-public class TopicConfigManagerTest {
+public class TopicConfigManagerTest extends BrokerTestHarness {
@Test - public void test_flushTopicConfig() throws Exception {
- BrokerController brokerController = new BrokerController(//
- new BrokerConfig(), //
- new NettyServerConfig(), //
- new NettyClientConfig(), //
- new MessageStoreConfig());
- boolean initResult = brokerController.initialize();
- System.out.println("initialize " + initResult);
- brokerController.start();
-
+ public void testFlushTopicConfig() throws Exception {
TopicConfigManager topicConfigManager = new TopicConfigManager(brokerController);
- TopicConfig topicConfig =
- topicConfigManager.createTopicInSendMessageMethod("TestTopic_SEND", MixAll.DEFAULT_TOPIC,
- null, 4, 0);
- assertTrue(topicConfig != null);
- - System.out.println(topicConfig);
-
for (int i = 0; i < 10; i++) { String topic = "UNITTEST-" + i; - topicConfig = - topicConfigManager - .createTopicInSendMessageMethod(topic, MixAll.DEFAULT_TOPIC, null, 4, 0); + TopicConfig topicConfig = topicConfigManager.createTopicInSendMessageMethod(topic, MixAll.DEFAULT_TOPIC, null, 4, 0); assertTrue(topicConfig != null); }-
topicConfigManager.persist();
- brokerController.shutdown();
+ topicConfigManager.getTopicConfigTable().clear();
+
+ for (int i = 0; i < 10; i++) { + String topic = "UNITTEST-" + i; + TopicConfig topicConfig = topicConfigManager.selectTopicConfig(topic); + assertTrue(topicConfig == null); + }+ topicConfigManager.load();
+ for (int i = 0; i < 10; i++) {
+ String topic = "UNITTEST-" + i;
+ TopicConfig topicConfig = topicConfigManager.selectTopicConfig(topic);
+ assertTrue(topicConfig != null);-
- End diff –
-
Please use assertNotNull directly.
Github user WillemJiang commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/7#discussion_r93738306
— Diff: rocketmq-broker/src/test/java/com/alibaba/rocketmq/broker/topic/TopicConfigManagerTest.java —
@@ -20,52 +20,43 @@
*/
package com.alibaba.rocketmq.broker.topic;
-import com.alibaba.rocketmq.broker.BrokerController;
-import com.alibaba.rocketmq.common.BrokerConfig;
+import com.alibaba.rocketmq.broker.BrokerTestHarness;
import com.alibaba.rocketmq.common.MixAll;
import com.alibaba.rocketmq.common.TopicConfig;
-import com.alibaba.rocketmq.remoting.netty.NettyClientConfig;
-import com.alibaba.rocketmq.remoting.netty.NettyServerConfig;
-import com.alibaba.rocketmq.store.config.MessageStoreConfig;
import org.junit.Test;
import static org.junit.Assert.assertTrue;
/**
- * @author shijia.wxr
+ * @author zander
*/
-public class TopicConfigManagerTest {
+public class TopicConfigManagerTest extends BrokerTestHarness {
@Test - public void test_flushTopicConfig() throws Exception {
- BrokerController brokerController = new BrokerController(//
- new BrokerConfig(), //
- new NettyServerConfig(), //
- new NettyClientConfig(), //
- new MessageStoreConfig());
- boolean initResult = brokerController.initialize();
- System.out.println("initialize " + initResult);
- brokerController.start();
-
+ public void testFlushTopicConfig() throws Exception {
TopicConfigManager topicConfigManager = new TopicConfigManager(brokerController);
- TopicConfig topicConfig =
- topicConfigManager.createTopicInSendMessageMethod("TestTopic_SEND", MixAll.DEFAULT_TOPIC,
- null, 4, 0);
- assertTrue(topicConfig != null);
- - System.out.println(topicConfig);
-
for (int i = 0; i < 10; i++) { String topic = "UNITTEST-" + i; - topicConfig = - topicConfigManager - .createTopicInSendMessageMethod(topic, MixAll.DEFAULT_TOPIC, null, 4, 0); + TopicConfig topicConfig = topicConfigManager.createTopicInSendMessageMethod(topic, MixAll.DEFAULT_TOPIC, null, 4, 0); assertTrue(topicConfig != null); }-
topicConfigManager.persist();
- brokerController.shutdown();
+ topicConfigManager.getTopicConfigTable().clear();
+
+ for (int i = 0; i < 10; i++) { + String topic = "UNITTEST-" + i; + TopicConfig topicConfig = topicConfigManager.selectTopicConfig(topic); + assertTrue(topicConfig == null); + }+ topicConfigManager.load();
+ for (int i = 0; i < 10; i++) {
+ String topic = "UNITTEST-" + i;
+ TopicConfig topicConfig = topicConfigManager.selectTopicConfig(topic);
+ assertTrue(topicConfig != null);
+ assertTrue(topicConfig.getTopicSysFlag() == 0);-
- End diff –
-
assertEquals could provider more information here.
Github user WillemJiang commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/7#discussion_r93737922
— Diff: rocketmq-broker/src/test/java/com/alibaba/rocketmq/broker/api/SendMessageTest.java —
@@ -20,68 +20,68 @@
*/
package com.alibaba.rocketmq.broker.api;
-import com.alibaba.rocketmq.broker.BrokerController;
+import com.alibaba.rocketmq.broker.BrokerTestHarness;
+import com.alibaba.rocketmq.client.ClientConfig;
import com.alibaba.rocketmq.client.hook.SendMessageContext;
import com.alibaba.rocketmq.client.impl.CommunicationMode;
import com.alibaba.rocketmq.client.impl.MQClientAPIImpl;
import com.alibaba.rocketmq.client.producer.SendResult;
-import com.alibaba.rocketmq.common.BrokerConfig;
+import com.alibaba.rocketmq.client.producer.SendStatus;
import com.alibaba.rocketmq.common.MixAll;
import com.alibaba.rocketmq.common.message.Message;
import com.alibaba.rocketmq.common.message.MessageDecoder;
import com.alibaba.rocketmq.common.protocol.header.SendMessageRequestHeader;
import com.alibaba.rocketmq.remoting.netty.NettyClientConfig;
-import com.alibaba.rocketmq.remoting.netty.NettyServerConfig;
-import com.alibaba.rocketmq.store.config.MessageStoreConfig;
+import org.junit.After;
+import org.junit.Before;
import org.junit.Test;
+import static org.junit.Assert.*;
+
/**
- * @author shijia.wxr
+ * @author zander
*/
-public class SendMessageTest { - @Test
- public void test_sendMessage() throws Exception {
- BrokerController brokerController = new BrokerController(//
- new BrokerConfig(), //
- new NettyServerConfig(), //
- new NettyClientConfig(), //
- new MessageStoreConfig());
- boolean initResult = brokerController.initialize();
- System.out.println("initialize " + initResult);
+public class SendMessageTest extends BrokerTestHarness{
- brokerController.start();
+ MQClientAPIImpl client = new MQClientAPIImpl(new NettyClientConfig(), null, null, new ClientConfig());
+ String topic = "UnitTestTopic";
- MQClientAPIImpl client = new MQClientAPIImpl(new NettyClientConfig(), null, null, null);
+ @Before
+ @Override
+ public void startup() throws Exception {
+ super.startup();
client.start();
- for (int i = 0; i < 100; i++) {
- String topic = "UnitTestTopic_" + i % 3;
- Message msg = new Message(topic, "TAG1 TAG2", "100200300", ("Hello, Nice world\t" + i).getBytes());
- msg.setDelayTimeLevel(i % 3 + 1);
- - try
{
- SendMessageRequestHeader requestHeader = new SendMessageRequestHeader();
- requestHeader.setProducerGroup("abc");
- requestHeader.setTopic(msg.getTopic());
- requestHeader.setDefaultTopic(MixAll.DEFAULT_TOPIC);
- requestHeader.setDefaultTopicQueueNums(4);
- requestHeader.setQueueId(i % 4);
- requestHeader.setSysFlag(0);
- requestHeader.setBornTimestamp(System.currentTimeMillis());
- requestHeader.setFlag(msg.getFlag());
- requestHeader.setProperties(MessageDecoder.messageProperties2String(msg.getProperties()));
-
- SendResult result = client.sendMessage("127.0.0.1:10911", "brokerName", msg, requestHeader, 1000 * 5,
- CommunicationMode.SYNC, new SendMessageContext(), null);
- System.out.println(i + "\t" + result);
- }
catch (Exception e)
{ - e.printStackTrace(); - } - }
+ }
+ @After
+ @Override
+ public void shutdown() throws Exception
- brokerController.shutdown();
+ @Test
+ public void testSendSingle() throws Exception {
+ Message msg = new Message(topic, "TAG1 TAG2", "100200300", "body".getBytes());
+ try { + SendMessageRequestHeader requestHeader = new SendMessageRequestHeader(); + requestHeader.setProducerGroup("abc"); + requestHeader.setTopic(msg.getTopic()); + requestHeader.setDefaultTopic(MixAll.DEFAULT_TOPIC); + requestHeader.setDefaultTopicQueueNums(4); + requestHeader.setQueueId(0); + requestHeader.setSysFlag(0); + requestHeader.setBornTimestamp(System.currentTimeMillis()); + requestHeader.setFlag(msg.getFlag()); + requestHeader.setProperties(MessageDecoder.messageProperties2String(msg.getProperties())); + + SendResult result = client.sendMessage(brokerAddr, BROKER_NAME, msg, requestHeader, 1000 * 5, + CommunicationMode.SYNC, new SendMessageContext(), null); + assertTrue(result.getSendStatus() == SendStatus.SEND_OK); + }catch (Exception e) {
-
- End diff –
-
It's not a good practice to catch the exception here, we can just throw the exception out and JUnit can take care of it. Please remove try ... catch here.
Github user WillemJiang commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/7
@dongeforever Please take look at the comments.
Github user vongosling commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/7#discussion_r93749430
— Diff: rocketmq-broker/src/test/java/com/alibaba/rocketmq/broker/topic/TopicConfigManagerTest.java —
@@ -20,52 +20,43 @@
*/
package com.alibaba.rocketmq.broker.topic;
-import com.alibaba.rocketmq.broker.BrokerController;
-import com.alibaba.rocketmq.common.BrokerConfig;
+import com.alibaba.rocketmq.broker.BrokerTestHarness;
import com.alibaba.rocketmq.common.MixAll;
import com.alibaba.rocketmq.common.TopicConfig;
-import com.alibaba.rocketmq.remoting.netty.NettyClientConfig;
-import com.alibaba.rocketmq.remoting.netty.NettyServerConfig;
-import com.alibaba.rocketmq.store.config.MessageStoreConfig;
import org.junit.Test;
import static org.junit.Assert.assertTrue;
/**
- * @author shijia.wxr
+ * @author zander
*/
-public class TopicConfigManagerTest {
+public class TopicConfigManagerTest extends BrokerTestHarness {
@Test - public void test_flushTopicConfig() throws Exception {
- BrokerController brokerController = new BrokerController(//
- new BrokerConfig(), //
- new NettyServerConfig(), //
- new NettyClientConfig(), //
- new MessageStoreConfig());
- boolean initResult = brokerController.initialize();
- System.out.println("initialize " + initResult);
- brokerController.start();
-
+ public void testFlushTopicConfig() throws Exception {
TopicConfigManager topicConfigManager = new TopicConfigManager(brokerController);
- TopicConfig topicConfig =
- topicConfigManager.createTopicInSendMessageMethod("TestTopic_SEND", MixAll.DEFAULT_TOPIC,
- null, 4, 0);
- assertTrue(topicConfig != null);
- - System.out.println(topicConfig);
-
for (int i = 0; i < 10; i++) { String topic = "UNITTEST-" + i; - topicConfig = - topicConfigManager - .createTopicInSendMessageMethod(topic, MixAll.DEFAULT_TOPIC, null, 4, 0); + TopicConfig topicConfig = topicConfigManager.createTopicInSendMessageMethod(topic, MixAll.DEFAULT_TOPIC, null, 4, 0); assertTrue(topicConfig != null); }-
topicConfigManager.persist();
- brokerController.shutdown();
+ topicConfigManager.getTopicConfigTable().clear();
+
+ for (int i = 0; i < 10; i++) { + String topic = "UNITTEST-" + i; + TopicConfig topicConfig = topicConfigManager.selectTopicConfig(topic); + assertTrue(topicConfig == null); + }+ topicConfigManager.load();
+ for (int i = 0; i < 10; i++) {
+ String topic = "UNITTEST-" + i;
+ TopicConfig topicConfig = topicConfigManager.selectTopicConfig(topic);
+ assertTrue(topicConfig != null);-
- End diff –
-
Serious Men~
Github user vongosling commented on a diff in the pull request:
https://github.com/apache/incubator-rocketmq/pull/7#discussion_r93749892
— Diff: rocketmq-broker/src/test/java/com/alibaba/rocketmq/broker/topic/TopicConfigManagerTest.java —
@@ -20,52 +20,43 @@
*/
package com.alibaba.rocketmq.broker.topic;
-import com.alibaba.rocketmq.broker.BrokerController;
-import com.alibaba.rocketmq.common.BrokerConfig;
+import com.alibaba.rocketmq.broker.BrokerTestHarness;
import com.alibaba.rocketmq.common.MixAll;
import com.alibaba.rocketmq.common.TopicConfig;
-import com.alibaba.rocketmq.remoting.netty.NettyClientConfig;
-import com.alibaba.rocketmq.remoting.netty.NettyServerConfig;
-import com.alibaba.rocketmq.store.config.MessageStoreConfig;
import org.junit.Test;
import static org.junit.Assert.assertTrue;
/**
- * @author shijia.wxr
+ * @author zander
*/
-public class TopicConfigManagerTest {
+public class TopicConfigManagerTest extends BrokerTestHarness {
@Test - public void test_flushTopicConfig() throws Exception {
- BrokerController brokerController = new BrokerController(//
- new BrokerConfig(), //
- new NettyServerConfig(), //
- new NettyClientConfig(), //
- new MessageStoreConfig());
- boolean initResult = brokerController.initialize();
- System.out.println("initialize " + initResult);
- brokerController.start();
-
+ public void testFlushTopicConfig() throws Exception {
TopicConfigManager topicConfigManager = new TopicConfigManager(brokerController);
- TopicConfig topicConfig =
- topicConfigManager.createTopicInSendMessageMethod("TestTopic_SEND", MixAll.DEFAULT_TOPIC,
- null, 4, 0);
- assertTrue(topicConfig != null);
- - System.out.println(topicConfig);
-
for (int i = 0; i < 10; i++) { String topic = "UNITTEST-" + i; - topicConfig = - topicConfigManager - .createTopicInSendMessageMethod(topic, MixAll.DEFAULT_TOPIC, null, 4, 0); + TopicConfig topicConfig = topicConfigManager.createTopicInSendMessageMethod(topic, MixAll.DEFAULT_TOPIC, null, 4, 0); assertTrue(topicConfig != null); }-
topicConfigManager.persist();
- brokerController.shutdown();
+ topicConfigManager.getTopicConfigTable().clear();
+
+ for (int i = 0; i < 10; i++) { + String topic = "UNITTEST-" + i; + TopicConfig topicConfig = topicConfigManager.selectTopicConfig(topic); + assertTrue(topicConfig == null); + }+ topicConfigManager.load();
+ for (int i = 0; i < 10; i++) {
+ String topic = "UNITTEST-" + i;
+ TopicConfig topicConfig = topicConfigManager.selectTopicConfig(topic);
+ assertTrue(topicConfig != null);
+ assertTrue(topicConfig.getTopicSysFlag() == 0);-
- End diff –
-
It is highly recommended to use AssertJ framework for more details assertion. because Junit hamcrest assert framework is not complete.
GitHub user dongeforever opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/9
ROCKETMQ-3 Clean up and perfect the unit test of rocketmq-broker
remove try catch, use assertNotNull etc
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dongeforever/incubator-rocketmq master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/9.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #9
commit 4a35f9575100ca04b379f7903f7134a406c3cd01
Author: zhendongliu.lzd <zhendongliu.lzd@alibaba-inc.com>
Date: 2016-12-23T06:49:29Z
ROCKETMQ-3 Clean up and perfect the unit test of rocketmq-broker
commit b82d36f977dbfa6d966efc3a33d05e50a49c3b7c
Author: zhendongliu.lzd <zhendongliu.lzd@alibaba-inc.com>
Date: 2016-12-23T07:14:15Z
Modify configs in BrokerTestHarness to avoid conflict
commit bc15c7345b58a4acc06c0a050602fed4faf01705
Author: zander <zhendongliu.lzd@alibaba-inc.com>
Date: 2016-12-23T07:34:31Z
Remove unused imports
commit 3f979a8d8500a488a50c1f9830c7e72d27e1d4fa
Author: zander <zhendongliu.lzd@alibaba-inc.com>
Date: 2016-12-23T10:21:23Z
ROCKETMQ-3 Clean up and perfect the unit test of rocketmq-broker, remove try catch, use assertNotNull etc
Github user dongeforever commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/9
@WillemJiang Thanks for your comments. And a new pull request was created to remove these defects
Github user WillemJiang commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/9
@dongeforever I had some trouble to resolve the rebase merge conflict just by pulling the changes from your branch( it looks like there some patch are already merged into the master branch). Can you rebase the master branch and use squash[1] create a single commit for it.
[1]https://makandracards.com/makandra/527-squash-several-git-commits-into-a-single-commit
Github user dongeforever commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/9
@WillemJiang the conflicts were resolved. And I tried squashing the commits, but it seemed does not work for the master branch because i had already pushed it to my 'origin' repository. Maybe you can close this PR, and i create a new one using a fresh branch.
Github user dongeforever closed the pull request at:
GitHub user dongeforever opened a pull request:
https://github.com/apache/incubator-rocketmq/pull/11
ROCKETMQ-3 Clean up unit tests, remove try...catch, using assertNull etc
Remove try...catch, using assertNull etc
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dongeforever/incubator-rocketmq apache_pr
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-rocketmq/pull/11.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #11
commit ec18e3530b7bc3067dcc7df1c04d3f5fff9244f0
Author: zhendongliu.lzd <zhendongliu.lzd@alibaba-inc.com>
Date: 2016-12-26T02:31:45Z
ROCKETMQ-3 Remove try...catch, using assertNull etc
Github user dongeforever commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/11
@WillemJiang I am sorry to bother you so many times. A new pr is created for squashing. And I will take care of my commits carefully next time.
Github user WillemJiang commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/11
There is only one line change here after rebasing the codes. I just commit the patch into master branch.
Github user vongosling commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/11
@shroman cool, we will add code-style parts in website. Every guys could comment it and help us to perfect it 
Github user dongeforever commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/11
This PR could be closed for the code has been merged into apache master.
Github user dongeforever closed the pull request at:
Github user WillemJiang commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/11
Please using the mailing list to discuss the code-style issue.
It is true for other subprojects too.