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] #296

Helped-by: Igor Munkin <[email protected]>

Close #299

Author: Oleg Chaplashkin

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(
             {'            <error type="', Output.xml_escape(node.message), '">\n',
              '                <![CDATA[', Output.xml_c_data_escape(node.trace), ']]>\n',
-             '                <artifacts>', Output.xml_escape(node.artifacts or ''), '</artifacts>\n',
+             '                <artifacts>', Output.xml_escape(artifacts), '</artifacts>\n',
              '            </error>\n'})
     elseif node:is('fail') then
         return table.concat(
             {'            <failure type="', Output.xml_escape(node.message), '">\n',
              '                <![CDATA[', Output.xml_c_data_escape(node.trace), ']]>\n',
-             '                <artifacts>', Output.xml_escape(node.artifacts or ''), '</artifacts>\n',
+             '                <artifacts>', Output.xml_escape(artifacts), '</artifacts>\n',
              '            </failure>\n'})
     elseif node:is('skip') then
         return table.concat({'            <skipped>', Output.xml_escape(node.message or ''),'</skipped>\n'})

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(('%s%s -> %s'):format(prefix, server.alias, server.artifacts))
+            end
         end
     end
 end

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()

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)

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,19 +348,31 @@ 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
         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
@@ -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)
 

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

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)

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)

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)