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 2015/08/24 18:50:28 UTC
Re: svn commit: r1697131 - in /lucene/dev/trunk/lucene:
JRE_VERSION_MIGRATION.txt
test-framework/src/java/org/apache/lucene/util/TestUtil.java
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/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
RE: svn commit: r1697131 - in /lucene/dev/trunk/lucene:
JRE_VERSION_MIGRATION.txt
test-framework/src/java/org/apache/lucene/util/TestUtil.java
Posted by Chris Hostetter <ho...@fucit.org>.
: I will change that as suggested. The assert in the StringBuilder loop
: can be removed completely, if we checked that before.
No worries -- I'll take care of it -- i'm the one that wants the test, i
don't mind doing the work.
I just didn't want to mess with it before until i understood the
motivation behind your assertchange, but in hind sight it's obvious.
-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: svn commit: r1697131 - in /lucene/dev/trunk/lucene: JRE_VERSION_MIGRATION.txt test-framework/src/java/org/apache/lucene/util/TestUtil.java
Posted by Uwe Schindler <uw...@thetaphi.de>.
Hi,
> Actaully, the more i think about it, the more i want to:
>
> a) add an explicit test that loops over this array and fails with a clear "your
> JVM is weird and doesn't consider thi char whitespace, that's messed up and
> this test and possible some other code needs updated if this is the new
> Java/unicode rules"
>
> b) leave the plain assert you have in place as a fallback to provide a clera
> assertion msg in case someone runs a test method/class that uses this utility
> method but don't run the sanity check test mentioned in (a) because of -
> Dtestcase or -Dtest.method.
That's fine. I will do this in the initialization block (as done in the other patch on the issue). My original "fix" was to not have a static array at all. I just made a static initializer that looped over all 16 bit char values and checked them for isWhitespace and adding them to a stringbuilder, that got converted to the char array. But Robert was not happy about that, because he argued that randomization seed would not be reproducible at all (which is correct, if you have test failure on Java 9 it does not fail on java 8, because the seed produces not the same chars). In that patch, I removed the assert completely (of course).
I will change that as suggested. The assert in the StringBuilder loop can be removed completely, if we checked that before.
Uwe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: svn commit: r1697131 - in /lucene/dev/trunk/lucene:
JRE_VERSION_MIGRATION.txt test-framework/src/java/org/apache/lucene/util/TestUtil.java
Posted by Dawid Weiss <da...@gmail.com>.
If you want to always run it and not rely on assert why not just put
it in an if (cond) { Assert.fail(...); }? This seems to strike the
balance between what you both need?
> Hmm, ok -- i thought this was happening when the JVM was forked.
The test runner doesn't mess with assertions at all, they're
configured as regular JVM parameters at the configuration level:
https://github.com/apache/lucene-solr/blob/trunk/lucene/common-build.xml#L139-L141
https://github.com/apache/lucene-solr/blob/trunk/lucene/common-build.xml#L997
What you have in Lucene (tests.asserts):
https://github.com/apache/lucene-solr/blob/trunk/lucene/common-build.xml#L1034-L1035
is to pass the information whether we should run without assertions
because, by default, we require them when running tests:
https://github.com/apache/lucene-solr/blob/trunk/lucene/test-framework/src/java/org/apache/lucene/util/TestRuleAssertionsRequired.java#L34-L52
Dawid
On Thu, Aug 27, 2015 at 2:01 AM, Chris Hostetter
<ho...@fucit.org> wrote:
>
> : 1) The test framework never randomly disables asserts (it cannot do
> : this, because asserts are enabled before the VM starts - you cannot do
>
> Hmm, ok -- i thought this was happening when the JVM was forked.
>
> In any case, my underlying concern still seems valid to me: confusing
> nonsensical test failures could occur if someone or some jenkins runs with
> -Dtests.asserts=false (which we should really do in at least one jenkins
> job to ensure we're not relying on side effects of method called in an
> assert ... wasn't that the whole point of adding tests.asserts in
> LUCENE-6019?)
>
> : (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.
>
> i think having sanity checks about our assumptions of the JVM are useful
> -- just like we (last time i checked) have sanity checks that filesystems
> behave as we expect.
>
> It's one thing to say "we don't need a check that 2 + 2 == 4" but in cases
> like this where assumptions (about hte definition of whitespace) can
> evidently change between JVM versions, it's nice to have a sanity check
> that our tests are testing the right thing (ie: fail fast because the JVM
> isn't behaving the way we expect, don't fail slow with a weird
> obscure NPE or AIOBE error because our code expected 2+2==4 and the JVM
> gave us -42 instead)
>
>
> : 2) Now the answer: I changed it because of performance. The
>
> Ah, ok -- totally fair point. Good call.
>
> : 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));
>
> Actaully, the more i think about it, the more i want to:
>
> a) add an explicit test that loops over this array and fails with a clear
> "your JVM is weird and doesn't consider thi char whitespace, that's messed
> up and this test and possible some other code needs updated if this is the
> new Java/unicode rules"
>
> b) leave the plain assert you have in place as a fallback to provide a
> clera assertion msg in case someone runs a test method/class that uses
> this utility method but don't run the sanity check test mentioned in (a)
> because of -Dtestcase or -Dtest.method.
>
> (yes, i realize only one test method currently uses it nad the utility has
> already been refactored up to live in that same test class, but this is
> all about future proofing in case it ever gets refactred again)
>
>
>
> -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
RE: svn commit: r1697131 - in /lucene/dev/trunk/lucene:
JRE_VERSION_MIGRATION.txt
test-framework/src/java/org/apache/lucene/util/TestUtil.java
Posted by Chris Hostetter <ho...@fucit.org>.
: 1) The test framework never randomly disables asserts (it cannot do
: this, because asserts are enabled before the VM starts - you cannot do
Hmm, ok -- i thought this was happening when the JVM was forked.
In any case, my underlying concern still seems valid to me: confusing
nonsensical test failures could occur if someone or some jenkins runs with
-Dtests.asserts=false (which we should really do in at least one jenkins
job to ensure we're not relying on side effects of method called in an
assert ... wasn't that the whole point of adding tests.asserts in
LUCENE-6019?)
: (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.
i think having sanity checks about our assumptions of the JVM are useful
-- just like we (last time i checked) have sanity checks that filesystems
behave as we expect.
It's one thing to say "we don't need a check that 2 + 2 == 4" but in cases
like this where assumptions (about hte definition of whitespace) can
evidently change between JVM versions, it's nice to have a sanity check
that our tests are testing the right thing (ie: fail fast because the JVM
isn't behaving the way we expect, don't fail slow with a weird
obscure NPE or AIOBE error because our code expected 2+2==4 and the JVM
gave us -42 instead)
: 2) Now the answer: I changed it because of performance. The
Ah, ok -- totally fair point. Good call.
: 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));
Actaully, the more i think about it, the more i want to:
a) add an explicit test that loops over this array and fails with a clear
"your JVM is weird and doesn't consider thi char whitespace, that's messed
up and this test and possible some other code needs updated if this is the
new Java/unicode rules"
b) leave the plain assert you have in place as a fallback to provide a
clera assertion msg in case someone runs a test method/class that uses
this utility method but don't run the sanity check test mentioned in (a)
because of -Dtestcase or -Dtest.method.
(yes, i realize only one test method currently uses it nad the utility has
already been refactored up to live in that same test class, but this is
all about future proofing in case it ever gets refactred again)
-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: svn commit: r1697131 - in /lucene/dev/trunk/lucene: JRE_VERSION_MIGRATION.txt test-framework/src/java/org/apache/lucene/util/TestUtil.java
Posted by Uwe Schindler <uw...@thetaphi.de>.
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
Re: svn commit: r1697131 - in /lucene/dev/trunk/lucene:
JRE_VERSION_MIGRATION.txt
test-framework/src/java/org/apache/lucene/util/TestUtil.java
Posted by Chris Hostetter <ho...@fucit.org>.
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