Use vim-testbed to run Vader tests on Travis #88

prashcr · 2016-10-11T05:02:25Z

In addition to the current vint script currently on Travis, I added infrastructure for automated testing with Vader inside a Docker container with multiple vim builds that are cached on Docker Hub.

Most of what I wrote is copied from https://github.com/tweekmonster/vim-testbed/tree/master/example, except I created a new Docker image based on vim-testbed containing vim v8.0.0000 and v8.0.0027.

It was quite a lot for me to wrap my head around at first as well, but I'm convinced the benefits are worth it. I understand the Makefile and Docker Hub image can be a bit daunting. However, from a tester's perspective, all they need to know is that

tests go in test/
they can modify the vimrc there which is used in tests

As this is quite a lot already for one PR so I only included a very basic Vader test as an example.

Versus our current approach

Our current approach limits us to whatever version of vim is installed on Ubuntu 12.04, which happens to be v7.3.429 and is useless for testing a vim 8 plugin. The only alternate solution is to build vim from source for every Travis build, but this is messy and takes minutes.
OS-agnostic testing of multiple vim versions
Travis builds in seconds, not minutes, allowing for quicker iterations during development
Contributors just need to link their Github to Travis and push to their fork to run tests, no need to install vint or vader
Eventual Neovim support Add support for installing Neovim Vimjas/vim-testbed#1
Refactoring #59

@KabbAmine Try running adding your Vader tests to this and let me know how it goes.

prashcr · 2016-10-11T05:07:32Z

The build is failing at the moment due to vint.

The previous .travis.yml was building successfully because vint was not outputting anything. Peculiarly, without having changed anything vint-related, vint seems to be outputting stuff now, failing the build.

Pretty strange to me and I have no clue why this is occurring but I guess having vint warnings means it's being useful so this should be a good thing.

prashcr · 2016-10-11T05:16:50Z

Ah, I realize what's wrong. vint is currently linting the vader.vim plugin instead of this plugin, let me fix that.

prashcr · 2016-10-11T05:36:21Z

Should be working now!

KabbAmine · 2016-10-11T08:24:53Z

Good job 👍 I'll prepare some tests then.

P.S: If this issue is related to this PR, note that you need the -s flag in vint to get style errors as ProhibitImplicitScopeVariable.

w0rp · 2016-10-11T08:31:09Z

.travis.yml

@@ -1,8 +1,10 @@
---


yaml-lint complains if there's no document start marker, so I'd put that back in.

w0rp · 2016-10-11T08:36:19Z

Makefile

@@ -0,0 +1,19 @@
+IMAGE ?= prashcr/vim8s


I wonder if we should use a Dockerfile hosted in this repo.

yeah, I would host the dockerfile in the repo and have it auto-build on the docker hub

Sure, I just changed this. You'll have to link your GitHub and set up an automated build for this repo at hub.docker.com

w0rp · 2016-10-11T08:38:03Z

Makefile

+	  $(DOCKER) $$vim '+Vader! test/*'; \
+	done
+
+.PHONY: test-setup test


Do we need a make clean rule in here?

Makefile doesn't create files anymore. We could still add a make clean for docker containers but that would undermine caching and people could just do that manually if they happened to be running Docker on their own machine. And we don't need clean for Travis anyway.

w0rp · 2016-10-11T08:38:37Z

Makefile

+
+$(PLUGINS)/vader.vim:
+	mkdir -p $(PLUGINS)
+	cd $(PLUGINS) && git clone https://github.com/junegunn/vader.vim.git


Should we reference a particular commit ID so tests won't start failing if Vader updates?

w0rp · 2016-10-11T09:12:24Z

I opened issue #89 to list some important tests we should focus on after this gets in.

w0rp · 2016-10-11T12:07:41Z

You'll have a conflict now the style checking branch as been merged. The -s flag has been added to vint.

prashcr · 2016-10-11T16:35:00Z

Thanks for the feedback, I've incorporated most of the requested changes so far.

I noticed 1 additional thing I'd like to fix, which I'm working on at the moment

Makefile clones vader.vim into test/, which is the only side effect of the Makefile currently. I'll try to move vader.vim installation into the Dockerfile, thereby eliminating the need for make clean and modification of the user's directory.

