Skip to content
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

Display source with exceptions #107

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

blueyed
Copy link
Collaborator

@blueyed blueyed commented Jan 6, 2017

This improves display of error messages.

E.g.

    ( 4/13) [EXECUTE] (X) Assertion failure
      > in <SNR>49_Bar[2]
      >   2: Assert a
      > in <SNR>49_Foo[1]
      >   1: call s:Bar()
      > 8: call s:Foo()

with

Execute (Foo):
  function! s:Bar()
    let a = 0
    Assert a
  endfunction
  function! s:Foo()
    call s:Bar()
  endfunction
  call s:Foo()

It would be nice to get the actual source file and location, e.g. tests/utils.vader:13, but that would involve more refactoring to pass through the file and location of blocks.

Or:

    ( 5/13) [EXECUTE] (X) Vim(return):E121: Undefined variable: g:maker
      > in GetSetting, line 1
      >   1: return neomake#utils#GetSetting('args', g:maker, 'default', ['myft'], bufnr('%'))
      > 16: AssertEqual GetSetting(), 'string'

Caveats:

  • it wraps the block in a function to get the location of exception that Vader throws itself. Therefore you have to use return instead of finish to return from a block. There is a hack to change this.
  • you need to use g: prefixes now to access variables/functions defined in Before etc. This was necessary before in some cases already, but it really required now.

What do you think?

TODO:

blueyed added a commit to neomake/neomake that referenced this pull request Jan 7, 2017
This is good in general, but required with the WIP Vader PR, which wraps
the tests in a function.
Ref: junegunn/vader.vim#107
@blueyed
Copy link
Collaborator Author

blueyed commented Jan 7, 2017

It also helps with custom assert messages, where you often might wonder what gets compared:

    (1/1) [EXECUTE] (X) a
      > /home/user/foo/tests/processing.vader:19: AssertEqual len(g:neomake_test_countschanged), 1, "a"

@junegunn
Copy link
Owner

junegunn commented Jan 7, 2017

Hi, thanks for your continuous effort on improving Vader. To be honest, I'm not highly motivated to improve Vader any further. I'm pretty much settled with my current Vim configuration and I'm not actively developing a new Vim plugin as of now. So I don't really have much reason to actively seek for improvements in Vader. I can review and merge small changes, but if a patch requires non-trivial time and effort to review, I may not respond in timely fashion as my time is limited, and I may turn out to be very conservative in making changes. Please understand that, and also consider maintaining and using your own branch of changes, if you find yourself wanting new features and improvements. I won't be making any changes so we won't diverge too much.

@blueyed
Copy link
Collaborator Author

blueyed commented Jan 7, 2017

Fair point, and thanks for explaining.

I will keep working on this branch slowly during the next days/weeks.
While it would be OK to keep it in my fork, I certainly think that these improvements really help all users of Vader.

@blueyed blueyed changed the title [RFC] Display source with exceptions [WIP] Display source with exceptions Jan 7, 2017
@blueyed
Copy link
Collaborator Author

blueyed commented Jan 9, 2017

What about creating an org for it, or add me as a contributor?

I've created /~https://github.com/Vimjas a while ago, which could be for Vim what /~https://github.com/jazzband/ / /~https://github.com/jazzband/ is for Vim: letting Vim jinjas maintain plugins of (partial) common interest.
(There is also /~https://github.com/vimmer-net created by @mhinz, but I like my name better.. ;))

@blueyed
Copy link
Collaborator Author

blueyed commented Jan 9, 2017

@junegunn
I guess your use case is more running Vader from Vim while developing, right?

@junegunn
Copy link
Owner

What about creating an org for it, or add me as a contributor?

I'm fine with either way. I'm okay with minor enhancements and usability improvements, but I'm not looking for more features as I'd like to keep Vader simple and small, or radical changes that might break backward compatibility. If you agree with my direction, I'll add you as a contributor to the repository. But if you don't, and would like to actively experiment with new ideas and stuff, I'd suggest that it should be done on your fork. Whether the fork is under your username, or some organization, the decision is up to you. That's totally fine for me. I probably won't be involved in the development of the fork.

I guess your use case is more running Vader from Vim while developing, right?

I guess, and to confirm that a pull request doesn't break anything.

euclio pushed a commit to euclio/neomake that referenced this pull request Jan 17, 2017
This is good in general, but required with the WIP Vader PR, which wraps
the tests in a function.
Ref: junegunn/vader.vim#107
@blueyed
Copy link
Collaborator Author

blueyed commented Jan 28, 2017

@junegunn
Any idea/input about the finish vs. return issue?
Currently you have to use finish in your tests, but when wrapped into a function (as with this PR), it needs to be return.
Not sure how many tests are using this, but can it be made compatible in some way?

@blueyed blueyed force-pushed the display-source-with-exceptions branch from 00445b0 to 616cee1 Compare January 28, 2017 15:42
@junegunn
Copy link
Owner

Hmm, I've never used finish in my test cases. Not sure how many users are relying on the behavior, but it was not documented anyway.

@blueyed blueyed force-pushed the display-source-with-exceptions branch from 616cee1 to fa9548b Compare January 29, 2017 15:10
blueyed added a commit to neomake/neomake that referenced this pull request Feb 5, 2017
blueyed added a commit to neomake/neomake that referenced this pull request Feb 5, 2017
blueyed added a commit to neomake/neomake that referenced this pull request Feb 6, 2017
@blueyed blueyed force-pushed the display-source-with-exceptions branch from fa9548b to 8b9a8e2 Compare February 26, 2017 19:50
@blueyed blueyed force-pushed the display-source-with-exceptions branch 2 times, most recently from cfb83a3 to f1b4559 Compare March 11, 2017 13:09
@blueyed blueyed force-pushed the display-source-with-exceptions branch from f1b4559 to beec226 Compare April 2, 2017 17:10
@blueyed blueyed changed the title [WIP] Display source with exceptions Display source with exceptions Apr 2, 2017
@blueyed
Copy link
Collaborator Author

blueyed commented Apr 2, 2017

@junegunn

I'm okay with minor enhancements and usability improvements, but I'm not looking for more features as I'd like to keep Vader simple and small, or radical changes that might break backward compatibility. If you agree with my direction, I'll add you as a contributor to the repository.

Oh, I've never replied to this really.
I would be happy to become a contributor, and agree with your direction.
I wouldn't merge anything without your ACK though anyway, so it's not really necessary to have write access, of course.

Regarding this PR I consider it to be a huge usability improvement.

The other big thing is #124, but that more like covering Vader's own tests, and maybe refactoring it in the future to make this (tests in Docker / using tmux) easily available for Vader users, e.g. by having contrib scripts.

@junegunn
Copy link
Owner

junegunn commented Apr 3, 2017

It sure seems to be a nice improvement, no argue with that. It's just that the patch is not trivial, and I haven't had the mental bandwidth to review the change, probably more so because I'm not actively writing new Vim plugins that I don't need more than what is currently available.

I'll try to look into it when I get some time. Hmm, or maybe I should just add you as a contributor and leave this all up to you, given that you won't break any backward compatibility?

@blueyed
Copy link
Collaborator Author

blueyed commented Apr 4, 2017

Hmm, or maybe I should just add you as a contributor and leave this all up to you, given that you won't break any backward compatibility?

Sure. I can understand if you have no time to review it, and I could merge (and maintain it afterwards!) myself.

I am using it myself since a while, and will now go ahead and use it with Vader by default already.
We can wait some more before merging this.

E.g.

    ( 1/13) [EXECUTE] (X) Vim(finish):E168: :finish used outside of a sourced file
      > /home/user/src/neomake/tests/utils.vader:11: finish  " foo

Also passes it through to `s:execute`.
This will use the actual file name for included lines.

Fixes junegunn#88.
Wrapping the code in a function will cause them to not be available otherwise,
or g:SyntaxAt() would have to be used.
While this means more overhead in general, it allows to access
script-local variables in the tests.

squash! FEAT: move s:prepare into vader#window#execute

Use `vader#log` directly instead of `:Log`, since the latter might not
be defined (yet).
This not only avoids multiple calls to `tempname()`, but makes Vim
use the same script ID also when not run in Docker.
For some reason it uses a new script ID (`<SID>`/`<SNR>`) when run
normally, but uses the same ID when run in Docker.
This means that the `!` with `function! s:vader_wrapper()` was not
required in an environment that behaves like running it in Docker (with
Neomake's tests).

This also removes the `delete()`, which is handled by Vim itself when
shutting down.
Vim does not generate profiling information for dict functions when the
dict is local-scoped, since it apparently gets garbage collected
automatically.

Works around vim/vim#2350.
This is relevant for when `Include` is used, and is the same as
`g:vader_file` otherwise.
`v:throwpoint` is empty in the first case.
This gets handled by Vim automatically, and works around making Docker
use the same inode or something similar
(vim/vim#3353).
This is supported with Vim patch v8.1.0362.
This might be good in general for something to end up in the window in
any case, but is mainly done to allow for testing it.

Should/could maybe only be done with Vader's own tests, or some
monkey-patching instead.
@blueyed blueyed force-pushed the display-source-with-exceptions branch from 6453672 to f7e0094 Compare February 13, 2020 13:26
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.

4 participants