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

Migrate Elasticsearch tests to NoSQLUnit Elasticsearch HTTP adapter #4125

Merged
merged 14 commits into from Sep 13, 2017

Conversation

@joschi
Copy link
Contributor

@joschi joschi commented Sep 5, 2017

Tests using Elasticsearch were previously relying on the NoSQLUnit Elasticsearch 2.x adapter to create an embedded Elasticsearch 2.x instance.

Unfortunately embedded Elasticsearch nodes are unsupported since Elasticsearch 5.0.0 (see https://www.elastic.co/blog/elasticsearch-the-server) so that these tests now have to rely on an externally spawned Elasticsearch instance.

In this pull request, we're utilizing the elasticsearch-maven-plugin which spawns an Elasticsearch node during Maven's verify lifecycle phase (integration-test to be precise) which then can be used by Graylog's Elasticsearch tests (ending in *IT instead of *Test).

Jochen Schalanda added 10 commits Sep 1, 2017
The test dependency on Fongo needs to be added explicitly now,
because the dependency scope in nosqlunit-mongodb has been changed to "provided".
If the database isn't dropped (and thus all collections and indices are purged),
`AccessTokenServiceImplTest#testExceptionForMultipleTokens()` will fail with the
following error message:

    java.lang.IllegalArgumentException: Unexpected error reading data set file.
    Caused by: com.mongodb.MongoWriteException: E11000 duplicate key error index: test.access_tokens.token_1  dup key : {[[foobar]] }

This is caused by `AccessTokenServiceImpl#save()` creating a unique index over the
"token" attribute.
The NoSQLUnit MongoDB adapter doesn't drop databases after each test but
our tests expected to start with a clean slate.
Tests using Elasticsearch were previously relying on the NoSQLUnit Elasticsearch adapter
to create an embedded Elasticsearch 2.x instance.

Unfortunately embedded Elasticsearch nodes are unsupported since Elasticsearch 5.0.0 so
that these tests now have to rely on an externally spawned Elasticsearch instance.

In this case, we're utilizing the elasticsearch-maven-plugin which spawns an Elasticsearch
node during Maven's "verify" lifecycle phase ("integration-test" to be precise) which can
be used by Graylog's integration tests (ending in "*IT" instead of "*Test").
The Elasticsearch node should only be started in the graylog2-server submodule
and skipped in all other submodules.
The `it.ElasticsearchVersion` system property now controls whether Elasticsearch 5.x (default)
or Elasticsearch 2.x (`mvn verify -Dit.ElasticsearchVersion=2`) should be started prior to running
the Elasticsearch tests.

This also means that tests have to ensure to run with either version of Elasticsearch or add an
assumption to disable the incompatible tests if the wrong version Elasticsearch is being used.

Example:

```java
assumeTrue(getElasticsearchVersion().getMajorVersion() == 2);
```
@@ -62,6 +62,14 @@
<maven.build.timestamp.format>yyyyMMddHHmmss</maven.build.timestamp.format>
<!-- to work around filtering bug, which makes maven.build.timestamp inaccessible -->
<build.timestamp>${maven.build.timestamp}</build.timestamp>

<it.es.httpPort>9200</it.es.httpPort>

This comment has been minimized.

@dennisoelkers

dennisoelkers Sep 11, 2017
Member

Using the standard port might interfere with an ES instance that is running locally. We should either use a fixed but different port or (even better) use a dynamically assigned port.

Jochen Schalanda added 3 commits Sep 12, 2017
To avoid socket collisions with locally running instances of Elasticsearch,
the integration tests now run on port 19200/tcp and 19300/tcp respectively.
@@ -57,6 +60,8 @@

public abstract class ElasticsearchBase {
private static final String PROPERTIES_RESOURCE_NAME = "elasticsearch.properties";
private static final String DEFAULT_PORT = "9200";

This comment has been minimized.

@dennisoelkers

dennisoelkers Sep 12, 2017
Member

Shouldn't it be 19200 now?

This comment has been minimized.

@joschi

joschi Sep 12, 2017
Author Contributor

No, because if you run the tests from the IDE, you usually want to use the default Elasticsearch HTTP port.

When the tests run in context of the integration-test phase of Maven, the property with the port of the Elasticsearch HTTP API of the "test" instance will be there and override the default.

This comment has been minimized.

@dennisoelkers dennisoelkers merged commit 90a0457 into master Sep 13, 2017
5 checks passed
5 checks passed
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 1893 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@garybot2
graylog-project/pr Jenkins build graylog-project-pr-snapshot 425 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@dennisoelkers dennisoelkers deleted the elasticsearch-tests branch Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants