LUCENE-3847: LuceneTestCase will now check for modifications of System properties before and after each test (and suite). If changes are detected, the test will fail. A rule can be used to reset system properties to before-scope state (and this has been used to make Solr tests pass).

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1296888 13f79535-47bb-0310-9956-ffa450edef68

lucene/CHANGES.txt

@@ -928,6 +928,12 @@ Documentation
 
 Build
 
+* LUCENE-3847: LuceneTestCase will now check for modifications of System 
+  properties before and after each test (and suite). If changes are detected,
+  the test will fail. A rule can be used to reset system properties to
+  before-scope state (and this has been used to make Solr tests pass).
+  (Dawid Weiss).
+
 * LUCENE-3228: Stop downloading external javadoc package-list files:
 
   - Added package-list files for Oracle Java javadocs and JUnit javadocs to

lucene/core/src/test/org/apache/lucene/util/junitcompat/TestSystemPropertiesInvariantRule.java

@@ -0,0 +1,74 @@
+package org.apache.lucene.util.junitcompat;
+
+import org.apache.lucene.util.LuceneTestCase;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.JUnitCore;
+import org.junit.runner.Result;
+import org.junit.runner.notification.Failure;
+
+public class TestSystemPropertiesInvariantRule {
+  public static final String PROP_KEY1 = "new-property-1";
+  public static final String VALUE1 = "new-value-1";
+  
+  public static class Base extends LuceneTestCase {
+    public void testEmpty() {}
+  }
+  
+  public static class InBeforeClass extends Base {
+    @BeforeClass
+    public static void beforeClass() {
+      System.setProperty(PROP_KEY1, VALUE1);
+    }
+  }
+  
+  public static class InAfterClass extends Base {
+    @AfterClass
+    public static void afterClass() {
+      System.setProperty(PROP_KEY1, VALUE1);
+    }
+  }
+  
+  public static class InTestMethod extends Base {
+    public void testMethod1() {
+      if (System.getProperty(PROP_KEY1) != null) {
+        throw new RuntimeException("Shouldn't be here.");
+      }
+      System.setProperty(PROP_KEY1, VALUE1);
+    }
+    
+    public void testMethod2() {
+      testMethod1();
+    }
+  }
+  
+  @Test
+  public void testRuleInvariantBeforeClass() {
+    Result runClasses = JUnitCore.runClasses(InBeforeClass.class);
+    Assert.assertEquals(1, runClasses.getFailureCount());
+    Assert.assertTrue(runClasses.getFailures().get(0).getMessage()
+        .contains(PROP_KEY1));
+    Assert.assertNull(System.getProperty(PROP_KEY1));
+  }
+  
+  @Test
+  public void testRuleInvariantAfterClass() {
+    Result runClasses = JUnitCore.runClasses(InAfterClass.class);
+    Assert.assertEquals(1, runClasses.getFailureCount());
+    Assert.assertTrue(runClasses.getFailures().get(0).getMessage()
+        .contains(PROP_KEY1));
+    Assert.assertNull(System.getProperty(PROP_KEY1));
+  }
+  
+  @Test
+  public void testRuleInvariantInTestMethod() {
+    Result runClasses = JUnitCore.runClasses(InTestMethod.class);
+    Assert.assertEquals(2, runClasses.getFailureCount());
+    for (Failure f : runClasses.getFailures()) {
+      Assert.assertTrue(f.getMessage().contains(PROP_KEY1));
+    }
+    Assert.assertNull(System.getProperty(PROP_KEY1));
+  }
+}

lucene/test-framework/src/java/org/apache/lucene/util/LuceneTestCase.java

@@ -97,6 +97,7 @@ import org.junit.Assert;
 import org.junit.Assume;
 import org.junit.Before;
 import org.junit.BeforeClass;
+import org.junit.ClassRule;
 import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.internal.AssumptionViolatedException;
