Feature/elastic back in green #893
Conversation
There was a problem hiding this comment.
Pull request overview
This PR re-enables Elasticsearch integration tests by updating test infrastructure, creating a new test configuration class for Elasticsearch clients, and updating dependencies. The changes address test failures and remove the @Disabled annotation from the test class.
- Created
TestElasticsearchClientConfigto provide Elasticsearch client beans for tests - Updated
ElasticsearchLastNR4ITto use embedded Elasticsearch container with proper initialization - Refactored
ElasticsearchBootSvcImplto use consistent naming for the Elasticsearch client field - Updated Testcontainers dependencies with explicit version declarations
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/test/java/ca/uhn/fhir/jpa/starter/elastic/TestElasticsearchClientConfig.java | New test configuration class for creating and managing Elasticsearch client beans with authentication support |
| src/test/java/ca/uhn/fhir/jpa/starter/ElasticsearchLastNR4IT.java | Updated test class with embedded Elasticsearch container initialization, ngram configuration, and removed @disabled annotation |
| src/main/java/ca/uhn/fhir/jpa/starter/elastic/ElasticsearchBootSvcImpl.java | Renamed private field from myRestHighLevelClient to elasticsearchClient for consistency |
| pom.xml | Updated Testcontainers dependencies with explicit versions and modified artifact IDs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/test/java/ca/uhn/fhir/jpa/starter/ElasticsearchLastNR4IT.java
Outdated
Show resolved
Hide resolved
src/test/java/ca/uhn/fhir/jpa/starter/elastic/TestElasticsearchClientConfig.java
Outdated
Show resolved
Hide resolved
src/test/java/ca/uhn/fhir/jpa/starter/ElasticsearchLastNR4IT.java
Outdated
Show resolved
Hide resolved
src/test/java/ca/uhn/fhir/jpa/starter/elastic/TestElasticsearchClientConfig.java
Outdated
Show resolved
Hide resolved
|
@XcrigX will you have a look? |
| private FhirContext ourCtx; | ||
|
|
||
| @Container | ||
| public static ElasticsearchContainer embeddedElastic = TestElasticsearchContainerHelper.getEmbeddedElasticSearch(); |
There was a problem hiding this comment.
What happened to these helper classes? I do not find them indexed in my IDE when testing locally.
Is there an associated change in HAPI or otherwise that is related to this?
There was a problem hiding this comment.
It's here: https://github.com/hapifhir/hapi-fhir/blob/master/hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/test/config/TestElasticsearchContainerHelper.java - but its locked to 7.17.3 which is about to go EOL (https://www.elastic.co/support/eol) - so I locally updated it to 8.x.x
There was a problem hiding this comment.
and now I've reverted back to 7.17.3
There was a problem hiding this comment.
I had a discussion with @tadgh about this earlier in the week, and we agreed it would be appropriate to update TestElasticsearchContainerHelper to the latest 8.x.x version.
I'm setting up a branch for that right now, and I'll see if these two will work together.
There was a problem hiding this comment.
A bit premature probably but what about 9.x or switching to opensearch ?
There was a problem hiding this comment.
A bit premature probably but what about 9.x or switching to opensearch ?
2 participants
No description provided.