Better testing by arp242 · Pull Request #1476 · fatih/vim-go

arp242 · 2017-09-25T20:48:06Z

Support multiple Vim versions (currently 7.4, 8.0, and Neovim).
Make sure that we always run Vim from a temporary installation in
/tmp/vim-go-test without loading ~/.vim. This makes it a lot
easier to run on people's computers.
Also add a handy ./script/run-vim script to run the installed Vim
from the temp directory. Useful for testing, debugging, etc. without a
(potentially large) ~/.vim/ dir.
Previously the tests weren't actually being run correctly; see:
https://travis-ci.org/fatih/vim-go/builds/279277579
Format the output of the tests a wee bit nicer, roughly similar to the
go test output.
Expand docs on testing a bit.

I also attempted to integrate code coverage support with
https://github.com/Vimjas/covimerage (which was the reason I started
working on this), but couldn't really get that to work. Need to look in
to that later.

fatih · 2017-09-26T06:42:10Z

scripts/install-vim

+set -euC
+cd "$(dirname "$0")"
+
+vimgodir="$(dirname "$(readlink -f .)")" # TODO: doesn't work on OSX I think.


Can you add an if clause that exits the script when run on OSX (checking with uname for example). I'm using macOS and don't want to blow my setup because of this. We can later add homebrew or a different kind of installation step here.

I just need to fix it; readlink -f doesn't work on OSX but there's a workaround (I just couldn't remember what it was exactly yesterday).

Is $( cd -P "$( dirname "${BASH_SOURCE[0]}" )" > /dev/null && pwd ) what you're thinking of @Carpetsmoker ?

fatih · 2017-09-26T06:43:33Z

scripts/test

+./run-vim $vim +':silent :GoInstallBinaries' +':qa'
+[ -d "$vimdir/share/vim/vimgo/pack/vim-go/start/vim-vimhelplint" ] || \
+  git clone --quiet https://github.com/machakann/vim-vimhelplint \
+    "$vimdir/share/vim/vimgo/pack/vim-go/start/vim-vimhelplint"


Don't think these should be part of the test. Without this I should still run the tests on macOS.

I'm afraid that I don't quite follow your comment. What do you mean with "this"? Cloning vimhelplint or the :GoInstallBinaries (or both?)

This is the whole section above that installs Vim on the machine. Test should just run the tests, it shouldn't try to install anything every time I execute it.

Okay; how would you do it? Have the script fail if the vim in /tmp/vim-go-test/... doesn't exist?

One of the reasons I like this approach is that it's very easy to run tests: just run ./bin/test vim-7.4 and presto! This makes it easy for people to actually start writing some tests. One of the reasons I've never written any is because I could never get the tests to run on master on my desktop.

All installation results are cached, so only the first time takes a few minutes, and outside of the :GoInstallBinaries it shouldn't modify anything outside of the /tmp/vim-go/test/ directory.

Maybe we could split it up in to ./bin/setup and ./bin/test or some such? Personally I don't really care as such, as long as running tests remains easy and "fool-proof" with just a few commands.

I'm in favor of splitting it. Maybe ./bin/install is a better name. We have makefile where we can combine both.

Alright, I'll work on that later 👍

Perhaps we should run the tests in a Docker container so that the user's machine/environment is never polluted with what tests do?

Perhaps we should run the tests in a Docker container so that the user's machine/environment is never polluted with what tests do?

Yeah, could add that. It would largely be the same scripts, but with a Docker wrapper.
Personally I've had a lot of problems running Docker on my desktop machine and would prefer to avoid it.

arp242 · 2017-10-13T13:46:52Z

Okay, I updated the PR with the requested changes. It seems to work well both on my Linux machine and Travis.

bhcleek · 2017-10-20T08:01:11Z

scripts/install-vim

+
+set -euC
+
+vimgodir=$(cd "$(dirname "$0")/.." && pwd)


vimgodir won't be what you expect here if CDPATH is set unless you redirect stdout to /dev/null.

Setting cdpath for non-interactive shells is probably not really a good idea btw. Pretty sure that more shell scripts will subtly break with that.

Agreed, but it is something that happens. I always think of it from the perspective of being a good script writer and being an environment author. Assume the worst, but adopt the best practice :-)

bhcleek · 2017-10-20T19:37:47Z

scripts/test

