Re: [Tarantool-patches] [PATCH v2] Divide replication/misc.test.lua

["Alexander V. Tikhonov" <avtikhon@tarantool.org>]

Hi Mergen, thanks for the review, please check my comments below on your
comments and diff of the changes.

On Mon, Sep 07, 2020 at 05:46:30PM +0300, Mergen Imeev wrote:
> Hi! Thank you for the patch. See 4 comments below.
> 
> On Fri, Sep 04, 2020 at 09:13:58PM +0300, Alexander V. Tikhonov wrote:
> > To fix flaky issues of replication/misc.test.lua the test had to be
> > divided into smaller tests to be able to localize the flaky results:
> > 
> >   gh-2991-misc-assert-on-server-die.test.lua
> >   gh-3111-misc-rebootstrap-from-ro-master.test.lua
> >   gh-3160-misc-heartbeats-on-master-changes.test.lua
> >   gh-3247-misc-value-not-replicated-on-iproto-request.test.lua
> >   gh-3510-misc-assert-replica-on-applier-disconnect.test.lua
> >   gh-3606-misc-crash-on-box-concurrent-update.test.lua
> >   gh-3610-misc-assert-connecting-master-twice.test.lua
> >   gh-3637-misc-no-panic-on-connected.test.lua
> >   gh-3642-misc-no-socket-leak-on-replica-disconnect.test.lua
> >   gh-3704-misc-replica-checks-cluster-id.test.lua
> >   gh-3711-misc-no-restart-on-same-configuration.test.lua
> >   gh-3760-misc-return-on-quorum-0.test.lua
> >   gh-4399-misc-no-failure-on-error-reading-wal.test.lua
> >   gh-4424-misc-orphan-on-reconfiguration-error.test.lua
> 1. I don't think 'misc' is needed in test names.

I don't think so. The test name must consists the name of the initial
test to be able to check the history of the changes.

> > diff --git a/test/replication/gh-3760-misc-return-on-quorum-0.test.lua b/test/replication/gh-3760-misc-return-on-quorum-0.test.lua
> > new file mode 100644
> > index 000000000..30089ac23
> > --- /dev/null
> > +++ b/test/replication/gh-3760-misc-return-on-quorum-0.test.lua
> > @@ -0,0 +1,14 @@
> > +--
> > +-- gh-3760: replication quorum 0 on reconfiguration should return
> > +-- from box.cfg immediately.
> > +--
> > +replication = box.cfg.replication
> > +box.cfg{                                                        \
> > +    replication = {},                                           \
> > +    replication_connect_quorum = 0,                             \
> > +    replication_connect_timeout = 1000000                       \
> > +}
> > +-- The call below would hang, if quorum 0 is ignored, or checked
> > +-- too late.
> > +box.cfg{replication = {'localhost:12345'}}
> > +box.info.status
> 2. I think there is something wrong with this test. I mean, you change the
> box.cfg options, but you don't restore them.

Ok, as we discussed I've returned back the code removed after the first
review.

> > diff --git a/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua b/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua
> > new file mode 100644
> > index 000000000..a926ae590
> > --- /dev/null
> > +++ b/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua
> > @@ -0,0 +1,38 @@
> > +test_run = require('test_run').new()
> > +test_run:cmd("restart server default")
> > +fiber = require('fiber')
> 3. This is not needed here.

Ok, sure, removed.

