Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Igor Munkin <imun@tarantool.org>, Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable
Date: Mon, 15 Aug 2022 15:08:22 +0300	[thread overview]
Message-ID: <d98192a9-1e10-1022-e8ec-9acdc1ff4cea@tarantool.org> (raw)
In-Reply-To: <d21c760d38561c2b790d0de6d0bb765763f203f3.1660216002.git.imun@tarantool.org>

Igor,

  thanks for the patch. See my comments inline.

On 11.08.2022 14:17, Igor Munkin wrote:
> Before the patch both memprof and sysprof artefacts are generated within
> the binary artefacts tree (i.e. in the same directory LuaJIT binary is
> generated). However, more convenient way is producing these temporary
> profiles in a separate directory (e.g. located on the partition with
> more strict space limits). As a result of the patch all memprof and
> sysprof test chunks consider LUAJIT_TEST_VARDIR environment variable to
> set the directory where test profiles are generated. For the case when
> LUAJIT_TEST_VARDIR is not set, everything works as before.

Commit message is inconsistent a bit with a patch itself.

As far as I understand many hunks are not related to introducing 
LUAJIT_TEST_VARDIR.

Probably it is better to change commit one-line message from "test: 
introduce LUAJIT_TEST_VARDIR variable"

to something like "test: refactoring and introduce LUAJIT_TEST_VARDIR 
variable". It allows to keep patch

as is and change expectations for those who will look at your patch.


