Re: [Tarantool-patches] [PATCH v1] Divide test box/net.box

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

Hi Sergey, thanks a lot for the review, please check my answers below.

On Thu, Jun 04, 2020 at 01:33:11PM +0300, Sergey Bronnikov wrote:
> Alexander,
> 
> I left comments inline, please see below
> 
> On 12:24 Mon 01 Jun , Alexander V. Tikhonov wrote:
> > Hi Sergey, thanks for the suggestions, I've tried to use Tarantool
> > packages 1.6.8 version and older, but met a lot of issue with it's
> > installing. Also the main issue that it's not possible to rebuild
> > Tarantool of the old packages in the new OS. The only found available
> > package fot Ubuntu:14.04, but it has issues with installation
> > dependencies, anyway it's version is later than checking patches.
> >
> > These issues were reported to out team and decided to avoid of checking
> > patches for fixes older than 1.7-1.9 versions.
> 
> 1. ok for me, I remember we agreed to skip reproducing on ancient tarantool
> versions
> 
> 2. Patch contains a result file test/box/gh-3107_rest1_net.box.result,
> but there is no test with same name. I've discovered another regression
> test for the same GH issue - net.box_collectgarbage_gh-3107.test.lua.
> Please clarify here.
> 

Right, that was draft version of the file
  test/box/net.box_timeout-gh-3107.result
removing.

> 3. Looks like you forgot to remove old test files:
> net.box.skipcond, net.box.result, net.box.test.lua.
> 
> --- /dev/null
> +++ b/test/box/net.box_call_blocks_gh-946.test.lua
> @@ -0,0 +1,67 @@
> +fiber = require 'fiber'
> +test_run = require('test_run').new()
> +
> +--space = box.schema.space.create('net_box_test_space')
> +--index = space:create_index('primary', { type = 'tree' })
>

Right, I've temporary restored it for merging with the new changes
and forgot to remove.

> 4. looks like we can remove lines above
> 
> +net = require('net.box')
> +socket = require('socket');
> 
> --- /dev/null
> +++ b/test/box/net.box_call_blocks_gh-946.test.lua
> @@ -0,0 +1,67 @@
> +fiber = require 'fiber'
> +test_run = require('test_run').new()
> +
> +--space = box.schema.space.create('net_box_test_space')
> +--index = space:create_index('primary', { type = 'tree' })
> 

Right, but only 'socket', 'net' stil in use:

--- box/net.box_call_blocks_gh-946.result	Thu Jun  4 18:43:38 2020
+++ var/001_box/net.box_call_blocks_gh-946.result	Thu Jun  4 18:44:00 2020
@@ -31,6 +31,7 @@
 ...
 c = net.connect(box.cfg.listen)
 ---
+- error: '[string "c = net.connect(box.cfg.listen) "]:1: variable ''net'' is not declared'


> 5. looks like we can remove lines above
> 
> +net = require('net.box')
> +socket = require('socket');
>

Removed 'socket' from result file too:
  test/box/net.box_call_blocks_gh-946.result

> 6. It would be more convenient to review if you will split single patch to
> multiple patches with addition new tests and removing old one.
> 

Ok.

> P.S. also code coverage [1] decreased slightly (-0.02%), but I don't
> think it's a problem.
>

Ok.

> [1] https://coveralls.io/github/tarantool/tarantool?branch=avtikhon/divide_tests
> 
> > On Mon, May 18, 2020 at 01:00:58PM +0300, Sergey Bronnikov wrote:
> > > Hi, Alexander
> > > 
> > > On 20:04 Fri 15 May , Alexander V. Tikhonov wrote:
> > > > Hi Sergey, thanks a lot for the review. As we discussed the ability to
> > > > reproduce the issues on the fixing commits I've checked group of tests:
> > > > 10 tests as you asked in the issue and 3 that I've already checked.
> > > > Whithin these tests results were:
> > > > 3 tests: all reproduced the issue
> > > > 10 tests:
> > > >   4 - reproduced the issue
> > > >   1 - didn't show the issue
> > > 
> > > could you reproduce issue using original test with source code without fix?
> > > 
> > > >   5 - couldn't build and test too old fixes committed in 1.6 Tarantool
> > > >   version.
> > > 
> > > could you reproduce them using tarantool from packages?
> > > 
> > > > To see the results with names please check the issue in github, where
> > > > I've created the table of results for the checked tests.
> > > > 
> > > > On Thu, Apr 02, 2020 at 06:44:44PM +0300, Sergey Bronnikov wrote:
> > > > > Hello!
> > > > > 
> > > > > before splitting test we had a net.box.skipcond file due to an issue
> > > > > https://github.com/tarantool/tarantool/issues/4271. I believe that file
> > > > > is useless now and we should add similar files for a new tests.
> > > > > 
> > > > > I have looked on tests without postfix "gh-xxx" in a filenames.
> > > > > LGTM for them:
> > > > > 
> > > > > net.box_connect_triggers.test.lua
> > > > > net.box_permissions.test.lua
> > > > > net.box_reconnect_after.test.lua
> > > > > net.box_get_connection_object.test.lua
> > > > > 
> > > > > I propose to run tests with postfix "gh-xxx" in filenames on commits
> > > > > before fix where bug covered by test was added. Broken test would mean
> > > > > it still works.
> > > > > 
> > > > > Sergey
> > > > > 
> > > 
> > > <snipped>