From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111])
 (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 2CE9645C304
 for <tarantool-patches@dev.tarantool.org>;
 Sun, 29 Nov 2020 04:36:16 +0300 (MSK)
Date: Sun, 29 Nov 2020 04:36:14 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20201129013614.pygfn47szxofgxbi@tkn_work_nb>
References: <d190f811a920a74bc1e67f59f9327bf429514bb6.1606225907.git.avtikhon@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <d190f811a920a74bc1e67f59f9327bf429514bb6.1606225907.git.avtikhon@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v1] Move .tarantoolctl to test-run
	tool submodule
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: "Alexander V. Tikhonov" <avtikhon@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

> Github: https://github.com/tarantool/tarantool/tree/avtikhon/move_tarantoolctl_config

> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
> index 0726e7f46..92a5c4225 100755
> --- a/extra/dist/tarantoolctl.in
> +++ b/extra/dist/tarantoolctl.in
> @@ -70,6 +70,9 @@ local function check_user_level()
>      local pwd = os.getenv('PWD')
>      local udir = pwd and pwd .. '/.tarantoolctl'
>      udir = udir and fio.stat(udir) and udir or nil
> +    -- or test-run dir
> +    udir = udir or pwd and pwd .. '/../test-run/.tarantoolctl'
> +    udir = udir and fio.stat(udir) and udir or nil
>      -- or home dir configuration
>      local homedir = os.getenv('HOME')
>      udir = udir or homedir and homedir .. '/.config/tarantool/tarantool'

Now test-run copies test/.tarantoolctl to vardir. After [1] it should
copy it either from test/.tarantoolctl or from test-run/.tarantoolctl.
We don't need and should not change tarantoolctl for that.

[1]: https://github.com/tarantool/test-run/pull/242

> diff --git a/test-run b/test-run
> index 96dea99e6..1638eaa38 160000
> --- a/test-run
> +++ b/test-run
> @@ -1 +1 @@
> -Subproject commit 96dea99e69242380258d8dbccfb3735cce753d5a
> +Subproject commit 1638eaa384c9d2c30b6c2240add11e45d27ecd95

I suggest to just mention the PR in test-run, where the feature is
implemented. The logic is the following: when you send a patch for
review, it should be in the state that is ready to push to master.
However we should not update a submodule in master to a transient commit
(one that is not in the submodule's master / long term branch).
Otherwise we'll hit by problems like [2].

[2]: https://github.com/tarantool/small/issues/20

> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index 10882c6a1..f1c8c8d46 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -80,10 +80,17 @@ add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/luajit/test
>  
>  # Move tarantoolctl config
>  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
> -    configure_file(
> -        "${PROJECT_SOURCE_DIR}/test/.tarantoolctl"
> -        "${PROJECT_BINARY_DIR}/test/.tarantoolctl"
> +    if(EXISTS "${PROJECT_SOURCE_DIR}/test/.tarantoolctl")
> +        configure_file(
> +            "${PROJECT_SOURCE_DIR}/test/.tarantoolctl"
> +            "${PROJECT_BINARY_DIR}/test/.tarantoolctl"
>          )
> +    else()
> +        configure_file(
> +            "${PROJECT_SOURCE_DIR}/test-run/.tarantoolctl"
> +            "${PROJECT_BINARY_DIR}/test/.tarantoolctl"
> +        )
> +    endif()
>  endif()

I didn't find anything like @BLABLA@ or ${BLABLA} in the file (see [3]).
So it just copies the file to the build directory. Since we want to ship
it within test-run, there is no need for copying anymore.

It seems, the block can be just removed with explanation in the commit
message. `git blame` is your friend.

[3]: https://cmake.org/cmake/help/latest/command/configure_file.html

> diff --git a/test/box/admin.result b/test/box/admin.result
> index 8c5626c36..59ab4791e 100644
> --- a/test/box/admin.result
> +++ b/test/box/admin.result
> @@ -86,7 +86,7 @@ cfg_filter(box.cfg)
>    - - replication_sync_lag
>      - 10
>    - - replication_sync_timeout
> -    - 300
> +    - 100
>    - - replication_synchro_quorum
>      - 1
>    - - replication_synchro_timeout

How about add it to _hide in test/box/box.lua with appropriate comment?
AFAIU we test default values here, but this one is not default anymore
(and controlled by test-run). If we'll pass another value into the
test-run option, the test would fail that does not look as a good
effect.

The same for test/box/cfg.test.lua.