You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by Chris Hostetter <ho...@fucit.org> on 2018/07/06 16:58:40 UTC
Re: lucene-solr:master: SOLR-12427: Correct status for invalid
'start', 'rows'
these tests should really be using...
SolrException e = expectThrows(() -> {...});
...and ideally we should be making assertions about the exception message
as well (ie: does it say what we expect it to say? does it give the user
the context of the failure -- ie: containing the "non_numeric_value" so
they know what they did wrong?
: private void validateCommonQueryParameters() throws Exception {
: ignoreException("parameter cannot be negative");
: +
: + try {
: + SolrQuery query = new SolrQuery();
: + query.setParam("start", "non_numeric_value").setQuery("*");
: + QueryResponse resp = query(query);
: + fail("Expected the last query to fail, but got response: " + resp);
: + } catch (SolrException e) {
: + assertEquals(ErrorCode.BAD_REQUEST.code, e.code());
: + }
: +
: try {
: SolrQuery query = new SolrQuery();
: query.setStart(-1).setQuery("*");
: @@ -1228,6 +1238,15 @@ public class TestDistributedSearch extends BaseDistributedSearchTestCase {
: } catch (SolrException e) {
: assertEquals(ErrorCode.BAD_REQUEST.code, e.code());
: }
: +
: + try {
: + SolrQuery query = new SolrQuery();
: + query.setParam("rows", "non_numeric_value").setQuery("*");
: + QueryResponse resp = query(query);
: + fail("Expected the last query to fail, but got response: " + resp);
: + } catch (SolrException e) {
: + assertEquals(ErrorCode.BAD_REQUEST.code, e.code());
: + }
: resetExceptionIgnores();
: }
: }
:
:
-Hoss
http://www.lucidworks.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: lucene-solr:master: SOLR-12427: Correct status for invalid
'start', 'rows'
Posted by Erick Erickson <er...@gmail.com>.
bq. " Having this elsewhere in the code encourages it to creep in more."
+1. While I hesitate to make lots of changes that are unnecessary, the
other side of that argument is that when we see code its easy to think
it's the norm rather than old....
On Mon, Jul 9, 2018 at 4:52 AM, Jason Gerlowski <ge...@gmail.com> wrote:
> I authored the recent change you're commenting on. I agree with your
> points; my only defense is consistency. Several other nearby
> assertions used the older try-catch based setup.
>
> I'll fix the spot you objected to, and file a JIRA around cleaning
> this up more broadly. Having this elsewhere in the code encourages it
> to creep in more.
>
> Best,
>
> Jason
> On Fri, Jul 6, 2018 at 12:58 PM Chris Hostetter
> <ho...@fucit.org> wrote:
>>
>>
>> these tests should really be using...
>>
>> SolrException e = expectThrows(() -> {...});
>>
>> ...and ideally we should be making assertions about the exception message
>> as well (ie: does it say what we expect it to say? does it give the user
>> the context of the failure -- ie: containing the "non_numeric_value" so
>> they know what they did wrong?
>>
>>
>> : private void validateCommonQueryParameters() throws Exception {
>> : ignoreException("parameter cannot be negative");
>> : +
>> : + try {
>> : + SolrQuery query = new SolrQuery();
>> : + query.setParam("start", "non_numeric_value").setQuery("*");
>> : + QueryResponse resp = query(query);
>> : + fail("Expected the last query to fail, but got response: " + resp);
>> : + } catch (SolrException e) {
>> : + assertEquals(ErrorCode.BAD_REQUEST.code, e.code());
>> : + }
>> : +
>> : try {
>> : SolrQuery query = new SolrQuery();
>> : query.setStart(-1).setQuery("*");
>> : @@ -1228,6 +1238,15 @@ public class TestDistributedSearch extends BaseDistributedSearchTestCase {
>> : } catch (SolrException e) {
>> : assertEquals(ErrorCode.BAD_REQUEST.code, e.code());
>> : }
>> : +
>> : + try {
>> : + SolrQuery query = new SolrQuery();
>> : + query.setParam("rows", "non_numeric_value").setQuery("*");
>> : + QueryResponse resp = query(query);
>> : + fail("Expected the last query to fail, but got response: " + resp);
>> : + } catch (SolrException e) {
>> : + assertEquals(ErrorCode.BAD_REQUEST.code, e.code());
>> : + }
>> : resetExceptionIgnores();
>> : }
>> : }
>> :
>> :
>>
>> -Hoss
>> http://www.lucidworks.com/
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>> For additional commands, e-mail: dev-help@lucene.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: lucene-solr:master: SOLR-12427: Correct status for invalid
'start', 'rows'
Posted by Jason Gerlowski <ge...@gmail.com>.
I authored the recent change you're commenting on. I agree with your
points; my only defense is consistency. Several other nearby
assertions used the older try-catch based setup.
I'll fix the spot you objected to, and file a JIRA around cleaning
this up more broadly. Having this elsewhere in the code encourages it
to creep in more.
Best,
Jason
On Fri, Jul 6, 2018 at 12:58 PM Chris Hostetter
<ho...@fucit.org> wrote:
>
>
> these tests should really be using...
>
> SolrException e = expectThrows(() -> {...});
>
> ...and ideally we should be making assertions about the exception message
> as well (ie: does it say what we expect it to say? does it give the user
> the context of the failure -- ie: containing the "non_numeric_value" so
> they know what they did wrong?
>
>
> : private void validateCommonQueryParameters() throws Exception {
> : ignoreException("parameter cannot be negative");
> : +
> : + try {
> : + SolrQuery query = new SolrQuery();
> : + query.setParam("start", "non_numeric_value").setQuery("*");
> : + QueryResponse resp = query(query);
> : + fail("Expected the last query to fail, but got response: " + resp);
> : + } catch (SolrException e) {
> : + assertEquals(ErrorCode.BAD_REQUEST.code, e.code());
> : + }
> : +
> : try {
> : SolrQuery query = new SolrQuery();
> : query.setStart(-1).setQuery("*");
> : @@ -1228,6 +1238,15 @@ public class TestDistributedSearch extends BaseDistributedSearchTestCase {
> : } catch (SolrException e) {
> : assertEquals(ErrorCode.BAD_REQUEST.code, e.code());
> : }
> : +
> : + try {
> : + SolrQuery query = new SolrQuery();
> : + query.setParam("rows", "non_numeric_value").setQuery("*");
> : + QueryResponse resp = query(query);
> : + fail("Expected the last query to fail, but got response: " + resp);
> : + } catch (SolrException e) {
> : + assertEquals(ErrorCode.BAD_REQUEST.code, e.code());
> : + }
> : resetExceptionIgnores();
> : }
> : }
> :
> :
>
> -Hoss
> http://www.lucidworks.com/
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org