Move api/ingest tests to mocha #8106
Conversation
spalger
force-pushed
the
apiTestsToMocha
branch
3 times, most recently
from
94950b7
to
a095119
Compare
|
I'm not sure how I feel about mixing these tests in with the regular unit tests. It could be really confusing if we want to add legitimate unit tests for any of the route code. It also means that every plugin has to come up with its own server initialization and scenario management infrastructure. I've been thinking for awhile that these tests could stay under |
In my opinion the mocha tests are not unit tests. They have unit tests, and integration tests, and all sorts of other tests that fall somewhere in a grey area, and I think the only time that confuses people is when they're trying to label them as a specific type of test. I just see it as mocha running function that make assertions about the system.
I don't see any reason to put the tests at the root of the repo. I think that putting the tests near the code:
|
|
I assume all the linting changes were included in error? |
Yes, exactly. I (and I think others) find our existing test setup confusing. I should be able to run individual unit test suites quickly and easily from the command line or my editor. But looking at any given tests directory, I can't easily tell if these tests are isolated or if they require a running kibana and/or elasticsearch server. I would bet that unit and integration tests are sometimes even mixed in the same files, because there's no separation. It doesn't matter as much on CI where we just run everything, but mixing things together makes development more difficult. It would also make it almost impossible if we ever want to run unit and integration tests separately on Jenkins. |
I totally agree, and my personal opinion is that there aren't any tests in kibana that are easier to run from the command line than the server tests in mocha:
Also agree, though this PR is not intending or claiming to solve that problem and definitely doesn't make it any worse. If anything, the fact that the code now has setup and teardown functions describing the fact that it starts the kibana server improve the ability for people to identify the dependencies of these tests. We could take it further, but I think we should discuss how we do that in a new issue and pr. |
|
@spalger so I thought about it some more after our Zoom and I'm ok with this setup for now, it's a dramatic improvement over being tied to the intern so I don't wanna get too hung up on other issues. I do like the idea of solving the isolation problem by providing our tests with a util that each test file can call out to if it needs an ES or Kibana server, and sort of memoizing it so if multiple tests need the same setup they can share the instance in a single run. I'll create a ticket for this. |
spalger
force-pushed
the
apiTestsToMocha
branch
from
42a9320
to
10ec581
Compare
this became auto-fixable in eslint v3, it was previously applied to only part of the code base
this became auto-fixable in eslint v3, it was previously fixed manually in part of the code base
this rule was added to the shared eslint config[1] and was automatically fixed [1]: https:/elastic/eslint-config-kibana
this became auto-fixable in eslint v3
this was added to the shared eslint config[1] a while ago and is not autofixable. Instead, a collection of custom jscodeshift transforms[2] were used to fix certain violation formats, and the rest was fixed manually. [1]: https:/elastic/eslint-config-kibana [2]: https:/kreeware/kibana-jscodeshift/tree/master/transforms
The import statements produced by jscodeshift do not put spaces between the properties and the curly-braces used in destructuring import statements. This is different from the style they are usually written in, so I enabled this rule to fix them and ensure we keep with the current style.
Several of the timer stubs created a reference to a clock variable, but never used that variable, so they were causing no-unused-vars errors. In the process of fixing those errors, some of those must have been removed, which caused some test failures.
When the tests for the api were written, they ended up using intern. This is not desirable for a number of reasons, but in an effort to remove all remaining `require()` calls I decided to give this migration a shot. Moving the tests to mocha was actually trivial.
spalger
force-pushed
the
apiTestsToMocha
branch
from
10ec581
to
76c9a3d
Compare
|
@spalger What's your plan here? |
|
This still relies on #8101, I plan to merge this once that is in |
|
Will be resubmitting this separately. |
Builds on top of #8101
The api/ingest tests were written to use intern, which we use for functional testing. Overtime these tests have fallen out of sync with the rest of the code because of this, and in an effort to get rid of all the
require()statements possible I figured it might be a good time to move these tests over with the rest of the server tests.Relevant work is in a095119