You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by barrotsteindev <gi...@git.apache.org> on 2018/07/23 03:41:35 UTC
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
GitHub user barrotsteindev opened a pull request:
https://github.com/apache/lucene-solr/pull/425
WIP SOLR-12555: refactor tests to use expectThrows
Refactor tests to use expectThrows instead of the old try catch method.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/barrotsteindev/lucene-solr SOLR-12555
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/lucene-solr/pull/425.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #425
----
commit ff89a0ecae1424254b6f19fd3d227d77a9398aa2
Author: Bar Rotstein <ba...@...>
Date: 2018-07-23T03:40:07Z
SOLR-12555: refactor tests to use expectThrows in solr/core/src/test/org/apache/solr/cloud and solr/core/src/test/org/apache/solr
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by gerlowskija <gi...@git.apache.org>.
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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #425: WIP SOLR-12555: refactor tests to use expectThrows
Posted by gerlowskija <gi...@git.apache.org>.
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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by gerlowskija <gi...@git.apache.org>.
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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by gerlowskija <gi...@git.apache.org>.
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","<delete><query>*:*</query></delete>");
- // 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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #425: WIP SOLR-12555: refactor tests to use expectThrows
Posted by barrotsteindev <gi...@git.apache.org>.
Github user barrotsteindev commented on the issue:
https://github.com/apache/lucene-solr/pull/425
Hopefully this is satisfactory after the last commit changes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/lucene-solr/pull/425
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by barrotsteindev <gi...@git.apache.org>.
Github user barrotsteindev commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r204858823
--- 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","<delete><query>*:*</query></delete>");
- // fail();
- } catch (SolrException e) {
- //expected
- }
+
--- End diff --
Yes, since it was commented out beforehand, I just changed the test to use the new syntax, but I figured there was a reason it was commented out.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by gerlowskija <gi...@git.apache.org>.
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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by gerlowskija <gi...@git.apache.org>.
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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by gerlowskija <gi...@git.apache.org>.
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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by gerlowskija <gi...@git.apache.org>.
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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by gerlowskija <gi...@git.apache.org>.
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<Object> reload = (SimpleOrderedMap<Object>) 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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by barrotsteindev <gi...@git.apache.org>.
Github user barrotsteindev commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r204860462
--- 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 --
Perhaps we should try and change it, though we have to make sure some other tests do not depend on this bug.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by barrotsteindev <gi...@git.apache.org>.
Github user barrotsteindev commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r204858410
--- 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","<delete><query>*:*</query></delete>");
- // fail();
- } catch (SolrException e) {
- //expected
- }
+
--- End diff --
Oops my bad, I'll remove it ASAP
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #425: WIP SOLR-12555: refactor tests to use expectT...
Posted by barrotsteindev <gi...@git.apache.org>.
Github user barrotsteindev commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/425#discussion_r204862363
--- Diff: solr/core/src/test/org/apache/solr/cloud/OverseerStatusTest.java ---
@@ -58,14 +59,13 @@ public void test() throws Exception {
SimpleOrderedMap<Object> reload = (SimpleOrderedMap<Object>) 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 --
Sure thing
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org