[5162] Add elasticsearch tests #5170
Conversation
llowinge
force-pushed
the
5162-add-elasticsearch-tests
branch
2 times, most recently
from
0a9e06d
to
c8e5feb
Compare
|
@jamesnetherton Can you please review ? |
|
|
||
| @SuppressWarnings("unused") | ||
| private static String[] componentNames() { | ||
| return new String[] { "elasticsearch" }; |
There was a problem hiding this comment.
In theory this is not required and we could just hard code the component name (using to instead of toD).
Or is it worth keeping in case we do eventually want to test the component with the Quarkus managed ES client?
There was a problem hiding this comment.
I left it there for the Quarkus managed ES client. But i agree it is not useful now at all. Maybe i should add comment there why it is done like that to that method ?
There was a problem hiding this comment.
Yes, please add a comment.
There was a problem hiding this comment.
I left comment, i hope it is OK.
llowinge
force-pushed
the
5162-add-elasticsearch-tests
branch
from
c8e5feb
to
5cf8578
Compare
|
LGTM. Do we need tests to cover things like |
We could maybe follow up with that. These initial tests are mostly a reproduction of what we had for the previous |
|
Yes, that's ok in this state. Plus if we promote it to native one day, then we could update the test coverage at this stage YAGNI style. |
|
@aldettinger That is great idea, but given it was only for JVM, i think we are safe there. Basically all the tests are probably overkill, but i've ported all of them for future benefits. I've created followup issue for the SSL #5174. |
|
Not sure, it's overkill. Those features are present in the camel component doc, so this is good to have them already covered in this PR 👍 And yes, ssl features could be in future. |
Covers: