From 41b3ab66b7e6dfd7d17238b5459d9477eb67ca01 Mon Sep 17 00:00:00 2001 From: larsrc Date: Thu, 3 Dec 2020 05:07:56 -0800 Subject: [PATCH] Create TestUtils methods to generate unique temp dirs, deprecate the current ones that are decidedly non-unique. Unique temp dirs may take longer to set up, but not as long as dealing with flaky tests. PiperOrigin-RevId: 345428489 --- .../sharding/ShardingEnvironmentTest.java | 5 +- .../lib/dynamic/DynamicSpawnStrategyTest.java | 11 +++- .../lib/exec/local/LocalSpawnRunnerTest.java | 10 +-- .../sandbox/SandboxfsSandboxedSpawnTest.java | 3 +- .../shell/CommandUsingLinuxSandboxTest.java | 2 +- .../shell/CommandUsingProcessWrapperTest.java | 2 +- .../lib/shell/ExecutionStatisticsTest.java | 3 +- .../google/devtools/build/lib/testutil/BUILD | 3 + .../build/lib/testutil/TestUtils.java | 61 ++++++++++++++++--- 9 files changed, 73 insertions(+), 27 deletions(-) diff --git a/src/java_tools/junitrunner/javatests/com/google/testing/junit/runner/sharding/ShardingEnvironmentTest.java b/src/java_tools/junitrunner/javatests/com/google/testing/junit/runner/sharding/ShardingEnvironmentTest.java index 69d9f20601d396..831317a7538d97 100644 --- a/src/java_tools/junitrunner/javatests/com/google/testing/junit/runner/sharding/ShardingEnvironmentTest.java +++ b/src/java_tools/junitrunner/javatests/com/google/testing/junit/runner/sharding/ShardingEnvironmentTest.java @@ -18,6 +18,7 @@ import com.google.devtools.build.lib.testutil.TestUtils; import java.io.File; +import java.io.IOException; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -28,8 +29,8 @@ public class ShardingEnvironmentTest { @SuppressWarnings({"ResultOfMethodCallIgnored"}) @Test - public void testTouchShardingFile() { - File shardFile = new File(TestUtils.tmpDirFile(), "shard_file_123"); + public void testTouchShardingFile() throws IOException { + File shardFile = TestUtils.createUniqueTmpDir(null).getChild("shard_file_123").getPathFile(); assertThat(shardFile.exists()).isFalse(); try { ShardingEnvironment.touchShardFile(shardFile); diff --git a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java index f3a2f5b9d0392f..80de0f1323c4e8 100644 --- a/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java +++ b/src/test/java/com/google/devtools/build/lib/dynamic/DynamicSpawnStrategyTest.java @@ -61,6 +61,7 @@ import com.google.devtools.build.lib.vfs.Root; import com.google.devtools.build.lib.vfs.util.FileSystems; import com.google.devtools.common.options.OptionsParser; +import java.io.FileNotFoundException; import java.io.IOException; import java.util.Arrays; import java.util.Collection; @@ -217,8 +218,7 @@ boolean succeeded() { @Before public void setUp() throws Exception { - testRoot = FileSystems.getNativeFileSystem().getPath(TestUtils.tmpDir()).getRelative("test"); - testRoot.deleteTreesBelow(); + testRoot = TestUtils.createUniqueTmpDir(FileSystems.getNativeFileSystem()); outErr = new FileOutErr(testRoot.getRelative("stdout"), testRoot.getRelative("stderr")); } @@ -400,6 +400,13 @@ public void tearDown() throws Exception { if (executorServiceForCleanup != null) { executorServiceForCleanup.shutdownNow(); } + if (testRoot != null) { + try { + testRoot.deleteTree(); + } catch (FileNotFoundException e) { + // This can happen if one of the dynamic threads are still cleaning up. No big deal. + } + } } /** Constructs a new spawn with a custom mnemonic and execution info. */ diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java index e1214cf8a33239..d3664a5412d949 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java @@ -640,10 +640,7 @@ public void waitFor() throws InterruptedException { public void interruptWaitsForProcessExit() throws Exception { assumeTrue(OS.getCurrent() != OS.WINDOWS); - File tempDirFile = TestUtils.makeTempDir(); - tempDirFile.deleteOnExit(); - FileSystem fs = new JavaIoFileSystem(DigestHashFunction.SHA256); - Path tempDir = fs.getPath(tempDirFile.getPath()); + Path tempDir = TestUtils.createUniqueTmpDir(new JavaIoFileSystem(DigestHashFunction.SHA256)); LocalSpawnRunner runner = new LocalSpawnRunner( @@ -852,10 +849,7 @@ private Path copyCpuTimeSpenderIntoExecRoot(Path execRoot) throws IOException { } private Path getTemporaryRoot(FileSystem fs, String name) throws IOException { - File tempDirFile = TestUtils.makeTempDir(); - tempDirFile.deleteOnExit(); - - Path tempDirPath = fs.getPath(tempDirFile.getPath()); + Path tempDirPath = TestUtils.createUniqueTmpDir(fs); assertThat(tempDirPath.exists()).isTrue(); Path root = tempDirPath.getRelative(name); diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawnTest.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawnTest.java index f843bd741a15ea..1fe66390c99bcd 100644 --- a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawnTest.java +++ b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxfsSandboxedSpawnTest.java @@ -46,8 +46,7 @@ public class SandboxfsSandboxedSpawnTest { @Before public final void setupTestDirs() throws IOException { FileSystem fileSystem = new InMemoryFileSystem(DigestHashFunction.SHA256); - testRoot = fileSystem.getPath(TestUtils.tmpDir()); - testRoot.createDirectoryAndParents(); + testRoot = TestUtils.createUniqueTmpDir(fileSystem); workspaceDir = testRoot.getRelative("workspace"); workspaceDir.createDirectory(); diff --git a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingLinuxSandboxTest.java b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingLinuxSandboxTest.java index 13ef62fb5336a2..143d2372546e11 100644 --- a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingLinuxSandboxTest.java +++ b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingLinuxSandboxTest.java @@ -93,7 +93,7 @@ private void checkLinuxSandboxStatistics(Duration userTimeToSpend, Duration syst Long.toString(userTimeToSpend.getSeconds()), Long.toString(systemTimeToSpend.getSeconds())); - Path outputDir = testFS.getPath(TestUtils.makeTempDir().getCanonicalPath()); + Path outputDir = TestUtils.createUniqueTmpDir(testFS); Path statisticsFilePath = outputDir.getRelative("stats.out"); List fullCommandLine = diff --git a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java index 45d1f1c3edc204..ae5b5eeaf6f2b6 100644 --- a/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java +++ b/src/test/java/com/google/devtools/build/lib/shell/CommandUsingProcessWrapperTest.java @@ -88,7 +88,7 @@ private void checkProcessWrapperStatistics(Duration userTimeToSpend, Duration sy Long.toString(userTimeToSpend.getSeconds()), Long.toString(systemTimeToSpend.getSeconds())); - Path outputDir = testFS.getPath(TestUtils.makeTempDir().getCanonicalPath()); + Path outputDir = TestUtils.createUniqueTmpDir(testFS); Path statisticsFilePath = outputDir.getRelative("stats.out"); List fullCommandLine = diff --git a/src/test/java/com/google/devtools/build/lib/shell/ExecutionStatisticsTest.java b/src/test/java/com/google/devtools/build/lib/shell/ExecutionStatisticsTest.java index d43807ba4f1689..50ddd854d3ce4b 100644 --- a/src/test/java/com/google/devtools/build/lib/shell/ExecutionStatisticsTest.java +++ b/src/test/java/com/google/devtools/build/lib/shell/ExecutionStatisticsTest.java @@ -38,8 +38,7 @@ public final class ExecutionStatisticsTest { @Before public final void createFileSystem() throws Exception { FileSystem testFS = new InMemoryFileSystem(DigestHashFunction.SHA256); - workingDir = testFS.getPath(TestUtils.makeTempDir().getCanonicalPath()); - workingDir.createDirectoryAndParents(); + workingDir = TestUtils.createUniqueTmpDir(testFS); } private Path createExecutionStatisticsProtoFile( diff --git a/src/test/java/com/google/devtools/build/lib/testutil/BUILD b/src/test/java/com/google/devtools/build/lib/testutil/BUILD index 65b8f1a5069caf..7131f19bf45aa2 100644 --- a/src/test/java/com/google/devtools/build/lib/testutil/BUILD +++ b/src/test/java/com/google/devtools/build/lib/testutil/BUILD @@ -113,6 +113,9 @@ java_library( java_library( name = "TestUtils", srcs = ["TestUtils.java"], + deps = [ + "//src/main/java/com/google/devtools/build/lib/vfs", + ], ) java_library( 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 180faaeb000e4c..1af73976710a5e 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 @@ -14,6 +14,10 @@ package com.google.devtools.build.lib.testutil; +import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.JavaIoFileSystem; +import com.google.devtools.build.lib.vfs.Path; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.File; @@ -49,8 +53,14 @@ 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 + * cases may run in parallel, prefer using {@link #createUniqueTmpDir} instead, which + * also guarantees that the directory is empty. + */ public static String tmpDir() { - return tmpDirFile().getAbsolutePath().replaceAll("\\\\", "/"); + return tmpDirFile().getAbsolutePath().replace('\\', '/'); } static String getUserValue(String key) { @@ -61,10 +71,48 @@ static String getUserValue(String key) { return value; } + /** + * Returns a fixed temporary directory, guaranteed to exist and be unique for the test + * target. Since test cases may run in parallel, prefer using {@link + * #createUniqueTmpDir} instead, which also guarantees that the directory is empty. + */ public static File tmpDirFile() { - File tmpDir; + File tmpDir = tmpDirRoot(); + + // Ensure tmpDir exists + if (!tmpDir.isDirectory()) { + tmpDir.mkdirs(); + } + return tmpDir; + } + + /** + * Creates a unique and empty temporary directory. + * + * @param fileSystem The file system the directory should be created on. If null, uses the Java + * file system. + * @return A newly created directory, extremely likely to be unique. + */ + public static Path createUniqueTmpDir(FileSystem fileSystem) throws IOException { + if (fileSystem == null) { + fileSystem = new JavaIoFileSystem(DigestHashFunction.SHA256); + } + File tmpDirRoot = tmpDirRoot(); + Path path = fileSystem.getPath(tmpDirRoot.getPath()).getRelative(UUID.randomUUID().toString()); + path.createDirectoryAndParents(); + return path; + } - // Flag value specified in environment? + /** + * 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"); if (tmpDirStr != null && tmpDirStr.length() > 0) { @@ -82,15 +130,10 @@ public static File tmpDirFile() { tmpDir = new File(tmpDir, username); tmpDir = new File(tmpDir, "tmp"); } - - // Ensure tmpDir exists - if (!tmpDir.isDirectory()) { - tmpDir.mkdirs(); - } return tmpDir; } - public static File makeTempDir() throws IOException { + 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);