[ROCKETMQ-264]Fix unit test cost too long and exception in unit test #145

lindzh · 2017-08-11T09:43:59Z

When run mvn test , it cost too much time and some times there is exception in unit test and the test result is pass

JIRA ticket https://issues.apache.org/jira/browse/ROCKETMQ-264

…pache#122, closes apache#71, closes apache#58

…evelop

coveralls · 2017-08-11T10:04:37Z

Coverage decreased (-0.1%) to 38.925% when pulling 91a3ec9 on lindzh:fix_test_exeception into aa1c757 on apache:develop.

coveralls · 2017-08-11T10:04:37Z

Coverage decreased (-0.1%) to 38.925% when pulling 91a3ec9 on lindzh:fix_test_exeception into aa1c757 on apache:develop.

coveralls · 2017-08-11T10:24:18Z

Coverage decreased (-0.2%) to 38.742% when pulling c818e36 on lindzh:fix_test_exeception into ccc2235 on apache:develop.

vongosling · 2017-08-11T11:12:24Z

How long it would take in your computer ? @Jaskey @vsair, please double check this optimized ut.

zhouxinyu · 2017-08-13T11:18:58Z

broker/src/test/java/org/apache/rocketmq/broker/BrokerControllerTest.java

+            new MessageStoreConfig());
+        assertThat(brokerController.initialize());
+        brokerController.start();
+        brokerController.shutdown();


If we only start/shutdown broker once, testBrokerRestart isn't a appropriate method~

zhouxinyu · 2017-08-13T11:22:51Z

broker/src/test/java/org/apache/rocketmq/broker/filter/MessageStoreWithFilterTest.java

-                e.printStackTrace();
-                assertThat(true).isFalse();
-            }
+            Thread.sleep(200);


Is 200ms enough to dispatch consume queue in a slow speed disk, or do we have another method to ensure the CQ has been constructed?

At this time,there is no way to ensure CQ constructed except adding countdownlatch to CQ,if only in test,I think there is no need to do this.

zhouxinyu · 2017-08-13T11:24:37Z

...ava/org/apache/rocketmq/client/consumer/rebalance/AllocateMessageQueueConsitentHashTest.java

