test: spawn_wait() starts a non-RPC Nvim process

Problem:
Can't use `n.clear()` to test non-RPC `nvim` invocations. So tests end
up creating ad-hoc wrappers around `system()` or `jobstart()`.

Solution:
- Introduce `n.spawn_wait()`
- TODO (followup PR): Rename `n.spawn()` and `n.spawn_wait()`.
  It's misleading that `n.spawn()` returns a RPC session...

MAINTAIN.md

@@ -143,6 +143,8 @@ These dependencies are "vendored" (inlined), we must update the sources manually
 
 * `src/mpack/`: [libmpack](https://github.com/libmpack/libmpack)
     * send improvements upstream!
+* `src/mpack/lmpack.c`: [libmpack-lua](https://github.com/libmpack/libmpack-lua)
+    * send improvements upstream!
 * `src/xdiff/`: [xdiff](https://github.com/git/git/tree/master/xdiff)
 * `src/cjson/`: [lua-cjson](https://github.com/openresty/lua-cjson)
 * `src/klib/`: [Klib](https://github.com/attractivechaos/klib)

runtime/lua/coxpcall.lua

@@ -1,6 +1,10 @@
 -------------------------------------------------------------------------------
+-- (Not needed for LuaJIT or Lua 5.2+)
+--
 -- Coroutine safe xpcall and pcall versions
 --
+-- https://keplerproject.github.io/coxpcall/
+--
 -- Encapsulates the protected calls with a coroutine based loop, so errors can
 -- be dealed without the usual Lua 5.x pcall/xpcall issues with coroutines
 -- yielding inside the call to pcall or xpcall.

src/nvim/main.c

@@ -634,6 +634,7 @@ int main(int argc, char **argv)
   if (params.luaf != NULL) {
     // Like "--cmd", "+", "-c" and "-S", don't truncate messages.
     msg_scroll = true;
+    DLOG("executing Lua -l script");
     bool lua_ok = nlua_exec_file(params.luaf);
     TIME_MSG("executing Lua -l script");
     if (msg_didout) {

test/client/rpc_stream.lua

@@ -1,34 +1,41 @@
+---
+--- Reading/writing of msgpack over any of the stream types from `uv_stream.lua`.
+--- Does not implement the RPC protocol, see `session.lua` for that.
+---
+
 local mpack = vim.mpack
 
 local Response = {}
 Response.__index = Response
 
-function Response.new(msgpack_rpc_stream, request_id)
+function Response.new(rpc_stream, request_id)
   return setmetatable({
-    _msgpack_rpc_stream = msgpack_rpc_stream,
+    _rpc_stream = rpc_stream,
     _request_id = request_id,
   }, Response)
 end
 
 function Response:send(value, is_error)
-  local data = self._msgpack_rpc_stream._session:reply(self._request_id)
+  local data = self._rpc_stream._session:reply(self._request_id)
   if is_error then
-    data = data .. self._msgpack_rpc_stream._pack(value)
+    data = data .. self._rpc_stream._pack(value)
-    data = data .. self._msgpack_rpc_stream._pack(mpack.NIL)
+    data = data .. self._rpc_stream._pack(mpack.NIL)
   else
-    data = data .. self._msgpack_rpc_stream._pack(mpack.NIL)
+    data = data .. self._rpc_stream._pack(mpack.NIL)
-    data = data .. self._msgpack_rpc_stream._pack(value)
+    data = data .. self._rpc_stream._pack(value)
   end
-  self._msgpack_rpc_stream._stream:write(data)
+  self._rpc_stream._stream:write(data)
 end
 
---- @class test.MsgpackRpcStream
+--- Nvim msgpack RPC stream.
+---
+--- @class test.RpcStream
 --- @field private _stream test.Stream
 --- @field private __pack table
-local MsgpackRpcStream = {}
+local RpcStream = {}
-MsgpackRpcStream.__index = MsgpackRpcStream
+RpcStream.__index = RpcStream
 
-function MsgpackRpcStream.new(stream)
+function RpcStream.new(stream)
   return setmetatable({
     _stream = stream,
     _pack = mpack.Packer(),
@@ -50,10 +57,10 @@ function MsgpackRpcStream.new(stream)
         },
       }),
     }),
-  }, MsgpackRpcStream)
+  }, RpcStream)
 end
 
-function MsgpackRpcStream:write(method, args, response_cb)
+function RpcStream:write(method, args, response_cb)
   local data
   if response_cb then
     assert(type(response_cb) == 'function')
@@ -66,10 +73,10 @@ function MsgpackRpcStream:write(method, args, response_cb)
   self._stream:write(data)
 end
 
-function MsgpackRpcStream:read_start(request_cb, notification_cb, eof_cb)
+function RpcStream:read_start(on_request, on_notification, on_eof)
   self._stream:read_start(function(data)
     if not data then
-      return eof_cb()
+      return on_eof()
     end
     local type, id_or_cb, method_or_error, args_or_result
     local pos = 1
@@ -78,9 +85,9 @@ function MsgpackRpcStream:read_start(request_cb, notification_cb, eof_cb)
       type, id_or_cb, method_or_error, args_or_result, pos = self._session:receive(data, pos)
       if type == 'request' or type == 'notification' then
         if type == 'request' then
-          request_cb(method_or_error, args_or_result, Response.new(self, id_or_cb))
+          on_request(method_or_error, args_or_result, Response.new(self, id_or_cb))
         else
-          notification_cb(method_or_error, args_or_result)
+          on_notification(method_or_error, args_or_result)
         end
       elseif type == 'response' then
         if method_or_error == mpack.NIL then
@@ -94,12 +101,12 @@ function MsgpackRpcStream:read_start(request_cb, notification_cb, eof_cb)
   end)
 end
 
-function MsgpackRpcStream:read_stop()
+function RpcStream:read_stop()
   self._stream:read_stop()
 end
 
-function MsgpackRpcStream:close(signal)
+function RpcStream:close(signal)
   self._stream:close(signal)
 end
 
-return MsgpackRpcStream
+return RpcStream

test/client/session.lua

@@ -1,13 +1,21 @@
-local uv = vim.uv
+---
-local MsgpackRpcStream = require('test.client.msgpack_rpc_stream')
+--- Nvim msgpack-RPC protocol session. Manages requests/notifications/responses.
+---
 
+local uv = vim.uv
+local RpcStream = require('test.client.rpc_stream')
+
+--- Nvim msgpack-RPC protocol session. Manages requests/notifications/responses.
+---
 --- @class test.Session
---- @field private _pending_messages string[]
+--- @field private _pending_messages string[] Requests/notifications received from the remote end.
---- @field private _msgpack_rpc_stream test.MsgpackRpcStream
+--- @field private _rpc_stream test.RpcStream
 --- @field private _prepare uv.uv_prepare_t
 --- @field private _timer uv.uv_timer_t
---- @field private _is_running boolean
 --- @field exec_lua_setup boolean
+--- @field private _is_running boolean true during `Session:run()` scope.
+--- @field private _stdout_buffer string[] Stores stdout chunks
+--- @field public stdout string Full stdout after the process exits
 local Session = {}
 Session.__index = Session
 if package.loaded['jit'] then
@@ -51,9 +59,10 @@ local function coroutine_exec(func, ...)
   end))
 end
 
