hooks: forbid direct hook assignment

Before this patch luatest accepted two ways to register hooks:

* call-style API: `group.before_all(function() ... end)`
* direct assignment: `group.before_all = function() ... end`

The second form does not work correctly with parametrized tests.
We already encountered this on the tarantool side and had to
rewrite such assignments in tarantool/tarantool#8187.

With this change luatest enforces a single, consistent way to
register hooks:

* hooks must be registered via calls like
  `before_each(function() ... end)`,
  `after_each(function() ... end)`,
  `before_all(function() ... end)`,
  `after_all(function() ... end)`,
  `before_suite(function() ... end)`,
  `after_suite(function() ... end)`,
  `before_test('name', function() ... end)` and
  `after_test('name', function() ... end)`;
* assigning to these names (e.g. `group.before_each = fn`) now
  raises an error with a clear message.

Legacy `group.setup` / `group.teardown` remain supported and are
not affected by this change.

Legacy `group.setup` / `group.teardown` are no longer supported;
use `before_each`/`after_each` instead.

Closes #390

Author: mandesero

CHANGELOG.md

@@ -1,5 +1,25 @@
 # Changelog
 
+## Unreleased
+
+- Group and suite hooks must now be registered using the call-style
+  API. Use:
+
+  - `before_each(function() ... end)`
+  - `after_each(function() ... end)`
+  - `before_all(function() ... end)`
+  - `after_all(function() ... end)`
+  - `before_suite(function() ... end)`
+  - `after_suite(function() ... end)`
+  - `before_test('name', function() ... end)`
+  - `after_test('name', function() ... end)`
+
+  Assigning hooks as fields (for example, `group.before_each = fn` or
+  `luatest.before_suite = fn`) is no longer allowed and results in an
+  error with a descriptive message.
+  Legacy `group.setup` and `group.teardown` are no longer supported; use
+  `before_each`/`after_each` instead (gh-390).
+
 ## 1.3.1
 
 - Fixed a bug when `assert_covers` didn't check array items for coverage and

luatest/hooks.lua

@@ -93,11 +93,56 @@ local function check_params(required, actual)
     return true
 end
 
+local function set_hook_assignment_guard(object, hooks_type, accessor, opts)
+    opts = opts or {}
+    local mt = getmetatable(object)
+    local guard = mt.__luatest_hook_guard
+
+    if not guard then
+        local new_mt = table.copy(mt)
+        guard = {
+            original_index = mt.__index,
+            original_newindex = mt.__newindex,
+            values = {},
+            error_messages = {},
+        }
+        new_mt.__luatest_hook_guard = guard
+        new_mt.__index = function(tbl, key)
+            if guard.values[key] ~= nil then
+                return guard.values[key]
+            end
+
+            local original_index = guard.original_index
+            if type(original_index) == 'function' then
+                return original_index(tbl, key)
+            end
+            if type(original_index) == 'table' then
+                return original_index[key]
+            end
+        end
+        new_mt.__newindex = function(tbl, key, value)
+            if guard.values[key] ~= nil then
+                local message = guard.error_messages[key] or
+                    string.format('Hook \'%s\' should be registered ' ..
+                                  'using %s(<function>)', key, key)
+                error(message)
+            end
+            if guard.original_newindex then
+                return guard.original_newindex(tbl, key, value)
+            end
+            rawset(tbl, key, value)
+        end
+        setmetatable(object, new_mt)
+    end
+    guard.values[hooks_type] = accessor
+    guard.error_messages[hooks_type] = opts.error_message
+end
+
 local function define_hooks(object, hooks_type, preloaded_hook)
     local hooks = {}
     object[hooks_type .. '_hooks'] = hooks
 
-    object[hooks_type] = function(...)
+    local register_hook = function(...)
         local params, fn = ...
         if fn == nil then
             fn = params
