Allow skipping tests for specific completers #328

mispencer · 2016-01-29T15:08:47Z

Currently, you can skip tests for the clang completion engine. This pull requests extends this feature for the other completion engines.

The existing filter for clang was based on the test name. I attempted to use this method for the other completions, but not all tests were consistently named with the language or completer engine. So instead of refactoring all those name, I moved the test files so that all the tests for each completion engine were in their own directory, and, using a new nose plugin, excluded tests in said directory. As an additional benefit, directory filtering is less brittle to future changes.

For consistence, I also moved the other tests outside the test folder into the test folder.

Valloric · 2016-01-29T23:03:50Z

I was just thinking the same thing yesterday! :)

Coveralls is saying that coverage went down; is this expected?

@micbou You reorganized the tests recently so you probably have some opinions on how this should be done.

I'm LGTM on this if @micbou is.

Review status: 0 of 25 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from the review on Reviewable.io

mispencer · 2016-01-30T00:52:30Z

Coveralls had 100% coverage of ycmd/completers/completer_utils_test.py. After this change, it's not longer listed. This was the only change in coverage. I assume that completer_utils_test.py never should have been listed in the first place, since it's a test file, so I think this is fine.

Comments from the review on Reviewable.io

puremourning · 2016-01-30T09:36:25Z

I haven't looked at this in too much detail, but a couple things spring to mind:

I normally just do ./run_tests.py --skip-build -- ycmd.tests.javascript or something
we kept the Integration and Module tests separate before. I think they are now merged. Is that right?
both @vheon and I have working vagrant files for ycmd tests presumably the issue is that there is quite a lot of setup required to run everything ?

Review status: 0 of 25 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from the review on Reviewable.io

vheon · 2016-01-30T13:53:07Z

I normally just do ./run_tests.py --skip-build -- ycmd.tests.javascript or something

That is basically what I usually do too.

On the other hand it would be tedious without this change to to run all the tests except for the omnisharp one for example. The question here is that this is a real use case 😕

micbou · 2016-01-30T15:36:12Z

I am not sure about moving unit tests with integration tests. It makes sense for unit tests to be near the module they are testing and integration tests to be at the root module. I would just move the ycmd/completers/completer_utils_test.py to ycmd/completers/tests/completer_utils_test.py to be consistent with other tests. It would also remove it from coverage.

Concerning the new options to exclude tests from completers, I don't think they are really useful. With them, we can do (for example):

./run_tests.py --skip-build --no-javascript-completer

instead of

./run_tests.py --skip-build --exclude-dir=ycmd/tests/javascript

Not a big gain.

Review status: 0 of 25 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from the review on Reviewable.io

mispencer · 2016-01-30T16:39:14Z

My use case is I want to run ./run_tests.py locally with appropriate arguments to test everything I can without getting superficial errors from tests for completers I don't have installed. Outside CI, this utility is only used by people working on the code. I assumed enough of the people doing this don't have every completer installed that this use cause would be worth while.

./run_tests.py --skip-build --no-javascript-completer and ./run_tests.py --skip-build --exclude-dir=ycmd/tests/javascript are equivalent. But./run_tests.py --no-omnisharp-completer and ./run_tests.py --exclude-dir=ycmd/tests/cs aren't, since --no-omnisharp-completer affects the build. I put in ones that don't affect the build mostly for consistence.

In general, if there's a use case for --no-clang-completer, there is a use case of --no-omnisharp-completer and all the others. So we should have them all, or remove --no-clang-completer.

Regarding the location, the pattern I am used to is having both unit tests and integration tests together, so that is the pattern I have implemented here. Indeed, I didn't realizes there were tests outside the tests directory until I put this PR together. If desired, I guess I can move them back. If so, at least FindGoCodeBinary_test should be moved, since that's a integration test. I am unsure whether the other tests in gocode_completer_test.py and filename_completer_test.py count as integration tests, since they use the file system. I am also unsure what to do about completer-specific unit tests in this case - should they be excluded under --no-whatever-completer?

Comments from the review on Reviewable.io

micbou · 2016-01-31T20:10:20Z

I changed my view on this. I think it is fine to move all the tests in the tests root folder. For the new options, I didn't think about the build so they make sense. Just one thing, could they all be named after the language they are targeting (--no-cs-completer instead of --no-omnisharp-completer, etc.)?

Reviewed 37 of 39 files at r1.
Review status: 23 of 25 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

ycmd/tests/go/get_completions_test.py, line 24 [r1] (raw file):
I am currently working on a overhaul of the Go completer and one of the change is to rename it GoCompleter so let's keep the current name of this class. Other tests will probably be moved here.

Comments from the review on Reviewable.io

vheon · 2016-01-31T21:13:18Z

What I'm afraid is that more languages we support more option we're going to have here which IMHO could go out of hand. If we do like @micbou suggested:

Just one thing, could they all be named after the language they are targeting (--no-cs-completer instead of --no-omnisharp-completer, etc.)

then we could make a single option like --no-completer=<language> <language> ... and be done with it. What do you think?