Skip to content
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

Merged
merged 14 commits into from
Jul 28, 2017

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Jul 25, 2017

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 an EsTestCluster 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 start

Example usage:

import { esTestCluster } from '../../test_utils/es'
describe('module/ThingBeingTested', () => {
  const es = esTestCluster.use({
    name: 'module/ThingBeingTested',
    port: 9242
  });

  before(() => es.start());
  after(() => es.stop());

  it('...')
})

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.

Copy link
Contributor

@kimjoar kimjoar left a 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

@spalger spalger force-pushed the implement/es-test-cluster-util branch from 1ebb79c to b66b779 Compare July 25, 2017 23:46
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 minute startup times?! :(

Copy link
Contributor Author

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)

this.slow(10000);
this.timeout(60000);
this.slow(60 * 1000);
this.timeout(3 * 60 * 1000);
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me


export class EsTestCluster {
_freshBranches = [];
_directory = resolve(ESVM_DIR);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why resolve again?

Copy link
Contributor Author

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

Copy link
Contributor

@epixa epixa left a 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.

name,
port = esTestServerUrlParts.port,
log = console.log,
branch = 'master',
Copy link
Contributor

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.

Copy link
Contributor

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?

let cluster;

return {
start: async () => {
Copy link
Contributor

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.

@spalger
Copy link
Contributor Author

spalger commented Jul 27, 2017

Changes should address your feedback @epixa and @tylersmalley

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}

getBranch() {
return process.env.TEST_ES_BRANCH || 'master';
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 😃

Copy link
Contributor

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

@spalger spalger merged commit 6748b22 into elastic:master Jul 28, 2017
spalger added a commit that referenced this pull request Jul 28, 2017
* [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)
spalger added a commit that referenced this pull request Jul 28, 2017
* [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)
@spalger
Copy link
Contributor Author

spalger commented Jul 28, 2017

6.0: 3e74f40
6.x/6.1: 48bfe7f

@spalger spalger deleted the implement/es-test-cluster-util branch July 28, 2017 19:38
spalger added a commit to spalger/kibana that referenced this pull request Aug 9, 2017
* [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)
@spalger spalger added the v5.6.0 label Aug 9, 2017
spalger added a commit that referenced this pull request Aug 9, 2017
* [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants