From 768809ef80341575ecf30a0d87470238824e23a5 Mon Sep 17 00:00:00 2001 From: Oleg Chaplashkin Date: Thu, 3 Aug 2023 19:34:03 +0400 Subject: [PATCH] Fix saving artifacts > What's the problem? When creating the logic for saving artifacts [1] a global variable has been created as `current_test` in the `runner.lua` module. Luatest has collected all the tests and runs a simple loop. At the beginning of each iteration the current test is written to the `current_test'. Server saved the value of the current test. However at some point the current test in the global variable and in the server object were different (the server stored information about the previous test). Also server does not store information about which test it was used in. > What is the solution? Redesign the logic of working with artifacts: now each test knows which server is linked to it, and each server knows about the test. For example: foo = Server:new() foo:start() -- only `foo` artifacts g.test_with_foo = function() foo:exec(...) end -- no server artifacts g.test_without_servers = function() ... end [1] tarantool/luatest#296 Helped-by: Igor Munkin Close #299 --- luatest/output/junit.lua | 11 ++++- luatest/output/tap.lua | 8 +++- luatest/output/text.lua | 9 ++-- luatest/runner.lua | 11 ++++- luatest/server.lua | 53 +++++++++++++++------ luatest/test_instance.lua | 10 +--- test/artifacts/common_test.lua | 63 +++++++++++++++++++++++++ test/artifacts/end_group_test.lua | 27 +++++++++++ test/artifacts/hooks_test.lua | 71 +++++++++++++++++++++++++++++ test/artifacts/replica_set_test.lua | 39 ++++++++++++++++ test/artifacts/sequence_test.lua | 42 +++++++++++++++++ test/artifacts/start_group_test.lua | 31 +++++++++++++ 12 files changed, 344 insertions(+), 31 deletions(-) create mode 100644 test/artifacts/common_test.lua create mode 100644 test/artifacts/end_group_test.lua create mode 100644 test/artifacts/hooks_test.lua create mode 100644 test/artifacts/replica_set_test.lua create mode 100644 test/artifacts/sequence_test.lua create mode 100644 test/artifacts/start_group_test.lua diff --git a/luatest/output/junit.lua b/luatest/output/junit.lua index a643b7ce..90e26ba5 100644 --- a/luatest/output/junit.lua +++ b/luatest/output/junit.lua @@ -1,3 +1,4 @@ +local utils = require('luatest.utils') local ROCK_VERSION = require('luatest.VERSION') -- See directory junitxml for more information about the junit format @@ -20,17 +21,23 @@ function Output.xml_c_data_escape(str) end function Output.node_status_xml(node) + local artifacts = '' + if utils.table_len(node.servers) > 0 then + for _, server in pairs(node.servers) do + artifacts = ('%s%s -> %s'):format(artifacts, server.alias, server.artifacts) + end + end if node:is('error') then return table.concat( {' \n', ' \n', - ' ', Output.xml_escape(node.artifacts or ''), '\n', + ' ', Output.xml_escape(artifacts), '\n', ' \n'}) elseif node:is('fail') then return table.concat( {' \n', ' \n', - ' ', Output.xml_escape(node.artifacts or ''), '\n', + ' ', Output.xml_escape(artifacts), '\n', ' \n'}) elseif node:is('skip') then return table.concat({' ', Output.xml_escape(node.message or ''),'\n'}) diff --git a/luatest/output/tap.lua b/luatest/output/tap.lua index 1292f650..6f064f93 100644 --- a/luatest/output/tap.lua +++ b/luatest/output/tap.lua @@ -1,3 +1,4 @@ +local utils = require('luatest.utils') -- For a good reference for TAP format, check: http://testanything.org/tap-specification.html local Output = require('luatest.output.generic'):new_class() @@ -28,8 +29,11 @@ function Output.mt:update_status(node) end if (node:is('fail') or node:is('error')) and self.verbosity >= self.class.VERBOSITY.VERBOSE then print(prefix .. node.trace:gsub('\n', '\n' .. prefix)) - if node.artifacts then - print(prefix .. node.artifacts:gsub('\n', '\n' .. prefix)) + if utils.table_len(node.servers) > 0 then + print(prefix .. 'artifacts:') + for _, server in pairs(node.servers) do + print(prefix .. server.alias .. ' -> ' .. server.artifacts) + end end end end diff --git a/luatest/output/text.lua b/luatest/output/text.lua index bf405969..2783de20 100644 --- a/luatest/output/text.lua +++ b/luatest/output/text.lua @@ -1,3 +1,4 @@ +local utils = require('luatest.utils') local Output = require('luatest.output.generic'):new_class() Output.BOLD_CODE = '\x1B[1m' @@ -60,10 +61,12 @@ function Output.mt:display_one_failed_test(index, fail) -- luacheck: no unused print(index..") " .. fail.name .. self.class.ERROR_COLOR_CODE) print(fail.message .. self.class.RESET_TERM) print(fail.trace) - if fail.artifacts then - print(fail.artifacts) + if utils.table_len(fail.servers) > 0 then + print('artifacts:') + for _, server in pairs(fail.servers) do + print(('\t%s -> %s'):format(server.alias, server.artifacts)) + end end - print() end function Output.mt:display_errored_tests() diff --git a/luatest/runner.lua b/luatest/runner.lua index e141869d..17eba907 100644 --- a/luatest/runner.lua +++ b/luatest/runner.lua @@ -413,17 +413,19 @@ end function Runner.mt:run_tests(tests_list) -- Make seed for ordering not affect other random numbers. math.randomseed(os.time()) + rawset(_G, 'current_test', {value = nil}) for _ = 1, self.exe_repeat_group or 1 do local last_group for _, test in ipairs(tests_list) do - rawset(_G, 'current_test', test) if last_group ~= test.group then if last_group then + rawget(_G, 'current_test').value = nil self:end_group(last_group) end self:start_group(test.group) last_group = test.group end + rawget(_G, 'current_test').value = test self:run_test(test) if self.result.aborted then break @@ -444,6 +446,13 @@ end function Runner.mt:invoke_test_function(test) local err = self:protected_call(test.group, test.method, test.name) self:update_status(test, err) + if not test:is('success') then + if utils.table_len(test.servers) > 0 then + for _, server in pairs(test.servers) do + server:save_artifacts() + end + end + end end function Runner.mt:find_test(groups, name) diff --git a/luatest/server.lua b/luatest/server.lua index cc0d2c9c..b655fb38 100644 --- a/luatest/server.lua +++ b/luatest/server.lua @@ -101,6 +101,27 @@ function Server:new(object, extra) object = utils.merge(object, extra) self:inherit(object) object:initialize() + + -- Each method of the server instance will be overridden by a new function + -- in which the association of the current test and server is performed first + -- and then the method itself. + -- It solves the problem when the server is not used in the test (should not + -- save artifacts) and when used. + for k, v in pairs(self) do + if type(v) == 'function' then + object[k] = function(...) + local t = rawget(_G, 'current_test') + if t and t.value then + t = t.value + if not object.tests[t.name] then + object.tests[t.name] = t + t.servers[object.id] = object + end + end + return v(...) + end + end + end return object end @@ -187,14 +208,12 @@ function Server:initialize() self.env.LUATEST_LUACOV_ROOT = os.getenv('LUATEST_LUACOV_ROOT') end - if self.current_test == nil then - self.current_test = rawget(_G, 'current_test') - if self.current_test then - local prefix = fio.pathjoin(Server.vardir, 'artifacts', self.rs_id or '') - self.artifacts = fio.pathjoin(prefix, self.id) - self.current_test:add_server_artifacts(self.alias, self.artifacts) - end + if not self.tests then + self.tests = {} end + + local prefix = fio.pathjoin(Server.vardir, 'artifacts', self.rs_id or '') + self.artifacts = fio.pathjoin(prefix, self.id) end -- Create a table with env variables based on the constructor params. @@ -329,12 +348,23 @@ function Server:restart(params, opts) log.debug('Restarted server PID: ' .. self.process.pid) end +-- Save server artifacts by copying the working directory. +-- Throws an error when the copying is not successful. +function Server:save_artifacts() + local ok, err = fio.copytree(self.workdir, self.artifacts) + if not ok then + error(('Failed to copy artifacts for server (alias: %s, workdir: %s, pid: %d): %s') + :format(self.alias, fio.basename(self.workdir), self.process.pid, err)) + end +end + -- Wait until the given condition is `true` (anything except `false` and `nil`). -- Throws an error when the server process is terminated or timeout exceeds. local function wait_for_condition(cond_desc, server, func, ...) local deadline = clock.time() + WAIT_TIMEOUT while true do if not server.process:is_alive() then + server:save_artifacts() error(('Process is terminated when waiting for "%s" condition for server (alias: %s, workdir: %s, pid: %d)') :format(cond_desc, server.alias, fio.basename(server.workdir), server.process.pid)) end @@ -342,6 +372,7 @@ local function wait_for_condition(cond_desc, server, func, ...) return end if clock.time() > deadline then + server:save_artifacts() error(('Timed out to wait for "%s" condition for server (alias: %s, workdir: %s, pid: %d) within %ds') :format(cond_desc, server.alias, fio.basename(server.workdir), server.process.pid, WAIT_TIMEOUT)) end @@ -387,13 +418,7 @@ end --- Stop the server and clean its working directory. function Server:drop() self:stop() - - if self.current_test and not self.current_test:is('success') then - local ok, err = fio.copytree(self.workdir, self.artifacts) - if not ok then - error(('Failed to copy server artifacts: %s'):format(err)) - end - end + self:save_artifacts() fio.rmtree(self.workdir) diff --git a/luatest/test_instance.lua b/luatest/test_instance.lua index 549a1026..a4db250e 100644 --- a/luatest/test_instance.lua +++ b/luatest/test_instance.lua @@ -16,7 +16,7 @@ end -- default constructor, test are PASS by default function TestInstance.mt:initialize() self.status = 'success' - self.artifacts = nil + self.servers = {} end function TestInstance.mt:update_status(status, message, trace) @@ -25,14 +25,6 @@ function TestInstance.mt:update_status(status, message, trace) self.trace = trace end -function TestInstance.mt:add_server_artifacts(alias, workdir) - local server_workdir = string.format('\n\t%s -> %s', alias, workdir) - if not self.artifacts then - self.artifacts = 'artifacts:' - end - self.artifacts = self.artifacts .. server_workdir -end - function TestInstance.mt:is(status) return self.status == status end diff --git a/test/artifacts/common_test.lua b/test/artifacts/common_test.lua new file mode 100644 index 00000000..182c6ba9 --- /dev/null +++ b/test/artifacts/common_test.lua @@ -0,0 +1,63 @@ +local t = require('luatest') +local utils = require('luatest.utils') + +local g = t.group() +local Server = t.Server + +g.public = Server:new({ alias = 'public'}) +g.public:start() + +g.test_servers_not_added_if_they_are_not_used = function() +end + +g.after_test('test_servers_not_added_if_they_are_not_used', function() + t.fail_if( + utils.table_len(rawget(_G, 'current_test').value.servers) ~= 0, + 'Test instance should not contain a servers') +end) + +g.test_only_public_server_has_been_added = function() + g.public:get_vclock() +end + +g.after_test('test_only_public_server_has_been_added', function() + t.fail_if( + rawget(_G, 'current_test').value.servers[g.public.id] == nil, + 'Test should contain only public server') +end) + + +g.test_only_private_server_has_been_added = function() + g.private = Server:new({alias = 'private'}) + g.private:start() +end + +g.after_test('test_only_private_server_has_been_added', function() + t.fail_if( + rawget(_G, 'current_test').value.servers[g.private.id] == nil, + 'Test should contain only private server') + +end) + +g.before_test('test_add_server_from_test_hooks', function() + g.before = Server:new({ alias = 'before' }) + g.before:start() +end) + +g.test_add_server_from_test_hooks = function() +end + +g.after_test('test_add_server_from_test_hooks', function() + g.after = Server:new({ alias = 'after' }) + g.after:start() + + local test_servers = rawget(_G, 'current_test').value.servers + + t.fail_if( + utils.table_len(test_servers) ~= 2, + 'Test should contain two servers (from before/after hooks)') + t.fail_if( + test_servers[g.before.id] == nil or + test_servers[g.after.id] == nil, + 'Test should contain only `before` and `after` servers') +end) diff --git a/test/artifacts/end_group_test.lua b/test/artifacts/end_group_test.lua new file mode 100644 index 00000000..ac08a74b --- /dev/null +++ b/test/artifacts/end_group_test.lua @@ -0,0 +1,27 @@ +local t = require('luatest') +local utils = require('luatest.utils') + +local Server = t.Server + +local g = t.group() + +g.test_foo = function() + g.foo_test = rawget(_G, 'current_test').value +end + +g.test_bar = function() + g.bar_test = rawget(_G, 'current_test').value +end + +g.after_all(function() + g.s = Server:new() + g.s:start() + + t.fail_if( + utils.table_len(g.foo_test.servers) ~= 0, + 'Test instance `foo` should not contain a servers') + + t.fail_if( + utils.table_len(g.bar_test.servers) ~= 0, + 'Test instance `bar` should not contain a servers') +end) diff --git a/test/artifacts/hooks_test.lua b/test/artifacts/hooks_test.lua new file mode 100644 index 00000000..2bce0b96 --- /dev/null +++ b/test/artifacts/hooks_test.lua @@ -0,0 +1,71 @@ +local t = require('luatest') +local utils = require('luatest.utils') +local fio = require('fio') + +local g = t.group() +local Server = t.Server + +local function is_server_in_test(server, test) + for _, s in pairs(test.servers) do + if server.id == s.id then + return true + end + end + return false +end + +g.public = Server:new({alias = 'public'}) +g.public:start() + +g.before_all(function() + g.all = Server:new({alias = 'all9'}) + g.all:start() +end) + +g.before_each(function() + g.each = Server:new({alias = 'each'}) + g.each:start() +end) + +g.before_test('test_association_between_test_and_servers', function() + g.test = Server:new({alias = 'test'}) + g.test:start() +end) + + +g.test_association_between_test_and_servers = function() + g.internal = Server:new({alias = 'internal'}) + g.internal:start() + + local test = rawget(_G, 'current_test').value + + -- test static association + t.assert(is_server_in_test(g.internal, test)) + t.assert(is_server_in_test(g.each, test)) + t.assert(is_server_in_test(g.test, test)) + t.assert_not(is_server_in_test(g.public, test)) + + g.public:exec(function() return 1 + 1 end) + g.all:exec(function() return 1 + 1 end) + + -- test dynamic association + t.assert(is_server_in_test(g.public, test)) + t.assert(is_server_in_test(g.all, test)) + + t.assert(utils.table_len(test.servers) == 5) +end + +g.after_test('test_association_between_test_and_servers', function() + g.test:drop() + t.assert(fio.path.exists(g.test.artifacts)) +end) + +g.after_each(function() + g.each:drop() + t.assert(fio.path.exists(g.each.artifacts)) +end) + +g.after_all(function() + g.all:drop() + t.assert(fio.path.exists(g.all.artifacts)) +end) diff --git a/test/artifacts/replica_set_test.lua b/test/artifacts/replica_set_test.lua new file mode 100644 index 00000000..508cbad7 --- /dev/null +++ b/test/artifacts/replica_set_test.lua @@ -0,0 +1,39 @@ +local t = require('luatest') +local utils = require('luatest.utils') +local ReplicaSet = require('luatest.replica_set') + +local g = t.group() + +g.box_cfg = { + replication_timeout = 0.1, + replication_connect_timeout = 3, + replication_sync_lag = 0.01, + replication_connect_quorum = 3 +} + +g.rs = ReplicaSet:new() +g.rs:build_and_add_server({alias = 'replica1', box_cfg = g.box_cfg}) +g.rs:build_and_add_server({alias = 'replica2', box_cfg = g.box_cfg}) + +g.test_foo = function() + g.rs:start() + g.foo_test = rawget(_G, 'current_test').value +end + +g.test_bar = function() + g.bar_test = rawget(_G, 'current_test').value +end + +g.after_test('test_foo', function() + t.fail_if( + utils.table_len(g.foo_test.servers) ~= 2, + 'Test instance should contain all servers from replica set' + ) +end) + +g.after_test('test_bar', function() + t.fail_if( + utils.table_len(g.bar_test.servers) ~= 0, + 'Test instance should not contain any servers' + ) +end) diff --git a/test/artifacts/sequence_test.lua b/test/artifacts/sequence_test.lua new file mode 100644 index 00000000..6dadc486 --- /dev/null +++ b/test/artifacts/sequence_test.lua @@ -0,0 +1,42 @@ +local t = require('luatest') +local utils = require('luatest.utils') + +local g = t.group() +local Server = t.Server + +g.s = Server:new() +g.s:start() + +g.before_each(function() + g.each = Server:new() + g.each:start() +end) + +g.test_foo = function() + g.foo_test = rawget(_G, 'current_test').value + g.foo_test_server = g.each.id +end + +g.test_bar = function() + g.bar_test = rawget(_G, 'current_test').value + g.bar_test_server = g.each.id +end + +g.after_test('test_foo', function() + t.fail_if( + utils.table_len(g.foo_test.servers) ~= 1, + 'Test instance should contain server') +end) + +g.after_test('test_bar', function() + t.fail_if( + utils.table_len(g.bar_test.servers) ~= 1, + 'Test instance should contain server') +end) + +g.after_all(function() + t.fail_if( + g.foo_test_server == g.bar_test_server, + 'Servers must be unique within the group' + ) +end) diff --git a/test/artifacts/start_group_test.lua b/test/artifacts/start_group_test.lua new file mode 100644 index 00000000..46821341 --- /dev/null +++ b/test/artifacts/start_group_test.lua @@ -0,0 +1,31 @@ +local t = require('luatest') +local utils = require('luatest.utils') + +local Server = t.Server + +local g = t.group() + +g.before_all(function() + g.s = Server:new() + g.s:start() +end) + +g.test_foo = function() + g.foo_test = rawget(_G, 'current_test').value +end + +g.after_test('test_foo', function() + t.fail_if( + utils.table_len(g.foo_test.servers) ~= 0, + 'Test instance should not contain a servers') +end) + +g.test_bar = function() + g.bar_test = rawget(_G, 'current_test').value +end + +g.after_test('test_bar', function() + t.fail_if( + utils.table_len(g.bar_test.servers) ~= 0, + 'Test instance should not contain a servers') +end)