Lucene - Core
  1. Lucene - Core
  2. LUCENE-3847

LuceneTestCase should check for modifications on System properties

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.6, 4.0-ALPHA
    • Component/s: general/test
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      • fail the test if changes have been detected.
      • revert the state of system properties before the suite.
      • cleanup after the suite.

        Issue Links

          Activity

          Hide
          Robert Muir added a comment -

          Thanks Dawid... these tests are now working in my IDE

          Show
          Robert Muir added a comment - Thanks Dawid... these tests are now working in my IDE
          Hide
          Dawid Weiss added a comment -

          Applied a fix for this. user.timezone is ignored (and is not reset).

          Show
          Dawid Weiss added a comment - Applied a fix for this. user.timezone is ignored (and is not reset).
          Hide
          Robert Muir added a comment -

          I suggest that we just ignore user.timezone as it is triggered from multiple locations and doesn't seem that important?

          +1, we know its a side effect of our testcase itself randomizing the locale/timezone...

          Show
          Robert Muir added a comment - I suggest that we just ignore user.timezone as it is triggered from multiple locations and doesn't seem that important? +1, we know its a side effect of our testcase itself randomizing the locale/timezone...
          Hide
          Dawid Weiss added a comment -

          I know what's changing it. Eh. So – there is a warning being printed:

          Mar 22, 2012 6:20:33 PM org.apache.solr.core.Config parseLuceneVersionString
          WARNING: You should not use LUCENE_CURRENT as luceneMatchVersion property: if you use this setting, and then Solr upgrades to a newer release of Lucene, sizable changes may happen. If precise back compatibility is important then you should instead explicitly specify an actual Lucene version.
          Mar 22, 2012 6:20:33 PM org.apache.solr.analysis.BaseTokenStreamFactory warnDeprecated
          WARNING: RussianLetterTokenizerFactory is deprecated. Use StandardTokenizerFactory instead.
          

          These warnings go through Java logging and this in turn is localized (date format, "warning" info, etc.). This in turn asks for the default TimeZone and this in turn sets the system property (I mentioned it a while ago).

          I suggest that we just ignore user.timezone as it is triggered from multiple locations and doesn't seem that important?

          Show
          Dawid Weiss added a comment - I know what's changing it. Eh. So – there is a warning being printed: Mar 22, 2012 6:20:33 PM org.apache.solr.core.Config parseLuceneVersionString WARNING: You should not use LUCENE_CURRENT as luceneMatchVersion property: if you use this setting, and then Solr upgrades to a newer release of Lucene, sizable changes may happen. If precise back compatibility is important then you should instead explicitly specify an actual Lucene version. Mar 22, 2012 6:20:33 PM org.apache.solr.analysis.BaseTokenStreamFactory warnDeprecated WARNING: RussianLetterTokenizerFactory is deprecated. Use StandardTokenizerFactory instead. These warnings go through Java logging and this in turn is localized (date format, "warning" info, etc.). This in turn asks for the default TimeZone and this in turn sets the system property (I mentioned it a while ago). I suggest that we just ignore user.timezone as it is triggered from multiple locations and doesn't seem that important?
          Hide
          Dawid Weiss added a comment -

          Well... something is changing it, the question is what it is. I'll take a look.

          Show
          Dawid Weiss added a comment - Well... something is changing it, the question is what it is. I'll take a look.
          Hide
          Robert Muir added a comment -

          Strangely i trip the timezone issue when running any solr tests from Eclipse... but not lucene tests?

          E.g. if i run TestDemo from lucene its fine, but if i run TestRussianFilter (org.apache.solr.analysis) then i hit:

          java.lang.AssertionError: System properties invariant violated.
          Different values:
            [old]user.timezone=
            [new]user.timezone=America/New_York
          
          	at org.apache.lucene.util.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:46)
          	at org.junit.rules.RunRules.evaluate(RunRules.java:18)
          	at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
          	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)
          	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
          	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
          	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
          	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
          	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)
          
          Show
          Robert Muir added a comment - Strangely i trip the timezone issue when running any solr tests from Eclipse... but not lucene tests? E.g. if i run TestDemo from lucene its fine, but if i run TestRussianFilter (org.apache.solr.analysis) then i hit: java.lang.AssertionError: System properties invariant violated. Different values: [old]user.timezone= [new]user.timezone=America/New_York at org.apache.lucene.util.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:46) at org.junit.rules.RunRules.evaluate(RunRules.java:18) at org.junit.runners.ParentRunner.run(ParentRunner.java:300) at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)
          Hide
          Mark Miller added a comment -

          This is great Dawid, thanks!

          Show
          Mark Miller added a comment - This is great Dawid, thanks!
          Hide
          Uwe Schindler added a comment -

          No prob. That is exactly the problem I wanted to prevent with the Enumeration and getProperty(). Code using those 2 functions is the only correct one to list all properties...

          Show
          Uwe Schindler added a comment - No prob. That is exactly the problem I wanted to prevent with the Enumeration and getProperty(). Code using those 2 functions is the only correct one to list all properties...
          Hide
          Dawid Weiss added a comment -

          As far as I remember, the main problem with the Map interface in Properties is the values coming from the parent "defaults" object.

          Exactly, I'm kind of slow today.

          Show
          Dawid Weiss added a comment - As far as I remember, the main problem with the Map interface in Properties is the values coming from the parent "defaults" object. Exactly, I'm kind of slow today.
          Hide
          Dawid Weiss added a comment -

          Now I remember what it was that I had a problem with. Properties has the notion of "defaults" so it falls back to another Properties by default if the key is not present. Unfortunately get() is not overridden in Properties (and everything else is) so things like propertyNames or getProperty will return a value from the fallback set, but get() won't.

            public static void main(String[] args) {
              Properties defaults = new Properties();
              Properties p = new Properties(defaults);
              String key = "custom-key";
              String value = "value";
              defaults.put(key, value);
              for (Enumeration<?> e = p.propertyNames(); e.hasMoreElements();) {
                Object currentKey = e.nextElement();
                if (currentKey == key) {
                  System.out.println("Default key found in propertyNames(), but...");
                  System.out.println("p.get()=" + p.get(key));
                  System.out.println("p.getProperty()=" + p.getProperty(key));
                }
              }
            }
          

          You can easily corrupt other programs by inserting Object values into the system property set – any enumeration will then fail with a classcast. Still don't remember where this caused a problem but it definitely was.

          Show
          Dawid Weiss added a comment - Now I remember what it was that I had a problem with. Properties has the notion of "defaults" so it falls back to another Properties by default if the key is not present. Unfortunately get() is not overridden in Properties (and everything else is) so things like propertyNames or getProperty will return a value from the fallback set, but get() won't. public static void main( String [] args) { Properties defaults = new Properties(); Properties p = new Properties(defaults); String key = "custom-key" ; String value = "value" ; defaults.put(key, value); for (Enumeration<?> e = p.propertyNames(); e.hasMoreElements();) { Object currentKey = e.nextElement(); if (currentKey == key) { System .out.println( "Default key found in propertyNames(), but..." ); System .out.println( "p.get()=" + p.get(key)); System .out.println( "p.getProperty()=" + p.getProperty(key)); } } } You can easily corrupt other programs by inserting Object values into the system property set – any enumeration will then fail with a classcast. Still don't remember where this caused a problem but it definitely was.
          Hide
          Uwe Schindler added a comment -

          Horrible!!! I just say: die, die, die

          Show
          Uwe Schindler added a comment - Horrible!!! I just say: die, die, die
          Hide
          Dawid Weiss added a comment -

          Right, good catch. Properties are seriously messed up, aren't they?

          Show
          Dawid Weiss added a comment - Right, good catch. Properties are seriously messed up, aren't they?
          Hide
          Uwe Schindler added a comment - - edited

          About your commit: It should be getProperty() [without cast] and not get()... Otherwise the defaults are again fcked up

          Show
          Uwe Schindler added a comment - - edited About your commit: It should be getProperty() [without cast] and not get()... Otherwise the defaults are again fcked up
          Hide
          Dawid Weiss added a comment -

          Yeah, it might have been that that I experienced. So back to enumeration we go

          Show
          Dawid Weiss added a comment - Yeah, it might have been that that I experienced. So back to enumeration we go
          Hide
          Uwe Schindler added a comment - - edited

          As far as I remember, the main problem with the Map interface in Properties is the values coming from the parent "defaults" object. When you wrap another properties object with one that has the other one as "defaults" given in ctor, then your are fcked up if you are using the Map interface. Which is not the case for system properties, but that's not guaranteed (e.g. the Java JVM could set some basic properties in another Properties object and make the changes available only through the overlayed properties object).

          To fix the "defaults" problem I would go with the Enumeration<?> (works in Java 5, too) in a "for(Enumeration<?> e=propertyNames(); e.hasMoreElements()" loop.

          If you use the Set interface, the inherited props would not have been seen and the system may delete properties which are in a parent/"defaults" properties object.

          Show
          Uwe Schindler added a comment - - edited As far as I remember, the main problem with the Map interface in Properties is the values coming from the parent "defaults" object. When you wrap another properties object with one that has the other one as "defaults" given in ctor, then your are fcked up if you are using the Map interface. Which is not the case for system properties, but that's not guaranteed (e.g. the Java JVM could set some basic properties in another Properties object and make the changes available only through the overlayed properties object). To fix the "defaults" problem I would go with the Enumeration<?> (works in Java 5, too) in a "for(Enumeration<?> e=propertyNames(); e.hasMoreElements() " loop. If you use the Set interface, the inherited props would not have been seen and the system may delete properties which are in a parent/"defaults" properties object.
          Hide
          Uwe Schindler added a comment -

          I don't mind, I think it's fine. But I had the same problem like you when I used the Properties class, it always feels bad and some things are not working at all.

          This comment in Javadocs for public Set<String> stringPropertyNames() makes me nervous, too:

          Returns a set of keys in this property list where the key and its corresponding value are strings, including distinct keys in the default property list if a key of the same name has not already been found from the main properties list. Properties whose key or value is not of type String are omitted.
          The returned set is not backed by the Properties object. Changes to this Properties are not reflected in the set, or vice versa.
          Returns:
          a set of keys in this property list where the key and its corresponding value are strings, including the keys in the default property list.
          Since:
          1.6

          This method is available since Java 6, so this would be the "most correct" solution. Retrieve the set, iterate via advanced for-loop and use getProperty(). But that does not work for Lucene 3.x.

          Show
          Uwe Schindler added a comment - I don't mind, I think it's fine. But I had the same problem like you when I used the Properties class, it always feels bad and some things are not working at all. This comment in Javadocs for public Set<String> stringPropertyNames() makes me nervous, too: Returns a set of keys in this property list where the key and its corresponding value are strings, including distinct keys in the default property list if a key of the same name has not already been found from the main properties list. Properties whose key or value is not of type String are omitted. The returned set is not backed by the Properties object. Changes to this Properties are not reflected in the set, or vice versa. Returns: a set of keys in this property list where the key and its corresponding value are strings, including the keys in the default property list. Since: 1.6 This method is available since Java 6, so this would be the "most correct" solution. Retrieve the set, iterate via advanced for-loop and use getProperty(). But that does not work for Lucene 3.x.
          Hide
          Dawid Weiss added a comment -

          Yeah, I know. I actually felt bad about entrySet too – I think the safest way would be to use enumeration, but it felt ancient. I'll leave as is if you don't mind. It's a bit more explicit, but won't hurt anybody I think.

          Show
          Dawid Weiss added a comment - Yeah, I know. I actually felt bad about entrySet too – I think the safest way would be to use enumeration, but it felt ancient. I'll leave as is if you don't mind. It's a bit more explicit, but won't hurt anybody I think.
          Hide
          Uwe Schindler added a comment -

          I don't know, to be honest. I didn't want to experiment because I remember being scalded at some point in my past when I used Map interface to manipulate system properties and in certain circumstances things didn't work as they should. I can't remember the details now, unfortunately.

          For copying you are already using the map interface (retrieving entrySet). For read access this is not a problem, but the whole Java API implementing Map is broken. Implementing the Map interface, which allows to put Object keys/values is unfortunately a no-go. But doing "new TreeMap<String,String>((Map<String,String>) properties)" is fine (with SuppressWarnings because of the cast) - because the putAll method of TreeMap is implemented exactly as your loop.

          Show
          Uwe Schindler added a comment - I don't know, to be honest. I didn't want to experiment because I remember being scalded at some point in my past when I used Map interface to manipulate system properties and in certain circumstances things didn't work as they should. I can't remember the details now, unfortunately. For copying you are already using the map interface (retrieving entrySet). For read access this is not a problem, but the whole Java API implementing Map is broken. Implementing the Map interface, which allows to put Object keys/values is unfortunately a no-go. But doing "new TreeMap<String,String>((Map<String,String>) properties)" is fine (with SuppressWarnings because of the cast) - because the putAll method of TreeMap is implemented exactly as your loop.
          Hide
          Dawid Weiss added a comment -

          I don't know, to be honest. I didn't want to experiment because I remember being scalded at some point in my past when I used Map interface to manipulate system properties and in certain circumstances things didn't work as they should. I can't remember the details now, unfortunately.

          As for non Strings and nulls – I assume only System.setProperty/clearProperty is used to manipulate the map; if so, it should only contain Strings and it should have non-null keys and values.

          Show
          Dawid Weiss added a comment - I don't know, to be honest. I didn't want to experiment because I remember being scalded at some point in my past when I used Map interface to manipulate system properties and in certain circumstances things didn't work as they should. I can't remember the details now, unfortunately. As for non Strings and nulls – I assume only System.setProperty/clearProperty is used to manipulate the map; if so, it should only contain Strings and it should have non-null keys and values.
          Hide
          Uwe Schindler added a comment -

          +1

          Properties implements java.util.Map<Object,Object>, clone as Map can simply be a "new WhateverMap(System.getProperties())". Although it's <Object,Object>, it should only contain Strings... Can we be sure?

          Show
          Uwe Schindler added a comment - +1 Properties implements java.util.Map<Object,Object>, clone as Map can simply be a "new WhateverMap(System.getProperties())". Although it's <Object,Object>, it should only contain Strings... Can we be sure?
          Hide
          Dawid Weiss added a comment -

          Patch with changes.

          Show
          Dawid Weiss added a comment - Patch with changes.
          Hide
          Dawid Weiss added a comment - - edited

          I've implemented this invariant and it works like a charm. Did you know TimeZone.getDefault() has a side-effect of setting user.timezone system property? If you did, I'll buy you a beer next time we meet

          Anyway, lots of Solr tests leave behind system properties. Instead of trying to fix them one by one I went kind of the easy route and created a class and test rule that simply reverts all system properties from before the entry scope. This has been applied to AbstractSolrTestCase and SolrTestCaseJ4 and since these are subclasses of LuceneTestCase, the invariant will always hold, no matter what.

          If Solr folks wish to fix tests one by one (which may be a good idea or a bad idea – there's lots of them), then comment out these two:

            @ClassRule
            public static TestRule solrClassRules = 
              RuleChain.outerRule(new SystemPropertiesRestoreRule());
          
            @Rule
            public TestRule solrTestRules = 
              RuleChain.outerRule(new SystemPropertiesRestoreRule());
          
          Show
          Dawid Weiss added a comment - - edited I've implemented this invariant and it works like a charm. Did you know TimeZone.getDefault() has a side-effect of setting user.timezone system property? If you did, I'll buy you a beer next time we meet Anyway, lots of Solr tests leave behind system properties. Instead of trying to fix them one by one I went kind of the easy route and created a class and test rule that simply reverts all system properties from before the entry scope. This has been applied to AbstractSolrTestCase and SolrTestCaseJ4 and since these are subclasses of LuceneTestCase, the invariant will always hold, no matter what. If Solr folks wish to fix tests one by one (which may be a good idea or a bad idea – there's lots of them), then comment out these two: @ClassRule public static TestRule solrClassRules = RuleChain.outerRule( new SystemPropertiesRestoreRule()); @Rule public TestRule solrTestRules = RuleChain.outerRule( new SystemPropertiesRestoreRule());

            People

            • Assignee:
              Dawid Weiss
              Reporter:
              Dawid Weiss
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development