lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Uwe Schindler" <...@thetaphi.de>
Subject RE: svn commit: r1697131 - in /lucene/dev/trunk/lucene: JRE_VERSION_MIGRATION.txt test-framework/src/java/org/apache/lucene/util/TestUtil.java
Date Wed, 26 Aug 2015 23:31:59 GMT
Hi Hoss,

sorry did not see your message!

1) The test framework never randomly disables asserts (it cannot do this, because asserts
are enabled before the VM starts - you cannot do that at runtime). So to disable asserts you
have to pass this explicitly to the test runner, and the default is asserts enabled. Currently
Policeman Jenkins does not disable asserts at the moment, but it could do that. For normal
developers running tests, they are always enabled (we also have a test that validates this).
In addition, our tests should not check the JVM for correctness, it is just there to make
an assumption about how WS should behave. So when this assert fails, the test is still not
wrong.

2) Now the answer: I changed it because of performance. The String.format / String concatenation
is executed on every single character and if the assert does not fail. If you create a large
whitespace string this takes endless (I tried it), because it creates a new string, formats
it,... A native Java assert only calls the string part if the assert actually failed.

Based on (1) and (2) this is OK. If you still want to revert to Assert.assertTrue like before,
please use an if-condition instead:
	if (not whitespace) Assert.fail(String.format(error message));

Uwe

-----
Uwe Schindler
H.-H.-Meier-Allee 63, D-28213 Bremen
http://www.thetaphi.de
eMail: uwe@thetaphi.de

> -----Original Message-----
> From: Chris Hostetter [mailto:hossman_lucene@fucit.org]
> Sent: Thursday, August 27, 2015 12:15 AM
> To: uwe@thetaphi.de; Lucene Dev
> Subject: Re: svn commit: r1697131 - in /lucene/dev/trunk/lucene:
> JRE_VERSION_MIGRATION.txt test-
> framework/src/java/org/apache/lucene/util/TestUtil.java
> 
> 
> Uwe: I'm still concerned about this change and the way it might result in
> confusing failure messages in the future (if the whitespace def of other
> characters changes) ... can you please explain your choice "Assert.assertTrue
> -> assert" ?
> 
> 
> On Mon, 24 Aug 2015, Chris Hostetter wrote:
> 
> : Uwe: why did you change this from "Assert.assertTrue" to "assert" ?
> :
> : In the old code the test would fail every time with a clear explanation of
> : hte problem -- in your new code, if assertions are randomly disabled by
> : the test framework, then the sanity check won't run and instead we'll get
> : a strange failure from whatever test called this method.
> :
> :
> :
> : :
> ==========================================================
> ====================
> : : --- lucene/dev/trunk/lucene/test-
> framework/src/java/org/apache/lucene/util/TestUtil.java (original)
> : : +++ lucene/dev/trunk/lucene/test-
> framework/src/java/org/apache/lucene/util/TestUtil.java Sat Aug 22
> 21:33:47 2015
> : : @@ -35,6 +35,7 @@ import java.util.Collections;
> : :  import java.util.HashMap;
> : :  import java.util.Iterator;
> : :  import java.util.List;
> : : +import java.util.Locale;
> : :  import java.util.Map;
> : :  import java.util.NoSuchElementException;
> : :  import java.util.Random;
> : : @@ -1188,7 +1189,7 @@ public final class TestUtil {
> : :        int offset = nextInt(r, 0, WHITESPACE_CHARACTERS.length-1);
> : :        char c = WHITESPACE_CHARACTERS[offset];
> : :        // sanity check
> : : -      Assert.assertTrue("Not really whitespace? (@"+offset+"): " + c,
> Character.isWhitespace(c));
> : : +      assert Character.isWhitespace(c) : String.format(Locale.ENGLISH, "Not
> really whitespace? WHITESPACE_CHARACTERS[%d] is '\\u%04X'", offset, (int)
> c);
> : :        out.append(c);
> : :      }
> : :      return out.toString();
> : : @@ -1307,9 +1308,9 @@ public final class TestUtil {
> : :      '\u001E',
> : :      '\u001F',
> : :      '\u0020',
> : : -    // '\u0085', faild sanity check?
> : : +    // '\u0085', failed sanity check?
> : :      '\u1680',
> : : -    '\u180E',
> : : +    // '\u180E', no longer whitespace in Unicode 7.0 (Java 9)!
> : :      '\u2000',
> : :      '\u2001',
> : :      '\u2002',
> : :
> : :
> : :
> :
> : -Hoss
> : http://www.lucidworks.com/
> :
> 
> -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


Mime
View raw message