-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Implement esTestCluster test util #13099
Implement esTestCluster test util #13099
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much better and gives much more control in the tests. LGTM
…test-cluster-util
…test-cluster-util
1ebb79c
to
b66b779
Compare
// NOTE: This can't be a fat-arrow function because `this` needs to refer to the execution | ||
// context, not to the parent context. | ||
this.timeout(60000); | ||
this.timeout(3 * 60 * 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 minute startup times?! :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might have to download es on a slow network connection (like mine, at the hotel I'm staying at)
src/server/http/__tests__/index.js
Outdated
this.slow(10000); | ||
this.timeout(60000); | ||
this.slow(60 * 1000); | ||
this.timeout(3 * 60 * 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than hardcode these times all over, and since they are directly related to the behaviors of esTestCluster, how about moving them to a constant from test_utils/es?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me
src/test_utils/es/es_test_cluster.js
Outdated
|
||
export class EsTestCluster { | ||
_freshBranches = []; | ||
_directory = resolve(ESVM_DIR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why resolve
again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use to be making modifications based on test cluster name
, but refactored so that all test clusters share a directory. will remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall these changes are great. I put a couple of notes inline.
src/test_utils/es/es_test_cluster.js
Outdated
name, | ||
port = esTestServerUrlParts.port, | ||
log = console.log, | ||
branch = 'master', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch should come from configuration rather than being hardcoded. Ideally using an existing configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, wouldn't this break if backported to 6.x?
src/test_utils/es/es_test_cluster.js
Outdated
let cluster; | ||
|
||
return { | ||
start: async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think start
should throw an error if cluster
is already defined, this way we don't get into a situation where calling start orphans a cluster object that was never shutdown.
Changes should address your feedback @epixa and @tylersmalley |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/test_utils/es/es_test_config.js
Outdated
} | ||
|
||
getBranch() { | ||
return process.env.TEST_ES_BRANCH || 'master'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We configure branch in package.json, how about making that the default? I'm not sure when we wouldn't want that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, branch in package.json is the Kibana branch and I didn't want to enforce a coupling there that wasn't necessary. But if you think that's a good idea I'm good with it too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welp, turns out merging #13149 does just that 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if we ever diverged from Elasticsearch, we should just introduce an esbranch variable or something in package.json anyway. Trying to consolidate the versions across the board into configuration
* [es/tests] remove unused module * [testUtil/es] add utility for starting es nodes in tests * [ftr/tests] use esTestCluster util to start es * [es/tests/routes] use esTestCluster to start own es * [testUtils/kbnServer] disable uiSettings unless plugins are enabled * [testUtils/esTestCluster] use standard test es port by default * [server/http/tests] add esTestCluster to setup * [test/config] unify es test config into a single module * [testUtils/esTestCluster] directory is no longer configurable * [testUtils/esTestCluster] throw when es.start() is called again without es.stop() * [testUtils/esTestCluster] is* checks should not mutate state (cherry picked from commit 6748b22)
* [es/tests] remove unused module * [testUtil/es] add utility for starting es nodes in tests * [ftr/tests] use esTestCluster util to start es * [es/tests/routes] use esTestCluster to start own es * [testUtils/kbnServer] disable uiSettings unless plugins are enabled * [testUtils/esTestCluster] use standard test es port by default * [server/http/tests] add esTestCluster to setup * [test/config] unify es test config into a single module * [testUtils/esTestCluster] directory is no longer configurable * [testUtils/esTestCluster] throw when es.start() is called again without es.stop() * [testUtils/esTestCluster] is* checks should not mutate state (cherry picked from commit 6748b22)
* [es/tests] remove unused module * [testUtil/es] add utility for starting es nodes in tests * [ftr/tests] use esTestCluster util to start es * [es/tests/routes] use esTestCluster to start own es * [testUtils/kbnServer] disable uiSettings unless plugins are enabled * [testUtils/esTestCluster] use standard test es port by default * [server/http/tests] add esTestCluster to setup * [test/config] unify es test config into a single module * [testUtils/esTestCluster] directory is no longer configurable * [testUtils/esTestCluster] throw when es.start() is called again without es.stop() * [testUtils/esTestCluster] is* checks should not mutate state (cherry picked from commit 6748b22)
* [partial backport] extract kbnServer test utils from #12554 * Implement esTestCluster test util (#13099) * [es/tests] remove unused module * [testUtil/es] add utility for starting es nodes in tests * [ftr/tests] use esTestCluster util to start es * [es/tests/routes] use esTestCluster to start own es * [testUtils/kbnServer] disable uiSettings unless plugins are enabled * [testUtils/esTestCluster] use standard test es port by default * [server/http/tests] add esTestCluster to setup * [test/config] unify es test config into a single module * [testUtils/esTestCluster] directory is no longer configurable * [testUtils/esTestCluster] throw when es.start() is called again without es.stop() * [testUtils/esTestCluster] is* checks should not mutate state (cherry picked from commit 6748b22) * [testUtils/esTestCluster] use more standard api style (#13197) (cherry picked from commit d36080b) * [scanner] use new esTestConfig service
Addresses part of #8130
This adds
esTestCluster
to the test utils which exposes a simplified API for starting elasticsearch nodes for tests.esTestCluster.use()
: creates anEsTestCluster
object that wraps libesvm with some defaults/helpers for use in tests. It accepts several options:options.name
, required, used for the cluster name and is prepended to the logs produced[options.port=9220]
, default is overridable, port for Elasticsearch's HTTP API[options.log=console.log]
a function called with log lines from es[options.branch=master]
the elasticsearch branch to startExample usage:
The benefit of this wrapper is that it enforces some good practices, like logging all Elasticsearch output, and only downloads each branch once since it knows when other tests have already downloaded that branch.