[CALCITE-3601] Update elasticsearch tests upgrade from junit4 to junit5#1681
[CALCITE-3601] Update elasticsearch tests upgrade from junit4 to junit5#1681XuQianJin-Stars wants to merge 4 commits intoapache:masterfrom
Conversation
| * </pre> | ||
| * | ||
| * @param index index of the index | ||
| * @param index index of the index |
There was a problem hiding this comment.
Please refrain from aligning parameter descriptions.
It creates lots of unrelated changes, and it is prone to merge conflicts in the future.
See sample reasoning (~ it creates problems for future maintenance ): https://google.github.io/styleguide/javaguide.html#s4.6.3-horizontal-alignment
There was a problem hiding this comment.
Please refrain from aligning parameter descriptions.
It creates lots of unrelated changes, and it is prone to merge conflicts in the future.See sample reasoning (~
it creates problems for future maintenance): https://google.github.io/styleguide/javaguide.html#s4.6.3-horizontal-alignment
well, Let me change it. There is something wrong with my code style.
|
It turned out to be simpler than I initially thought. I don't quite like that the PR includes javadoc re-formats for |
| return node.httpAddress(); | ||
| } | ||
|
|
||
| @Override public void afterAll(ExtensionContext context) throws Exception { |
There was a problem hiding this comment.
Just a small note: could you please move the methods to the location of previous before and after?
There was a problem hiding this comment.
Well , Ok Let me change it. In addition, can we add style to the calite project to define the code format in calite?
There was a problem hiding this comment.
What do you mean by "code format"?
I'm inclined that we should prefer https://github.com/apache/calcite/blob/master/.editorconfig when possible.
There was a problem hiding this comment.
PS. please move afterAll as well
There was a problem hiding this comment.
hi @vlsi Is your original idea to implement declarative registration extensions and kotlin?
There was a problem hiding this comment.
No, I just had no idea how to approach the test.
The approach you use seems to be nice. I have not checked TestContainer best practices yet. They have a case for "keeping test container up for multiple test classes", so there might be something to learn from.
There was a problem hiding this comment.
No, I just had no idea how to approach the test.
The approach you use seems to be nice. I have not checked TestContainer best practices yet. They have a case for "keeping test container up for multiple test classes", so there might be something to learn from.
I am also interested in learning about "keeping test container up for multiple test classes".
There was a problem hiding this comment.
It looks like JUnit provides no solution yet, so TestContainers use their own: https://github.com/testcontainers/testcontainers-java/pull/887/files#diff-0dcf4d2cadeb3e526020477bc12e1420R73
There was a problem hiding this comment.
It looks like JUnit provides no solution yet, so TestContainers use their own: https://github.com/testcontainers/testcontainers-java/pull/887/files#diff-0dcf4d2cadeb3e526020477bc12e1420R73
Well, indeed, TestContainers also looks good. Thank you very much for letting me learn something new.
…t5 (Qianjin Xu) closes apache#1681
…t5 (Qianjin Xu) closes apache#1681
…t5 (Qianjin Xu) closes apache#1681
2 participants
As illustrated in CALCITE-3601
Update
elasticsearchtests upgrade from junit4 to junit5.