> > --- a/test/replication/suite.cfg
> > +++ b/test/replication/suite.cfg
> > @@ -1,6 +1,19 @@
> >  {
> >      "anon.test.lua": {},
> > -    "misc.test.lua": {},
> > +    "misc_assert_connecting_master_twice_gh-3610.test.lua": {},
> > +    "misc_assert_on_server_die_gh-2991.test.lua": {},
> > +    "misc_assert_replica_on_applier_disconnect_gh-3510.test.lua": {},
> > +    "misc_crash_on_box_concurrent_update_gh-3606.test.lua": {},
> > +    "misc_heartbeats_on_master_changes_gh-3160.test.lua": {},
> > +    "misc_no_failure_on_error_reading_wal_gh-4399.test.lua": {},
> > +    "misc_no_panic_on_connected_gh-3637.test.lua": {},
> > +    "misc_no_restart_on_same_configuration_gh-3711.test.lua": {},
> > +    "misc_no_socket_leak_on_replica_disconnect_gh-3642.test.lua": {},
> > +    "misc_orphan_on_reconfiguration_error_gh-4424.test.lua": {},
> > +    "misc_rebootstrap_from_ro_master_gh-3111.test.lua": {},
> > +    "misc_replica_checks_cluster_id_gh-3704.test.lua": {},
> > +    "misc_return_on_quorum_0_gh-3760.test.lua": {},
> > +    "misc_value_not_replicated_on_iproto_request_gh-3247.test.lua": {},
> 4. Wrong names of the tests.

Right, corrected.

Check the changes bellow:

diff --git a/test/replication/gh-3760-misc-return-on-quorum-0.result b/test/replication/gh-3760-misc-return-on-quorum-0.result
index 79295f5c2..ed8ae529b 100644
--- a/test/replication/gh-3760-misc-return-on-quorum-0.result
+++ b/test/replication/gh-3760-misc-return-on-quorum-0.result
@@ -1,3 +1,12 @@
+replication_connect_timeout = box.cfg.replication_connect_timeout
+---
+...
+replication_connect_quorum = box.cfg.replication_connect_quorum
+---
+...
+box.cfg{replication="12345", replication_connect_timeout=0.1, replication_connect_quorum=1}
+---
+...
 --
 -- gh-3760: replication quorum 0 on reconfiguration should return
 -- from box.cfg immediately.
@@ -21,3 +30,10 @@ box.info.status
 ---
 - running
 ...
+box.cfg{                                                        \
+    replication = {},                                           \
+    replication_connect_quorum = replication_connect_quorum,    \
+    replication_connect_timeout = replication_connect_timeout   \
+}
+---
+...
diff --git a/test/replication/gh-3760-misc-return-on-quorum-0.test.lua b/test/replication/gh-3760-misc-return-on-quorum-0.test.lua
index 30089ac23..dc8508004 100644
--- a/test/replication/gh-3760-misc-return-on-quorum-0.test.lua
+++ b/test/replication/gh-3760-misc-return-on-quorum-0.test.lua
@@ -1,3 +1,7 @@
+replication_connect_timeout = box.cfg.replication_connect_timeout
+replication_connect_quorum = box.cfg.replication_connect_quorum
+box.cfg{replication="12345", replication_connect_timeout=0.1, replication_connect_quorum=1}
+
 --
 -- gh-3760: replication quorum 0 on reconfiguration should return
 -- from box.cfg immediately.
@@ -12,3 +16,8 @@ box.cfg{                                                        \
 -- too late.
 box.cfg{replication = {'localhost:12345'}}
 box.info.status
+box.cfg{                                                        \
+    replication = {},                                           \
+    replication_connect_quorum = replication_connect_quorum,    \
+    replication_connect_timeout = replication_connect_timeout   \
+}
diff --git a/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.result b/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.result
index 46b4f6464..a105e54b3 100644
--- a/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.result
+++ b/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.result
@@ -2,9 +2,6 @@ test_run = require('test_run').new()
 ---
 ...
 test_run:cmd("restart server default")
-fiber = require('fiber')
----
-...
 --
 -- gh-4399 Check that an error reading WAL directory on subscribe
 -- doesn't lead to a permanent replication failure.
diff --git a/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua b/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua
index a926ae590..f071d97c1 100644
--- a/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua
+++ b/test/replication/gh-4399-misc-no-failure-on-error-reading-wal.test.lua
@@ -1,6 +1,5 @@
 test_run = require('test_run').new()
 test_run:cmd("restart server default")
-fiber = require('fiber')

 --
 -- gh-4399 Check that an error reading WAL directory on subscribe
diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg
index e21daa5ad..f9f7055b7 100644
--- a/test/replication/suite.cfg
+++ b/test/replication/suite.cfg
@@ -1,19 +1,19 @@
 {
     "anon.test.lua": {},
-    "misc_assert_connecting_master_twice_gh-3610.test.lua": {},
-    "misc_assert_on_server_die_gh-2991.test.lua": {},
-    "misc_assert_replica_on_applier_disconnect_gh-3510.test.lua": {},
-    "misc_crash_on_box_concurrent_update_gh-3606.test.lua": {},
-    "misc_heartbeats_on_master_changes_gh-3160.test.lua": {},
-    "misc_no_failure_on_error_reading_wal_gh-4399.test.lua": {},
-    "misc_no_panic_on_connected_gh-3637.test.lua": {},
-    "misc_no_restart_on_same_configuration_gh-3711.test.lua": {},
-    "misc_no_socket_leak_on_replica_disconnect_gh-3642.test.lua": {},
-    "misc_orphan_on_reconfiguration_error_gh-4424.test.lua": {},
-    "misc_rebootstrap_from_ro_master_gh-3111.test.lua": {},
-    "misc_replica_checks_cluster_id_gh-3704.test.lua": {},
-    "misc_return_on_quorum_0_gh-3760.test.lua": {},
-    "misc_value_not_replicated_on_iproto_request_gh-3247.test.lua": {},
+    "gh-3610-misc-assert-connecting-master-twice.test.lua": {},
+    "gh-2991-misc-assert-on-server-die.test.lua": {},
+    "gh-3510-misc-assert-replica-on-applier-disconnect.test.lua": {},
+    "gh-3606-misc-crash-on-box-concurrent-update.test.lua": {},
+    "gh-3160-misc-heartbeats-on-master-changes.test.lua": {},
+    "gh-4399-misc-no-failure-on-error-reading-wal.test.lua": {},
+    "gh-3637-misc-no-panic-on-connected.test.lua": {},
+    "gh-3711-misc-no-restart-on-same-configuration.test.lua": {},
+    "gh-3642-misc-no-socket-leak-on-replica-disconnect.test.lua": {},
+    "gh-4424-misc-orphan-on-reconfiguration-error.test.lua": {},
+    "gh-3111-misc-rebootstrap-from-ro-master.test.lua": {},
+    "gh-3704-misc-replica-checks-cluster-id.test.lua": {},
+    "gh-3760-misc-return-on-quorum-0.test.lua": {},
+    "gh-3247-misc-value-not-replicated-on-iproto-request.test.lua": {},
     "once.test.lua": {},
     "on_replace.test.lua": {},
     "status.test.lua": {},