+--- Creates a new msgpack-RPC session.
 function Session.new(stream)
   return setmetatable({
-    _msgpack_rpc_stream = MsgpackRpcStream.new(stream),
+    _rpc_stream = RpcStream.new(stream),
     _pending_messages = {},
     _prepare = uv.new_prepare(),
     _timer = uv.new_timer(),
@@ -91,10 +100,13 @@ function Session:next_message(timeout)
   return table.remove(self._pending_messages, 1)
 end
 
+--- Sends a notification to the RPC endpoint.
 function Session:notify(method, ...)
-  self._msgpack_rpc_stream:write(method, { ... })
+  self._rpc_stream:write(method, { ... })
 end
 
+--- Sends a request to the RPC endpoint.
+---
 --- @param method string
 --- @param ... any
 --- @return boolean, table
@@ -114,8 +126,16 @@ function Session:request(method, ...)
   return true, result
 end
 
---- Runs the event loop.
+--- Processes incoming RPC requests/notifications until exhausted.
+---
+--- TODO(justinmk): luaclient2 avoids this via uvutil.cb_wait() + uvutil.add_idle_call()?
+---
+--- @param request_cb function Handles requests from the sever to the local end.
+--- @param notification_cb function Handles notifications from the sever to the local end.
+--- @param setup_cb function
+--- @param timeout number
 function Session:run(request_cb, notification_cb, setup_cb, timeout)
+  --- Handles an incoming request.
   local function on_request(method, args, response)
     coroutine_exec(request_cb, method, args, function(status, result, flag)
       if status then
@@ -126,6 +146,7 @@ function Session:run(request_cb, notification_cb, setup_cb, timeout)
     end)
   end
 
+  --- Handles an incoming notification.
   local function on_notification(method, args)
     coroutine_exec(notification_cb, method, args)
   end
@@ -160,39 +181,45 @@ function Session:close(signal)
   if not self._prepare:is_closing() then
     self._prepare:close()
   end
-  self._msgpack_rpc_stream:close(signal)
+  self._rpc_stream:close(signal)
   self.closed = true
 end
 
+--- Sends a request to the RPC endpoint, without blocking (schedules a coroutine).
 function Session:_yielding_request(method, args)
   return coroutine.yield(function(co)
-    self._msgpack_rpc_stream:write(method, args, function(err, result)
+    self._rpc_stream:write(method, args, function(err, result)
       resume(co, err, result)
     end)
   end)
 end
 
+--- Sends a request to the RPC endpoint, and blocks (polls event loop) until a response is received.
 function Session:_blocking_request(method, args)
   local err, result
 
+  -- Invoked when a request is received from the remote end.
   local function on_request(method_, args_, response)
     table.insert(self._pending_messages, { 'request', method_, args_, response })
   end
 
+  -- Invoked when a notification is received from the remote end.
   local function on_notification(method_, args_)
     table.insert(self._pending_messages, { 'notification', method_, args_ })
   end
 
-  self._msgpack_rpc_stream:write(method, args, function(e, r)
+  self._rpc_stream:write(method, args, function(e, r)
     err = e
     result = r
     uv.stop()
   end)
 
+  -- Poll for incoming requests/notifications received from the remote end.
   self:_run(on_request, on_notification)
   return (err or self.eof_err), result
 end
 
+--- Polls for incoming requests/notifications received from the remote end.
 function Session:_run(request_cb, notification_cb, timeout)
   if type(timeout) == 'number' then
     self._prepare:start(function()
@@ -202,14 +229,15 @@ function Session:_run(request_cb, notification_cb, timeout)
       self._prepare:stop()
     end)
   end
-  self._msgpack_rpc_stream:read_start(request_cb, notification_cb, function()
+  self._rpc_stream:read_start(request_cb, notification_cb, function()
     uv.stop()
     self.eof_err = { 1, 'EOF was received from Nvim. Likely the Nvim process crashed.' }
   end)
   uv.run()
   self._prepare:stop()
   self._timer:stop()
-  self._msgpack_rpc_stream:read_stop()
+  self._rpc_stream:read_stop()
 end
 
+--- Nvim msgpack-RPC session.
 return Session

test/client/uv_stream.lua

@@ -1,3 +1,8 @@
+---
+--- Basic stream types.
+--- See `rpc_stream.lua` for the msgpack layer.
+---
+
 local uv = vim.uv
 
 --- @class test.Stream
@@ -6,6 +11,8 @@ local uv = vim.uv
 --- @field read_stop fun(self)
 --- @field close fun(self, signal?: string)
 
+--- Stream over given pipes.
+---
 --- @class vim.StdioStream : test.Stream
 --- @field private _in uv.uv_pipe_t
 --- @field private _out uv.uv_pipe_t
@@ -45,6 +52,8 @@ function StdioStream:close()
   self._out:close()
 end
 
+--- Stream over a named pipe or TCP socket.
+---
 --- @class test.SocketStream : test.Stream
 --- @field package _stream_error? string
 --- @field package _socket uv.uv_pipe_t
@@ -109,26 +118,46 @@ function SocketStream:close()
   uv.close(self._socket)
 end
 
---- @class test.ChildProcessStream : test.Stream
+--- Stream over child process stdio.
+---
+--- @class test.ProcStream : test.Stream
 --- @field private _proc uv.uv_process_t
 --- @field private _pid integer
 --- @field private _child_stdin uv.uv_pipe_t
 --- @field private _child_stdout uv.uv_pipe_t
+--- @field private _child_stderr uv.uv_pipe_t
+--- @field stdout string
+--- @field stderr string
+--- @field stdout_eof boolean
+--- @field stderr_eof boolean
+--- @field private collect_output boolean
+--- Exit code
 --- @field status integer
 --- @field signal integer
-local ChildProcessStream = {}
+local ProcStream = {}
-ChildProcessStream.__index = ChildProcessStream
+ProcStream.__index = ProcStream
 
+--- Starts child process specified by `argv`.
+---
 --- @param argv string[]
 --- @param env string[]?
 --- @param io_extra uv.uv_pipe_t?
---- @return test.ChildProcessStream
+--- @return test.ProcStream
-function ChildProcessStream.spawn(argv, env, io_extra)
+function ProcStream.spawn(argv, env, io_extra)
   local self = setmetatable({
-    _child_stdin = uv.new_pipe(false),
+    collect_output = false,
-    _child_stdout = uv.new_pipe(false),
+    output = '',
+    stdout = '',
+    stderr = '',
+    stdout_error = nil, -- TODO: not used, remove
+    stderr_error = nil, -- TODO: not used, remove
+    stdout_eof = false,
+    stderr_eof = false,
+    _child_stdin = assert(uv.new_pipe(false)),
+    _child_stdout = assert(uv.new_pipe(false)),
+    _child_stderr = assert(uv.new_pipe(false)),
     _exiting = false,
-  }, ChildProcessStream)
+  }, ProcStream)
   local prog = argv[1]
   local args = {} --- @type string[]
   for i = 2, #argv do
@@ -136,13 +165,14 @@ function ChildProcessStream.spawn(argv, env, io_extra)
   end
   --- @diagnostic disable-next-line:missing-fields
   self._proc, self._pid = uv.spawn(prog, {
-    stdio = { self._child_stdin, self._child_stdout, 1, io_extra },
+    stdio = { self._child_stdin, self._child_stdout, self._child_stderr, io_extra },
     args = args,
     --- @diagnostic disable-next-line:assign-type-mismatch
     env = env,
   }, function(status, signal)
-    self.status = status
     self.signal = signal
+    -- "Abort" exit may not set status; force to nonzero in that case.
+    self.status = (0 ~= (status or 0) or 0 == (signal or 0)) and status or (128 + (signal or 0))
   end)
 
   if not self._proc then
@@ -153,24 +183,56 @@ function ChildProcessStream.spawn(argv, env, io_extra)
   return self
 end
 
-function ChildProcessStream:write(data)
+function ProcStream:write(data)
   self._child_stdin:write(data)
 end
 
-function ChildProcessStream:read_start(cb)
+function ProcStream:on_read(stream, cb, err, chunk)
-  self._child_stdout:read_start(function(err, chunk)
   if err then
-      error(err)
+    -- stderr_error/stdout_error
+    self[stream .. '_error'] = err ---@type string
+    -- error(err)
+  elseif chunk then
+    -- 'stderr' or 'stdout'
+    if self.collect_output then
+      self[stream] = self[stream] .. chunk ---@type string
+      --- Collects both stdout + stderr.
+      self.output = self[stream] .. chunk ---@type string
     end
+  else
+    -- stderr_eof/stdout_eof
+    self[stream .. '_eof'] = true ---@type boolean
+  end
+
+  -- Handler provided by the caller.
+  if cb then
     cb(chunk)
+  end
+end
+
+--- Collects output until the process exits.
+function ProcStream:wait()
+  self.collect_output = true
+  while not (self.stdout_eof and self.stderr_eof and (self.status or self.signal)) do
+    uv.run('once')
+  end
+end
+
+function ProcStream:read_start(on_stdout, on_stderr)
+  self._child_stdout:read_start(function(err, chunk)
+    self:on_read('stdout', on_stdout, err, chunk)
+  end)
+  self._child_stderr:read_start(function(err, chunk)
+    self:on_read('stderr', on_stderr, err, chunk)
   end)
 end
 
-function ChildProcessStream:read_stop()
+function ProcStream:read_stop()
   self._child_stdout:read_stop()
+  self._child_stderr:read_stop()
 end
 
-function ChildProcessStream:close(signal)
+function ProcStream:close(signal)
   if self._closed then
     return
   end
@@ -178,6 +240,7 @@ function ChildProcessStream:close(signal)
   self:read_stop()
   self._child_stdin:close()
   self._child_stdout:close()
+  self._child_stderr:close()
   if type(signal) == 'string' then
     self._proc:kill('sig' .. signal)
   end
@@ -189,6 +252,6 @@ end
 
 return {
   StdioStream = StdioStream,
-  ChildProcessStream = ChildProcessStream,
+  ProcStream = ProcStream,
   SocketStream = SocketStream,
 }

test/functional/core/startup_spec.lua

@@ -154,8 +154,9 @@ describe('startup', function()
 
     it('failure modes', function()
       -- nvim -l <empty>
-      matches('nvim%.?e?x?e?: Argument missing after: "%-l"', fn.system({ nvim_prog, '-l' }))
+      local proc = n.spawn_wait('-l')
-      eq(1, eval('v:shell_error'))
+      matches('nvim%.?e?x?e?: Argument missing after: "%-l"', proc.stderr)
+      eq(1, proc.status)
     end)
 
     it('os.exit() sets Nvim exitcode', function()
@@ -182,12 +183,11 @@ describe('startup', function()
     end)
 
     it('Lua-error sets Nvim exitcode', function()
+      local proc = n.spawn_wait('-l', 'test/functional/fixtures/startup-fail.lua')
+      matches('E5113: .* my pearls!!', proc.output)
+      eq(1, proc.status)
+
       eq(0, eval('v:shell_error'))
-      matches(
-        'E5113: .* my pearls!!',
-        fn.system({ nvim_prog, '-l', 'test/functional/fixtures/startup-fail.lua' })
-      )
-      eq(1, eval('v:shell_error'))
       matches(
         'E5113: .* %[string "error%("whoa"%)"%]:1: whoa',
         fn.system({ nvim_prog, '-l', '-' }, 'error("whoa")')

test/functional/testnvim.lua

@@ -4,7 +4,7 @@ local t = require('test.testutil')
 local Session = require('test.client.session')
 local uv_stream = require('test.client.uv_stream')
 local SocketStream = uv_stream.SocketStream
-local ChildProcessStream = uv_stream.ChildProcessStream
+local ProcStream = uv_stream.ProcStream
 
 local check_cores = t.check_cores
 local check_logs = t.check_logs
@@ -465,10 +465,12 @@ function M.check_close()
   session = nil
 end
 
+--- Starts `argv` process as a Nvim msgpack-RPC session.
+---
 --- @param argv string[]
 --- @param merge boolean?
 --- @param env string[]?
---- @param keep boolean?
+--- @param keep boolean? Don't close the current global session.
 --- @param io_extra uv.uv_pipe_t? used for stdin_fd, see :help ui-option
 --- @return test.Session
 function M.spawn(argv, merge, env, keep, io_extra)
@@ -476,9 +478,8 @@ function M.spawn(argv, merge, env, keep, io_extra)
     M.check_close()
   end
 
-  local child_stream =
+  local proc = ProcStream.spawn(merge and M.merge_args(prepend_argv, argv) or argv, env, io_extra)
-    ChildProcessStream.spawn(merge and M.merge_args(prepend_argv, argv) or argv, env, io_extra)
+  return Session.new(proc)
-  return Session.new(child_stream)
 end
 
 -- Creates a new Session connected by domain socket (named pipe) or TCP.
@@ -489,31 +490,59 @@ function M.connect(file_or_address)
   return Session.new(stream)
 end
 
--- Starts (and returns) a new global Nvim session.
+--- Starts (and returns) a new global Nvim session.
---
+---
--- Parameters are interpreted as startup args, OR a map with these keys:
+--- Use `spawn_argv()` to get a new session without replacing the current global session.
---    args:       List: Args appended to the default `nvim_argv` set.
+---
---    args_rm:    List: Args removed from the default set. All cases are
+--- Parameters are interpreted as startup args, OR a map with these keys:
---                removed, e.g. args_rm={'--cmd'} removes all cases of "--cmd"
+--- - args:       List: Args appended to the default `nvim_argv` set.
---                (and its value) from the default set.
+--- - args_rm:    List: Args removed from the default set. All cases are
---    env:        Map: Defines the environment of the new session.
+---               removed, e.g. args_rm={'--cmd'} removes all cases of "--cmd"
---
+---               (and its value) from the default set.
--- Example:
+--- - env:        Map: Defines the environment of the new session.
---    clear('-e')
+---
---    clear{args={'-e'}, args_rm={'-i'}, env={TERM=term}}
+--- Example:
+--- ```
+--- clear('-e')
+--- clear{args={'-e'}, args_rm={'-i'}, env={TERM=term}}
+--- ```
+---
+--- @param ... string Nvim CLI args
+--- @return test.Session
+--- @overload fun(opts: test.new_argv.Opts): test.Session
 function M.clear(...)
   M.set_session(M.spawn_argv(false, ...))
   return M.get_session()
 end
 
---- same params as clear, but does returns the session instead
+--- Same as clear(), but doesn't replace the current global session.
---- of replacing the default session
+---
+--- @param keep boolean Don't close the current global session.
+--- @param ... string Nvim CLI args
 --- @return test.Session
+--- @overload fun(opts: test.new_argv.Opts): test.Session
 function M.spawn_argv(keep, ...)
   local argv, env, io_extra = M.new_argv(...)
   return M.spawn(argv, nil, env, keep, io_extra)
 end
 
+--- Starts a (`--headless`, non-RPC) Nvim process, waits for exit, and returns output + info.
+---
+--- @param ... string Nvim CLI args
+--- @return test.ProcStream
+--- @overload fun(opts: test.new_argv.Opts): test.ProcStream
+function M.spawn_wait(...)
+  local opts = type(...) == 'string' and { args = { ... } } or ...
+  opts.args_rm = opts.args_rm and opts.args_rm or {}
+  table.insert(opts.args_rm, '--embed')
+  local argv, env, io_extra = M.new_argv(opts)
+  local proc = ProcStream.spawn(argv, env, io_extra)
+  proc:read_start()
+  proc:wait()
+  proc:close()
+  return proc
+end
+
 --- @class test.new_argv.Opts
 --- @field args? string[]
 --- @field args_rm? string[]
@@ -522,11 +551,11 @@ end
 
 --- Builds an argument list for use in clear().
 ---
---- @see clear() for parameters.
+--- @param ... string See clear().
---- @param ... string
 --- @return string[]
 --- @return string[]?
 --- @return uv.uv_pipe_t?
+--- @overload fun(opts: test.new_argv.Opts): string[], string[]?, uv.uv_pipe_t?
 function M.new_argv(...)
   local args = { unpack(M.nvim_argv) }
   table.insert(args, '--headless')