-            int queueSize = new Random().nextInt(100)+1;//1-100
-            testAllocate(queueSize,consumerSize);
+    public void testRun100RandomCase() {
+        for (int i = 0; i < 10; i++) {


Method name should be polish~

zhouxinyu · 2017-08-13T11:25:35Z

...ava/org/apache/rocketmq/client/consumer/rebalance/AllocateMessageQueueConsitentHashTest.java

            try {
                Thread.sleep(1);
-            } catch (InterruptedException e) {}
+            } catch (InterruptedException e) {


Add exception to method signature is better~

…evelop

coveralls · 2017-08-14T02:39:53Z

Coverage decreased (-0.06%) to 39.011% when pulling 7c6d268 on lindzh:fix_test_exeception into 332df78 on apache:develop.

vongosling · 2017-08-15T09:49:02Z

broker/src/test/java/org/apache/rocketmq/broker/filter/MessageStoreWithFilterTest.java

+        try {
+            master = gen(filterManager);
+        } catch (Exception e) {
+            e.printStackTrace();


redundant code, when you use assert in your exception scenario, right ?

Yep,that's a good idea and this exception will be fixed at next commit.

This is a DefaultMessageStore start progress that used in annotation Before.And can't add expect Exception in before,I think there is no need to add exception expect in before and all we want is while the messageStore is started or not.

shroman · 2017-08-18T01:05:32Z

broker/src/test/java/org/apache/rocketmq/broker/BrokerControllerTest.java

-            brokerController.start();
-            brokerController.shutdown();
-        }
+        BrokerController brokerController = new BrokerController(


By changing this you change the expected behavior -- it is a check for proper restart.

As there is more whole startup in test case , I think there no need to do this. Any questions?IMO

shroman · 2017-08-18T01:13:20Z

namesrv/src/test/java/org/apache/rocketmq/namesrv/NamesrvControllerTest.java

@@ -1,46 +0,0 @@
-/*


Why is it deleted??

shroman · 2017-08-18T01:22:56Z

...ava/org/apache/rocketmq/client/consumer/rebalance/AllocateMessageQueueConsitentHashTest.java

-            int consumerSize = new Random().nextInt(200) + 1;//1-200
-            int queueSize = new Random().nextInt(100) + 1;//1-100
+        for (int i = 0; i < 10; i++) {
+            int consumerSize = new Random().nextInt(20) + 1;//1-20


Guys, what's the point to reduce these numbers (in other tests in this PR too)?
@vongosling @zhouxinyu

IMO, we need to have a pretty high number for messages/queues/etc. in unit tests, because it is a large-scale messaging system. Unless it makes tests really slow.

@shroman I would like to verify the program correction though the specific trick not iteration count(such as mock) especially in unit-test, while you mentioned "have a pretty high number for messages/queues/etc", IMO, we can do it in integration test. thoughts ?

@vongosling If you have stress tests for this, I think low numbers here are ok.
Btw, how do/will you verify it?

@shroman we can talk about these problems in dev mail. How to unit test, integration-test, performance test and stress test, and what's the main goal when we do it :-)

Does it spend too much time to allocate queue ? If not, i think it does't matter to run 10 or 100 times.

We aim at making test cost less time, make iteration count less is a good idea as unit test is only test for function.

vongosling · 2017-08-24T09:15:30Z

@Jaskey @vsair Looking forward to your opinion about this PR

vsair · 2017-08-24T09:28:45Z

store/src/test/java/org/apache/rocketmq/store/DefaultMessageStoreTest.java

        }
        //wait for consume queue build
-        Thread.sleep(100);
+        Thread.sleep(10);


Is 10ms enough to build consume queue?

In test scenario, 10ms is enough.

vsair · 2017-08-24T09:34:40Z

store/src/test/java/org/apache/rocketmq/store/DefaultMessageStoreTest.java

    }

    @After
    public void destory() {
+        messageStore.shutdown();
+        messageStore.destroy();


As i remember, method of destroy may not clean all files generated.

Delete created files has been did before at this file after destroy.

vsair · 2017-08-24T09:36:51Z

...ava/org/apache/rocketmq/client/consumer/rebalance/AllocateMessageQueueConsitentHashTest.java

-            int consumerSize = new Random().nextInt(200) + 1;//1-200
-            int queueSize = new Random().nextInt(100) + 1;//1-100
+        for (int i = 0; i < 10; i++) {
+            int consumerSize = new Random().nextInt(20) + 1;//1-20


Does it spend too much time to allocate queue ? If not, i think it does't matter to run 10 or 100 times.

vsair · 2017-08-24T09:37:22Z

store/src/test/java/org/apache/rocketmq/store/DefaultMessageStoreTest.java

        }
        //wait for consume queue build
-        Thread.sleep(100);
+        Thread.sleep(10);


Is it enough to build consume queue ?

coveralls · 2017-08-28T04:08:10Z

Coverage decreased (-0.4%) to 38.703% when pulling 25e3791 on lindzh:fix_test_exeception into 332df78 on apache:develop.

vongosling · 2017-08-28T10:06:32Z

@lindzh Thanks, I made some modifications and merged this PR. You could close it :-)

…tc file closes #145

…tc file closes apache#145

zhouxinyu and others added 3 commits August 1, 2017 20:35

Closes some timeout PRs, closes apache#125, closes apache#124, closes a…

421a22c

…pache#122, closes apache#71, closes apache#58

Merge branch 'develop' of github.com:apache/incubator-rocketmq into d…

1ae0693

…evelop

Fix unit test,delete invalid test and make test cost less time

91a3ec9

lindzh changed the base branch from master to develop August 11, 2017 09:44

lindzh changed the title ~~[ROCKETMQ-264] Fix unit test cost too long and exception in unit test~~ [ROCKETMQ-264]Fix unit test cost too long and exception in unit test Aug 11, 2017

lindzh added 2 commits August 11, 2017 18:02

Merge branch 'develop' of github.com:apache/incubator-rocketmq into d…

f0e243c

…evelop

Merge branch 'develop' into fix_test_exeception

c818e36

zhouxinyu reviewed Aug 13, 2017

View reviewed changes

lindzh added 2 commits August 14, 2017 10:11

Merge branch 'develop' of github.com:apache/incubator-rocketmq into d…

1810be4

…evelop

Merge branch 'develop' into fix_test_exeception

7c6d268

vongosling reviewed Aug 15, 2017

View reviewed changes

shroman reviewed Aug 18, 2017

View reviewed changes

namesrv/src/test/java/org/apache/rocketmq/namesrv/NamesrvControllerTest.java

@@ -1,46 +0,0 @@

/*

Copy link

Contributor

shroman Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it deleted??

shroman reviewed Aug 18, 2017

View reviewed changes

vsair reviewed Aug 24, 2017

View reviewed changes

add client logger test and fix log4j2 test

25e3791

asfgit pushed a commit that referenced this pull request Aug 28, 2017

ROCKETMQ-264 Fix ut's time-consuming problems closes #145

76464ba

lindzh closed this Aug 28, 2017

asfgit pushed a commit that referenced this pull request Aug 29, 2017

ROCKETMQ-277 Fix UnknownHostException when nothing configuration in e…

bbe825f

…tc file closes #145

lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019

ROCKETMQ-277 Fix UnknownHostException when nothing configuration in e…

66aca91

…tc file closes apache#145

lizhanhui pushed a commit to lizhanhui/rocketmq that referenced this pull request Jun 25, 2019

ROCKETMQ-277 Fix UnknownHostException when nothing configuration in e…

38f9f23

…tc file closes apache#145

JiaMingLiu93 pushed a commit to JiaMingLiu93/rocketmq that referenced this pull request May 28, 2020

ROCKETMQ-264 Fix ut's time-consuming problems closes apache#145

0959bac

JiaMingLiu93 pushed a commit to JiaMingLiu93/rocketmq that referenced this pull request May 28, 2020

ROCKETMQ-277 Fix UnknownHostException when nothing configuration in e…

602a214

…tc file closes apache#145

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ROCKETMQ-264]Fix unit test cost too long and exception in unit test #145

[ROCKETMQ-264]Fix unit test cost too long and exception in unit test #145

lindzh commented Aug 11, 2017

coveralls commented Aug 11, 2017 •

edited

Loading

coveralls commented Aug 11, 2017

coveralls commented Aug 11, 2017 •

edited

Loading

vongosling commented Aug 11, 2017

zhouxinyu Aug 13, 2017

zhouxinyu Aug 13, 2017

lindzh Aug 24, 2017

zhouxinyu Aug 13, 2017

zhouxinyu Aug 13, 2017

coveralls commented Aug 14, 2017 •

edited

Loading

vongosling Aug 15, 2017

lindzh Aug 16, 2017

lindzh Aug 28, 2017

shroman Aug 18, 2017

lindzh Aug 24, 2017

shroman Aug 18, 2017

shroman Aug 18, 2017

vongosling Aug 24, 2017 •

edited

Loading

shroman Aug 24, 2017

vongosling Aug 24, 2017

vsair Aug 24, 2017

lindzh Aug 24, 2017

vongosling commented Aug 24, 2017

vsair Aug 24, 2017

lindzh Aug 28, 2017

vsair Aug 24, 2017

lindzh Aug 28, 2017

vsair Aug 24, 2017

vsair Aug 24, 2017

coveralls commented Aug 28, 2017

vongosling commented Aug 28, 2017 •

edited

Loading

[ROCKETMQ-264]Fix unit test cost too long and exception in unit test #145

[ROCKETMQ-264]Fix unit test cost too long and exception in unit test #145

Conversation

lindzh commented Aug 11, 2017

coveralls commented Aug 11, 2017 • edited Loading

coveralls commented Aug 11, 2017

coveralls commented Aug 11, 2017 • edited Loading

vongosling commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vongosling Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vongosling commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 28, 2017

vongosling commented Aug 28, 2017 • edited Loading

coveralls commented Aug 11, 2017 •

edited

Loading

coveralls commented Aug 11, 2017 •

edited

Loading

coveralls commented Aug 14, 2017 •

edited

Loading

vongosling Aug 24, 2017 •

edited

Loading

vongosling commented Aug 28, 2017 •

edited

Loading