Skip to content

Remove luatest submodule (v2) #455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 28, 2025
Merged

Conversation

locker
Copy link
Member

@locker locker commented May 27, 2025

The test-run project is rarely updated these days while luatest is rapidly developing. Let's remove the luatest submodule from test-run to ease luatest bumps in projects using test-run. Now, luatest is supposed to be installed from rocks.

Closes #453

The first version of this patch used a different approach. It required the superproject to pull luatest as a submodule1. We've decided to reject that approach.

Footnotes

  1. https://github.com/tarantool/test-run/pull/454

Ubuntu 20.04 is no longer supported by GitHub runners.

Note that Tarantool 2.8 and Python 3.7 are too old and not available on
ubuntu-latest so they're removed from the test matrix.
@locker
Copy link
Member Author

locker commented May 27, 2025

There's a problem with this update: it breaks auto-requiring of the luatest module from a Tarantool test server. One way to fix that issue is using the original search root, like this:

diff --git a/luatest/server.lua b/luatest/server.lua
index 96b1e40e114a..c303952403cc 100644
--- a/luatest/server.lua
+++ b/luatest/server.lua
@@ -347,6 +347,7 @@ function Server:build_env()
         TARANTOOL_HTTP_PORT = self.http_port,
         TARANTOOL_LISTEN = self.net_box_port or self.net_box_uri,
         TARANTOOL_ALIAS = self.alias,
+        TARANTOOL_SEARCHROOT = package.searchroot()
     }
     if self.box_cfg ~= nil then
         res.TARANTOOL_BOX_CFG = json.encode(self.box_cfg)
@@ -834,7 +835,10 @@ function Server:exec(fn, args, options)
         for i = 1, debug.getinfo(fn, 'u').nups do
             local name, _ = debug.getupvalue(fn, i)
             if passthrough_ups[name] then
+                local old_root = package.searchroot()
+                package.setsearchroot(os.getenv('TARANTOOL_SEARCHROOT'))
                 debug.setupvalue(fn, i, require(passthrough_ups[name]))
+                package.setsearchroot(old_root)
             end
         end
         local result = {xpcall(function()

Is there a better way?

@coveralls
Copy link

coveralls commented May 27, 2025

Coverage Status

coverage: 62.54% (-0.03%) from 62.573%
when pulling 39bbb2d on locker/rm-luatest-submodule-v2
into 95bbee5 on master.

@Totktonada
Copy link
Member

Is there a better way?

I would prefer to pass LUA_PATH into the child environment instead of flipping searchroot back and forth. We can split package.path, make all the paths absolute, and pass it to the child's environment.

In fact, it solves the problem that is more general than just luatest's autorequire. Even two problems:

  • The child process has its own current working directory that means that all the default relative search paths that successfully works with . and .rocks in the parent process doesn't work in the child process.
  • All the non-standard package.path additions from the test that are working in the parent process do not passed to the child process.

It seems quite convenient to pass all these paths to the child process from the parent one.

Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

I've no objections here. It is interesting to try the proposed development flow in practice.

@Totktonada Totktonada assigned locker and unassigned Totktonada May 27, 2025
@locker locker force-pushed the locker/rm-luatest-submodule-v2 branch from ce9a3ae to 1e0c633 Compare May 28, 2025 08:04
@locker
Copy link
Member Author

locker commented May 28, 2025

The update with the luatest fix from #455 (comment) still results in 18 Tarantool tests failing. 5 of them are caused by the recent logging update, see tarantool/luatest@2f16ac7. Other tests fail because they explicitly require the luatest module in scripts. Here's the list:

test name								failure reason
---------
box-luatest/box_cfg_env_test.lua					require('luatest')
metrics-luatest/tarantool_spaces_test.lua				require('luatest') in test utils
box-luatest/downgrade_test.lua						require('luatest') in helper
box-luatest/gh_5809_bitset_index_len_crash_test.lua			require('luatest'); may be removed
box-luatest/gh_9309_errors_in_triggers_test.lua				grep_log
replication-luatest/gh_10088_apply_deletion_of_replica_from_cluster_on_deleted_replica_test.lua		log_file
box-luatest/gh_10032_update_upsert_splice_test.lua			require('luatest') in helper
box-luatest/gh_7904_export_box_schema_version_to_public_api_test.lua	log_file
app-luatest/gh_7288_console_flavours_vs_txn_test.lua			require('luatest') in helper
app-luatest/trigger_module_test.lua					grep_log
box-luatest/box_schema_before_box_cfg_test.lua				require('luatest')
app-luatest/require_first_test.lua					require('luatest')
box-luatest/gh_7231_memtx_hash_idx_gt_iter_deprecation_test.lua		log_file
replication-luatest/gh_9723_sync_system_spaces_test.lua			require('luatest')
box-luatest/gh_8192_feedback_daemon_metrics_test.lua			require('luatest'); may be removed
config-luatest/vars_test.lua						require('luatest')
config-luatest/config_test.lua						require('luatest')
config-luatest/box_cfg_env_test.lua					require('luatest') in helper

@locker locker force-pushed the locker/rm-luatest-submodule-v2 branch from 1e0c633 to 540c01b Compare May 28, 2025 16:23
@locker
Copy link
Member Author

locker commented May 28, 2025

Is there a better way?

@Totktonada suggested an approach that doesn't require patching luatest:

diff --git a/bin/luatest b/bin/luatest
index 8bfb9b5626cd..4afd068e5236 100755
--- a/bin/luatest
+++ b/bin/luatest
@@ -1,5 +1,17 @@
 #!/usr/bin/env tarantool
 
+--
+-- Add the luatest module to LUA_PATH so that it can be used in processes
+-- spawned by tests.
+--
+local fio = require('fio')
+local path = package.search('luatest')
+path = fio.dirname(path) -- strip init.lua
+path = fio.dirname(path) -- strip luatest
+os.setenv('LUA_PATH',
+          path .. '/?.lua;' .. path .. '/?/init.lua;' ..
+          (os.getenv('LUA_PATH') or ';'))
+
 print(('Tarantool version is %s'):format(require('tarantool').version))
 
 require('luatest.cli_entrypoint')()

It adds the path to the luatest module to LUA_PATH from bin/luatest (which has to be created by test-run anyways because finding a binary installed from rocks is complicated). @Totktonada PTAL if that's what you meant.

Update: Also added an explicit error in case the luatest module isn't found:

diff --git a/bin/luatest b/bin/luatest
index 4afd068e5236..e51b98b75178 100755
--- a/bin/luatest
+++ b/bin/luatest
@@ -6,6 +6,9 @@
 --
 local fio = require('fio')
 local path = package.search('luatest')
+if path == nil then
+    error('luatest not found')
+end
 path = fio.dirname(path) -- strip init.lua
 path = fio.dirname(path) -- strip luatest
 os.setenv('LUA_PATH',

The test-run project is rarely updated these days while luatest is
rapidly developing. Let's remove the luatest submodule from test-run
to ease luatest bumps in projects using test-run. Now, `luatest` is
supposed to be installed from rocks.

Closes #453
@locker locker force-pushed the locker/rm-luatest-submodule-v2 branch from 540c01b to 39bbb2d Compare May 28, 2025 16:38
@locker locker merged commit 5f82c70 into master May 28, 2025
13 checks passed
@locker locker deleted the locker/rm-luatest-submodule-v2 branch May 28, 2025 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove luatest submodule
3 participants