From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 BB768469710 for ; Thu, 4 Jun 2020 13:34:11 +0300 (MSK) Date: Thu, 4 Jun 2020 13:33:11 +0300 From: Sergey Bronnikov Message-ID: <20200604103311.GA19065@pony.bronevichok.ru> References: <20200402154444.GA8132@pony.bronevichok.ru> <20200515170409.GA10228@hpalx> <20200518100058.GC20053@pony.bronevichok.ru> <20200601092449.GA5590@hpalx> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200601092449.GA5590@hpalx> 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: "Alexander V. Tikhonov" Cc: tarantool-patches@dev.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 > > > > > > > >