[READY] Refactoring integration tests #270

micbou · 2015-12-11T13:41:35Z

Changes

Move related completer tests to their own subdirectories:

ycmd
  └── tests        
      ├── clang
      │   ├── testdata
      │   ├── __init__.py
      │   ├── get_completions_test.py
      │   ├── diagnostics_test.py
      │   ├── subcommands_test.py
      │   └── utils.py 
      ├── cs ── ... 
      ├── python ── ...
      ├── go ── ...
      ...

Regroup tests in classes and subclasses: take advantage of fixtures (setUp and tearDown methods) and inheritance.
Rename tests: names are now shorter;
Fix coding style;
Update copyright notices;
No tests were harmed during the making of this PR: 458 tests before, 458 tests after.

Issues?

Code to set the Bottle debug mode:

import bottle

bottle.debug( True )

was removed from all tests inheriting from Handlers_test where it is set in the setUp method. Is it enough to enable debug mode in all tests? I don't know how to check this.

Basic_test and ZeroBasedLineAndColumn_test tests in Cs_Diagnostics_test are near to be identical. Should we remove one of them?

puremourning · 2015-12-11T22:02:31Z

This is really nice work. Must have been pretty tedious, so thanks!

That said, it's going to be a right pain to rebase onto! :) Ah well - all in the name of progress!

I only have a couple of vague comments. Otherwise LGTM.

Reviewed 184 of 184 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

ycmd/tests/cs/diagnostics_test.py, line 119 [r1] (raw file):
Maybe for another change, but i keep thinking that we could optimise these tests by only starting and stopping the server once. The CS server in particular takes a lot of the test running time.

ycmd/tests/python/utils.py, line 23 [r1] (raw file):
The definition of this method is duplicated in multiple utils.py files. I wonder if it is work having a top-level one, like return ycmd_test.PathToTestDataDir( __name__ )

maybe also for PathToTestFile

They are pretty much the same in each implementation (the only difference is the runtime value of __file__) I think

Comments from the review on Reviewable.io

vheon · 2015-12-11T22:05:25Z

I have just a little comment :)

That said, it's going to be a right pain to rebase onto!

