StringHelper should check for empty string of "tests.seed" system property [LUCENE-5645]

[asfimport]

As of #6666 (committed to v4.8), StringHelper will initialize GOOD_FAST_HASH_SEED based on the system property "tests.seed" if it is set. Unfortunately it doesn't do an empty-string check, and it's common at least in my setup that copies Lucene's maven pom.xml that the string will be empty unless I set it on the command line. FWIW Randomized Testing does do an empty-string check.

---

Migrated from LUCENE-5645 by David Smiley (@dsmiley), 1 vote, updated May 09 2016
Attachments: LUCENE-5645_StringHelper_empty_tests_seed_condition.patch

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

The change is obviously trivial; I won't provide a patch. Change line 158 from:

if (prop != null) {

to

if (prop != null && prop.length() > 0) {

I'll commit it in 24 hours or upon the first +1 I see.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Wait, please take a step back before rushing in a change to the hashing function of the indexer into a bugfix release.

Why isn't this hitting lucene's tests? solr? elasticsearch? these all support maven and are working perfectly well...

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

I wonder the same; I'll try and see why. Bure surely my suggestion is innocent enough?

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I'm not sure. If tests.seed is empty, then things won't reproduce right? Because it will initialize itsself with System.currentTimeMillis().

So perhaps the exception is actually warranted. Is there really a better exception we can give than the current one (i assume NumberFormatException as it tries to parse the empty string)? I think thats appropriate.

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

So it turns out this has to do with IntelliJ. When IntelliJ configures itself off of a maven pom.xml, it apparently takes a close look at what system properties are being passed to the Surefire plugin, which runs tests. If you tell IntelliJ to run a test class, it automatically passes along all System properties it saw that Surefire is configured to use (in addition to any you explicitly configure in a run configuration). IntelliJ passes along the empty string if the System property is empty. Apparently, an actual Maven Surefire invoked test skips over empty-value properties and doesn't set those.

Well that was interesting, and I'll file a bug with Jetbrains, but nonetheless I think my proposal in this issue remains sound – don't try to parse tests.seed if it's "" because we already know it won't parse and because RandomizedTesting skips "" as well.

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

IntelliJ bug report: http://youtrack.jetbrains.com/issue/IDEA-124784

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

In case you have more to add, I'll commit this tomorrow instead of now.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

I don't think being lenient is a good option here? Why is randomized testing lenient?

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

Most option/setting parsing code I seen in all my experience treat null and an empty string as the same thing. Hasn't that been the same experience for you? I realize there can be a meaningful distinction but usually there isn't. If tests.seed has zero length, there is no seed.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

If tests.seed is set as a sysprop here, but not parseable, tests are not going to reproduce. I think an exception is good.

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

How about this: if the non-null tests.seed fails to parse, then simply take the hash code of whatever string it is, initialize a new Random() with that as the seed, and grab a long from the random.

[asfimport]

David Smiley (@dsmiley) (migrated from JIRA)

The following patch addresses my concern (throwing an exception) as well as Rob's (reproducible test, even when tests.seed is the empty string).

I also noticed oddly that the string length condition was checking if the string was greater than characters to then parse all but the last 8 characters (which hypothetically might be more than 8 characters). I changed this to only grab the first 8 characters, and to use Integer.parseInt since an int is needed instead of using a Long then casting.

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Maybe we could simply use prop.hashCode() entirely, instead of trying to parseInt base 16? It's not important that the value we use here "matches" the value that randomized testing extracted from the seed, just that it's the same for the same seed....

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I'm still confused: since when was the empty string a supported value for 'tests.seed'? I don't think it should be: its unnecessary and just makes the test framework complicated. I'm not sure it will even really "work".

its sad that StringHelper gets the blame here, LuceneTestCase should have thrown an exception earlier.

[asfimport]

Dawid Weiss (@dweiss) (migrated from JIRA)

> Why is randomized testing lenient?

It parses an empty value of the property because it's more convenient sometimes to pass it from ant as empty (rather then not defining it at all).

The start = (prop == null ? 0 : prop.hashCode()) seems like a neat trick for seeding the hash random though.