From 29e8538e6fc8d16086e27a96aad7b48f5968edfb Mon Sep 17 00:00:00 2001 From: Maksim Tiushev Date: Thu, 4 Dec 2025 08:38:01 +0000 Subject: [PATCH] server: fix artifact handling reset on restart Fix a bug when server initialization didn't reset artifact handling, which caused stale artifacts to persist after server restarts. Closes #409 --- CHANGELOG.md | 2 + luatest/replica_set.lua | 6 +-- luatest/runner.lua | 14 +++--- luatest/server.lua | 8 +-- luatest/test_instance.lua | 4 ++ test/artifacts/hooks_test.lua | 20 ++++++-- test/collect_rs_artifacts_test.lua | 24 ++++++--- test/collect_server_artifacts_test.lua | 30 +++++++---- test/replica_set_test.lua | 57 +++++++++++++-------- test/server_test.lua | 70 +++++++++++++++++++++++--- 10 files changed, 169 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d008d25..fcada198 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Fixed a bug when server initialization didn't reset artifact handling, + causing stale artifacts to persist after server restarts (gh-409). - Fixed a bug when the JUnit reporter generated invalid XML for parameterized tests with string arguments (gh-407). - Group and suite hooks must now be registered using the call-style diff --git a/luatest/replica_set.lua b/luatest/replica_set.lua index 6b1716f6..9d05975f 100644 --- a/luatest/replica_set.lua +++ b/luatest/replica_set.lua @@ -184,12 +184,10 @@ function ReplicaSet:stop() end end ---- Stop all servers in the replica set and save their artifacts if the test fails. +--- Stop all servers in the replica set. -- This function should be used only at the end of the test (`after_test`, -- `after_each`, `after_all` hooks) to terminate all server processes in --- the replica set. Besides process termination, it saves the contents of --- each server working directory to the `/artifacts` directory --- for further analysis if the test fails. +-- the replica set. function ReplicaSet:drop() for _, server in ipairs(self.servers) do server:drop() diff --git a/luatest/runner.lua b/luatest/runner.lua index 82c50ffe..4f2bf710 100644 --- a/luatest/runner.lua +++ b/luatest/runner.lua @@ -377,11 +377,6 @@ function Runner.mt:update_status(node, err) elseif err.status == 'fail' or err.status == 'error' or err.status == 'skip' or err.status == 'xfail' or err.status == 'xsuccess' then node:update_status(err.status, err.message, err.trace) - if utils.table_len(node.servers) > 0 then - for _, server in pairs(node.servers) do - server:save_artifacts() - end - end else error('No such status: ' .. pp.tostring(err.status)) end @@ -391,6 +386,13 @@ end function Runner.mt:end_test(node) node.duration = clock.time() - node.start_time node.start_time = nil + + if (node.had_failure or not node:is('success')) and utils.table_len(node.servers) > 0 then + for _, server in pairs(node.servers) do + server:save_artifacts() + end + end + self.output:end_test(node) if node:is('error') or node:is('fail') or node:is('xsuccess') then @@ -479,7 +481,7 @@ 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}) + rawset(_G, 'current_test', {runner = self, value = nil}) for _ = 1, self.exe_repeat_group or 1 do local last_group for _, test in ipairs(tests_list) do diff --git a/luatest/server.lua b/luatest/server.lua index 839afbc9..6a5ca7cd 100644 --- a/luatest/server.lua +++ b/luatest/server.lua @@ -531,7 +531,6 @@ 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 @@ -539,7 +538,6 @@ 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 @@ -585,15 +583,11 @@ function Server:stop() end end ---- Stop the server and save its artifacts if the test fails. +-- Stop the server. -- This function should be used only at the end of the test (`after_test`, -- `after_each`, `after_all` hooks) to terminate the server process. --- Besides process termination, it saves the contents of the server --- working directory to the `/artifacts` directory for further --- analysis if the test fails. function Server:drop() self:stop() - self:save_artifacts() self.instance_id = nil self.instance_uuid = nil diff --git a/luatest/test_instance.lua b/luatest/test_instance.lua index a4db250e..89f457cb 100644 --- a/luatest/test_instance.lua +++ b/luatest/test_instance.lua @@ -17,9 +17,13 @@ end function TestInstance.mt:initialize() self.status = 'success' self.servers = {} + self.had_failure = false end function TestInstance.mt:update_status(status, message, trace) + if status ~= 'success' then + self.had_failure = true + end self.status = status self.message = message self.trace = trace diff --git a/test/artifacts/hooks_test.lua b/test/artifacts/hooks_test.lua index 9b805b2a..2e1d4322 100644 --- a/test/artifacts/hooks_test.lua +++ b/test/artifacts/hooks_test.lua @@ -4,6 +4,7 @@ local fio = require('fio') local g = t.group() local Server = t.Server +local deferred_artifact_checks = {} local function is_server_in_test(server, test) for _, s in pairs(test.servers) do @@ -55,18 +56,31 @@ g.test_association_between_test_and_servers = function() end g.after_test('test_association_between_test_and_servers', function() + local ctx = rawget(_G, 'current_test') + local test = ctx.value + ctx.runner:update_status(test, {status = 'fail'}) g.internal:drop() g.test:drop() - t.assert(fio.path.exists(g.test.artifacts)) + test:update_status('success') + table.insert(deferred_artifact_checks, function() + t.assert(fio.path.exists(g.test.artifacts)) + end) end) g.after_each(function() g.each:drop() - t.assert(fio.path.exists(g.each.artifacts)) + table.insert(deferred_artifact_checks, function() + t.assert(fio.path.exists(g.each.artifacts)) + end) end) g.after_all(function() g.all:drop() - t.assert(fio.path.exists(g.all.artifacts)) + table.insert(deferred_artifact_checks, function() + t.assert(fio.path.exists(g.all.artifacts)) + end) g.public:drop() + for _, check in ipairs(deferred_artifact_checks) do + check() + end end) diff --git a/test/collect_rs_artifacts_test.lua b/test/collect_rs_artifacts_test.lua index a785018c..395d7e56 100644 --- a/test/collect_rs_artifacts_test.lua +++ b/test/collect_rs_artifacts_test.lua @@ -4,6 +4,7 @@ local utils = require('luatest.utils') local ReplicaSet = require('luatest.replica_set') local g = t.group() +local deferred_artifact_checks = {} local Server = t.Server local function build_specific_replica_set(alias_suffix) @@ -87,15 +88,24 @@ g.before_test('test_foo', function() end) g.test_foo = function() - local test = rawget(_G, 'current_test') + local ctx = rawget(_G, 'current_test') + local test = ctx.value - test.status = 'fail' g.rs_test:drop() g.rs_each:drop() g.rs_all:drop() - test.status = 'success' - - assert_artifacts_paths(g.rs_test, 'test') - assert_artifacts_paths(g.rs_each, 'each') - assert_artifacts_paths(g.rs_all, 'all') + ctx.runner:update_status(test, {status = 'fail'}) + test:update_status('success') + + table.insert(deferred_artifact_checks, function() + assert_artifacts_paths(g.rs_test, 'test') + assert_artifacts_paths(g.rs_each, 'each') + assert_artifacts_paths(g.rs_all, 'all') + end) end + +g.after_all(function() + for _, check in ipairs(deferred_artifact_checks) do + check() + end +end) diff --git a/test/collect_server_artifacts_test.lua b/test/collect_server_artifacts_test.lua index 00e39ed9..a5decde6 100644 --- a/test/collect_server_artifacts_test.lua +++ b/test/collect_server_artifacts_test.lua @@ -2,6 +2,7 @@ local fio = require('fio') local t = require('luatest') local g = t.group() +local deferred_artifact_checks = {} local Server = t.Server @@ -35,21 +36,30 @@ g.before_test('test_foo', function() end) g.test_foo = function() - local test = rawget(_G, 'current_test') + local ctx = rawget(_G, 'current_test') + local test = ctx.value - test.status = 'fail' g.s_test:drop() g.s_test2:drop() g.s_each:drop() g.s_each2:drop() g.s_all:drop() g.s_all2:drop() - test.status = 'success' - - assert_artifacts_path(g.s_test) - assert_artifacts_path(g.s_test2) - assert_artifacts_path(g.s_each) - assert_artifacts_path(g.s_each2) - assert_artifacts_path(g.s_all) - assert_artifacts_path(g.s_all2) + ctx.runner:update_status(test, {status = 'fail'}) + test:update_status('success') + + table.insert(deferred_artifact_checks, function() + assert_artifacts_path(g.s_test) + assert_artifacts_path(g.s_test2) + assert_artifacts_path(g.s_each) + assert_artifacts_path(g.s_each2) + assert_artifacts_path(g.s_all) + assert_artifacts_path(g.s_all2) + end) end + +g.after_all(function() + for _, check in ipairs(deferred_artifact_checks) do + check() + end +end) diff --git a/test/replica_set_test.lua b/test/replica_set_test.lua index 45ca3466..346d088e 100644 --- a/test/replica_set_test.lua +++ b/test/replica_set_test.lua @@ -4,6 +4,7 @@ local ReplicaSet = require('luatest.replica_set') local g = t.group() local Server = t.Server +local deferred_artifact_checks = {} g.before_each(function() g.rs = ReplicaSet:new() @@ -33,23 +34,26 @@ g.before_test('test_save_rs_artifacts_when_test_failed', function() end) g.test_save_rs_artifacts_when_test_failed = function() - local test = rawget(_G, 'current_test') + local ctx = rawget(_G, 'current_test') + local test = ctx.value -- the test must be failed to save artifacts - test.status = 'fail' + ctx.runner:update_status(test, {status = 'fail'}) g.rs:drop() - test.status = 'success' + test:update_status('success') - t.assert_equals(fio.path.exists(g.rs_artifacts), true) - t.assert_equals(fio.path.is_dir(g.rs_artifacts), true) + table.insert(deferred_artifact_checks, function() + t.assert_equals(fio.path.exists(g.rs_artifacts), true) + t.assert_equals(fio.path.is_dir(g.rs_artifacts), true) - t.assert_equals(fio.path.exists(g.s1_artifacts), true) - t.assert_equals(fio.path.is_dir(g.s1_artifacts), true) + t.assert_equals(fio.path.exists(g.s1_artifacts), true) + t.assert_equals(fio.path.is_dir(g.s1_artifacts), true) - t.assert_equals(fio.path.exists(g.s2_artifacts), true) - t.assert_equals(fio.path.is_dir(g.s2_artifacts), true) + t.assert_equals(fio.path.exists(g.s2_artifacts), true) + t.assert_equals(fio.path.is_dir(g.s2_artifacts), true) - t.assert_equals(fio.path.exists(g.s3_artifacts), true) - t.assert_equals(fio.path.is_dir(g.s3_artifacts), true) + t.assert_equals(fio.path.exists(g.s3_artifacts), true) + t.assert_equals(fio.path.is_dir(g.s3_artifacts), true) + end) end g.before_test('test_save_rs_artifacts_when_server_workdir_passed', function() @@ -69,26 +73,35 @@ g.before_test('test_save_rs_artifacts_when_server_workdir_passed', function() end) g.test_save_rs_artifacts_when_server_workdir_passed = function() - local test = rawget(_G, 'current_test') + local ctx = rawget(_G, 'current_test') + local test = ctx.value -- the test must be failed to save artifacts - test.status = 'fail' + ctx.runner:update_status(test, {status = 'fail'}) g.rs:drop() - test.status = 'success' + test:update_status('success') - t.assert_equals(fio.path.exists(g.rs_artifacts), true) - t.assert_equals(fio.path.is_dir(g.rs_artifacts), true) + table.insert(deferred_artifact_checks, function() + t.assert_equals(fio.path.exists(g.rs_artifacts), true) + t.assert_equals(fio.path.is_dir(g.rs_artifacts), true) - t.assert_equals(fio.path.exists(g.s1_artifacts), true) - t.assert_equals(fio.path.is_dir(g.s1_artifacts), true) + t.assert_equals(fio.path.exists(g.s1_artifacts), true) + t.assert_equals(fio.path.is_dir(g.s1_artifacts), true) - t.assert_equals(fio.path.exists(g.s2_artifacts), true) - t.assert_equals(fio.path.is_dir(g.s2_artifacts), true) + t.assert_equals(fio.path.exists(g.s2_artifacts), true) + t.assert_equals(fio.path.is_dir(g.s2_artifacts), true) - t.assert_equals(fio.path.exists(g.s3_artifacts), true) - t.assert_equals(fio.path.is_dir(g.s3_artifacts), true) + t.assert_equals(fio.path.exists(g.s3_artifacts), true) + t.assert_equals(fio.path.is_dir(g.s3_artifacts), true) + end) end +g.after_all(function() + for _, check in ipairs(deferred_artifact_checks) do + check() + end +end) + g.test_rs_no_socket_collision_with_custom_alias = function() local s1 = g.rs:build_server({alias = 'foo'}) local s2 = g.rs:build_server({alias = 'bar'}) diff --git a/test/server_test.lua b/test/server_test.lua index c7b7d8c8..335c40f2 100644 --- a/test/server_test.lua +++ b/test/server_test.lua @@ -11,6 +11,7 @@ local helper = require('test.helpers.general') local Process = t.Process local Server = t.Server +local deferred_artifact_checks = {} local root = fio.dirname(fio.abspath('test.helpers')) local datadir = fio.pathjoin(root, 'tmp', 'db_test') @@ -437,21 +438,30 @@ g.test_save_server_artifacts_when_test_failed = function() local s1_artifacts = ('%s/artifacts/%s'):format(s1.vardir, s1.id) local s2_artifacts = ('%s/artifacts/%s'):format(s2.vardir, s2.id) - local test = rawget(_G, 'current_test') + local ctx = rawget(_G, 'current_test') + local test = ctx.value -- the test must be failed to save artifacts - test.status = 'fail' + ctx.runner:update_status(test, {status = 'fail'}) s1:drop() s2:drop() - test.status = 'success' + test:update_status('success') - t.assert_equals(fio.path.exists(s1_artifacts), true) - t.assert_equals(fio.path.is_dir(s1_artifacts), true) + table.insert(deferred_artifact_checks, function() + t.assert_equals(fio.path.exists(s1_artifacts), true) + t.assert_equals(fio.path.is_dir(s1_artifacts), true) - t.assert_equals(fio.path.exists(s2_artifacts), true) - t.assert_equals(fio.path.is_dir(s2_artifacts), true) + t.assert_equals(fio.path.exists(s2_artifacts), true) + t.assert_equals(fio.path.is_dir(s2_artifacts), true) + end) end +g.after_all(function() + for _, check in ipairs(deferred_artifact_checks) do + check() + end +end) + g.test_server_build_listen_uri = function() local uri = Server.build_listen_uri('foo') t.assert_equals(uri, ('%s/foo.sock'):format(Server.vardir)) @@ -645,3 +655,49 @@ g.test_assertion_failure = function() server:connect_net_box() helper.assert_failure(server.exec, server, function() t.assert(false) end) end + +g.test_save_artifacts_after_restart_when_test_failed = function() + local s = Server:new() + + s:start() + s:exec(function() + require('log').info('before_restart_artifacts_marker') + end) + + s:restart() + s:exec(function() + require('log').info('after_restart_artifacts_marker') + end) + + local ctx = rawget(_G, 'current_test') + local test = ctx.value + ctx.runner:update_status(test, {status = 'fail'}) + s:drop() + test:update_status('success') + + table.insert(deferred_artifact_checks, function() + local log_path = fio.pathjoin(s.artifacts, s.alias .. '.log') + local log_file = fio.open(log_path) + local log_stat = log_file:stat() + local log_content = log_file:read(log_stat.size) + log_file:close() + + t.assert_str_contains(log_content, 'before_restart_artifacts_marker') + t.assert_str_contains(log_content, 'after_restart_artifacts_marker') + end) +end + +g.test_do_not_save_server_artifacts_when_test_succeeded = function() + local s = Server:new() + fio.rmtree(s.artifacts) + + s:start() + + local ctx = rawget(_G, 'current_test') + local test = ctx.value + test:update_status('success') + + s:drop() + + t.assert_equals(fio.path.exists(s.artifacts), false) +end