Skip to content

Commit

Permalink
Clean up some unused and under-used TestUtils
Browse files Browse the repository at this point in the history
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
  • Loading branch information
michajlo authored and Copybara-Service committed Dec 29, 2021
1 parent 9ad8918 commit a731acd
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 65 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -49,6 +50,7 @@ public class GlobCacheTest {

private Path packageDirectory;
private Path buildFile;
private ExecutorService cacheThreadPool;
private GlobCache cache;

@Before
Expand Down Expand Up @@ -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,
Expand All @@ -118,7 +129,7 @@ public String getBaseNameForLoadedPackage(PackageIdentifier packageName) {
}
},
null,
TestUtils.getPool(),
cacheThreadPool,
-1,
ThreadStateReceiver.NULL_INSTANCE);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -1240,29 +1241,34 @@ private static void assertGlob(Package pkg, List<String> expected, String... inc
private static void assertGlob(
Package pkg, List<String> expected, List<String> include, List<String> 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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 <em>target</em>. Since test
Expand Down Expand Up @@ -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");
Expand All @@ -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;
Expand Down

0 comments on commit a731acd

Please sign in to comment.