-
Notifications
You must be signed in to change notification settings - Fork 3
tests/lapi: add string.buffer tests #148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
d0a24a8
to
36e5ef8
Compare
36e5ef8
to
26ef371
Compare
The patch add tests for LuaJIT's string buffer library [1]. Note, as it is stated in documentation [1] this serialization format is designed for internal use by LuaJIT applications, and this format is explicitly not intended to be a 'public standard' for structured data interchange across computer languages (like JSON or MessagePack). The purpose of the proposed tests is testing the library because other LuaJIT components relies on it and also the proposed tests indirectly tests FFI library. 1. https://luajit.org/ext_buffer.html
26ef371
to
0837c06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
local obj = fdp:consume_string(test_lib.MAX_STR_LEN) | ||
|
||
local MAX_SIZE = 1000 | ||
local buf_size = fdp:consume_integer(0, MAX_SIZE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it is better to use the same length as for the created string or MAX_STR_LEN
.
"tab_mt", | ||
"true", | ||
"uint64", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this table?
-- Appends the formatted arguments to the buffer. The format | ||
-- string supports the same options as `string.format()`. | ||
-- Usage: buf = buf:putf(format, ...) | ||
local function buffer_putf(self) -- luacheck: no unused |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this luacheck ignore?
-- string supports the same options as `string.format()`. | ||
-- Usage: buf = buf:putf(format, ...) | ||
local function buffer_putf(self) -- luacheck: no unused | ||
local MAX_N = 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this value is the same accross methods.
I suppose it could be set to the self
on initialization.
-- to by the FFI cdata object to the buffer. The object needs to | ||
-- be convertible to a (constant) pointer. | ||
-- Usage: buf = buf:putcdata(cdata, len) | ||
local function buffer_putcdata(self) -- luacheck: no unused |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this luacheck ignore?
local function buffer_decode(self) | ||
local MAX_N = 1000 | ||
local str = self.fdp:consume_string(0, MAX_N) | ||
local obj = self.buf:decode(str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may raise the error "unexpected end of buffer".
local MAX_N = 1000 | ||
local str = self.fdp:consume_string(0, MAX_N) | ||
local obj = self.buf:decode(str) | ||
assert(type(obj) == "cdata") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it should be cdata? I suppose the result value may have any type.
} | ||
|
||
local function buffer_random_op(self) | ||
local buffer_method= self.fdp:oneof(buffer_methods) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: s/=/ =/
end | ||
|
||
local function buffer_new(fdp) | ||
local buf_size = fdp:consume_number(1, test_lib.MAX_INT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This max value may be too big (argument out-of-range). Let's use MAX_N
here as well.
|
||
local function TestOneInput(buf, _size) | ||
local fdp = luzer.FuzzedDataProvider(buf) | ||
local nops = fdp:consume_number(1, test_lib.MAX_INT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This max value is too big. Let's use MAX_N
here as well.
Without it the 1 run may take too long.
The patch add tests for LuaJIT's string buffer library.