@@ -112,7 +157,7 @@ local function define_hooks(object, hooks_type, preloaded_hook)
         params = params or {}
         table.insert(hooks, {fn, params})
     end
-    object['_original_' .. hooks_type] = object[hooks_type] -- for leagacy hooks support
+    set_hook_assignment_guard(object, hooks_type, register_hook)
 
     local function run_preloaded_hooks()
         if preloaded_hook == nil then
@@ -160,7 +205,7 @@ local function define_named_hooks(object, hooks_type)
     local hooks = {}
     object[hooks_type .. '_hooks'] = hooks
 
-    object[hooks_type] = function(...)
+    local register_hook = function(...)
         local test_name, params, fn = ...
         if fn == nil then
             fn = params
@@ -182,6 +227,8 @@ local function define_named_hooks(object, hooks_type)
         table.insert(hooks[test_name], {fn, params})
     end
 
+    set_hook_assignment_guard(object, hooks_type, register_hook)
+
     object['run_' .. hooks_type] = function(test)
         local active_hooks = object[hooks_type .. '_hooks']
         local test_name = test.name
@@ -214,6 +261,16 @@ function export._define_group_hooks(group)
     define_hooks(group, 'before_all',  preloaded_hooks.before_all)
     define_hooks(group, 'after_all',   preloaded_hooks.after_all)
 
+    local setup_error = 'Hook \'setup\' is removed. Use \'before_each\' instead'
+    set_hook_assignment_guard(group, 'setup', function()
+        error(setup_error)
+    end, {error_message = setup_error})
+
+    local teardown_error = 'Hook \'teardown\' is removed. Use \'after_each\' instead'
+    set_hook_assignment_guard(group, 'teardown', function()
+        error(teardown_error)
+    end, {error_message = teardown_error})
+
     define_named_hooks(group, 'before_test')
     define_named_hooks(group, 'after_test')
     return group
@@ -228,28 +285,18 @@ end
 local function run_group_hooks(runner, group, hooks_type)
     local result
     local hook = group and group['run_' .. hooks_type]
-    -- If _original_%hook_name% is not equal to %hook_name%, it means
-    -- that this method was assigned by user (legacy API).
-    if hook and group[hooks_type] == group['_original_' .. hooks_type] then
+    if hook then
         result = runner:protected_call(group, hook, group.name .. '.run_before_all_hooks')
-    elseif group and group[hooks_type] then
-        result = runner:protected_call(group, group[hooks_type], group.name .. '.before_all')
     end
     if result and result.status ~= 'success' then
         return result
     end
 end
 
-local function run_test_hooks(self, test, hooks_type, legacy_name)
+local function run_test_hooks(self, test, hooks_type)
     log.info('Run hook %s', hooks_type)
     local group = test.group
-    local hook
-    -- Support for group.setup/teardown methods (legacy API)
-    hook = group[legacy_name]
-    if hook and type(hook) == 'function' then
-        self:update_status(test, self:protected_call(group, hook, group.name .. '.' .. legacy_name))
-    end
-    hook = group['run_' .. hooks_type]
+    local hook = group['run_' .. hooks_type]
     if hook then
         self:update_status(test, self:protected_call(group, hook))
     end
@@ -282,7 +329,7 @@ function export._patch_runner(Runner)
                 break
             end
 
-            run_test_hooks(self, test, 'before_each', 'setup')
+            run_test_hooks(self, test, 'before_each')
             run_named_test_hooks(self, test, 'before_test')
 
             if test:is('success') then
@@ -292,7 +339,7 @@ function export._patch_runner(Runner)
             end
 
             run_named_test_hooks(self, test, 'after_test')
-            run_test_hooks(self, test, 'after_each', 'teardown')
+            run_test_hooks(self, test, 'after_each')
         end
     end end)
 

luatest/parametrizer.lua

@@ -37,18 +37,19 @@ end
 
 local function redirect_index(group)
     local super_group_mt = getmetatable(group)
