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/10/07 18:57:14 UTC
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests for package or...
GitHub user barrotsteindev opened a pull request:
https://github.com/apache/lucene-solr/pull/464
WIP SOLR-12555: refactor tests for package org.apache.solr.search
Tests are refactored to use expectThrows instead of try/catch.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/barrotsteindev/lucene-solr 12555-2
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/lucene-solr/pull/464.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 #464
----
commit 2a5071e4ae7f55a16e0be43b41331b9ef641041f
Author: Bar Rotstein <ba...@...>
Date: 2018-10-07T18:55:33Z
SOLR-12555 refactor tests for package org.apache.solr.search
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
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/464#discussion_r227038483
--- 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 --
The only problem is that if the variables are not final, they can not be used inside the lambda that is passed to expectThrows.
Should I just revert these 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 #464: WIP SOLR-12555: refactor tests in package org...
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/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, but....kindof weird that there's just not an `assertFuncNotEquals`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
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/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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #464: SOLR-12555: refactor tests in package org.apache.sol...
Posted by gerlowskija <gi...@git.apache.org>.
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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
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/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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
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/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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
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/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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
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/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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #464: SOLR-12555: refactor tests in package org.apa...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/lucene-solr/pull/464
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
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/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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #464: WIP SOLR-12555: refactor tests in package org...
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/464#discussion_r230120449
--- 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 --
My bad,
I sort of missed that one.
Just pushed a new commit fixing it though.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org