Yeah :( It would be the second time for me 😝 (the subcommand map refactoring) but

Ah well - all in the name of progress!

👍

Reviewed 145 of 184 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

vheon · 2015-12-11T22:06:42Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

ycmd/tests/typescript/utils.py, line 29 [r1] (raw file):
I don't like the fact that this utils file is basically alway the same for each filetype we're going to test :(

Comments from the review on Reviewable.io

vheon · 2015-12-11T22:11:56Z

Reviewed 14 of 184 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

ycmd/tests/clang/diagnostics_test.py, line 76 [r1] (raw file):
Don't we have some object that encapsulate what this is? Or some Matcher would be nice I think.

Comments from the review on Reviewable.io

micbou · 2015-12-12T16:14:01Z

Update

Add a subclass of Handlers_test for each language completer that initializes the _file attribute to the filepath in which the subclass is defined. Thus, the PathToTestData method from Handlers_test points to the right testdata folder.
Move management server functions to those subclasses;
Move test_utils.py functions to the Handlers_test class;
Remove Setup function from test_utils.py;

If you are wondering about the leading underscore in the method names of the Handlers_test class, the answer is that I am considering those methods as protected ones.

Review status: 89 of 111 files reviewed at latest revision, 4 unresolved discussions.

ycmd/tests/clang/diagnostics_test.py, line 76 [r1] (raw file):
Like CompletionEntryMatcher and CompletionLocationMatcher?

ycmd/tests/cs/diagnostics_test.py, line 119 [r1] (raw file):
I agree and I'll take a look if this is feasible.

ycmd/tests/python/python_handlers_test.py, line 23 [r1] (raw file):
See my last changes.

ycmd/tests/typescript/typescript_handlers_test.py, line 29 [r1] (raw file):
See my last changes.

Comments from the review on Reviewable.io

vheon · 2015-12-12T20:43:21Z

Review status: 89 of 111 files reviewed at latest revision, 2 unresolved discussions.

ycmd/tests/clang/diagnostics_test.py, line 76 [r1] (raw file):
Yes

Comments from the review on Reviewable.io

Valloric · 2015-12-12T23:48:46Z

I've been meaning to do this for more than a year, but I knew it would be so tedious I couldn't bring myself to do it. You're a better man than I! Thank you so much for doing this.

No tests were harmed during the making of this PR: 458 tests before, 458 tests after.

Yay! :)

Could you also use @puremourning's coverage setup to verify the number is the same (or higher) after these changes? Just as a sanity check.

With that,

Review status: 89 of 111 files reviewed at latest revision, 3 unresolved discussions.

ycmd/tests/test_utils.py, line 4 [r3] (raw file):
This should be "ycmd contributors" instead of just "ycmd".

Comments from the review on Reviewable.io

micbou · 2015-12-13T02:10:19Z

I added coverage to my repository and there is a very small decrease that I don't understand. I get the exact same coverage results on Travis for the two commits: without the refactoring and with. Is this a coveralls.io bug?

Review status: 89 of 111 files reviewed at latest revision, 3 unresolved discussions.

ycmd/tests/test_utils.py, line 4 [r3] (raw file):
Done.

Comments from the review on Reviewable.io

Valloric · 2015-12-13T06:09:38Z

Is this a coveralls.io bug?

Maybe. For such a small decrease, I wouldn't worry about it.

Review status: 89 of 111 files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

micbou · 2015-12-13T22:08:13Z

I found the issue. The tests folder was not ignored by coveralls.io. By ignoring it, I obtain about the same coverage (there is still a difference but it is so small that we can ignore it). Since we already merged PR #271, I'll send a PR to ignore the tests and also the __init__.py files which are empty files.

Remaining things to do (maybe for another PR?):

add a new matcher to simplify tests in ycmd/tests/clang/diagnostics_test.py;
try to speed up the C♯ tests by only starting and stopping the server one time.

Review status: 89 of 111 files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

puremourning · 2015-12-13T22:17:38Z

For the second item, i think the limiting factor in the existing tests is the creation of the TestApp, which you're already abstracting, so I hope it should be a matter of only creating that once-per-script rather than once-per-test, using class-level fixtures. From nose docs:

Class-level setup fixtures may be named setup_class, setupClass, setUpClass, setupAll or setUpAll; teardown fixtures may be named teardown_class, teardownClass, tearDownClass, teardownAll or tearDownAll. Class-level setup and teardown fixtures must be class methods.

Reviewed 10 of 27 files at r2, 3 of 4 files at r3.
Review status: 101 of 111 files reviewed at latest revision, 4 unresolved discussions.

ycmd/tests/clang/diagnostics_test.py, line 76 [r1] (raw file):
I think that's what @vheon means, yeah

ycmd/tests/handlers_test.py, line 43 [r3] (raw file):
It seems strange to me to call static methods via an instance pointer (these are always called via self.). I just kinda feel that they were better as module methods where they can be imported and namespaced as required by the user and are somewhat less verbose ::/

Comments from the review on Reviewable.io

vheon · 2015-12-13T22:21:33Z

Wouldn't we open us to issues where we run tests in a non clean environment?

Review status: 100 of 111 files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

micbou · 2015-12-13T22:24:05Z

Review status: 100 of 111 files reviewed at latest revision, 3 unresolved discussions.

ycmd/tests/handlers_test.py, line 43 [r3] (raw file):
My goal was to remove the test_utils.pyfile but I found out that some function in it was used elsewhere so no way to completely remove it.
If this is really an issue, I can easily revert the change. It is all in one commit.

Comments from the review on Reviewable.io

puremourning · 2015-12-13T22:25:35Z

Reviewed 12 of 27 files at r2, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

ycmd/tests/handlers_test.py, line 43 [r3] (raw file):
not a big deal, no.

Comments from the review on Reviewable.io

mispencer · 2015-12-13T22:43:14Z

Regarding the C# test performance, I implemented a persistence instance of Omnisharp to improve this in my fork a while ago, though I never got around to making a pull request for it. I don't know how useful it is now, but you're welcome to use it if you want.

Comments from the review on Reviewable.io

micbou · 2015-12-13T23:07:47Z

Thanks. I'll take a look.

Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

Valloric · 2015-12-14T02:01:58Z

@micbou Both of the "remaining work" points you've mentioned sound like they should be separate PRs. This one is nice and focused so let's keep it that way.

Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

homu · 2015-12-14T02:33:01Z

☔ The latest upstream changes (presumably #272) made this pull request unmergeable. Please resolve the merge conflicts.

micbou · 2015-12-14T22:27:24Z

@puremourning I'm currently moving the Javascript completer tests to the new class structure but I'm encountering an issue with the DirtyNamedBuffers_test. I am getting a ServerError with the message:

Request missing required field: file_data["C:\\Users\\micbou\\.vim\\bundle\\YouCompleteMe\\third_party\\ycmd\\ycmd\\tests\\javascript\\testdata\\requirejs_test.js"]

Did you already get this kind of error? I can't find what is causing this. I'm correctly changing the current directory at the start of the test and the arguments passed to the _RunTest method are the same as the old test. I pushed the current changes if you want take a look.

Review status: 110 of 116 files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

puremourning · 2015-12-14T22:36:03Z

I can repo with your branch. I will take a look.

Review status: 110 of 116 files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

puremourning · 2015-12-14T22:42:07Z

@micbou I think you lost my change to BuildRequest where it checked for type of dict so that it can "extend" the file_data dictionary : https://github.com/puremourning/ycmd-1/blob/tern-completer-locking/ycmd/tests/test_utils.py#L45-L55

Review status: 110 of 116 files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

micbou · 2015-12-14T22:44:07Z

Yes, that's it. Thanks!

Review status: 110 of 116 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from the review on Reviewable.io

puremourning · 2015-12-14T22:46:33Z

Yo dawg. I heard you liked PRs, so I made a PR on your PR. So you can merge when you merge.

Review status: 110 of 116 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from the review on Reviewable.io

micbou · 2015-12-14T22:50:05Z

Ok, I'll merge it but only because it's you!

Review status: 110 of 116 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from the review on Reviewable.io

Fix failing javascript test

puremourning · 2015-12-14T22:54:14Z

Reviewed 1 of 6 files at r6.
Review status: 110 of 116 files reviewed at latest revision, 3 unresolved discussions.

ycmd/tests/javascript/event_notification_test.py, line 54 [r6] (raw file):
was there a specific reason why you didn't use @with_cwd ? The reason i did it that way was to undo it in the case of a failure (or success), so that no subsequent tests fail because they are in the wrong dir (be they javascript tests or not). The biggest historical problem I have found with large test suites is individual tests doing some setup and then not resetting their state to the previous state, particularly if they fail. I'm super sensitive to it because the project i work on has 10s of thousands of tests and any one test leaving garbage behind can cause random failures somewhere else in the suite. Good test citizens undo any stuff they change, even if they fail.

in any case, removing @with_cwd means that contextlib2 is no longer a test requirement (i think)

Comments from the review on Reviewable.io

micbou · 2015-12-14T22:57:59Z

Review status: 110 of 116 files reviewed at latest revision, 3 unresolved discussions.

ycmd/tests/javascript/event_notification_test.py, line 54 [r6] (raw file):
State is reset by the tearDown method in the Javascript_Handlers_testclass. It should work exactly the same as before.

Comments from the review on Reviewable.io

puremourning · 2015-12-14T22:59:17Z

Review status: 110 of 116 files reviewed at latest revision, 3 unresolved discussions.

ycmd/tests/javascript/event_notification_test.py, line 54 [r6] (raw file):
ah i see. nice one. can probably still remove contextlib2 then.

Comments from the review on Reviewable.io

micbou · 2015-12-14T22:59:38Z

Review status: 110 of 116 files reviewed at latest revision, 3 unresolved discussions.

ycmd/tests/javascript/event_notification_test.py, line 54 [r6] (raw file):
Yes, it was the goal but forgot to remove it.

Comments from the review on Reviewable.io

puremourning · 2015-12-14T23:02:07Z

Review status: 110 of 116 files reviewed at latest revision, 3 unresolved discussions.

ycmd/tests/javascript/javascript_handlers_test.py, line 51 [r6] (raw file):
Pedantry: One more line of whitespace here :)

Comments from the review on Reviewable.io

puremourning · 2015-12-14T23:03:11Z

Reviewed 5 of 6 files at r6.
Review status: 115 of 116 files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

puremourning · 2015-12-14T23:08:04Z

Reviewed 1 of 3 files at r8.
Review status: 114 of 117 files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

puremourning · 2015-12-14T23:08:44Z

Reviewed 1 of 1 files at r7, 2 of 3 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

micbou · 2015-12-14T23:11:32Z

One remaining thing I would like to do is move the Tern server starting and stopping to the corresponding setUp and the tearDown methods.

Review status: all files reviewed at latest revision, 3 unresolved discussions.

ycmd/tests/javascript/javascript_handlers_test.py, line 51 [r6] (raw file):
Done.

Comments from the review on Reviewable.io

Move Tern server starting and stopping in tests to the corresponding setUp and tearDown methods in Javascript_Handlers_test

micbou · 2015-12-15T01:10:17Z

I think this is ready.

Things to do in other PRs:

use class-level fixtures for starting and stopping servers;
add matchers to simplify tests.

Review status: 113 of 117 files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

Valloric · 2015-12-15T02:28:34Z

Nice! Thanks for doing this!

@homu r+

Review status: 113 of 117 files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

homu · 2015-12-15T02:28:35Z

📌 Commit 2e75b9c has been approved by Valloric

homu · 2015-12-15T02:28:47Z

⚡ Test exempted - status

[READY] Refactoring integration tests ### Changes - Move related completer tests to their own subdirectories: ``` ycmd └── tests ├── clang │ ├── testdata │ ├── __init__.py │ ├── get_completions_test.py │ ├── diagnostics_test.py │ ├── subcommands_test.py │ └── utils.py ├── cs ── ... ├── python ── ... ├── go ── ... ... ``` - Regroup tests in classes and subclasses: take advantage of fixtures (`setUp` and `tearDown` methods) and inheritance. - Rename tests: names are now shorter; - Fix coding style; - Update copyright notices; - No tests were harmed during the making of this PR: 458 tests before, 458 tests after. ### Issues? - Code to set the Bottle debug mode: ```python import bottle bottle.debug( True ) ``` was removed from all tests inheriting from `Handlers_test` where it is set in the `setUp` method. Is it enough to enable debug mode in all tests? I don't know how to check this. - `Basic_test` and `ZeroBasedLineAndColumn_test` tests in `Cs_Diagnostics_test` are near to be identical. Should we remove one of them?  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/270)

micbou mentioned this pull request Dec 12, 2015

[READY] JavaScript completer using Tern.js #272

Merged

micbou force-pushed the refactor-tests branch from d9ecb84 to a859173 Compare December 13, 2015 23:12

micbou force-pushed the refactor-tests branch from a859173 to fda5434 Compare December 14, 2015 22:12

micbou added 2 commits December 14, 2015 23:50

Merge pull request #1 from puremourning/refactor-tests

174a84e

Fix failing javascript test

Remove print

49469b2

Remove contextlib2 test requirement

7f54151

Add missing blank line

86dffe3

micbou added 2 commits December 15, 2015 00:14

Change variable name in Javascript_Handlers_test

1ea52e0

Move Tern server management to fixtures in tests

2e75b9c

Move Tern server starting and stopping in tests to the corresponding setUp and tearDown methods in Javascript_Handlers_test

micbou changed the title ~~Refactoring integration tests~~ [READY] Refactoring integration tests Dec 15, 2015

homu merged commit 2e75b9c into ycm-core:master Dec 15, 2015

micbou deleted the refactor-tests branch December 15, 2015 11:15

[READY] Refactoring integration tests #270

[READY] Refactoring integration tests #270

Conversation

micbou commented Dec 11, 2015

Changes

Issues?

puremourning commented Dec 11, 2015

vheon commented Dec 11, 2015

vheon commented Dec 11, 2015

vheon commented Dec 11, 2015

micbou commented Dec 12, 2015

Update

vheon commented Dec 12, 2015

Valloric commented Dec 12, 2015

micbou commented Dec 13, 2015

Valloric commented Dec 13, 2015

micbou commented Dec 13, 2015

puremourning commented Dec 13, 2015

vheon commented Dec 13, 2015

micbou commented Dec 13, 2015

puremourning commented Dec 13, 2015

mispencer commented Dec 13, 2015

micbou commented Dec 13, 2015

Valloric commented Dec 14, 2015

homu commented Dec 14, 2015

micbou commented Dec 14, 2015

puremourning commented Dec 14, 2015

puremourning commented Dec 14, 2015

micbou commented Dec 14, 2015

puremourning commented Dec 14, 2015

micbou commented Dec 14, 2015

puremourning commented Dec 14, 2015

micbou commented Dec 14, 2015

puremourning commented Dec 14, 2015

micbou commented Dec 14, 2015

puremourning commented Dec 14, 2015

puremourning commented Dec 14, 2015

puremourning commented Dec 14, 2015

puremourning commented Dec 14, 2015

micbou commented Dec 14, 2015

micbou commented Dec 15, 2015

Valloric commented Dec 15, 2015

homu commented Dec 15, 2015

homu commented Dec 15, 2015