Provide a vimrunner/rspec helper

[mudge]

As previously discussed at https://gist.github.com/mudge/4971520, provide a helper that can be used in conjunction with RSpec to remove boilerplate when test-driving Vim script.

Ideally, it should:

• Be configurable as to whether a single Vim server is shared between example groups;
• Be configurable as to whether a GUI Vim is preferred or not;
• Handle the (lazy) starting and stopping of Vim servers as appropriate;
• Provide metadata-based filters to wrap example groups in temporary directories (and the associated lcd command in the server).

[AndrewRadev]

I implemented a first draft of this in the rspec-integration branch. I simply copied the gist and tweaked it a bit. Notable changes:

• I changed Vimrunner.instance and the like to Vimrunner::RSpec.instance. Even if it's only required in testing code, I'd rather namespace this to keep it entirely separate.
• I added an after_start configuration attribute for a callback. This can be used for an add_plugin call. The problem it tries to solve is that, depending on your choice of reuse_server, you may have to call it only once or multiple times, and it's nice if the change is only in a single setting (the after_start callback is invoked whenever a new Vim is started, regardless of the setting).

This results in client code that would look like this:

require 'vimrunner'
require 'vimrunner/rspec'

Vimrunner::RSpec.configure do |config|
  config.reuse_server = true

  config.after_start = lambda do |vim|
    vim.add_plugin(File.expand_path('.'), 'plugin/myplugin.vim')
  end
end

A few directions we could go from here:

• The configuration object could be a separate class, like Vimrunner::RSpec::Config, since that would allow a nicer DSL for after_start -- instead of assigning a proc, we could just after_start { |vim| vim.do_stuff }
• The Testing module seems a bit pointless now, since it's included in RSpec by default anyway. Theoretically, it could be used with a different testing framework, so it's still a plus to have the code extensions separate. Perhaps we could extract them to an object? Vimrunner::TestingClient.new(vim_client)? Not sure how much the helper methods make sense on a Vim instance, though. This would also allow something like vim.edit_and_write(filename) { |vim| ... }, since it won't be part of the Client API, but of the TestingClient one.

[mudge]

We should definitely make it more natural to specify after_start callbacks, perhaps even with:

module Vimrunner
  module RSpec
    class Configuration < OpenStruct
      def after_start(&blk)
        self.after_start_callback = blk
      end
    end
  end
end

[AndrewRadev]

Yeah, that seems quite good. I pushed a commit with that change, although I didn't inherit from OpenStruct, since I think it might be better to have the accessors easily visible from the class definition.

Another problem I'm seeing is the :filesystem => true check to limit execution only to specifically tagged tests. This sounded really good to me, but when I ran the splitjoin tests, I got a lot of leftover files in the directory :). I don't really have any use case in which I'd want to not create a temporary file and edit it, so to me, this is definitely the more common case. More importantly, if the user forgets the group, the results could be fairly dangerous -- files might be created and deleted at the root of the project. Can you think of a way to opt out instead of opting in?

[mudge]

Mmm, we should look into why the :filesystem => true filter didn't run: it just takes advantage of RSpec 2's Filter Hooks.

Hopefully I can be more helpful after Tuesday!

[AndrewRadev]

I know why it didn't run -- I forgot to put a :type => :filesystem tag on the it clauses :). That's my point, that it's possible to forget to do it for a test which uses the filesystem, and that can be annoying and maybe even slightly dangerous.

Hopefully I can be more helpful after Tuesday!

No rush :). Good luck with the talk.

[mudge]

The public_send needed for specifying the start_method makes me uneasy: what if we provided a single hook to start and return a Vimrunner::Client, thereby combining the after_start and start_method into one block:

Vimrunner::RSpec.configure do |config|

  # By default, this would just be Vimrunner.start
  config.start_vim do
    vim = Vimrunner.start
    vim.add_plugin("~/Desktop/foo.vim", "plugin/foo.vim")

    vim
  end
end

This means that Vimrunner::RSpec::Configuration becomes very dumb indeed: just an object that allows specification of blocks by name. Perhaps something like:

module Vimrunner
  module RSpec
    class Configuration
      attr_accessor :reuse_server

      def start_vim(&block)
        @start_vim_method = block
      end

      def start_vim_method
        @start_vim_method || lambda { Vimrunner.start }
      end
    end

    def self.configuration
      @configuration ||= Configuration.new
    end

    def self.configure
      yield configuration
    end
  end

  module Testing
    class << self
      attr_accessor :instance
    end

    def vim
      Testing.instance ||= Vimrunner::RSpec.configuration.start_vim_method.call
    end
  end
end

Vimrunner::RSpec.configure do |config|
  config.reuse_server = false
end

RSpec.configure do |config|
  config.include(Vimrunner::Testing)

  config.before(:each) do
    unless Vimrunner::RSpec.configuration.reuse_server
      Vimrunner::Testing.instance.kill if Vimrunner::Testing.instance
      Vimrunner::Testing.instance = nil
    end
  end

  config.after(:suite) do
    Vimrunner::Testing.instance.kill if Vimrunner::Testing.instance
  end
end

I've moved some things into Testing as you're right, we needn't couple ourselves too tightly to RSpec.

[mudge]

Also, I agree that the value of Testing is an odd one: it smells like a generic utility class that actually belongs in another object...

[AndrewRadev]

The start_vim call looks much better to me, too. I've pushed your proposed changes. I've also made Vim use a separate directory for each test regardless of the filesystem tag. It's safer that way, since you might easily forget to do it and end up writing files in the main directory of the project. It does seem like a waste for some kinds of tests, but I don't think the overhead is significant enough. If you can figure out a way to enable people to opt-out of the separate-directory-per-test, I'd be okay with that, though.

The only thing I'm still wondering about before merging this into master is the Vimrunner::Testing module. I'm not sure I want to include it by default, since this effectively adds a couple of functions to the tests' namespace, and might lead to conflicts. I'm leaning towards just letting people include Vimrunner::Testing if they want to -- they can take a look at the helpers and decide whether they'll have conflicts or not. Alternatively, we could have a TestingClient instance, which delegates to Client, but also adds some testing-related methods, and we can move out some of the more general-purpose helpers to rspec matchers. Something like:

vim.write

File.read('foo.txt').should match_content <<-EOF
  bar
EOF

I'll think about it some more tomorrow, but let me know if you have any ideas.

[AndrewRadev]

I merged the branch in master, but I'm still having problems with error checking and output capturing... I also need to write some info about this in the README, but I'll tackle that after figuring out the error handling problems. For now, closing this issue.