-    if super_group_mt.__newindex then
-        return
-    end
-
+    local origin_newindex = super_group_mt.__newindex
     super_group_mt.__newindex = function(_group, key, value)
         if _group.pgroups then
             for _, pgroup in ipairs(_group.pgroups) do
                 pgroup[key] = value
             end
-        else
-            rawset(_group, key, value)
         end
+
+        if origin_newindex then
+            return origin_newindex(_group, key, value)
+        end
+
+        rawset(_group, key, value)
     end
 end
 

test/capture_test.lua

@@ -8,11 +8,14 @@ local capture = Capture:new()
 -- Disable luatest logging to avoid capturing it.
 require('luatest.log').info = function() end
 
-g.setup = function() capture:enable() end
-g.teardown = function()
+g.before_each(function()
+    capture:enable()
+end)
+
+g.after_each(function()
     capture:flush()
     capture:disable()
-end
+end)
 
 g.before_all(function()
     local err

test/capturing_test.lua

@@ -8,11 +8,14 @@ local capture = Capture:new()
 -- Disable luatest logging to avoid capturing it.
 require('luatest.log').info = function() end
 
-g.setup = function() capture:enable() end
-g.teardown = function()
+g.before_each(function()
+    capture:enable()
+end)
+
+g.after_each(function()
     capture:flush()
     capture:disable()
-end
+end)
 
 local function assert_capture_restored()
     io.stdout:write('capture-restored')
@@ -67,8 +70,8 @@ g.test_example_failed = function()
     -- Don't show captures from group hooks when test failed.
     assert_captured(function(lu2)
         local group = lu2.group('test')
-        group.before_all = write_to_io
-        group.after_all = write_to_io
+        group.before_all(write_to_io)
+        group.after_all(write_to_io)
         group.test = function() error('custom-error') end
     end)
 
@@ -83,34 +86,34 @@ end
 g.test_example_hook = function()
     assert_captured(function(lu2)
         local group = lu2.group('test')
-        group.setup = write_to_io
-        group.teardown = group.setup
+        group.before_each(write_to_io)
+        group.after_each(write_to_io)
         group.test = function() end
     end)
 end
 
 g.test_example_hook_failed = function()
     assert_shown(function(lu2)
         local group = lu2.group('test')
-        group.setup = function()
+        group.before_each(function()
             write_to_io()
             error('test')
-        end
+        end)
         group.test = function() end
     end)
 
     assert_shown(function(lu2)
         local group = lu2.group('test')
-        group.teardown = function()
+        group.after_each(function()
             write_to_io()
             error('test')
-        end
+        end)
         group.test = function() end
     end)
 
     assert_shown(function(lu2)
         local group = lu2.group('test')
-        group.setup = write_to_io
+        group.before_each(write_to_io)
         group.test = function() error('test') end
     end)
 end
@@ -129,33 +132,34 @@ end
 g.test_group_hook = function()
     assert_captured(function(lu2)
         local group = lu2.group('test')
-        group.before_all = write_to_io
-        group.after_all = group.before_all
+        local hook = write_to_io
+        group.before_all(hook)
+        group.after_all(hook)
         group.test = function() end
 
         local group2 = lu2.group('test2')
-        group2.before_all = write_to_io
-        group2.after_all = group2.before_all
+        group2.before_all(hook)
+        group2.after_all(hook)
         group2.test = function() end
     end)
 end
 
 g.test_group_hook_failed = function()
     assert_shown(function(lu2)
         local group = lu2.group('test')
-        group.before_all = function()
+        group.before_all(function()
             write_to_io()
             error('custom-error')
-        end
+        end)
         group.test = function() end
     end)
 
     assert_shown(function(lu2)
         local group = lu2.group('test')
-        group.after_all = function()
+        group.after_all(function()
             write_to_io()
             error('custom-error')
-        end
+        end)
         group.test = function() end
     end)
 end