This commit introduces the @DataElasticSearchTest annotation.
See: gh-8544
Comment From: philwebb
@snicoll did you mean to put this in the 1.5.x bucket?
Comment From: snicoll
Wrong call, should be 2.0 I suppose.
Comment From: philwebb
It's tempting to try an sneak it into 1.5.x, but 2.0 is better I think.
Comment From: snicoll
OK I've polished the PR (see my branch). @mdeinum could you please configure your IDE to at least use tabs?
So there is one thing I don't like is that the tests create an index and I really don't want that. Discussing with @wilkinsona we think it should probably default to something else with a property to specify it maybe.
If someone wants to work on that, feel free to submit a PR on my fork.
Comment From: snicoll
@philwebb I tried to implement this and it looks like a lot of work. On DataElasticSearchTest we could have a property that specifies if we should attempt to relocate the data directory. We would probably need an extension of SpringBootTestContextBootstrapper to look for the annotation and create a PropertySource with a sensible precedence.
It's not very clear what we should do. Detect a target or build directory and create a sub-directory there? This feels a bit magic to me.
Comment From: philwebb
@snicoll What's creating the data directory? Is it something in ElasticsearchAutoConfiguration? I'm not sure I'm following what needs to happen with the SpringBootTestContextBootstrapper and additional property source. Is it because we need to dynamically detect the directory based on the target/build folder?
Comment From: snicoll
@philwebb our auto-configuration defaults to the current work directory.
So, yes. We need to add a PropertySource where that property is set with a sensible directory. I might argue you probably want a property on the annotation to be able to tune it (and that should create another context if it's changed).
Comment From: philwebb
@snicoll What if we offered some kind of ElasticsearchPropertiesPostProcessor or some plugable strategy that's specific to ElasticsearchAutoConfiguration. That would reduce the problem to register a bean and read some mapped property, hopefully saving the need for any SpringBootTestContextBootstrapper changes.
Comment From: snicoll
Perhaps the main auto-configuration should change then. Two things:
- Nobody wants the indexes generated at the same level of their build file I suppose
- There are cases where you want a fresh index and cases where you don't want to repeat the initialization of the index for every test class
if that was a feature of the auto-configuration, the test auto-config would just flip a switch.
Comment From: mdeinum
In our specific situation, as we used gradle, we put the index in the build directory and recreate before every test. But as that isn't very portable that won't work here. We basically set the path.home using spring.data.elasticsearch.properties.path.home this will then override the default. At least it worked for us.
For testing this could, maybe, be put in the java.io.tmpdir or a sub directory in there instead of the current user.dir?
Could a specific TestExecutionListener be used to override/set the property?
Comment From: snicoll
recreate before every test.
How do you do that?
Comment From: mdeinum
We basically used the sledge hammer approach and tapped into the dirties context support much like the ServletTestExecutionListener. It was better then the approach we had.
Would be nice if we could have something along the lines of @AutoConfigureTestDatabase and replace the client instead of the whole context.
Comment From: snicoll
I am not following. What do you mean by "context"?
Comment From: mdeinum
My bad. When using the dirties context approach a new ApplicationContext is created, which is what I meant with context.
Would it make sense to create an @AutoConfigureTestElasticSearch with a separate namespace (like the @AutoConfigureTestDatabase which could be preconfigured to use a different default for the path.home? and maybe some other defaults to optimize an ElasticSearch instance for testing? maybe with reusing parts of the current ElasticSearchAutoConfiguration?
Comment From: snicoll
@mdeinum you're the one submitting this PR so if you know good defaults, by all means share them.
Comment From: snicoll
@mdeinum are you going to refine your PR with what we've discussed here?
Comment From: wilkinsona
In Elasticsearch 5 they have dropped official support for running embedded Elasticsearch. They do, however, provide EsIntegTestCase. Unfortunately, it's very picky about what's on the classpath and tests fail to launch if it finds any duplicate classes. For example it pulls in org.elasticsearc:securemock which contains lots of Mockito classes. This then prevents the use of Mockito itself within the same project. Even with lots of exclusions in place to tune the classpath, it still couldn't run a unit test in Eclipse as the check found a duplicate internal Eclipse class.
The alternative is to roll our own code to bootstrap an Elasticsearch node as part of @DataElasticsearchTest that skips the jar hell check. That would probably be sufficient for integration testing purposes.
Comment From: mdeinum
That is too bad :( . I'm currently a bit swamped in work and finishing a book. If all goes well I probably have time next week to take another look at this.
Could we take some inspiration from the EsIntegTestCase (this is what I initially did for the 2.x version).
Comment From: wilkinsona
It looks like this has been scuppered by the current situation with embedding Elasticsearch and the severe restrictions of EsIntegTestCase. It doesn't look like that situation is going to change in the foreseeable future so, with some regret, I'm going to close this. Thanks anyway, @mdeinum. If another approach comes to light, we can resurrect this PR.
Comment From: hantsy
Oh, this PR finally missed merging into Spring Boot?
Comment From: wilkinsona
Yes. Please see the various comments above for the reasons why it could not be merged.