From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <olegrok@tarantool.org>
Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98])
 (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 1083A469711
 for <tarantool-patches@dev.tarantool.org>;
 Tue,  2 Jun 2020 10:52:20 +0300 (MSK)
From: Oleg Babin <olegrok@tarantool.org>
References: <20200601222507.560415-1-gorcunov@gmail.com>
 <20200601222507.560415-10-gorcunov@gmail.com>
Message-ID: <368b0b95-c71c-dc2b-2a7e-1188af05813f@tarantool.org>
Date: Tue, 2 Jun 2020 10:52:17 +0300
MIME-Version: 1.0
In-Reply-To: <20200601222507.560415-10-gorcunov@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH v7 09/11] test: use direct log module
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Cyrill Gorcunov <gorcunov@gmail.com>, tml <tarantool-patches@dev.tarantool.org>

Hi! Thanks for your patch. See comments below.

On 02/06/2020 01:25, Cyrill Gorcunov wrote:
> To test if we can setup logging module before the box.cfg{}.
> 
> Part-of #689
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>   test/app-tap/logger.test.lua | 56 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 53 insertions(+), 3 deletions(-)
> 
> diff --git a/test/app-tap/logger.test.lua b/test/app-tap/logger.test.lua
> index 676c860d3..c6f207908 100755
> --- a/test/app-tap/logger.test.lua
> +++ b/test/app-tap/logger.test.lua
> @@ -1,7 +1,7 @@
>   #!/usr/bin/env tarantool
>   
>   local test = require('tap').test('log')
> -test:plan(25)
> +test:plan(34)
>   
>   -- gh-3946: Assertion failure when using log_format() before box.cfg()
>   local log = require('log')
> @@ -10,16 +10,66 @@ _,err = pcall(log, {log_format = 'json'})
>   test:ok(err ~= nil)
>   
>   --
> --- Check that Tarantool creates ADMIN session for #! script
> +-- gh-689: Operate with logger via log module without calling box.cfg{}
>   --
> +local json = require('json')
>   local filename = "1.log"
> +
> +-- Test json output
> +log.cfg({log=filename, format='json', level=5})
> +local m = "info message"
> +
> +local file = io.open(filename)
> +while file:read() do
> +end
> +
> +log.info(m)
> +local line = file:read()
> +local message = json.decode(line)
> +file:close()
> +
> +test:is(type(message), 'table', "(log) json valid in log.info")
> +test:is(message.level, "INFO", "(log) check type info")
> +test:is(message.message, m, "(log) check message content")
> +
> +-- Test runtime change to plain format
> +log.log_format('plain')
> +local file = io.open(filename)
> +while file:read() do
> +end
> +log.info(m)
> +local line = file:read()
> +file:close()
> +test:is(line:sub(-m:len()), m, m)

I think it could be checked as `test:ok(line:endswith(m))`.

> +
> +-- Test wrong arguments
> +_, err = pcall(log.cfg, {format = 'unknown'})
> +test:ok(err ~= nil)

I suggest to check the text of error messages.

> +
> +-- Test that changes in log module are propagated
> +-- back to the box module correctly

I think that here we should try to change static log parameters via 
box.cfg and check that it's impossible.

> +box.cfg{
> +    log=filename,
> +    log_format='json',
> +    log_level=6,
> +    memtx_memory=107374182,
> +}
> +test:is(box.cfg.log_format, 'json', 'box in json format')
> +test:is(box.cfg.log_level, 6, 'box level 6')
> +log.cfg({format='plain', level=5})
> +test:is(box.cfg.log_format, 'plain', 'box sees plain format')
> +test:is(box.cfg.log_level, 5, 'box sees level change' > +
> +--
> +-- Check that Tarantool creates ADMIN session for #! script
> +--
>   local message = "Hello, World!"
>   box.cfg{
>       log=filename,
> +    log_format='plain',
>       memtx_memory=107374182,
>   }
>   local fio = require('fio')
> -local json = require('json')
>   local fiber = require('fiber')
>   local file = io.open(filename)
>   while file:read() do
>