Implement SkipIf command to skip tests if a condition is met

[MisanthropicBit]

As per #178, this PR implements a SkipIf command to skip a test if a condition is met. I have purposefully left out changes to vader's documentation and tests until the implementation has been discussed.

The command has been tested with one of my own plugins under neovim v0.3.8 and vim v8.0 on Mac OSX 10.14.6. Internally, if a test is skipped, we continue with the next case instead of throwing an exception since continuing seemed to fit better with the existing code. This is my first PR for vader.vim so bear with me if the implementation is not spot on.

The command is used as follows.

SkipIf [(reason for skipping)]:
  [oneline vimscript]

Currently, a single line for the condition (e.g. a function call) must be used in order to avoid a E488: Trailing characters error when calling eval on the condition. The output produced is as follows. Notice that pending is 1 since a single test is skipped.

Starting Vader: 1 suite(s), 4 case(s)
  Starting Vader: mytest.vader
    (1/4) [  GIVEN]
    (1/4) [EXECUTE]
    (1/4) [ EXPECT]
    (2/4) [  GIVEN]
    (2/4) [EXECUTE]
    (2/4) [ EXPECT]
    (3/4) [  GIVEN]
    (3/4) [EXECUTE]
    (3/4) [ EXPECT]
    (4/4) [ SKIPIF] reason for skipping
  Success/Total: 3/4 (1 pending)
Success/Total: 3/4 (1 pending, assertions: 0/0)
Elapsed time: 0.179752 sec.

Screenshots with the seoul256.vim colorscheme:

Things left to do:

• Add log message for skip
• Add documentation
• Add vader tests
• Resolve any security concerns from use of eval

[codecov-io]

Codecov Report

Merging #201 into master will decrease coverage by 1.6%.
The diff coverage is 56%.

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
- Coverage   86.26%   84.66%   -1.61%     
==========================================
  Files          12       12              
  Lines         830      854      +24     
==========================================
+ Hits          716      723       +7     
- Misses        114      131      +17

Flag	Coverage Δ
#nvim	81.96% <56%> (-0.93%)	⬇️
#vim	83.95% <56%> (-0.99%)	⬇️

Impacted Files	Coverage Δ
syntax/vader.vim	98.64% <100%> (+0.11%)	⬆️
autoload/vader/parser.vim	93.51% <100%> (ø)	⬆️
autoload/vader.vim	84.56% <38.88%> (-3.83%)	⬇️
ftplugin/vader.vim	89.65% <0%> (-3.45%)	⬇️
autoload/vader/assert.vim	92.42% <0%> (-1.52%)	⬇️
autoload/vader/window.vim	68.61% <0%> (-0.73%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de8a976...452e773. Read the comment docs.

[blueyed]

Great!

LGTM already.

I've thought about being able to skip a test from within the execute block also / instead, which could be done via a Skip … command, which itself could be inside some conditional then.

This would e.g. allow for having some basic test, and only extended ones later in the same block.

Internally this could throw a VaderSkipped exception.

[MisanthropicBit]

Glad to hear it 😄

I've thought about being able to skip a test from within the execute block also / instead, which could be done via a Skip … command, which itself could be inside some conditional then.

Interesting idea. The only downside I could think of is that you would still have to run Before, Given and possibly Do blocks before getting to the Execute block which might skip the test anyway. This is one upside to having a separate SkipIf block which can be run first.

However, that leads me to suggest another approach: The SkipIf block is kept but its body is like the Execute block. This allows for more than a single line of vimscript where your suggested Skip command can be used to skip the test. Wrapping the code for execution of the SkipIf block in :command Skip .../delcommand Skip would ensure that the Skip command can only be called in the SkipIf block. Furthermore, we can still run the SkipIf block first before other blocks and reuse the s:execute function.

Internally this could throw a VaderSkipped exception.

Would this then report the skip via exceptions in the quickfix window? I personally think reporting the skip only in the vader-result window makes more sense since the skip is not an error per se. Alternatively, the exception could be caught and reported as a warning in the quickfix window instead. Let me know what is more vader-ish.

[idbrii]

This sounds like a great feature. I found this PR when looking for a way to skip tests if the tested version of vim is missing required features.

It's not clear to me from the examples whether SkipIf applies only to a following block or to the entire vader script. It might be good to require SkipIf to be the first block in the script and it always applies to the whole file -- that would be clearer. Then you could add a Skip command for more fine-grained control

[MisanthropicBit]

@idbrii I agree :) it would be very useful.

It's not clear to me from the examples whether SkipIf applies only to a following block or to the entire vader script.

It currently applies only to a single following Given block. Letting it apply to the whole script seems a bit too coarse-grained IMO.

It might be good to require SkipIf to be the first block in the script and it always applies to the whole file -- that would be clearer. Then you could add a Skip command for more fine-grained control

So if I understand you correctly, the SkipIf command would control whether or not the entire script is skipped under certain conditions while the Skip command would control if a single Given block etc. is skipped or not? So in fact, the two commands are independent? I like this idea as it offers both coarse- and fine-grained control. Additionally, there's also @blueyed's suggestion of a command to skip tests inside Execute blocks.

I still need some input on these directions from @blueyed.

[idbrii]

