Add GlobalsHelper class to manage our global state

Use GlobalsHelper.pushState()/restoreState() before and after tests to ensure that state isn't carried between tests. This is applied to the AtomCacheTest to fix test regressions while simplifying the code.
biojava · sbliven · Jun 18, 2018 · Jun 4, 2018 · Jun 4, 2018 · Jun 8, 2018
commit 13f267c322d05de3aaa04e8daaebc4e5940529ff
diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/StructureIO.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/StructureIO.java
@@ -119,6 +119,11 @@ private static void checkInitAtomCache() {
 	public static void setAtomCache(AtomCache c){
 		cache = c;
 	}
+
+	public static AtomCache getAtomCache() {
+		checkInitAtomCache();
+		return cache;
+	}
 
 	/**
 	 * Returns the first biologicalAssembly that is available for a protein structure. For more documentation on quaternary structures see:

diff --git a/...structure/src/main/java/org/biojava/nbio/structure/io/mmcif/DownloadChemCompProvider.java b/...structure/src/main/java/org/biojava/nbio/structure/io/mmcif/DownloadChemCompProvider.java
@@ -95,25 +95,28 @@ public class DownloadChemCompProvider implements ChemCompProvider {
 	boolean downloadAll = false;
 
 	public DownloadChemCompProvider(){
-		logger.debug("Initialising DownloadChemCompProvider");
-
-		// note that path is static, so this is just to make sure that all non-static methods will have path initialised
-		initPath();
+		this(null);
 	}
 
 	public DownloadChemCompProvider(String cacheFilePath){
 		logger.debug("Initialising DownloadChemCompProvider");
 
 		// note that path is static, so this is just to make sure that all non-static methods will have path initialised
-		path = new File(cacheFilePath);
+		if(cacheFilePath != null) {
+			path = new File(cacheFilePath);
+		}
 	}
 
-	private static void initPath(){
-
+	/**
+	 * Get this provider's cache path
+	 * @return
+	 */
+	public static File getPath(){
 		if (path==null) {
 			UserConfiguration config = new UserConfiguration();
 			path = new File(config.getCacheFilePath());
 		}
+		return path;
 	}
 
 	/**
@@ -130,7 +133,7 @@ public void checkDoFirstInstall(){
 
 		// this makes sure there is a file separator between every component,
 		// if path has a trailing file separator or not, it will work for both cases
-		File dir = new File(path, CHEM_COMP_CACHE_DIRECTORY);
+		File dir = new File(getPath(), CHEM_COMP_CACHE_DIRECTORY);
 		File f = new File(dir, "components.cif.gz");
 
 		if ( ! f.exists()) {
@@ -164,7 +167,7 @@ private void split() throws IOException {
 
 		logger.info("Installing individual chem comp files ...");
 
-		File dir = new File(path, CHEM_COMP_CACHE_DIRECTORY);
+		File dir = new File(getPath(), CHEM_COMP_CACHE_DIRECTORY);
 		File f = new File(dir, "components.cif.gz");
 
 
@@ -215,7 +218,7 @@ private void split() throws IOException {
 	 */
 	private void writeID(String contents, String currentID) throws IOException{
 
-		String localName = DownloadChemCompProvider.getLocalFileName(currentID);
+		String localName = getLocalFileName(currentID);
 
 		try ( PrintWriter pw = new PrintWriter(new GZIPOutputStream(new FileOutputStream(localName))) ) {
 
@@ -322,16 +325,15 @@ public static String getLocalFileName(String recordName){
 			recordName = "_" + recordName;
 		}
 
-		initPath();
-
-		File f = new File(path, CHEM_COMP_CACHE_DIRECTORY);
+		File f = new File(getPath(), CHEM_COMP_CACHE_DIRECTORY);
 		if (! f.exists()){
 			logger.info("Creating directory " + f);
 
 			boolean success = f.mkdir();
 			// we've checked in initPath that path is writable, so there's no need to check if it succeeds
 			// in the unlikely case that in the meantime it isn't writable at least we log an error
-			if (!success) logger.error("Directory {} could not be created",f);
+			if (!success)
+				logger.error("Directory {} could not be created",f);
 
 		}
 

diff --git a/biojava-structure/src/test/java/org/biojava/nbio/structure/align/util/AtomCacheTest.java b/biojava-structure/src/test/java/org/biojava/nbio/structure/align/util/AtomCacheTest.java
@@ -55,12 +55,12 @@
 import org.biojava.nbio.structure.io.LocalPDBDirectory.ObsoleteBehavior;
 import org.biojava.nbio.structure.io.MMCIFFileReader;
 import org.biojava.nbio.structure.io.mmcif.ChemCompGroupFactory;
-import org.biojava.nbio.structure.io.mmcif.ChemCompProvider;
 import org.biojava.nbio.structure.io.mmcif.DownloadChemCompProvider;
 import org.biojava.nbio.structure.io.mmcif.model.ChemComp;
 import org.biojava.nbio.structure.io.util.FileDownloadUtils;
 import org.biojava.nbio.structure.scop.ScopDatabase;
 import org.biojava.nbio.structure.scop.ScopFactory;
+import org.biojava.nbio.structure.test.util.GlobalsHelper;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -77,32 +77,22 @@ public class AtomCacheTest {
 
 	private static Logger logger = LoggerFactory.getLogger(AtomCacheTest.class);
 	private AtomCache cache;
-	private String previousPDB_DIR;
-	private String previousPDB_CACHE_DIR;
-	private AtomCache cleanCache = new AtomCache();
-	private ChemCompProvider previousChemCompProvider = ChemCompGroupFactory.getChemCompProvider();
 
 	@Before
 	public void setUp() {
-		previousPDB_DIR = System.getProperty(UserConfiguration.PDB_DIR, null);
-		previousPDB_CACHE_DIR = System.getProperty(UserConfiguration.PDB_CACHE_DIR, null);
+		GlobalsHelper.pushState();
+
 		cache = new AtomCache();
 		cache.setObsoleteBehavior(ObsoleteBehavior.FETCH_OBSOLETE);
 		StructureIO.setAtomCache(cache);
 
 		// Use a fixed SCOP version for stability
 		ScopFactory.setScopDatabase(ScopFactory.VERSION_1_75B);
-		logger.warn("setUp()");
 	}
 
 	@After
 	public void tearDown() {
-		if (previousPDB_DIR != null) {
-			System.setProperty(UserConfiguration.PDB_DIR, previousPDB_DIR);
-			System.setProperty(UserConfiguration.PDB_CACHE_DIR, previousPDB_CACHE_DIR);
-		}
-		StructureIO.setAtomCache(cleanCache);
-		ChemCompGroupFactory.setChemCompProvider(previousChemCompProvider);
+		GlobalsHelper.restoreState();
 	}
 
 	/**

diff --git a/biojava-structure/src/test/java/org/biojava/nbio/structure/test/util/GlobalsHelper.java b/biojava-structure/src/test/java/org/biojava/nbio/structure/test/util/GlobalsHelper.java
@@ -0,0 +1,107 @@
+package org.biojava.nbio.structure.test.util;
+
+import java.util.Deque;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.NoSuchElementException;
+
+import org.biojava.nbio.structure.StructureIO;
+import org.biojava.nbio.structure.align.util.AtomCache;
+import org.biojava.nbio.structure.align.util.UserConfiguration;
+import org.biojava.nbio.structure.io.mmcif.ChemCompGroupFactory;
+import org.biojava.nbio.structure.io.mmcif.ChemCompProvider;
+import org.biojava.nbio.structure.io.mmcif.DownloadChemCompProvider;
+import org.biojava.nbio.structure.scop.ScopDatabase;
+import org.biojava.nbio.structure.scop.ScopFactory;
+
+/**
+ * Helper class to manage all the global state changes in BioJava.
+ * For instance, this should be used in tests before modifying PDB_PATH. 
+ * 
+ * Used by tests during setup and teardown to ensure a clean environment
+ * 
+ * This class is a singleton.
+ * @author Spencer Bliven
+ *
+ */
+public final class GlobalsHelper {
+
+	private static class PathInfo {
+		public final String pdbPath;
+		public final String pdbCachePath;
+		public final AtomCache atomCache;
+		public final ChemCompProvider chemCompProvider;
+		public final String downloadChemCompProviderPath;
+		public final ScopDatabase scop;
+
+		public PathInfo() {
+			pdbPath = System.getProperty(UserConfiguration.PDB_DIR, null);
+			pdbCachePath = System.getProperty(UserConfiguration.PDB_CACHE_DIR, null);
+			atomCache = StructureIO.getAtomCache();
+			chemCompProvider = ChemCompGroupFactory.getChemCompProvider();
+			downloadChemCompProviderPath = DownloadChemCompProvider.getPath().getPath();
+			scop = ScopFactory.getSCOP();
+		}
+	}
+
+	// Saves defaults as stack
+	private static Deque<PathInfo> stack = new LinkedList<>();
+	static {
+		// Save default state
+		pushState();
+	}
+
+	/**
+	 * GlobalsHelper should not be instantiated.
+	 */
+	private GlobalsHelper() {}
+
+	/**
+	 * Save current global state to the stack
+	 */
+	public static void pushState() {
+		PathInfo paths = new PathInfo();
+		stack.addFirst(paths);
+	}
+
+	/**
+	 * Sets a new PDB_PATH and PDB_CACHE_PATH consistently.
+	 * 
+	 * Previous values can be restored with {@link #restoreState()}.
+	 * @param path
+	 */
+	public static void setPdbPath(String path) {
+		pushState();
+		System.setProperty(UserConfiguration.PDB_DIR, path);
+		System.setProperty(UserConfiguration.PDB_CACHE_DIR, path);
+
+		AtomCache cache = new AtomCache(path);
+		StructureIO.setAtomCache(cache);
+
+		// Note side effect setting the path for all DownloadChemCompProvider due to static state
+		ChemCompProvider provider = new DownloadChemCompProvider(path);
+		ChemCompGroupFactory.setChemCompProvider(provider);
+	}
+
+	/**
+	 * Restore global state to the previous settings
+	 * @throws NoSuchElementException if there is no prior state to restore
+	 */
+	public static void restoreState() {
+		PathInfo paths = stack.removeFirst();
+
+		System.setProperty(UserConfiguration.PDB_DIR, paths.pdbPath);
+		System.setProperty(UserConfiguration.PDB_CACHE_DIR, paths.pdbCachePath);
+
+		StructureIO.setAtomCache(paths.atomCache);
+
+		// Use side effect setting the path for all DownloadChemCompProvider due to static state
+		new DownloadChemCompProvider(paths.downloadChemCompProviderPath);
+
+		ChemCompGroupFactory.setChemCompProvider(paths.chemCompProvider);
+
+		ScopFactory.setScopDatabase(paths.scop);
+	}
+
+
+}