Re: [tarantool-patches] Re: [PATCH] test: rework box/misc test part regarding on_shutdown triggers

[Serge Petrenko <sergepetrenko@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2950 bytes --]

Hi! Thank you for review.

> 12 апр. 2019 г., в 17:00, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> On Thu, Apr 11, 2019 at 04:37:20PM +0300, Serge Petrenko wrote:
>> Rewrite the test so that it doesn't depend on timeouts, which can be
>> exceeded under high load, too much and simplify it a little.
>> 
>> Also extract the whole part regarding on_shutdown triggers into a
>> separate test, so that its sporadic failures would be easier to
>> investigate.
>> 
>> Part of #4134
>> ---
>> https://github.com/tarantool/tarantool/issues/4134
>> https://github.com/tarantool/tarantool/tree/sp/gh-4134-fix-on-shutdown-tests
>> 
>> test/box/misc.result                   | 161 -------------------------
>> test/box/misc.test.lua                 |  62 ----------
>> test/box/on_shutdown_triggers.result   | 160 ++++++++++++++++++++++++
>> test/box/on_shutdown_triggers.test.lua |  62 ++++++++++
> 
> Better call simply box/on_shutdown.test.lua.
> 
> Anyway, this patch is impossible to review. Please split it in two - one
> moves the test case to a separate file, another - fixes it.

Will do.
> 
> I managed to extract the diff. Pasting it here for review.
> 
>> @@ -33,7 +33,7 @@
>> box.space.shutdown:select{}
>> box.space.shutdown:drop()
>> 
>> --- Check that os.exit invokes triggers
>> +-- Check that os.exit invokes on_shutdown triggers
>> fiber = require("fiber")
>> test_run:cmd("create server test with script='box/proxy.lua'")
>> test_run:cmd("start server test")
>> @@ -46,19 +46,17 @@
>> _ = box.ctl.on_shutdown(function() print("on_shutdown 5") end)
>> -- Check that we don't hang infinitely after os.exit()
>> -- even if the following code doesn't yield.
>> -fiber = require("fiber")
>> -_ = fiber.create(function() fiber.sleep(0.05) os.exit() while true do end end)
>> +fiber = require('fiber')
> 
> This 'require' isn't needed anymore, no?
> 
>> test_run:cmd("switch default")
>> +test_run:eval("test", "_ = fiber.new(function() os.exit() while true do end end)")
> 
> This is supposed to stop the default instance?!

No, it’s evaled on ’test’, which is the other instance. And that’s why we need the require(‘fiber’) above.
Anyways, will split the patch in two and resend.
> 
>> fiber.sleep(0.1)
>> -- The server should be already stopped by os.exit(),
>> -- but start doesn't work without a prior call to stop.
>> test_run:cmd("stop server test")
>> test_run:cmd("start server test")
>> -test_run:cmd("switch test")
>> -test_run:grep_log('test', 'on_shutdown 5', nil, {noreset=true})
>> +test_run:wait_log('test', 'on_shutdown 5', nil, 5, {noreset=true})
>> -- make sure we exited because of os.exit(), not a signal.
>> test_run:grep_log('test', 'signal', nil, {noreset=true})
>> -test_run:cmd("switch default")
>> test_run:cmd("stop server test")
>> test_run:cmd("cleanup server test")
>> test_run:cmd("delete server test")


[-- Attachment #2: Type: text/html, Size: 13482 bytes --]