I also just started considering the process of adding linters, and similar to above, I believe they should also be added to the Dockerfile, consisting of a git clone and ENV PATH of the linter as well as its pre-requisite languages (e.g. Node.js, Ruby)

w0rp · 2016-10-11T16:42:12Z

Cool. See if you can get it to somehow cache installation of pip modules and Vader.vim. Maybe we could create a new image which contains what we need, and use that pre-defined image.

prashcr · 2016-10-12T00:55:39Z

It caches a particular commit of Vader.vim now, cloning directly inside the container.

Once the automated docker hub build is created, Travis should pass.

Installing linters, pip, npm, etc. should be trivial to add, I'll do that in another commit though, enough rebasing for a day 😅

prashcr · 2016-10-12T04:22:24Z

Amended again to fix a typo.

prashcr · 2016-10-12T08:36:03Z

Hmm, there's a problem with my commit. Even though I clone vader.vim into the /home/plugins directory, it simply doesn't exist in the container.

w0rp · 2016-10-12T08:48:55Z

.travis.yml

+install: |
+    pip install vim-vint==0.3.9
+script: |
+    vint -s . && make test


Is there some way we can run both programs here? If a pull request contains some style issues, then it won't run the other tests.

Yep I can change that to a ||.
However, as a logical OR this means that if only one of them fail, the Travis build will still be a "pass"

There must be some bashism to accomplish returning non-zero if either of them error yet still being able to run both, I'll try and find it

Found it
http://stackoverflow.com/questions/16080716/execute-multiple-commands-in-a-bash-script-sequentially-and-fail-if-at-least-one

Maybe you can just run them line after line on Travis, and it will capture the error codes of all programs run.

Setting two env items would be one way to do it. Set a variable different in different environments for Travis, and run one command or the other based on the variable.

If we put them line after line, we will only get the error code of the last line. I've implemented the solution I found and it's working well.

w0rp · 2016-10-12T08:53:38Z

I haven't ever really used Docker, so I'm not sure myself. I'll leave it to you.

prashcr · 2016-10-12T12:29:18Z

Looking good to go now, I did a demo build on a separate branch using the same Dockerfile (hosted on my Docker Hub): https://travis-ci.org/prashcr/ale/builds/167034172

I moved vint from .travis.yml to the Docker container so that the entire process can be run with a make test, and works the same way as it does on Travis as well as locally with Docker.

I'm not 100% if that's the best idea, so let me know.

prashcr · 2016-10-12T12:32:44Z

.travis.yml

@@ -1,8 +1,7 @@
 ---
+sudo: required
+services:
+    - docker
 language: python


travis-yaml still requires a language so I left that there.

w0rp · 2016-10-12T13:09:43Z

It looks like it's not quite running at the moment.

w0rp · 2016-10-12T13:11:11Z

Makefile

+	for vim in $$vims; do \
+	  $(DOCKER) $$vim '+Vader! test/*' || EXIT=$$?; \
+	done; \
+	$(DOCKER) vint -s /testplugin || EXIT=$$?; \


Does this check all of the .vim files in the repo, outside of tests?

It checks the entire repo, because of this line

DOCKER = docker run -a stderr --rm -v $(PWD):/testplugin -v $(PWD)/test:/home "$(IMAGE)"

This part in particular mounts pwd, which is this repo, into the /testplugin directory of the container.

-v $(PWD):/testplugin

I did this because that's how the vim-testbed scripts seemed to work.

prashcr · 2016-10-12T14:36:28Z

Yep it won't work here at the moment, let me explain.

We have a Dockerfile that describes a Docker image which is basically a built cache of all the pre-requisites (vim, vint, vader, etc.) for our make test script.

           "build"       "run"
Dockerfile -> Docker image -> Container

I use Docker Hub to host the Docker image. The neat feature of Docker Hub is that you can link it to a GitHub repo containing a Dockerfile such that every push to a branch on the GitHub repo triggers a build that creates a new image(e.g. downloads and builds vim, vint, vader, etc.)

The bottom line is that the Travis build consists of just two steps

Fetch the image from Docker Hub
Run container

