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

[Sergey Bronnikov <sergeyb@tarantool.org>]

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.

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' })

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' })

5. looks like we can remove lines above

+net = require('net.box')
+socket = require('socket');

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.

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

[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>