From a731acd1b40ca888e4f371c2e6fa265e4eb95fd6 Mon Sep 17 00:00:00 2001 From: michajlo Date: Wed, 29 Dec 2021 12:34:00 -0800 Subject: [PATCH] Clean up some unused and under-used TestUtils getPool was reusing the same thread pool across tests, instead create new pools for each test. advanceCurrentTimeSeconds' name gives the illusion it actually moves the clock, as is customary for mock clocks these days, however it actually slept, which isn't ideal in tests either. The rest is unused. PiperOrigin-RevId: 418832178 --- .../build/lib/packages/GlobCacheTest.java | 15 +++++- .../lib/packages/PackageFactoryTest.java | 54 ++++++++++--------- .../build/lib/testutil/TestUtils.java | 39 -------------- 3 files changed, 43 insertions(+), 65 deletions(-) diff --git a/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java b/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java index f1b693fe7aceac..b830b661482603 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/GlobCacheTest.java @@ -22,7 +22,6 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.packages.Globber.BadGlobException; import com.google.devtools.build.lib.testutil.Scratch; -import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.util.Pair; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -31,6 +30,8 @@ import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -49,6 +50,7 @@ public class GlobCacheTest { private Path packageDirectory; private Path buildFile; + private ExecutorService cacheThreadPool; private GlobCache cache; @Before @@ -92,7 +94,16 @@ public final void createFiles() throws Exception { createCache(); } + @After + public void shutDownThreadPoolIfExists() { + if (cacheThreadPool != null) { + cacheThreadPool.shutdownNow(); + } + } + private void createCache(PathFragment... ignoredDirectories) { + shutDownThreadPoolIfExists(); + cacheThreadPool = Executors.newFixedThreadPool(10); cache = new GlobCache( packageDirectory, @@ -118,7 +129,7 @@ public String getBaseNameForLoadedPackage(PackageIdentifier packageName) { } }, null, - TestUtils.getPool(), + cacheThreadPool, -1, ThreadStateReceiver.NULL_INSTANCE); } diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java index 17105880932945..d3df2e490fa21d 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java @@ -31,7 +31,6 @@ import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.PackageValidator.InvalidPackageException; import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase; -import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.FileSystem; @@ -47,6 +46,8 @@ import java.util.HashMap; import java.util.List; import java.util.Set; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import net.starlark.java.eval.Starlark; import net.starlark.java.syntax.ParserInput; import net.starlark.java.syntax.StarlarkFile; @@ -1240,29 +1241,34 @@ private static void assertGlob(Package pkg, List expected, String... inc private static void assertGlob( Package pkg, List expected, List include, List exclude) throws Exception { - GlobCache globCache = - new GlobCache( - pkg.getFilename().asPath().getParentDirectory(), - pkg.getPackageIdentifier(), - ImmutableSet.of(), - // a package locator that finds no packages - new CachingPackageLocator() { - @Override - public Path getBuildFileForPackage(PackageIdentifier packageName) { - return null; - } - - @Override - public String getBaseNameForLoadedPackage(PackageIdentifier packageName) { - return null; - } - }, - null, - TestUtils.getPool(), - -1, - ThreadStateReceiver.NULL_INSTANCE); - assertThat(globCache.globUnsorted(include, exclude, false, true)) - .containsExactlyElementsIn(expected); + ExecutorService executorService = Executors.newFixedThreadPool(10); + try { + GlobCache globCache = + new GlobCache( + pkg.getFilename().asPath().getParentDirectory(), + pkg.getPackageIdentifier(), + ImmutableSet.of(), + // a package locator that finds no packages + new CachingPackageLocator() { + @Override + public Path getBuildFileForPackage(PackageIdentifier packageName) { + return null; + } + + @Override + public String getBaseNameForLoadedPackage(PackageIdentifier packageName) { + return null; + } + }, + null, + executorService, + -1, + ThreadStateReceiver.NULL_INSTANCE); + assertThat(globCache.globUnsorted(include, exclude, false, true)) + .containsExactlyElementsIn(expected); + } finally { + executorService.shutdownNow(); + } } private Path emptyFile(String path) { diff --git a/src/test/java/com/google/devtools/build/lib/testutil/TestUtils.java b/src/test/java/com/google/devtools/build/lib/testutil/TestUtils.java index fc067626c2db9c..1fca90264fec0e 100644 --- a/src/test/java/com/google/devtools/build/lib/testutil/TestUtils.java +++ b/src/test/java/com/google/devtools/build/lib/testutil/TestUtils.java @@ -21,34 +21,14 @@ import java.io.File; import java.io.IOException; import java.util.UUID; -import java.util.concurrent.Executors; -import java.util.concurrent.ThreadPoolExecutor; /** * Some static utility functions for testing. */ public class TestUtils { - public static final ThreadPoolExecutor POOL = - (ThreadPoolExecutor) Executors.newFixedThreadPool(10); public static final UUID ZERO_UUID = UUID.fromString("00000000-0000-0000-0000-000000000000"); - /** - * Wait until the {@link System#currentTimeMillis} / 1000 advances. - * - * This method takes 0-1000ms to run, 500ms on average. - */ - public static void advanceCurrentTimeSeconds() throws InterruptedException { - long currentTimeSeconds = System.currentTimeMillis() / 1000; - do { - Thread.sleep(50); - } while (currentTimeSeconds == System.currentTimeMillis() / 1000); - } - - public static ThreadPoolExecutor getPool() { - return POOL; - } - /** * Returns the path to a fixed temporary directory, with back-slashes turned into slashes. The * directory is guaranteed to exist and be unique for the test target. Since test @@ -99,14 +79,6 @@ public static Path createUniqueTmpDir(FileSystem fileSystem) throws IOException return path; } - /** - * Creates a unique and empty temporary directory and returns the path, with backslashes turned - * into slashes. - */ - public static String createUniqueTmpDirString() throws IOException { - return createUniqueTmpDir(null).getPathString().replace('\\', '/'); - } - private static File tmpDirRoot() { File tmpDir; // Flag value specified in environment? String tmpDirStr = getUserValue("TEST_TMPDIR"); @@ -129,17 +101,6 @@ private static File tmpDirRoot() { return tmpDir; } - public static File makeTmpDir() throws IOException { - File dir = File.createTempFile(TestUtils.class.getName(), ".temp", tmpDirFile()); - if (!dir.delete()) { - throw new IOException("Cannot remove a temporary file " + dir); - } - if (!dir.mkdir()) { - throw new IOException("Cannot create a temporary directory " + dir); - } - return dir; - } - public static int getRandomSeed() { // Default value if not running under framework int randomSeed = 301;