From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 41DFC469710 for ; Thu, 4 Jun 2020 18:51:00 +0300 (MSK) Date: Thu, 4 Jun 2020 18:50:57 +0300 From: "Alexander V. Tikhonov" Message-ID: <20200604155057.GA27465@hpalx> References: <20200402154444.GA8132@pony.bronevichok.ru> <20200515170409.GA10228@hpalx> <20200518100058.GC20053@pony.bronevichok.ru> <20200601092449.GA5590@hpalx> <20200604103311.GA19065@pony.bronevichok.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200604103311.GA19065@pony.bronevichok.ru> Subject: Re: [Tarantool-patches] [PATCH v1] Divide test box/net.box List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Bronnikov Cc: tarantool-patches@dev.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 > > > > > > > > > > >