Previously, the Makefile pointed to a Docker Hub image that I created, linked to a GitHub repo of my own. (As is still the case with the demo build in my previous comment). As per the suggestion to move the Dockerfile here instead, I pointed the Makefile to the w0rp/ale Docker Hub image, which doesn't exist yet.

In order for the Travis build to work with the Dockerfile in this repo,

You'll have to create a Docker Hub account, link it to your GitHub and create an automated build. Only the GitHub repo owner can set up an automated build on Docker Hub.
Even after that however, the Dockerfile has to be merged into the repo before you can see the build succeed, so it's a bit of a chicken-and-egg problem. I guess we could merge into another branch before master.

w0rp · 2016-10-12T14:38:31Z

I see. I'll have a go at finishing this when I have some time.

prashcr · 2016-10-12T14:47:13Z

Hmm, one easy solution I can think of is to just build the image in Travis for now. It'll take 2-3 minutes to run but should just work. Then later you can remove the build step and pull from Docker Hub instead if you choose.

w0rp · 2016-10-12T16:55:01Z

Nah, I'll set up the Docker image and such. I'll probably merge this tonight and temporarily break Travis CI builds for a bit until it starts running the tests, after some shouting at it.

w0rp · 2016-10-12T21:17:10Z

Right, time to break things and fix them again.

prashcr force-pushed the master branch 3 times, most recently from a6b21f3 to 3245f5d Compare October 11, 2016 05:32

w0rp requested changes Oct 11, 2016

View reviewed changes

prashcr force-pushed the master branch from 3245f5d to 1548d40 Compare October 12, 2016 00:39

prashcr force-pushed the master branch from 1548d40 to 6b0b42e Compare October 12, 2016 04:21

w0rp requested changes Oct 12, 2016

View reviewed changes

prashcr force-pushed the master branch 2 times, most recently from a140653 to 6147a4d Compare October 12, 2016 09:07

Use vim-testbed to run Vader tests on Travis

210d464

prashcr force-pushed the master branch from 6147a4d to e313373 Compare October 12, 2016 11:49

prashcr added 2 commits October 12, 2016 20:13

Move vint to Docker

9a0a663

Remove vint from Travis

51b4efd

prashcr force-pushed the master branch from e313373 to 51b4efd Compare October 12, 2016 12:19

prashcr commented Oct 12, 2016

View reviewed changes

w0rp reviewed Oct 12, 2016

View reviewed changes

w0rp merged commit e058f9d into dense-analysis:master Oct 12, 2016

Use vim-testbed to run Vader tests on Travis #88

Use vim-testbed to run Vader tests on Travis #88

Conversation

prashcr commented Oct 11, 2016 • edited Loading

Versus our current approach

prashcr commented Oct 11, 2016 • edited Loading

prashcr commented Oct 11, 2016

prashcr commented Oct 11, 2016

KabbAmine commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashcr Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

w0rp Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w0rp commented Oct 11, 2016

w0rp commented Oct 11, 2016

prashcr commented Oct 11, 2016

w0rp commented Oct 11, 2016

prashcr commented Oct 12, 2016

prashcr commented Oct 12, 2016

prashcr commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashcr Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w0rp commented Oct 12, 2016

prashcr commented Oct 12, 2016

Choose a reason for hiding this comment

w0rp commented Oct 12, 2016

Choose a reason for hiding this comment

prashcr Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

prashcr commented Oct 12, 2016 • edited Loading

w0rp commented Oct 12, 2016

prashcr commented Oct 12, 2016 • edited Loading

w0rp commented Oct 12, 2016 • edited Loading

w0rp commented Oct 12, 2016

prashcr commented Oct 11, 2016 •

edited

Loading

prashcr commented Oct 11, 2016 •

edited

Loading

prashcr Oct 12, 2016 •

edited

Loading

w0rp Oct 11, 2016 •

edited

Loading

prashcr Oct 12, 2016 •

edited

Loading

prashcr Oct 12, 2016 •

edited

Loading

prashcr commented Oct 12, 2016 •

edited

Loading

prashcr commented Oct 12, 2016 •

edited

Loading

w0rp commented Oct 12, 2016 •

edited

Loading