[GitHub] lucene-solr issue #509: [SOLR-13019] Fix typo in MailEntityProcessor.java
Github user gerlowskija commented on the issue: https://github.com/apache/lucene-solr/pull/509 Thanks Tommy; merged to master. --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #464: SOLR-12555: refactor tests in package org.apache.sol...
Github user gerlowskija commented on the issue: https://github.com/apache/lucene-solr/pull/464 Thanks again for the work Bar. I've merged this to `master`. Should see this PR get closed out shortly. --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/464#discussion_r227202749
--- Diff: solr/core/src/test/org/apache/solr/search/TestRealTimeGet.java ---
@@ -414,56 +391,44 @@ public void testOptimisticLocking() throws Exception {
version2 = addAndGetVersion(sdoc("id","1", "_version_",
Long.toString(version)), null);
assertTrue(version2 > version);
-try {
- // overwriting the previous version should now fail
- version2 = addAndGetVersion(sdoc("id","1"), params("_version_",
Long.toString(version)));
- fail();
-} catch (SolrException se) {
- assertEquals(409, se.code());
-}
+// overwriting the previous version should now fail
+se = expectThrows(SolrException.class, "overwriting previous version
should fail",
+() -> addAndGetVersion(sdoc("id","1"), params("_version_",
Long.toString(version;
+assertEquals(409, se.code());
-try {
- // deleting the previous version should now fail
- version2 = deleteAndGetVersion("1", params("_version_",
Long.toString(version)));
- fail();
-} catch (SolrException se) {
- assertEquals(409, se.code());
-}
+// deleting the previous version should now fail
+se = expectThrows(SolrException.class, "deleting the previous version
should now fail",
+() -> deleteAndGetVersion("1", params("_version_",
Long.toString(version;
+assertEquals(409, se.code());
-version = version2;
+final long prevVersion = version2;
--- End diff --
Oof, yeah good point, that would be a problem. No need to revert. I'll
just need to pay particular attention to this class when testing things out.
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/464#discussion_r227030813
--- Diff: solr/core/src/test/org/apache/solr/search/TestRealTimeGet.java ---
@@ -414,56 +391,44 @@ public void testOptimisticLocking() throws Exception {
version2 = addAndGetVersion(sdoc("id","1", "_version_",
Long.toString(version)), null);
assertTrue(version2 > version);
-try {
- // overwriting the previous version should now fail
- version2 = addAndGetVersion(sdoc("id","1"), params("_version_",
Long.toString(version)));
- fail();
-} catch (SolrException se) {
- assertEquals(409, se.code());
-}
+// overwriting the previous version should now fail
+se = expectThrows(SolrException.class, "overwriting previous version
should fail",
+() -> addAndGetVersion(sdoc("id","1"), params("_version_",
Long.toString(version;
+assertEquals(409, se.code());
-try {
- // deleting the previous version should now fail
- version2 = deleteAndGetVersion("1", params("_version_",
Long.toString(version)));
- fail();
-} catch (SolrException se) {
- assertEquals(409, se.code());
-}
+// deleting the previous version should now fail
+se = expectThrows(SolrException.class, "deleting the previous version
should now fail",
+() -> deleteAndGetVersion("1", params("_version_",
Long.toString(version;
+assertEquals(409, se.code());
-version = version2;
+final long prevVersion = version2;
--- End diff --
[0] I'm still somewhat leery of changing how the version variables are used
here. I agree with what seems like your intent here - that `final` variables
often make it much easier to reason about Java code. But with how flaky the
tests are, I'd rather not introduce such changes here.
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/464#discussion_r227022552
--- Diff: solr/core/src/test/org/apache/solr/search/QueryEqualityTest.java
---
@@ -1214,29 +1208,23 @@ public void testPayloadScoreQuery() throws
Exception {
// I don't see a precedent to test query inequality in here, so doing
a `try`
// There was a bug with PayloadScoreQuery's .equals() method that said
two queries were equal with different includeSpanScore settings
-try {
- assertQueryEquals
- ("payload_score"
- , "{!payload_score f=foo_dpf v=query func=min
includeSpanScore=false}"
- , "{!payload_score f=foo_dpf v=query func=min
includeSpanScore=true}"
- );
- fail("queries should not have been equal");
-} catch(AssertionFailedError e) {
- assertTrue("queries were not equal, as expected", true);
-}
+expectThrows(AssertionFailedError.class, "queries were not equal, as
expected",
+() -> assertQueryEquals
+("payload_score"
+, "{!payload_score f=foo_dpf v=query func=min
includeSpanScore=false}"
+, "{!payload_score f=foo_dpf v=query func=min
includeSpanScore=true}"
+)
+);
}
public void testPayloadCheckQuery() throws Exception {
-try {
- assertQueryEquals
- ("payload_check"
- , "{!payload_check f=foo_dpf payloads=2}one"
- , "{!payload_check f=foo_dpf payloads=2}two"
- );
- fail("queries should not have been equal");
-} catch(AssertionFailedError e) {
- assertTrue("queries were not equal, as expected", true);
-}
+expectThrows(AssertionFailedError.class, "queries were not equal, as
expected",
--- End diff --
[-1] I think this exception message here is backwards. This assertion
fails if the queries _were_ equal, but the message indicates that the problem
is that they were !=. Using the message from the original `fail()` invocation
would probably work better here.
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/464#discussion_r227021582
--- Diff: solr/core/src/test/org/apache/solr/search/QueryEqualityTest.java
---
@@ -1190,14 +1190,8 @@ public void testCompares() throws Exception {
assertFuncEquals("gte(foo_i,2)", "gte(foo_i,2)");
assertFuncEquals("eq(foo_i,2)", "eq(foo_i,2)");
-boolean equals = false;
-try {
- assertFuncEquals("eq(foo_i,2)", "lt(foo_i,2)");
- equals = true;
-} catch (AssertionError e) {
- //expected
-}
-assertFalse(equals);
+expectThrows(AssertionError.class, "expected error, functions are not
equal",
--- End diff --
[0] Not suggesting you change it here, butkindof weird that there's
just not an `assertFuncNotEquals`
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/464#discussion_r227024512
--- Diff: solr/core/src/test/org/apache/solr/search/QueryEqualityTest.java
---
@@ -1260,16 +1248,14 @@ public void testBoolQuery() throws Exception {
"must='{!lucene}foo_s:c' filter='{!lucene}foo_s:d'
filter='{!lucene}foo_s:e'}",
"{!bool must='{!lucene}foo_s:c' filter='{!lucene}foo_s:d' " +
"must_not='{!lucene}foo_s:a' should='{!lucene}foo_s:b'
filter='{!lucene}foo_s:e'}");
-try {
- assertQueryEquals
- ("bool"
- , "{!bool must='{!lucene}foo_s:a'}"
- , "{!bool should='{!lucene}foo_s:a'}"
- );
- fail("queries should not have been equal");
-} catch(AssertionFailedError e) {
- assertTrue("queries were not equal, as expected", true);
-}
+
+expectThrows(AssertionFailedError.class, "queries were not equal, as
expected",
--- End diff --
[-1] ditto re: wrong String message in `expectThrows` here
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/464#discussion_r227024256
--- Diff: solr/core/src/test/org/apache/solr/search/QueryEqualityTest.java
---
@@ -1214,29 +1208,23 @@ public void testPayloadScoreQuery() throws
Exception {
// I don't see a precedent to test query inequality in here, so doing
a `try`
// There was a bug with PayloadScoreQuery's .equals() method that said
two queries were equal with different includeSpanScore settings
-try {
- assertQueryEquals
- ("payload_score"
- , "{!payload_score f=foo_dpf v=query func=min
includeSpanScore=false}"
- , "{!payload_score f=foo_dpf v=query func=min
includeSpanScore=true}"
- );
- fail("queries should not have been equal");
-} catch(AssertionFailedError e) {
- assertTrue("queries were not equal, as expected", true);
-}
+expectThrows(AssertionFailedError.class, "queries were not equal, as
expected",
--- End diff --
[-1] I think this exception message here is backwards. This assertion
fails if the queries _were_ equal, but the message implies that the problem is
that they were !=. Using the message from the original `fail()` invocation
would probably work better here ("queries should not have been equal")
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/464#discussion_r227025974
--- Diff:
solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java ---
@@ -656,45 +656,38 @@ public void testAliasingBoost() throws Exception {
public void testCyclicAliasing() throws Exception {
try {
ignoreException(".*Field aliases lead to a cycle.*");
- try {
-h.query(req("defType","edismax", "q","blarg", "qf","who",
"f.who.qf","name","f.name.qf","who"));
-fail("Simple cyclic alising not detected");
- } catch (SolrException e) {
-assertTrue(e.getCause().getMessage().contains("Field aliases lead
to a cycle"));
- }
-
- try {
-h.query(req("defType","edismax", "q","blarg", "qf","who",
"f.who.qf","name","f.name.qf","myalias", "f.myalias.qf","who"));
-fail("Cyclic alising not detected");
- } catch (SolrException e) {
-assertTrue(e.getCause().getMessage().contains("Field aliases lead
to a cycle"));
- }
-
+
+ SolrException e = expectThrows(SolrException.class, "Simple cyclic
alising not detected",
+ () -> h.query(req("defType","edismax", "q","blarg", "qf","who",
"f.who.qf","name","f.name.qf","who")));
+ assertCyclicDetectionErrorMessage(e);
+
+ e = expectThrows(SolrException.class, "Cyclic alising not detected",
+ () -> h.query(req("defType","edismax", "q","blarg", "qf","who",
"f.who.qf","name","f.name.qf","myalias", "f.myalias.qf","who")));
+ assertCyclicDetectionErrorMessage(e);
+
try {
h.query(req("defType","edismax", "q","blarg", "qf","field1",
"f.field1.qf","field2 field3","f.field2.qf","field4 field5",
"f.field4.qf","field5", "f.field5.qf","field6", "f.field3.qf","field6"));
- } catch (SolrException e) {
-assertFalse("This is not cyclic alising",
e.getCause().getMessage().contains("Field aliases lead to a cycle"));
-assertTrue(e.getCause().getMessage().contains("not a valid field
name"));
- }
-
- try {
-h.query(req("defType","edismax", "q","blarg", "qf","field1",
"f.field1.qf","field2 field3", "f.field2.qf","field4 field5",
"f.field4.qf","field5", "f.field5.qf","field4"));
-fail("Cyclic alising not detected");
- } catch (SolrException e) {
-assertTrue(e.getCause().getMessage().contains("Field aliases lead
to a cycle"));
- }
-
- try {
-h.query(req("defType","edismax", "q","who:(Zapp Pig)",
"qf","text", "f.who.qf","name","f.name.qf","myalias", "f.myalias.qf","who"));
-fail("Cyclic alising not detected");
- } catch (SolrException e) {
-assertTrue(e.getCause().getMessage().contains("Field aliases lead
to a cycle"));
+ } catch (SolrException ex) {
--- End diff --
[Q] Is there a reason that this example also couldn't be changed into an
`expectThrows`?
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #425: WIP SOLR-12555: refactor tests to use expectThrows
Github user gerlowskija commented on the issue: https://github.com/apache/lucene-solr/pull/425 Thanks for all the work Bar. I'm going to run the tests throughout the day and I hope to get this merged this evening or this weekend. --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r204599561
--- Diff: solr/core/src/test/org/apache/solr/cloud/OverseerStatusTest.java
---
@@ -58,14 +59,13 @@ public void test() throws Exception {
SimpleOrderedMap reload = (SimpleOrderedMap)
collection_operations.get(CollectionParams.CollectionAction.RELOAD.toLower());
assertEquals("No stats for reload in OverseerCollectionProcessor", 1,
reload.get("requests"));
-try {
- CollectionAdminRequest.splitShard("non_existent_collection")
- .setShardName("non_existent_shard")
- .process(cluster.getSolrClient());
- fail("Split shard for non existent collection should have failed");
-} catch (Exception e) {
- // expected because we did not correctly specify required params for
split
-}
+HttpSolrClient.RemoteSolrException e =
expectThrows(HttpSolrClient.RemoteSolrException.class, () ->
CollectionAdminRequest
+.splitShard("non_existent_collection")
+.setShardName("non_existent_shard")
+.process(cluster.getSolrClient())
+);
+assertTrue("expected failure to be caused by non existent collection",
e.getMessage().contains("Could not find collection"));
--- End diff --
[-1] I agree with your intention here (it's a good assertion to add), but
I'd rather keep non-refactor changes out of this PR. Just in the interest of
changing the test-behavior as little as possible for this refactor JIRA. Would
you mind taking out this assertion?
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r204601997
--- Diff:
solr/core/src/test/org/apache/solr/cloud/TestSolrCloudWithSecureImpersonation.java
---
@@ -223,37 +223,26 @@ private String getExpectedHostExMsg(String user) {
@Test
public void testProxyNoConfigGroups() throws Exception {
-try {
- solrClient.request(getProxyRequest("noGroups","bar"));
- fail("Expected RemoteSolrException");
-}
-catch (HttpSolrClient.RemoteSolrException ex) {
-
assertTrue(ex.getMessage().contains(getExpectedGroupExMsg("noGroups", "bar")));
-}
+HttpSolrClient.RemoteSolrException e =
expectThrows(HttpSolrClient.RemoteSolrException.class,
+() -> solrClient.request(getProxyRequest("noGroups","bar"))
+);
+assertTrue(e.getMessage().contains(getExpectedGroupExMsg("noGroups",
"bar")));
}
@Test
public void testProxyWrongHost() throws Exception {
-try {
- solrClient.request(getProxyRequest("wrongHost","bar"));
- fail("Expected RemoteSolrException");
-}
-catch (HttpSolrClient.RemoteSolrException ex) {
-
assertTrue(ex.getMessage().contains(getExpectedHostExMsg("wrongHost")));
-}
+HttpSolrClient.RemoteSolrException e =
expectThrows(HttpSolrClient.RemoteSolrException.class,
+() -> solrClient.request(getProxyRequest("wrongHost","bar"))
+);
+assertTrue(e.getMessage().contains(getExpectedHostExMsg("wrongHost")));
}
@Test
public void testProxyNoConfigHosts() throws Exception {
-try {
- solrClient.request(getProxyRequest("noHosts","bar"));
- fail("Expected RemoteSolrException");
-}
-catch (HttpSolrClient.RemoteSolrException ex) {
- // FixMe: this should return an exception about the host being
invalid,
- // but a bug (HADOOP-11077) causes an NPE instead.
-
//assertTrue(ex.getMessage().contains(getExpectedHostExMsg("noHosts")));
-}
+HttpSolrClient.RemoteSolrException e =
expectThrows(HttpSolrClient.RemoteSolrException.class,
+() -> solrClient.request(getProxyRequest("noHosts","bar"))
+);
+assertTrue(e.getMessage().contains(getExpectedHostExMsg("noHosts")));
--- End diff --
[-1] I'd prefer this assertion remain commented out (and the attached
comment stick around as well).
I'm really paranoid about introducing actual test changes in with the
refactor-only JIRA. If you'd like to see it fixed, I'm happy to review it as a
part of a different JIRA/PR. But I'd rather not have the two mix.
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r204597595
--- Diff: solr/core/src/test/org/apache/solr/TestDistributedSearch.java ---
@@ -910,13 +910,11 @@ public void test() throws Exception {
//SOLR 3161 ensure shards.qt=/update fails (anything but search
handler really)
// Also see TestRemoteStreaming#testQtUpdateFails()
-try {
- ignoreException("isShard is only acceptable");
- //
query("q","*:*","shards.qt","/update","stream.body","*:*");
- // fail();
-} catch (SolrException e) {
- //expected
-}
+
--- End diff --
[0] Did you intend to leave this commented out? (I know this PR is a WIP,
so totally possible you're aware of this already. Just wanted to mention it
in-case).
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r204600183
--- Diff:
solr/core/src/test/org/apache/solr/cloud/TestAuthenticationFramework.java ---
@@ -78,17 +79,11 @@ public void testBasics() throws Exception {
MockAuthenticationPlugin.expectedPassword = "s0lrRocks";
// Should fail with 401
-try {
- collectionCreateSearchDeleteTwice();
- fail("Should've returned a 401 error");
-} catch (Exception ex) {
- if (!ex.getMessage().contains("Error 401")) {
-fail("Should've returned a 401 error");
- }
-} finally {
- MockAuthenticationPlugin.expectedUsername = null;
- MockAuthenticationPlugin.expectedPassword = null;
-}
+HttpSolrClient.RemoteSolrException e =
expectThrows(HttpSolrClient.RemoteSolrException.class,
+this::collectionCreateSearchDeleteTwice);
+assertTrue("Should've returned a 401 error",
e.getMessage().contains("Error 401"));
+MockAuthenticationPlugin.expectedUsername = null;
--- End diff --
[-1] I think we need to keep the `MockAuthenticationPlugin` calls within a
"finally"-block. See the related comment in SolrXmlInZkTest.java above.
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r20468
--- 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
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r204601140
--- Diff:
solr/core/src/test/org/apache/solr/cloud/TestPullReplicaErrorHandling.java ---
@@ -151,12 +151,14 @@ public void testCantConnectToPullReplica() throws
Exception {
assertNumDocs(10 + i, leaderClient);
}
}
- try (HttpSolrClient pullReplicaClient =
getHttpSolrClient(s.getReplicas(EnumSet.of(Replica.Type.PULL)).get(0).getCoreUrl()))
{
-pullReplicaClient.query(new
SolrQuery("*:*")).getResults().getNumFound();
-fail("Shouldn't be able to query the pull replica");
- } catch (SolrServerException e) {
-//expected
- }
+
+ SolrServerException e = expectThrows(SolrServerException.class, ()
-> {
+try(HttpSolrClient pullReplicaClient =
getHttpSolrClient(s.getReplicas(EnumSet.of(Replica.Type.PULL)).get(0).getCoreUrl()))
{
+ pullReplicaClient.query(new
SolrQuery("*:*")).getResults().getNumFound();
+}
+ });
+ assertTrue("Shouldn't be able to query the pull replica",
e.getMessage().contains("refused connection"));
--- End diff --
[-1] I like the idea, and agree this is a good assertion to add, but I'd
rather not add it in this PR since it's intended to be a pure refactor. Would
you mind removing this assertion?
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r204598388
--- Diff: solr/core/src/test/org/apache/solr/cloud/BasicZkTest.java ---
@@ -150,15 +150,13 @@ public void testBasic() throws Exception {
zkController.getZkClient().setData("/configs/conf1/solrconfig.xml",
new byte[0], true);
// we set the solrconfig to nothing, so this reload should fail
-try {
+SolrException e = expectThrows(SolrException.class, () -> {
ignoreException("solrconfig.xml");
h.getCoreContainer().reload(h.getCore().getName());
- fail("The reloaded SolrCore did not pick up configs from zookeeper");
-} catch(SolrException e) {
- resetExceptionIgnores();
- assertTrue(e.getMessage().contains("Unable to reload core
[collection1]"));
- assertTrue(e.getCause().getMessage().contains("Error loading solr
config from solrconfig.xml"));
-}
+});
+resetExceptionIgnores();
--- End diff --
[0] This isn't behavior you introduced, but I would've expected this to be
in a `finally` block.
As I read this, if an IOException or anything else gets triggered by
reloading the core, our exception-ignoring will never be reset.
I'm not asking you to change anything here. Just musing aloud. Will look
into it.
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Github user gerlowskija commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r204596268
--- Diff: solr/core/src/test/org/apache/solr/cloud/BasicZkTest.java ---
@@ -150,15 +150,13 @@ public void testBasic() throws Exception {
zkController.getZkClient().setData("/configs/conf1/solrconfig.xml",
new byte[0], true);
// we set the solrconfig to nothing, so this reload should fail
-try {
+SolrException e = expectThrows(SolrException.class, () -> {
ignoreException("solrconfig.xml");
h.getCoreContainer().reload(h.getCore().getName());
- fail("The reloaded SolrCore did not pick up configs from zookeeper");
--- End diff --
One downside I've noticed in moving towards the `expectThrows` pattern is
that we lose a lot of the nice error messages that would appear if no exception
was thrown at all.
To help with this, I added another version of `expectThrows`, which takes
an additional argument: a String message to display if no exception was thrown.
(Link to that new method here:
https://github.com/apache/lucene-solr/blob/master/lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java#L2672)
If you're revising this PR (I noticed you marked it as WIP), consider using
that version of `expectThrows` where it makes sense, to preserve the better
error messages.
---
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