It currently applies only to a single following Given block

But if I delete a given block is deleted, that doesn't skip the test does it? Why would skipping a given block skip a test?

[MisanthropicBit]

But if I delete a given block is deleted, that doesn't skip the test does it?

If you delete a Given block, I guess the SkipIf would become associated with some other Given/Do/Execute/Expect blocks. I just tested this in my own code and that is what happens (the parser groups the SkipIf together with another test case).

Why would skipping a given block skip a test?

I think I did not elaborate my last post enough. The SkipIf would skip the Given block and all blocks that comprise the test, i.e. associated Doand Execute blocks etc. that rely on that Given block. Essentially, we would jump as in the following although that is not exactly what happens in the code.

SkipIf [(reason for skipping)]:
  [oneline vimscript]

Given python:
  ...

Do:
  ...

Expect:
  ...

The SkipIf skips all the way here if the condition is true.

I hope it is more clear what I mean :)

[idbrii]

I think if SkipIf was instead called Require, it would make sense to me.

Require ... Given ... Do/Execute ... Then

The flow makes more sense.

Is making Require/SkipIf like an Execute simpler in implementation?

Require:
  if !has('patch-8.2.0077')
    throw "VaderSkipped: not supported"
  endif

[MisanthropicBit]

I think if SkipIf was instead called Require, it would make sense to me.

Sure, sounds good. Makes it more distinguishable from the Skip... stuff.

Is making Require/SkipIf like an Execute simpler in implementation?

I believe so. I think the main point here is that it would plug into the existing code that Execute uses and would allow for multiple lines of vimscript which is nice if the user has a sophisticated condition that is not just checking a boolean flag. See my earlier comments.

Personally, I would prefer using some kind of Skip command in the Require/Skip blocks and throw a VaderSkipped exception internally as suggested by @blueyed. Requiring users to manually throw exceptions exposes an implementation detail. The command could take an optional message to convey why a file/test was skipped.

Thanks for your input @idbrii! I should have some time tomorrow and next week to start implementing some of this stuff :)

[MisanthropicBit]

I've gotten around to implementing some of the stuff discussed so far.

• Implemented a Require and a SkipIf block.
• Both blocks work like Execute in the sense that they are followed by a block of code, but only vimscript is allowed. Ideally, other languages should be allowed in case you want to test if e.g. a Python import fails.
• The Require block skips the entire test file if some condition is met. Duplicate Require block are not allowed. If skipped, all tests are marked as pending and a single result is reported with the comment provided by the user, if any (see below). If no comment is provided, it reports 'Skipping all cases' at the moment.
• The SkipIf block skips a single test and must follow its Given block. Only one SkipIf block is allowed by test.
• A special VaderSkip command is available in both blocks to perform the skip. Internally, a VaderSkipped exception is thrown to signal skips.

Require (some config variable is true):
  if g:plugin#config
    VaderSkip
  endif

Given:
  ...

SkipIf (unsupported vim patch version):
  if !has('patch-8.2.0077')
    VaderSkip
  endif

Do:
  ...

...

Require report example:

Starting Vader: 1 suite(s), 2 case(s)
  Starting Vader: require.vader
     [REQUIRE] some config variable is true
  Success/Total: 0/1 (1 pending)
Success/Total: 0/1 (1 pending, assertions: 0/0)
Elapsed time: 0.075197 sec.

I have run a few tests but more is needed, and the parser needs some more work. Feedback is welcome.

[MisanthropicBit]

@blueyed Any update on this pull request? (apologies for mentioning you if you are not the maintainer. I could not figure out who to bump).

[lelutin]

Hello! for what it's worth I've just tested out this PR and it seem to work as expected.

I've just tried using the changes, I unfortunately can't be too assertive about code style and other details about the code itself.

On thing though: the PR defines a hilight region named vaderSkipIfRaw but it's not matched to anything or being contained by anything. Is this dead code that should be removed?

Otherwise this PR would need to add some details about the new blocks in the README file. It would be especially interesting to document the non-trivial bits of what they do and how to use them. Like the fact that Require can only be present once in a file and that it will provoke a skip of all tests in the file (also what happens if it's not placed at the top of the file, will tests before it still get run? .. is an interesting detail to document), and that the SkipIf block needs to be placed after a Given block even though it may need to be an empty one (e.g. for the test file that I modified, all of the tests are using Execute so in order to use SkipIf I needed to also add an empty Given block to the test and place the SkipIf sandwitched in between Given and Execute)

Cheers! and thanks for this PR!

[MisanthropicBit]

Thanks for the comments. I'm not using vader.vim anymore and the project appears to be dead so I'm not intending to continue working on this PR. I unfortunately don't remember much of this work since I did it ~5 years ago.

On thing though: the PR defines a hilight region named vaderSkipIfRaw but it's not matched to anything or being contained by anything. Is this dead code that should be removed?

I honestly don't remember what purpose this region served but there are raw and non-raw counterparts for some of the other regions already defined in the syntax file so that might give you a clue.

I recall opening this PR mostly as a proof-of-concept to see if it would gain acceptance before writing a bunch of documentation/tests in case it would be scraped which would explain the incomplete state of the PR. You're more than welcome to continue developing it though 🙂