From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 49DC64696C3 for ; Fri, 17 Apr 2020 09:25:56 +0300 (MSK) Date: Fri, 17 Apr 2020 09:25:53 +0300 From: Mergen Imeev Message-ID: <20200417062552.GA10540@tarantool.org> References: <1b7ea654465f9a92cb0949ce6286fee63e2be1ca.1587063872.git.imeevma@gmail.com> <1587064169.460537645@f501.i.mail.ru> <20200416191851.GA8455@tarantool.org> <20200416202451.GA15068@tarantool.org> <20200416210205.GB8455@tarantool.org> <20200416211547.GA19535@tarantool.org> <20200416220415.GC8455@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200416220415.GC8455@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v1 1/1] lua: fix test sql/func-recreate.test.lua List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org On Thu, Apr 16, 2020 at 10:04:16PM +0000, Nikita Pettik wrote: > On 17 Apr 00:15, Mergen Imeev wrote: > > On Thu, Apr 16, 2020 at 09:02:05PM +0000, Nikita Pettik wrote: > > > On 16 Apr 23:24, Mergen Imeev wrote: > > > > Hi! Thank you for review. My answer below. > > > > > > > > On Thu, Apr 16, 2020 at 07:18:51PM +0000, Nikita Pettik wrote: > > > > > On 16 Apr 22:09, Alexander Tikhonov wrote: > > > > > > > > > > > > Hi Mergen, thanks for the patch, I’ve checked it on 2.2 and it runs fine, patch LGTM. > > > > > > > > > > > > > > > > Did you verify that modified test still reproduces initial problem? > > > > > I hope so, but ask just in case. > > > > According to history, the main goal of this part of the > > > > test is to show that sql_create_function() throws proper > > > > error in case a new function is created when transaction > > > > is active. Since the error still here, we may say that > > > > the test works. > > > > > > So the answer to my question is - no, you didn't test it. > > > Better safe than sorry - I'd better verify that updated > > > test still covers the initial problem. Thanks. > > > > > Doesn't my answer says that we checked it? > > No, it doesn't. 'reproduce' means that you are supposed to checkout > to branch, revert particular fix, run updated test without that fix > and verify that test fails without fix. Any other actions do not refer > to my original question. > > >However, I agree that it is never wrong to check it again. > > Please, check it for current fix and for any other test > upgrade in future. > To check this I changed the value in fiber.sleep() function on line 12 to 0.3 in the test before the patch. After that the test fails with this result: [001] Test failed! Result content mismatch: [001] --- sql/func-recreate.result Fri Apr 17 09:13:48 2020 [001] +++ sql/func-recreate.reject Fri Apr 17 09:17:52 2020 [001] @@ -21,13 +21,11 @@ [001] _ = fiber.create(function () ch:put(box.execute('select WAITFOR(0.2)')) end) [001] --- [001] ... [001] -fiber.sleep(0.1) [001] +fiber.sleep(0.3) [001] --- [001] ... [001] box.internal.sql_create_function('WAITFOR', 'INT', function (n) require('fiber').sleep(n) return n end) [001] --- [001] -- error: 'Failed to create function ''WAITFOR'': unable to create function due to [001] - active statements' [001] ... [001] ch:get() [001] --- As you can see, the error dissappeared. This time we got this fail after we changed the test, but it is possible to get it even without changing. This happens when the computher which runs this test is too heavy loaded. But this won't happen after the patch. Is this good enough?