Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r204600008
--- Diff: solr/core/src/test/org/apache/solr/cloud/SolrXmlInZkTest.java ---
@@ -139,16 +139,13 @@ public void testNotInZkFallbackLocal() throws
Exception {
@Test
public void testNotInZkOrOnDisk() throws Exception {
- try {
+ SolrException e = expectThrows(SolrException.class, () -> {
--- End diff --
[-1] I think the `closeZK()` here should stay in a finally, to make sure it
executes even if other unexpected exceptions pop up.
So it'd have to look something like:
```
try {
SolrException e = expectThrows(SolrException.class, () -> {
...
}
assertTrue(.....);
} finally {
closeZk();
}
```
It is a little weird to have both the try-finally, and the expectThrows.
If you think it's not even worth it to add in the expectThrows in a case like
this, I could see that argument. I could go either way on it really. Our hands
are tied unfortunately, as we've gotta have that finally-block though.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
