Error reporting when testing functions

[mudge]

One thing to come out of tonight's talk was that the output of echo can be misleading: when trying to inspect the outcome of a function, for example, vim.echo("missing#FakeFunction()") will simply return an empty string instead of raising Vim's own error message.

I'm guessing this is swallowed by command and perhaps echo is the wrong way to do this but we need a way to call Vim functions directly and get meaningful feedback whether it be the successful return value or an error.

[mudge]

Having looked into this a little, it's a bit tricky:

My exact use case was to test-drive an autoloaded function, caesar#ToRoman(). The first failing test should give some error indicating that the function does not exist, namely:

E117: Unknown function: caesar#ToRoman
E15: Invalid expression: caesar#ToRoman()

Using remote-expr directly gives an error but with a very vague message:

$ vim --servername FOO --remote-expr "caesar#ToRoman(1)"
E449: Invalid expression received: Send expression failed.

Using the VimrunnerEvaluateCommandOutput helper function gives no output at all:

$ vim --servername FOO --remote-expr "VimrunnerEvaluateCommandOutput('echo caesar#ToRoman(1)')"
$

[AndrewRadev]

This is an extremely weird problem, and it's been there since day one. Originally, I thought it's caused by redirecting output and error handling in the same area, but I'm now convinced it's a remote execution issue. If the code is executed remotely, no error tracking works, at all. I tried using all forms of try-catch, silent! exe with a v:errmsg, but nothing seems to work -- no errors are handled with the remote interface. The simplest example is probably:

function! Test()
  try
    exe "frobble"
    return 1
  catch
    return 2
  endtry
endfunction

Running this within Vim gives you 2, running this through --remote-expr gives you 1.

The good news is, I managed to find a horrible workaround:

let g:_vimrunner_output = ''

function! VimrunnerEvaluateCommandOutput(command)
  redir => g:_vimrunner_output
  silent! exe a:command
  redir END

  if v:errmsg != ''
    let g:_vimrunner_output = v:errmsg
    let v:errmsg = ''
  endif
endfunction

def command(commands)
  server.remote_send(":call VimrunnerEvaluateCommandOutput('#{escape(commands)}')<cr>")
  server.remote_expr('g:_vimrunner_output').tap do |output|
    raise InvalidCommandError.new(output) if output =~ /^E\d+:/
  end
end

The specs are failing, however, and I'm not entirely sure why. Timing is one thing that worries me, since typing doesn't wait for the completion of commands, but delaying doesn't seem to help. I'll try to figure it out.

I'll also post to the Vim mailing list some time in the next few days. To me, this looks very much like a bug.

[mudge]

Thanks for looking into it, it does seem like it might be a limitation with remote_send(): I really would expect this to be more verbose:

$ vim --servername FOO --remote-expr "caesar#ToRoman(1)"
E449: Invalid expression received: Send expression failed.

[AndrewRadev]

I pushed a commit which should give us proper error messages, using my horrible workaround. I also had to add some horrible escaping, since I couldn't find a way to remote-send <c-X> and the likes without them being immediately evaluated. Now that I think about it, I should do the same for <cr>, <space> and everything else I can think of... Perhaps I'll get to it later (and write some specs), this should be good enough for the moment.

Give it a shot, though, and let me know what you think.

[AndrewRadev]

Turns out it was a simple fix in Vim: https://groups.google.com/forum/?fromgroups=#!topic/vim_dev/3qX-Yps3Ftk. Now I'm not entirely sure what to do, though :). I guess a good solution would be to implement it "the right way" using --remote-expr and try-catch blocks and put a line in the README saying "You should install Vim version X to get proper error messages". Unfortunately, this would mean that travis would not give good error messages, since the version of Vim probably won't be up to date on their servers...

[AndrewRadev]

Hmm, now I have a different problem... Apparently, output capturing with redir also captures errors that happen during silent!. This is a problem, since I have calls to repeat#set and foldopen with a silent! modifier in splitjoin (for example). I'll try to figure out a workaround, but I'll probably resort to writing to the Vim mailing list again.

[AndrewRadev]

After reading the documentation more carefully, it seems like redir is not defined with "captures output", but with "captures messages". This explains perfectly well why output from silent! shows up. As a solution, I've implemented some code that filters errors out of the captured output by looking at the final error before the real output and deleting everything above that.

Of course, now that I think about this, it's fairly naive. There could easily be significant output followed by an error from silent!, followed by more output. I guess I have to experiment more with it, and write some documentation before closing the issue.

However, proper error handling for calling missing functions should work just fine, provided you're on a Vim 7.3 with patchlevel greater than 860 (I think), since a vim.echo('FunctionName()') would rely only on the last line of the output.

[mudge]

Thanks for looking into this.

As for Travis: if need be, we could prepare our own deb for testing (in fact, we could do a matrix build with both Vim < 860 and > 860) and install that in a pre-build step. I've taken to doing this for my re2 gem: see https://github.com/mudge/re2/blob/master/.travis.yml

[AndrewRadev]

I don't know about testing both < 860 and > 860. There are two tests that would fail on < 860, is there some way we can get the build to work with this?

Other than that, preparing our own deb seems reasonable, except I have no idea how to do it :). I could google around, of course, but since you seem to already have some experience, could you do it instead (whenever you have some time)?

[mudge]

Re the failing tests, shouldn't we degrade gracefully for older versions of Vim where possible (ideally with feature detection or, failing that, just switching on the Vim version)?

I'll try to create the packages and set up Travis CI (hence the importance of having all green tests even when on Vim < 860).

[AndrewRadev]

Hmm, I guess you have a point. What I could do is restore some of the old "command doesn't exist" code, but only run it if the version is < 860. This would provide a better user experience, since it's the old behaviour. Though, the code there is definitely not bulletproof, and it's quite hacky, so it may cause other trouble. Not to mention being quite limited, it only checks for existing/non-existing commands.

Alternatively, I could simply not execute these specs with an old version of Vim and put a comment there that explains the version requirement. This would be the easy way out, and I'm tempted to just do that. That does mean zero error handling, though -- if you get an error in the specs due to a failed expectancy, you'll never know what exactly blew up without firing up Vim yourself and replicating the test case (which is the way I've been doing things, and I admit it's annoying :))

What do you think?

[mudge]

I'm leaning towards maintaining compatibility with versions prior to 860 but offer degraded functionality (viz. the old behaviour of just failing silently); in this way, I don't mind putting version guards in the spec (e.g. "with a version of Vim greater than 860, behave like x") and it reminds me of RubySpec.

Then again, seeing as we are pre-1.0, we could make the call not to support older versions of Vim but it seems like a harsh call to make.

[AndrewRadev]

I committed some code to use a different variant of VimrunnerEvaluateCommandOutput depending on the Vim version. Thanks to that, the build is now passing. We should eventually get rid of this (when most distros get to patchlevel 860), but for now, I guess it's a good idea to maintain compatibility.

With this, I'll close the issue. Provided your Vim version is high enough, you'll get error handling for missing functions (and anything else, really). There are still some edge cases that won't work well for various reasons, but for now, I'll skip handling those.