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

Better testing #1476

Merged
merged 19 commits into from Oct 30, 2017

Conversation

3 participants
@Carpetsmoker
Copy link
Collaborator

Carpetsmoker commented Sep 25, 2017

  • 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.

set -euC
cd "$(dirname "$0")"

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

This comment has been minimized.

@fatih

fatih Sep 26, 2017

Owner

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.

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Sep 26, 2017

Collaborator

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).

This comment has been minimized.

@bhcleek

bhcleek Oct 3, 2017

Collaborator

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

./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"

This comment has been minimized.

@fatih

fatih Sep 26, 2017

Owner

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

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Sep 26, 2017

Collaborator

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 comment has been minimized.

@fatih

fatih Sep 26, 2017

Owner

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.

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Sep 26, 2017

Collaborator

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

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Sep 26, 2017

Collaborator

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.

This comment has been minimized.

@fatih

fatih Sep 26, 2017

Owner

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

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Sep 26, 2017

Collaborator

Alright, I'll work on that later 👍

This comment has been minimized.

@bhcleek

bhcleek Sep 26, 2017

Collaborator

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

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Oct 5, 2017

Collaborator

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.

@Carpetsmoker Carpetsmoker force-pushed the Carpetsmoker:coverage branch 6 times, most recently from 8828bdb to a1ff1b9 Oct 13, 2017

@Carpetsmoker

This comment has been minimized.

Copy link
Collaborator

Carpetsmoker commented Oct 13, 2017

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

@Carpetsmoker Carpetsmoker force-pushed the Carpetsmoker:coverage branch from a1ff1b9 to de61cac Oct 13, 2017


set -euC

vimgodir=$(cd "$(dirname "$0")/.." && pwd)

This comment has been minimized.

@bhcleek

bhcleek Oct 20, 2017

Collaborator

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

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Oct 21, 2017

Collaborator

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.

This comment has been minimized.

@bhcleek

bhcleek Oct 21, 2017

Collaborator

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 :-)


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

This comment has been minimized.

@bhcleek

bhcleek Oct 20, 2017

Collaborator

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

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Oct 21, 2017

Collaborator

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.

"$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 \

This comment has been minimized.

@bhcleek

bhcleek Oct 20, 2017

Collaborator

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

@fatih

This comment has been minimized.

Copy link
Owner

fatih commented Oct 20, 2017

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

@bhcleek

This comment has been minimized.

Copy link
Collaborator

bhcleek commented Oct 20, 2017

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

This comment has been minimized.

Copy link
Collaborator

bhcleek commented Oct 20, 2017

@Carpetsmoker Carpetsmoker force-pushed the Carpetsmoker:coverage branch from 560791c to ac27e13 Oct 21, 2017

.travis.yml Outdated
script: ./scripts/test.sh
cache:
directories:
- /tmp/vim-go-test

This comment has been minimized.

@bhcleek

bhcleek Oct 21, 2017

Collaborator

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

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Oct 21, 2017

Collaborator

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

This comment has been minimized.

@bhcleek

bhcleek Oct 21, 2017

Collaborator

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

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Oct 21, 2017

Collaborator

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.

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Oct 21, 2017

Collaborator

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).

This comment has been minimized.

@bhcleek

bhcleek Oct 21, 2017

Collaborator

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.

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Oct 30, 2017

Collaborator

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

This comment has been minimized.

Copy link
Collaborator

bhcleek commented Oct 21, 2017

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?

@@ -566,13 +566,13 @@ CTRL-t
*:GoFreevars*
:GoFreevars

Enumerates the free variables of the selection. Free variables is a
Enumerates the free variables of the selection. "Free variables" is a

This comment has been minimized.

@Carpetsmoker

Carpetsmoker Oct 23, 2017

Collaborator

You're not smart quote hip :-(

This comment has been minimized.

@bhcleek

bhcleek Oct 23, 2017

Collaborator

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

Carpetsmoker and others added some commits Sep 25, 2017

Better testing
- 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.
Redirect potential `cd` output
Setting cdpath for non-interactive shells is not a good thing, but just
in case...
Split ./scripts/test to ./scripts/lint
That way it's faster to run just the tests and it looks nicer in Travis.

Carpetsmoker and others added some commits Oct 21, 2017

Exit 6 for lint script on failures
So it's easier to distinguish from e.g. exit 1 on vim failing to run.
Replace multi-byte characters with ASCII equivalents
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).
Enable modeline explicitly
Enable modeline explicitly so that scripts/lint can be run as root.
Run the test container as a user other than 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 documentation modeline
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.

@Carpetsmoker Carpetsmoker force-pushed the Carpetsmoker:coverage branch from 94c793e to f281e34 Oct 30, 2017

Carpetsmoker added some commits Oct 30, 2017

@bhcleek

This comment has been minimized.

Copy link
Collaborator

bhcleek commented Oct 30, 2017

LGTM

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Carpetsmoker Carpetsmoker deleted the Carpetsmoker:coverage branch Oct 30, 2017

@fatih

This comment has been minimized.

Copy link
Owner

fatih commented Oct 30, 2017

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.

@Carpetsmoker

This comment has been minimized.

Copy link
Collaborator

Carpetsmoker commented Oct 30, 2017

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

This comment has been minimized.

Copy link
Owner

fatih commented Oct 30, 2017

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)

@Carpetsmoker

This comment has been minimized.

Copy link
Collaborator

Carpetsmoker commented Oct 30, 2017

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

This comment has been minimized.

Copy link
Owner

fatih commented Oct 30, 2017

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

@bhcleek

This comment has been minimized.

Copy link
Collaborator

bhcleek commented Oct 30, 2017

@fatih to test in a clean environment, just:

docker build -t vim-go-test .
docker run --rm vim-go-test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment