Re: [Tarantool-patches] [PATCH luajit 0/2] Mach-O generation fixes

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi,

On 11.04.2024 11:27, Sergey Kaplun wrote:
> Hi,
>
> On 11.04.24, Sergey Bronnikov wrote:
>> Hi,
>>
>> On 08.04.2024 10:47, Sergey Kaplun wrote:
>>> Hi, Sergey!
>>> Thanks for the patch set!
>>>
>>> I'll proceed with the review according version on the branch:
>>>
>>> ===================================================================
>>>> Subject: [PATCH 1/3] test: introduce a helper read_file
>>> This patch LGTM, after fixing several nits below.
>>>
>>>> The test `lj-366-strtab-correct-size.test.lua` has a test helper
>>>> `read_file` that reads a file's content and returns it.
>>>> This helper will be useful for a test upcoming in the next commit,
>>>> so it is moved to test tools.
>>> So, missed the ticket mentioning:
>>> | Needed for tarantool/tarantool#9595
>> It is actually not needed for 9595.
> OK, not insisting.
>
>>>> ---
>>>>    .../lj-366-strtab-correct-size.test.lua                | 10 +---------
>>>>    test/tarantool-tests/utils/tools.lua                   |  8 ++++++++
>>>>    2 files changed, 9 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>>>> index 8a97a441..0bb92da6 100644
>>>> --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>>>> +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
>>>> @@ -138,14 +138,6 @@ local function create_obj_file(name)
>>>>      return elf_filename
>>>>    end
>>>>    
>>>> --- Reads a file located in a specified path and returns its content.
>>>> -local function read_file(path)
>>>> -  local file = assert(io.open(path), 'cannot open an object file')
>>>> -  local content = file:read('*a')
>>>> -  file:close()
>>>> -  return content
>>>> -end
>>>> -
>>>>    -- Parses a buffer in an ELF format and returns an offset and a size of strtab
>>>>    -- and symtab sections.
>>>>    local function read_elf(elf_content)
>>>> @@ -172,7 +164,7 @@ end
>>>>    test:plan(3)
>>>>    
>>>>    local elf_filename = create_obj_file(MODULE_NAME)
>>>> -local elf_content = read_file(elf_filename)
>>>> +local elf_content = require('utils').tools.read_file(elf_filename)
>>> Minor: I suggest avoiding the change at this line.
>>> According to codestyle in other tests, we require helpers separately,
>>> even if they are used only once.
>> Where can I read a codestyle guide which you want to follow?
> This is just a general tradition for all test files. Usually, it is a
> good thing to do it this way because we don't want to use a lookup for
> the module every time the function is called. This is not related to
> your case directly, but if somebody moves this code to the function, he
> should also change this part anyway. Plus, as a bonus, we will get an
> earlier failure if the `read_file` module doesn't exist.
>
So, the answer is "we don't have nor a codestyle guide

nor a static analysis that will force a certain style".

That's why we always get stuck with things like this in code review.


I've updated as you suggested.


--- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
+++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua
@@ -9,6 +9,7 @@ local test = 
tap.test('lj-366-strtab-correct-size'):skipcond({
  })

  local ffi = require 'ffi'
+local utils = require('utils')

  -- Command below exports bytecode as an object file in ELF format:
  -- $ luajit -b -n 'lango_team' -e 'print()' xxx.obj
@@ -130,7 +131,7 @@ local is64 = is64_arch[jit.arch]
  local function create_obj_file(name)
    local elf_filename = os.tmpname() .. '.obj'
    local lua_path = os.getenv('LUA_PATH')
-  local lua_bin = require('utils').exec.luacmd(arg):match('%S+')
+  local lua_bin = utils.exec.luacmd(arg):match('%S+')
    local cmd_fmt = 'LUA_PATH="%s" %s -b -n "%s" -e "print()" %s'
    local cmd = (cmd_fmt):format(lua_path, lua_bin, name, elf_filename)
    local ret = os.execute(cmd)

>>>>    assert(#elf_content ~= 0, 'cannot read an object file')
>>>>    
>>>>    local strtab, symtab = read_elf(elf_content)
>>>> diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua
>>>> index f35c6922..26b8c08d 100644
>>>> --- a/test/tarantool-tests/utils/tools.lua
>>>> +++ b/test/tarantool-tests/utils/tools.lua
>>>> @@ -12,4 +12,12 @@ function M.profilename(name)
>>>>      return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern))
>>>>    end
>>>>    
>>>> +-- Reads a file located in a specified path and returns its content.
>>> Typo: s/in/at/
>> Everything fine from Quillbot point of view.
> But I have it, see [1].
> Sometimes it freezes, so you should reload the current page.

Seems so.

Fixed.

>
>>
>>>> +function M.read_file(path)
>>>> +  local file = assert(io.open(path), 'cannot open an object file')
>>>> +  local content = file:read('*a')
>>>> +  file:close()
>>>> +  return content
>>>> +end
>>>> +
>>>>    return M
>>>> -- 
>>>> 2.44.0
>>> ===================================================================
>>>
> [1]: https://ibb.co/XzCPC13
>