@@ -257,6 +258,11 @@ public abstract class LuceneTestCase extends Assert {
   private static TimeZone timeZone;
   private static TimeZone savedTimeZone;
 
+  /**
+   * Restore these system property values in {@link #afterClassLuceneTestCaseJ4()}.
+   */
+  private static HashMap<String, String> restoreProperties = new HashMap<String,String>();
+
   protected static Map<MockDirectoryWrapper,StackTraceElement[]> stores;
 
   /** @deprecated (4.0) until we fix no-fork problems in solr tests */
@@ -269,10 +275,13 @@ public abstract class LuceneTestCase extends Assert {
     random.setSeed(staticSeed);
     random.initialized = true;
   }
-  
+
   @Deprecated
   private static boolean icuTested = false;
 
+  @ClassRule
+  public static TestRule classRules = RuleChain.outerRule(new SystemPropertiesInvariantRule());
+
   @BeforeClass
   public static void beforeClassLuceneTestCaseJ4() {
     initRandom();
@@ -282,6 +291,7 @@ public abstract class LuceneTestCase extends Assert {
     // enable this by default, for IDE consistency with ant tests (as its the default from ant)
     // TODO: really should be in solr base classes, but some extend LTC directly.
     // we do this in beforeClass, because some tests currently disable it
+    restoreProperties.put("solr.directoryFactory", System.getProperty("solr.directoryFactory"));
     if (System.getProperty("solr.directoryFactory") == null) {
       System.setProperty("solr.directoryFactory", "org.apache.solr.core.MockDirectoryFactory");
     }
@@ -363,6 +373,9 @@ public abstract class LuceneTestCase extends Assert {
     
     locale = TEST_LOCALE.equals("random") ? randomLocale(random) : localeForName(TEST_LOCALE);
     Locale.setDefault(locale);
+    // TimeZone.getDefault will set user.timezone to the default timezone of the user's locale.
+    // So store the original property value and restore it at end.
+    restoreProperties.put("user.timezone", System.getProperty("user.timezone"));
     savedTimeZone = TimeZone.getDefault();
     timeZone = TEST_TIMEZONE.equals("random") ? randomTimeZone(random) : TimeZone.getTimeZone(TEST_TIMEZONE);
     TimeZone.setDefault(timeZone);
@@ -372,6 +385,15 @@ public abstract class LuceneTestCase extends Assert {
 
   @AfterClass
   public static void afterClassLuceneTestCaseJ4() {
+    for (Map.Entry<String,String> e : restoreProperties.entrySet()) {
+      if (e.getValue() == null) {
+        System.clearProperty(e.getKey());
+      } else {
+        System.setProperty(e.getKey(), e.getValue());
+      }
+    }
+    restoreProperties.clear();
+
     Throwable problem = null;
     
     if (! "false".equals(TEST_CLEAN_THREADS)) {
@@ -587,6 +609,7 @@ public abstract class LuceneTestCase extends Assert {
   public final TestRule ruleChain = RuleChain
     .outerRule(new RememberThreadRule())
     .around(new TestResultInterceptorRule())
+    .around(new SystemPropertiesInvariantRule())
     .around(new InternalSetupTeardownRule())
     .around(new SubclassSetupTeardownRule());
 

lucene/test-framework/src/java/org/apache/lucene/util/SystemPropertiesInvariantRule.java

@@ -0,0 +1,94 @@
+package org.apache.lucene.util;
+
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.TreeMap;
+import java.util.TreeSet;
+
+import org.junit.rules.TestRule;
+import org.junit.runner.Description;
+import org.junit.runners.model.MultipleFailureException;
+import org.junit.runners.model.Statement;
+
+public class SystemPropertiesInvariantRule implements TestRule {
+  @Override
+  public Statement apply(final Statement s, Description d) {
+    return new Statement() {
+      @Override
+      public void evaluate() throws Throwable {
+        TreeMap<String,String> before = SystemPropertiesRestoreRule.cloneAsMap(System.getProperties());
+        ArrayList<Throwable> errors = new ArrayList<Throwable>();
+        try {
+          s.evaluate();
+        } catch (Throwable t) {
+          errors.add(t);
+        } finally {
+          TreeMap<String,String> after = SystemPropertiesRestoreRule.cloneAsMap(System.getProperties());
+          if (!after.equals(before)) {
+            errors.add(
+                new AssertionError("System properties invariant violated.\n" + 
+                    collectErrorMessage(before, after)));
+          }
+
+          // Restore original properties.
+          SystemPropertiesRestoreRule.restore(before, after);
+        }
+
+        MultipleFailureException.assertEmpty(errors);
+      }
+
+      private StringBuilder collectErrorMessage(
+          TreeMap<String,String> before, TreeMap<String,String> after) {
+        TreeSet<String> newKeys = new TreeSet<String>(after.keySet());
+        newKeys.removeAll(before.keySet());
+        
+        TreeSet<String> missingKeys = new TreeSet<String>(before.keySet());
+        missingKeys.removeAll(after.keySet());
+        
+        TreeSet<String> differentKeyValues = new TreeSet<String>(before.keySet());
+        differentKeyValues.retainAll(after.keySet());
+        for (Iterator<String> i = differentKeyValues.iterator(); i.hasNext();) {
+          String key = i.next();
+          String valueBefore = before.get(key);
+          String valueAfter = after.get(key);
+          if ((valueBefore == null && valueAfter == null) ||
+              (valueBefore.equals(valueAfter))) {
+            i.remove();
+          }
+        }
+
+        final StringBuilder b = new StringBuilder();
+        if (!missingKeys.isEmpty()) {
+          b.append("Missing keys:\n");
+          for (String key : missingKeys) {
+            b.append("  ").append(key)
+              .append("=")
+              .append(before.get(key))            
+              .append("\n");
+          }
+        }
+        if (!newKeys.isEmpty()) {
+          b.append("New keys:\n");
+          for (String key : newKeys) {
+            b.append("  ").append(key)
+              .append("=")
+              .append(after.get(key))
+              .append("\n");
+          }
+        }
+        if (!differentKeyValues.isEmpty()) {
+          b.append("Different values:\n");
+          for (String key : differentKeyValues) {
+            b.append("  [old]").append(key)
+              .append("=")
+              .append(before.get(key)).append("\n");
+            b.append("  [new]").append(key)
+              .append("=")
+              .append(after.get(key)).append("\n");
+          }
+        }
+        return b;
+      }
+    };
+  }
+}

lucene/test-framework/src/java/org/apache/lucene/util/SystemPropertiesRestoreRule.java

@@ -0,0 +1,59 @@
+package org.apache.lucene.util;
+
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Properties;
+import java.util.TreeMap;
+
+import org.junit.rules.TestRule;
+import org.junit.runner.Description;
+import org.junit.runners.model.Statement;
+
+/**
+ * Restore system properties from before the nested {@link Statement}.
+ */
+public class SystemPropertiesRestoreRule implements TestRule {
+  @Override
+  public Statement apply(final Statement s, Description d) {
+    return new Statement() {
+      @Override
+      public void evaluate() throws Throwable {
+        TreeMap<String,String> before = cloneAsMap(System.getProperties());
+        try {
+          s.evaluate();
+        } finally {
+          TreeMap<String,String> after = cloneAsMap(System.getProperties());
+          if (!after.equals(before)) {
+            // Restore original properties.
+            restore(before, after);
+          }
+        }
+      }
+    };
+  }
+  
+  static TreeMap<String,String> cloneAsMap(Properties properties) {
+    TreeMap<String,String> result = new TreeMap<String,String>();
+    for (Entry<Object,Object> e : properties.entrySet()) {
+      // We can be sure it's always strings, can't we?
+      result.put((String) e.getKey(), (String) e.getValue());
+    }
+    return result;
+  }
+
+  static void restore(
+      TreeMap<String,String> before,
+      TreeMap<String,String> after) {
+    after.keySet().removeAll(before.keySet());
+    for (String key : after.keySet()) {
+      System.clearProperty(key);
+    }
+    for (Map.Entry<String,String> e : before.entrySet()) {
+      if (e.getValue() == null) {
+        System.clearProperty(e.getKey()); // Can this happen?
+      } else {
+        System.setProperty(e.getKey(), e.getValue());
+      }
+    }
+  }  
+}

solr/core/src/test/org/apache/solr/schema/TestBinaryField.java

@@ -16,7 +16,14 @@
  */
 package org.apache.solr.schema;
 
+import java.io.File;
+import java.io.FileOutputStream;
+import java.nio.ByteBuffer;
+import java.util.List;
+
+import org.apache.commons.io.IOUtils;
 import org.apache.lucene.util.LuceneTestCase;
+import org.apache.lucene.util.SystemPropertiesRestoreRule;
 import org.apache.solr.client.solrj.SolrQuery;
 import org.apache.solr.client.solrj.beans.Field;
 import org.apache.solr.client.solrj.embedded.JettySolrRunner;
@@ -26,13 +33,9 @@ import org.apache.solr.common.SolrDocument;
 import org.apache.solr.common.SolrDocumentList;
 import org.apache.solr.common.SolrInputDocument;
 import org.apache.solr.core.SolrResourceLoader;
-import org.apache.commons.io.FileUtils;
-import org.apache.commons.io.IOUtils;
-
-import java.nio.ByteBuffer;
-import java.io.File;
-import java.io.FileOutputStream;
-import java.util.List;
+import org.junit.Rule;
+import org.junit.rules.RuleChain;
+import org.junit.rules.TestRule;
 
 public class TestBinaryField extends LuceneTestCase {
   CommonsHttpSolrServer server;
@@ -41,6 +44,10 @@ public class TestBinaryField extends LuceneTestCase {
   int port = 0;
   static final String context = "/example";
 
+  @Rule
+  public TestRule solrTestRules = 
+    RuleChain.outerRule(new SystemPropertiesRestoreRule());
+
   @Override
   public void setUp() throws Exception {
     super.setUp();

solr/solrj/src/test/org/apache/solr/client/solrj/embedded/JettyWebappTest.java

@@ -21,11 +21,14 @@ import java.io.File;
 import java.net.URL;
 import java.util.Random;
 
-import org.apache.lucene.util.LuceneTestCase;
-
 import org.apache.commons.io.IOUtils;
+import org.apache.lucene.util.LuceneTestCase;
+import org.apache.lucene.util.SystemPropertiesRestoreRule;
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.util.ExternalPaths;
+import org.junit.Rule;
+import org.junit.rules.RuleChain;
+import org.junit.rules.TestRule;
 import org.mortbay.jetty.Connector;
 import org.mortbay.jetty.Server;
 import org.mortbay.jetty.bio.SocketConnector;
@@ -40,7 +43,11 @@ public class JettyWebappTest extends LuceneTestCase
 {
   int port = 0;
   static final String context = "/test";
-  
+ 
+  @Rule
+  public TestRule solrTestRules = 
+    RuleChain.outerRule(new SystemPropertiesRestoreRule());
+
   Server server;
   
   @Override

solr/solrj/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServer.java

@@ -7,6 +7,7 @@ import java.util.List;
 import junit.framework.Assert;
 
 import org.apache.lucene.util.LuceneTestCase;
+import org.apache.lucene.util.SystemPropertiesRestoreRule;
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.common.util.FileUtils;
 import org.apache.solr.core.CoreContainer;
@@ -14,11 +15,18 @@ import org.apache.solr.core.SolrCore;
 import org.apache.solr.util.AbstractSolrTestCase;
 import org.junit.After;
 import org.junit.Before;
+import org.junit.Rule;
+import org.junit.rules.RuleChain;
+import org.junit.rules.TestRule;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 public class TestEmbeddedSolrServer extends LuceneTestCase {
 
+  @Rule
+  public TestRule solrTestRules = 
+    RuleChain.outerRule(new SystemPropertiesRestoreRule());
+
   protected static Logger log = LoggerFactory.getLogger(TestEmbeddedSolrServer.class);
   
   protected CoreContainer cores = null;

solr/solrj/src/test/org/apache/solr/client/solrj/embedded/TestSolrProperties.java

@@ -29,6 +29,7 @@ import javax.xml.xpath.XPathFactory;
 
 import org.apache.commons.io.IOUtils;
 import org.apache.lucene.util.LuceneTestCase;
+import org.apache.lucene.util.SystemPropertiesRestoreRule;
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.client.solrj.SolrQuery;
 import org.apache.solr.client.solrj.SolrServer;
@@ -43,7 +44,10 @@ import org.apache.solr.core.CoreContainer;
 import org.apache.solr.util.AbstractSolrTestCase;
 import org.junit.After;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.RuleChain;
+import org.junit.rules.TestRule;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.w3c.dom.Document;
@@ -59,6 +63,10 @@ public class TestSolrProperties extends LuceneTestCase {
   private File home;
   private File solrXml;
   
+  @Rule
+  public TestRule solrTestRules = 
+    RuleChain.outerRule(new SystemPropertiesRestoreRule());
+
   private static final XPathFactory xpathFactory = XPathFactory.newInstance();
 
   public String getSolrHome() {

solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java

@@ -19,8 +19,25 @@
 package org.apache.solr;
 
 
-import org.apache.lucene.store.MockDirectoryWrapper;
+import java.io.File;
+import java.io.IOException;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.logging.ConsoleHandler;
+import java.util.logging.Handler;
+import java.util.logging.Level;
+
+import javax.xml.xpath.XPathExpressionException;
+
 import org.apache.lucene.util.LuceneTestCase;
+import org.apache.lucene.util.SystemPropertiesInvariantRule;
+import org.apache.lucene.util.SystemPropertiesRestoreRule;
 import org.apache.noggit.CharArr;
 import org.apache.noggit.JSONUtil;
 import org.apache.noggit.ObjectBuilder;
@@ -43,25 +60,16 @@ import org.apache.solr.schema.SchemaField;
 import org.apache.solr.search.SolrIndexSearcher;
 import org.apache.solr.servlet.DirectSolrConnection;
 import org.apache.solr.util.TestHarness;
-import org.apache.zookeeper.server.LogFormatter;
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.rules.RuleChain;
+import org.junit.rules.TestRule;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.xml.sax.SAXException;
 
-import javax.xml.xpath.XPathExpressionException;
-
-import java.io.Closeable;
-import java.io.File;
-import java.io.IOException;
-import java.io.StringWriter;
-import java.util.*;
-import java.util.Map.Entry;
-import java.util.logging.ConsoleHandler;
-import java.util.logging.Handler;
-import java.util.logging.Level;
-
 /**
  * A junit4 Solr test harness that extends LuceneTestCaseJ4.
  * Unlike AbstractSolrTestCase, a new core is not created for each test method.
@@ -69,6 +77,13 @@ import java.util.logging.Level;
  */
 public abstract class SolrTestCaseJ4 extends LuceneTestCase {
 
+  @ClassRule
+  public static TestRule solrClassRules = 
+    RuleChain.outerRule(new SystemPropertiesRestoreRule());
+
+  @Rule
+  public TestRule solrTestRules = 
+    RuleChain.outerRule(new SystemPropertiesRestoreRule());
 
   @BeforeClass
   public static void beforeClassSolrTestCase() throws Exception {

solr/test-framework/src/java/org/apache/solr/util/AbstractSolrTestCase.java

@@ -19,27 +19,34 @@
 package org.apache.solr.util;
 
 
+import java.io.File;
+import java.io.IOException;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+
+import javax.xml.xpath.XPathExpressionException;
+
 import org.apache.lucene.util.LuceneTestCase;
+import org.apache.lucene.util.SystemPropertiesRestoreRule;
 import org.apache.solr.SolrTestCaseJ4;
-import org.apache.solr.core.SolrConfig;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrInputDocument;
 import org.apache.solr.common.SolrInputField;
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.util.XML;
-import org.apache.solr.request.*;
+import org.apache.solr.core.SolrConfig;
+import org.apache.solr.request.SolrQueryRequest;
 import org.junit.AfterClass;
 import org.junit.BeforeClass;
-
-import org.xml.sax.SAXException;
-import org.slf4j.LoggerFactory;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.rules.RuleChain;
+import org.junit.rules.TestRule;
 import org.slf4j.Logger;
-import javax.xml.xpath.XPathExpressionException;
-
-import java.io.*;
-import java.util.HashSet;
-import java.util.List;
-import java.util.ArrayList;
+import org.slf4j.LoggerFactory;
+import org.xml.sax.SAXException;
 
 /**
  * An Abstract base class that makes writing Solr JUnit tests "easier"
@@ -55,7 +62,8 @@ import java.util.ArrayList;
  * @see #tearDown
  */
 public abstract class AbstractSolrTestCase extends LuceneTestCase {
-    protected SolrConfig solrConfig;
+  protected SolrConfig solrConfig;
+
   /**
    * Harness initialized by initTestHarness.
    *
@@ -94,6 +102,14 @@ public abstract class AbstractSolrTestCase extends LuceneTestCase {
     return SolrTestCaseJ4.TEST_HOME();
   }
   
+  @ClassRule
+  public static TestRule solrClassRules = 
+    RuleChain.outerRule(new SystemPropertiesRestoreRule());
+
+  @Rule
+  public TestRule solrTestRules = 
+    RuleChain.outerRule(new SystemPropertiesRestoreRule());
+  
   @BeforeClass
   public static void beforeClassAbstractSolrTestCase() throws Exception {
     SolrTestCaseJ4.startTrackingSearchers();