[Tarantool-patches] [PATCH v1 1/1] lua: fix test sql/func-recreate.test.lua

[Mergen Imeev]

Hi! I tested our solution.

I run these two versions of the test on two different commits.
The result showed that both versions work as intended.

The main idea behind these tests is to show an error to user when
sql_create_function() fails. To show that sql_create_function()
fails I added one more call of the function. When we receive
result of this call we may say which version of the function was
executed. I didn't include check which shows that when function
with the same name is created, it replaces old function. I checked
it and I hope it is not needed to be included here.


Old version:

fiber = require('fiber')
-- First version of the function
box.internal.sql_create_function('WAITFOR', 'INT', function (n) fiber.sleep(n) return n end)
ch = fiber.channel(1)
_ = fiber.create(function () ch:put(box.sql.execute('select WAITFOR(0.2)')) end)
fiber.sleep(0.1)
-- Second version of the function
box.internal.sql_create_function('WAITFOR', 'INT', function (n) return 3 end)
ch:get()
box.sql.execute('select WAITFOR(0.01)')


New version:

fiber = require('fiber')
flag = true
-- First version of the function
box.internal.sql_create_function('WAITFOR', 'INT', function (n) while flag do fiber.sleep(0.01) end return n end)
ch = fiber.channel(1)
_ = fiber.create(function () ch:put(box.sql.execute('select WAITFOR(0.2)')) end)
-- Second version of the function
box.internal.sql_create_function('WAITFOR', 'INT', function (n) return 3 end)
flag = false
ch:get()
box.sql.execute('select WAITFOR(0.01)')


1) Commit cfa7da0 (which is just before 14ba68f).

Result of the old version:

tarantool> box.internal.sql_create_function('WAITFOR', 'INT', function (n) return n + 1 end)
---
...

tarantool> ch:get()
---
- - [0.2]
...

tarantool> box.sql.execute('select WAITFOR(0.01)')
---
- - [0.01]
...


Result of the new version:

tarantool> box.internal.sql_create_function('WAITFOR', 'INT', function (n) return 3 end)
---
...

tarantool> flag = false
---
...

tarantool> ch:get()
---
- - [0.2]
...

tarantool> box.sql.execute('select WAITFOR(0.01)')
---
- - [0.01]
...


We can see, that in both cases sql_create_function() fails
(since we receive 0.01 in the last coll in both cases), but we
have no idea of this before we executed "select WAITFOR(0.01)".


2) Commit 14ba68f:

Result of the old version:

tarantool> box.internal.sql_create_function('WAITFOR', 'INT', function (n) return n + 1 end)
---
- error: database is locked
...

tarantool> ch:get()
---
- - [0.2]
...

tarantool> box.sql.execute('select WAITFOR(0.01)')
---
- - [0.01]
...


Result of the new version:

tarantool> box.internal.sql_create_function('WAITFOR', 'INT', function (n) return 3 end)
---
- error: database is locked
...

tarantool> flag = false
---
...

tarantool> ch:get()
---
- - [0.2]
...

tarantool> box.sql.execute('select WAITFOR(0.01)')
---
- - [0.01]
...


As we can see, an error appeared in both versions.

I think this says that our version of the test works properly.

On Fri, Apr 17, 2020 at 12:45:08PM +0000, Nikita Pettik wrote:
> On 17 Apr 09:05, Alexander Tikhonov wrote:
> > 
> > 
> > 
> >   
> > >Пятница, 17 апреля 2020, 1:04 +03:00 от Nikita Pettik <korablev at tarantool.org>:
> > > 
> > >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.
> > I still don’t understand this check way. Please clarify:
> > *  checkout the branch with fix based on 2.2 release branch (understood)
> > *  revert particular fix (understood, but it is the same as checkout native 2.2 release branch, anyway ok)
> 
> False. You have to revert this fix: 14ba68f93
> 
> > *  «run updated test without that fix» — what does it mean «updated», ok if you mean previous revert to native 2.2 release branch, than we did it manually and in regular testings, check gitlab-ci for results, in both variants test fails
> > Please point what is the difference between the way I understood and the way you suggest.
> > >
> > >>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.
> > >  
> >  
> >  
> > --
> > Alexander Tikhonov
> >