>
> Part of tarantool/tarantool#7472
>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
>   .../gh-5813-resolving-of-c-symbols.test.lua   |  6 +++--
>   .../misclib-memprof-lapi.test.lua             | 22 ++++++++++---------
>   .../misclib-sysprof-lapi.test.lua             |  8 ++++---
>   test/tarantool-tests/utils.lua                | 12 ++++++++++
>   4 files changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua b/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua
> index d589dddf..e0b6d86d 100644
> --- a/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua
> +++ b/test/tarantool-tests/gh-5813-resolving-of-c-symbols.test.lua
> @@ -1,5 +1,7 @@
>   -- Memprof is implemented for x86 and x64 architectures only.
> -require("utils").skipcond(
> +local utils = require("utils")
> +
> +utils.skipcond(
>     jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux",
>     jit.arch.." architecture or "..jit.os..
>     " OS is NIY for memprof c symbols resolving"
> @@ -18,7 +20,7 @@ local testboth = require "resboth"
>   local testhash = require "reshash"
>   local testgnuhash = require "resgnuhash"
>   
> -local TMP_BINFILE = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%.%1.memprofdata.tmp.bin")
> +local TMP_BINFILE = utils.profilename("memprofdata.tmp.bin")
>   
>   local function tree_contains(node, name)
>     if node == nil then
> diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> index a11f0be1..bae0c27c 100644
> --- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
> @@ -1,5 +1,7 @@
>   -- Memprof is implemented for x86 and x64 architectures only.
> -require("utils").skipcond(
> +local utils = require("utils")
> +
> +utils.skipcond(
>     jit.arch ~= "x86" and jit.arch ~= "x64",
>     jit.arch.." architecture is NIY for memprof"
>   )
> @@ -26,8 +28,8 @@ local memprof = require "memprof.parse"
>   local process = require "memprof.process"
>   local symtab = require "utils.symtab"
>   
> -local TMP_BINFILE = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%.%1.memprofdata.tmp.bin")
> -local BAD_PATH = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%1/memprofdata.tmp.bin")
> +local TMP_BINFILE = utils.profilename("memprofdata.tmp.bin")
> +local BAD_PATH = utils.profilename("memprofdata/tmp.bin")
>   local SRC_PATH = "@"..arg[0]
>   
>   local function default_payload()
> @@ -189,9 +191,9 @@ test:test("output", function(subtest)
>     -- one is the number of allocations. 1 event - alocation of
>     -- table by itself + 1 allocation of array part as far it is
>     -- bigger than LJ_MAX_COLOSIZE (16).
> -  subtest:ok(check_alloc_report(alloc, { line = 35, linedefined = 33 }, 2))
> +  subtest:ok(check_alloc_report(alloc, { line = 37, linedefined = 35 }, 2))
>     -- 20 strings allocations.
> -  subtest:ok(check_alloc_report(alloc, { line = 40, linedefined = 33 }, 20))
> +  subtest:ok(check_alloc_report(alloc, { line = 42, linedefined = 35 }, 20))
What are these magic numbers mean and why we should change them for 
introducing LUAJIT_TEST_VARDIR?
>   
>     -- Collect all previous allocated objects.
>     subtest:ok(free.INTERNAL.num == 22)
> @@ -199,8 +201,8 @@ test:test("output", function(subtest)
>     -- Tests for leak-only option.
>     -- See also https://github.com/tarantool/tarantool/issues/5812.
>     local heap_delta = process.form_heap_delta(events, symbols)
> -  local tab_alloc_stats = heap_delta[form_source_line(35)]
> -  local str_alloc_stats = heap_delta[form_source_line(40)]
> +  local tab_alloc_stats = heap_delta[form_source_line(37)]
> +  local str_alloc_stats = heap_delta[form_source_line(42)]
>     subtest:ok(tab_alloc_stats.nalloc == tab_alloc_stats.nfree)
>     subtest:ok(tab_alloc_stats.dbytes == 0)
>     subtest:ok(str_alloc_stats.nalloc == str_alloc_stats.nfree)
> @@ -291,10 +293,10 @@ test:test("jit-output", function(subtest)
>     -- 10 allocations in interpreter mode, 1 allocation for a trace
>     -- recording and assembling and next 9 allocations will happen
>     -- while running the trace.
> -  subtest:ok(check_alloc_report(alloc, { line = 40, linedefined = 33 }, 11))
> -  subtest:ok(check_alloc_report(alloc, { traceno = 1, line = 38 }, 9))
> +  subtest:ok(check_alloc_report(alloc, { line = 42, linedefined = 35 }, 11))
> +  subtest:ok(check_alloc_report(alloc, { traceno = 1, line = 40 }, 9))
>     -- See same checks with jit.off().
> -  subtest:ok(check_alloc_report(alloc, { line = 35, linedefined = 33 }, 2))
> +  subtest:ok(check_alloc_report(alloc, { line = 37, linedefined = 35 }, 2))
>   
>     -- Restore default JIT settings.
>     jit.opt.start(unpack(jit_opt_default))
> diff --git a/test/tarantool-tests/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> index 9e0a8a77..dbff41b0 100644
> --- a/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> +++ b/test/tarantool-tests/misclib-sysprof-lapi.test.lua
> @@ -1,6 +1,8 @@
>   -- Sysprof is implemented for x86 and x64 architectures only.
>   local ffi = require("ffi")
> -require("utils").skipcond(
> +local utils = require("utils")
> +
> +utils.skipcond(
>     jit.arch ~= "x86" and jit.arch ~= "x64" or jit.os ~= "Linux"
>       or ffi.abi("gc64"),
>     jit.arch.." architecture or "..jit.os..
> @@ -19,8 +21,8 @@ local bufread = require("utils.bufread")
>   local symtab = require("utils.symtab")
>   local sysprof = require("sysprof.parse")
>   
> -local TMP_BINFILE = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%.%1.sysprofdata.tmp.bin")
> -local BAD_PATH = arg[0]:gsub(".+/([^/]+)%.test%.lua$", "%1/sysprofdata.tmp.bin")
> +local TMP_BINFILE = utils.profilename("sysprofdata.tmp.bin")
> +local BAD_PATH = utils.profilename("sysprofdata/tmp.bin")
>   
>   local function payload()
>     local function fib(n)
> diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua
> index a1906498..87f7ff15 100644
> --- a/test/tarantool-tests/utils.lua
> +++ b/test/tarantool-tests/utils.lua
> @@ -123,4 +123,16 @@ function M.hasbc(f, bytecode)
>     return hasbc
>   end
>   
> +function M.profilename(name)
> +  local vardir = os.getenv('LUAJIT_TEST_VARDIR')
> +  -- Replace pattern will change directory name of the generated
> +  -- profile to LUAJIT_TEST_VARDIR if it is set in the process
> +  -- environment. Otherwise, the original dirname is left intact.
> +  -- As a basename for this profile the test name is concatenated
> +  -- with the name given as an argument.
> +  local replacepattern = ('%s/%s-%s'):format(vardir or '%1', '%2', name)
> +  -- XXX: return only the resulting string.
> +  return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern))
> +end
> +
>   return M

  reply	other threads:[~2022-08-15 12:08 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-11 11:17 [Tarantool-patches] [PATCH luajit 0/8] LuaJIT tests and CI enhancements Igor Munkin via Tarantool-patches
2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable Igor Munkin via Tarantool-patches
2022-08-15 12:08   ` Sergey Bronnikov via Tarantool-patches [this message]
2022-08-18  8:27     ` Sergey Kaplun via Tarantool-patches
2022-08-31 14:53     ` Igor Munkin via Tarantool-patches
2022-09-02 12:06       ` Sergey Bronnikov via Tarantool-patches
2022-10-05 19:51         ` Igor Munkin via Tarantool-patches
2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 2/8] test: introduce MakeLuaPath.cmake helper Igor Munkin via Tarantool-patches
2022-08-15 12:08   ` Sergey Bronnikov via Tarantool-patches
2022-08-31 15:07     ` Igor Munkin via Tarantool-patches
2022-09-02 12:09       ` Sergey Bronnikov via Tarantool-patches
2022-08-18  9:37   ` Sergey Kaplun via Tarantool-patches
2022-08-31 15:19     ` Igor Munkin via Tarantool-patches
2022-09-01 10:16       ` Sergey Kaplun via Tarantool-patches
2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 3/8] test: fix tarantool suite for out of source build Igor Munkin via Tarantool-patches
2022-08-15 12:10   ` Sergey Bronnikov via Tarantool-patches
2022-08-18  9:49   ` Sergey Kaplun via Tarantool-patches
2022-08-31 17:20     ` Igor Munkin via Tarantool-patches
2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 4/8] ci: use out of source build in GitHub Actions Igor Munkin via Tarantool-patches
2022-08-15 12:13   ` Sergey Bronnikov via Tarantool-patches
2022-08-18  9:58     ` Sergey Kaplun via Tarantool-patches
2022-08-31 15:34       ` Igor Munkin via Tarantool-patches
2022-08-31 15:33     ` Igor Munkin via Tarantool-patches
2022-09-02 12:09       ` Sergey Bronnikov via Tarantool-patches
2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 5/8] ci: remove excess parallel level setup Igor Munkin via Tarantool-patches
2022-08-15 12:14   ` Sergey Bronnikov via Tarantool-patches
2022-08-18 10:09   ` Sergey Kaplun via Tarantool-patches
2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 6/8] ci: remove arch prefix for macOS M1 workflow Igor Munkin via Tarantool-patches
2022-08-15 12:17   ` Sergey Bronnikov via Tarantool-patches
2022-08-18 10:14   ` Sergey Kaplun via Tarantool-patches
2022-08-31 15:55     ` Igor Munkin via Tarantool-patches
2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 7/8] ci: merge x86_64 and ARM64 workflows Igor Munkin via Tarantool-patches
2022-08-15 12:22   ` Sergey Bronnikov via Tarantool-patches
2022-08-18 10:21   ` Sergey Kaplun via Tarantool-patches
2022-08-31 16:02     ` Igor Munkin via Tarantool-patches
2022-08-11 11:17 ` [Tarantool-patches] [PATCH luajit 8/8] ci: merge Linux and macOS workflows Igor Munkin via Tarantool-patches
2022-08-15 12:27   ` Sergey Bronnikov via Tarantool-patches
2022-08-18 10:32   ` Sergey Kaplun via Tarantool-patches
2022-11-11  8:56 ` [Tarantool-patches] [PATCH luajit 0/8] LuaJIT tests and CI enhancements Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d98192a9-1e10-1022-e8ec-9acdc1ff4cea@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit 1/8] test: introduce LUAJIT_TEST_VARDIR variable' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox