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

Author: mandesero

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

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 `<vardir>/artifacts` directory
--- for further analysis if the test fails.
+-- the replica set.
 function ReplicaSet:drop()
     for _, server in ipairs(self.servers) do
         server:drop()

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

luatest/server.lua

@@ -531,15 +531,13 @@ 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
         if func(...) then
             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 `<vardir>/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

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

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)

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)

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)

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'})