+
+### Run vimhelplint.
+####################
+# TODO: Doesn't work in nvim? Not sure why, just don't run it for now.


This doesn't seem to work in any Vim.... Every time I try, it's unable to find VimhelpLintEcho.

Not sure how to fix that to be honest. Does it work for you in master?

I split out the linting stuff to a ./scripts/lint script. That makes it look nicer in Travis as well, and makes sure the tests at least run.

bhcleek · 2017-10-20T19:39:16Z

scripts/install-vim

+"$vimgodir/scripts/run-vim" $vim +':silent :GoUpdateBinaries' +':qa'
+
+[ -d "$installdir/share/vim/vimgo/pack/vim-go/start/vim-vimhelplint" ] || \
+  git clone --quiet https://github.com/machakann/vim-vimhelplint \


There's no need for the full history; a shallow clone will be fine.... (e.g. --depth 1).

fatih · 2017-10-20T19:46:29Z

Btw, before merging let me also know as I want to test it on my local Macbook (unless you both tested it) :)

bhcleek · 2017-10-20T20:09:53Z

Yeah, I'm testing it on my Macbook and also in a Docker container. I'll put up a PR to this branch to bring the Dockerfile in....

bhcleek · 2017-10-20T20:11:46Z

@fatih https://github.com/Carpetsmoker/vim-go/pull/1

bhcleek · 2017-10-21T05:42:54Z

.travis.yml

-script: ./scripts/test.sh
+cache:
+  directories:
+    - /tmp/vim-go-test


How will this affect the builds when we decide to update to a newer patch version of the installed versions?

The cache is per-pr, so we should be fine.

Travis docs say the cache for a branch/pr may come from master, too. Couldn't that be a problem for us?

I don't know, but it's never has been for me in the past. But if it is, we can manually delete it in the Travis UI. Updating patch-levels is not going to be a super-frequent event I think.

Could also add patchlevels to the paths in /tmp/vim-go. I think there was a reason I didn't, but I forgot what it was (been a few weeks).

We shouldn't have to go change Travis configuration because of a change in which Vim versions we support.

I understand the draw of caching this, but long term I think it will cause us to spend cycles debugging what we could solve now by not caching (though at the cost of longer build times).

I'll digress, but please consider (maybe even test to try to invalidate each of our understandings) the concerns I raised.

I don't think it'll be an issue, but OTOH builds are already very fast so removing the cache isn't going to hurt us very much (so I've just removed it).

bhcleek · 2017-10-21T16:36:03Z

Because of the nvim tarball, testing against nvim fails on Mac. That's mitigated by the Dockerfile, though. @fatih, is docker acceptable to you, or do you want the nvim tests to work on Mac?

arp242 · 2017-10-23T23:51:15Z

doc/vim-go.txt

 :GoFreevars

-    Enumerates the free variables of the selection. “Free variables” is a
+    Enumerates the free variables of the selection. "Free variables" is a


You're not smart quote hip :-(

I'm hip, but vimhelplint wasn't in Linux without some particular encodings installed.

- Support multiple Vim versions (currently 7.4, 8.0, and Neovim). - Make sure that we always run Vim from a temporary installation in `/tmp/vim-go-test` without loading `~/.vim`. This makes it a lot easier to run on people's computers. - Also add a handy `./script/run-vim` script to run the installed Vim from the temp directory. Useful for testing, debugging, etc. without a (potentially large) ~/.vim/ dir. - Previously the tests weren't actually being run correctly; see: https://travis-ci.org/fatih/vim-go/builds/279277579 - Format the output of the tests a wee bit nicer, roughly similar to the `go test` output. - Expand docs on testing a bit. I also attempted to integrate code coverage support with https://github.com/Vimjas/covimerage (which was the reason I started working on this), but couldn't really get that to work. Need to look in to that later.

Setting cdpath for non-interactive shells is not a good thing, but just in case...

That way it's faster to run just the tests and it looks nicer in Travis.

So it's easier to distinguish from e.g. exit 1 on vim failing to run.

Enable modeline explicitly so that scripts/lint can be run as root.

Run the test container as a user other than root so that tests run under conditions that more faithfully represent the common use case, because some Vim options are disabled by default when run as root.

Fix the documentation modeline so that it conforms to the modeline rules and guidance in 'help-writing': * space before the vim: * set textwidth option (tw=78) Separate options in the modeline with a space for readability.

Replace multi-byte characters in documentation with ASCII equivalents for consistency within the documentation and so that vimhelplint does not report differently on different operating systems (the multi-byte characters caused the detected line length to be longer than 78 characters on some platforms).

bhcleek · 2017-10-30T05:33:06Z

LGTM

fatih · 2017-10-30T06:18:40Z

This is great. Thanks @Carpetsmoker @bhcleek

How do I run the tests locally now? Do I have to install each Vim version via install-vim first? When I call make test it fails as it can't find Vim (though I have it on my machine). I wonder what the preferred workflow here is.

arp242 · 2017-10-30T06:23:02Z

Yeah, you'll have to run install-vim [version] first @fatih. If you use make all then it should do that for you for all three Vim versions (although running neovim tests doesn't work on macOS yet).

We can tweak the workflow a bit if we want.

it fails as it can't find Vim (though I have it on my machine).

Tests are always run with a fresh Vim version now, and never with the version that may or may not be installed on your machine.

fatih · 2017-10-30T06:32:30Z

Thanks @Carpetsmoker I'm flying in couple of days to Japan for VimConf. I'll take a look when I'm back to add a Docker image that makes it easy to run (for those who use macOS)

arp242 · 2017-10-30T06:46:22Z

Billy already added a Dockerfile @fatih; I also wrote a wrapper script to make that a bit easier to use, but that doesn't quite work for some reason (I'll open a PR in a bit).

fatih · 2017-10-30T06:47:42Z

Nice! Seems like I've totally missed it 🤦‍♂️

bhcleek · 2017-10-30T07:19:22Z

@fatih to test in a clean environment, just:

docker build -t vim-go-test .
docker run --rm vim-go-test

arp242 mentioned this pull request Sep 26, 2017

Error detected while processing function <SNR>47_auto_same_ids[3]..go#guru#SameIds[24] #1477

Closed

fatih reviewed Sep 26, 2017

View reviewed changes

arp242 mentioned this pull request Oct 18, 2017

improve test output handling #1513

Merged

bhcleek reviewed Oct 20, 2017

View reviewed changes

bhcleek reviewed Oct 21, 2017

View reviewed changes

arp242 commented Oct 23, 2017

View reviewed changes

arp242 and others added 18 commits October 30, 2017 04:08

Make sure that a failure shows up correctly

270e812

Fix build again

5eb4ffc

Don't run install automatically for tests

593a888

Add Dockerfile to isolate tests

ca7df35

Redirect potential cd output

72f788d

Setting cdpath for non-interactive shells is not a good thing, but just in case...

Add --depth 1 for vimhelplint

a70dd5e

Split ./scripts/test to ./scripts/lint

0e6bcf2

That way it's faster to run just the tests and it looks nicer in Travis.

Fix cache

3698da3

Do we need this?

1b02c07

Exit 6 for lint script on failures

9dd7017

So it's easier to distinguish from e.g. exit 1 on vim failing to run.

Enable modeline explicitly

53bf975

Enable modeline explicitly so that scripts/lint can be run as root.

Run the test container as a user other than root

b9492fa

Run the test container as a user other than root so that tests run under conditions that more faithfully represent the common use case, because some Vim options are disabled by default when run as root.

fix documentation modeline

df2fa69

Fix the documentation modeline so that it conforms to the modeline rules and guidance in 'help-writing': * space before the vim: * set textwidth option (tw=78) Separate options in the modeline with a space for readability.

Remove cache

f281e34

Final cleanup: fix some indent, add set -e, and add intentional fail

c7f12db

Fix intentional failures

d531405

Run lint on "make all"

ebb5030

arp242 merged commit 7efc3fe into fatih:master Oct 30, 2017

arp242 deleted the coverage branch October 30, 2017 05:33

Uh oh!

Conversation

arp242 commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arp242 Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arp242 commented Oct 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fatih commented Oct 20, 2017

Uh oh!

bhcleek commented Oct 20, 2017

Uh oh!

bhcleek commented Oct 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhcleek commented Oct 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bhcleek commented Oct 30, 2017

Uh oh!

fatih commented Oct 30, 2017

Uh oh!

arp242 commented Oct 30, 2017

Uh oh!

fatih commented Oct 30, 2017

Uh oh!

arp242 commented Oct 30, 2017

Uh oh!

arp242 Sep 26, 2017 •

edited

Loading