Cleanup: move some tests {ArtifactFunction,FileArtifactValue}Test

These tests don't require a full Skyframe instance, so we might as well move
them to a lighter-weight test class. Also, it turns out that we have duplicate
tests for equality and hashing - this is now explicit.

PiperOrigin-RevId: 181285144

4 files changed, 135 insertions, 123 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
index 4e087dbbac..a525bf236a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java
@@ -261,8 +261,7 @@ public class ActionMetadataHandler implements MetadataHandler {
     // metadata separately.
     // Use the FileValue's digest if no digest was injected, or if the file can't be digested.
     injectedDigest = injectedDigest != null || !isFile ? injectedDigest : data.getDigest();
-    FileArtifactValue value =
-        FileArtifactValue.create(artifact, isFile, isFile ? data.getSize() : 0, injectedDigest);
+    FileArtifactValue value = FileArtifactValue.create(artifact, data, injectedDigest);
     FileArtifactValue oldValue = additionalOutputData.putIfAbsent(artifact, value);
     checkInconsistentData(artifact, oldValue, value);
     return metadataFromValue(value);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java
index 6bc9f5a1da..5a7ed29dc8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileArtifactValue.java
@@ -203,30 +203,40 @@ public abstract class FileArtifactValue implements SkyValue, Metadata {
 
   @VisibleForTesting
   public static FileArtifactValue create(Artifact artifact) throws IOException {
-    Path path = artifact.getPath();
-    FileStatus stat = path.stat();
-    boolean isFile = stat.isFile();
-    return create(artifact, isFile, isFile ? stat.getSize() : 0, null);
+    return create(artifact.getPath());
   }
 
   static FileArtifactValue create(Artifact artifact, FileValue fileValue) throws IOException {
     boolean isFile = fileValue.isFile();
-    return create(artifact, isFile, isFile ? fileValue.getSize() : 0,
+    return create(artifact.getPath(), isFile, isFile ? fileValue.getSize() : 0,
         isFile ? fileValue.getDigest() : null);
   }
 
-  static FileArtifactValue create(Artifact artifact, boolean isFile, long size,
-      @Nullable byte[] digest) throws IOException {
+  static FileArtifactValue create(
+      Artifact artifact, FileValue fileValue, @Nullable byte[] injectedDigest) throws IOException {
+    boolean isFile = fileValue.isFile();
+    return create(artifact.getPath(), isFile, isFile ? fileValue.getSize() : 0, injectedDigest);
+  }
+
+  @VisibleForTesting
+  static FileArtifactValue create(Path path) throws IOException {
+    FileStatus stat = path.stat();
+    boolean isFile = stat.isFile();
+    return create(path, isFile, isFile ? stat.getSize() : 0, null);
+  }
+
+  private static FileArtifactValue create(
+      Path path, boolean isFile, long size, @Nullable byte[] digest) throws IOException {
     if (isFile && digest == null) {
-      digest = DigestUtils.getDigestOrFail(artifact.getPath(), size);
+      digest = DigestUtils.getDigestOrFail(path, size);
     }
     if (!isFile) {
       // In this case, we need to store the mtime because the action cache uses mtime for
       // directories to determine if this artifact has changed. We want this code path to go away
       // somehow (maybe by implementing FileSet in Skyframe).
-      return new DirectoryArtifactValue(artifact.getPath().getLastModifiedTime());
+      return new DirectoryArtifactValue(path.getLastModifiedTime());
     }
-    Preconditions.checkState(digest != null, artifact);
+    Preconditions.checkState(digest != null, path);
     return createNormalFile(digest, size);
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 4004e85b8f..6e321c53aa 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -21,7 +21,6 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
-import com.google.common.testing.EqualsTester;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata.MiddlemanType;
@@ -151,27 +150,6 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase {
         .containsExactly(Pair.of(input1, create(input1)), Pair.of(input2, create(input2)));
   }
 
-  @Test
-  public void testIOException() throws Exception {
-    fastDigest = false;
-    final IOException exception = new IOException("beep");
-    setupRoot(
-        new CustomInMemoryFs() {
-          @Override
-          public byte[] getDigest(Path path, HashFunction hf) throws IOException {
-            throw exception;
-          }
-        });
-    Artifact artifact = createDerivedArtifact("no-read");
-    writeFile(artifact.getPath(), "content");
-    try {
-      create(createDerivedArtifact("no-read"));
-      fail();
-    } catch (IOException e) {
-      assertThat(e).isSameAs(exception);
-    }
-  }
-
   /**
    * Tests that ArtifactFunction rethrows transitive {@link IOException}s as
    * {@link MissingInputFileException}s.
@@ -198,95 +176,6 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase {
   }
 
   @Test
-  public void testNoMtimeIfNonemptyFile() throws Exception {
-    Artifact artifact = createDerivedArtifact("no-digest");
-    Path path = artifact.getPath();
-    writeFile(path, "hello"); //Non-empty file.
-    FileArtifactValue value = create(artifact);
-    assertThat(value.getDigest()).isEqualTo(path.getDigest());
-    try {
-      value.getModifiedTime();
-      fail("mtime for non-empty file should not be stored.");
-    } catch (UnsupportedOperationException e) {
-      // Expected.
-    }
-  }
-
-  @Test
-  public void testDirectory() throws Exception {
-    Artifact artifact = createDerivedArtifact("dir");
-    Path path = artifact.getPath();
-    FileSystemUtils.createDirectoryAndParents(path);
-    path.setLastModifiedTime(1L);
-    FileArtifactValue value = create(artifact);
-    assertThat(value.getDigest()).isNull();
-    assertThat(value.getModifiedTime()).isEqualTo(1L);
-  }
-
-  // Empty files are the same as normal files -- mtime is not stored.
-  @Test
-  public void testEmptyFile() throws Exception {
-    Artifact artifact = createDerivedArtifact("empty");
-    Path path = artifact.getPath();
-    writeFile(path, "");
-    path.setLastModifiedTime(1L);
-    FileArtifactValue value = create(artifact);
-    assertThat(value.getDigest()).isEqualTo(path.getDigest());
-    assertThat(value.getSize()).isEqualTo(0L);
-  }
-
-  @Test
-  public void testEquality() throws Exception {
-    Artifact artifact1 = createDerivedArtifact("artifact1");
-    Artifact artifact2 = createDerivedArtifact("artifact2");
-    Artifact diffDigest = createDerivedArtifact("diffDigest");
-    Artifact diffMtime = createDerivedArtifact("diffMtime");
-    Artifact empty1 = createDerivedArtifact("empty1");
-    Artifact empty2 = createDerivedArtifact("empty2");
-    Artifact empty3 = createDerivedArtifact("empty3");
-    Artifact dir1 = createDerivedArtifact("dir1");
-    Artifact dir2 = createDerivedArtifact("dir2");
-    Artifact dir3 = createDerivedArtifact("dir3");
-    Path path1 = artifact1.getPath();
-    Path path2 = artifact2.getPath();
-    Path digestPath = diffDigest.getPath();
-    Path mtimePath = diffMtime.getPath();
-    writeFile(artifact1.getPath(), "content");
-    writeFile(artifact2.getPath(), "content");
-    path1.setLastModifiedTime(0);
-    path2.setLastModifiedTime(0);
-    writeFile(diffDigest.getPath(), "1234567"); // Same size as artifact1.
-    digestPath.setLastModifiedTime(0);
-    writeFile(mtimePath, "content");
-    mtimePath.setLastModifiedTime(1);
-    Path emptyPath1 = empty1.getPath();
-    Path emptyPath2 = empty2.getPath();
-    Path emptyPath3 = empty3.getPath();
-    writeFile(emptyPath1, "");
-    writeFile(emptyPath2, "");
-    writeFile(emptyPath3, "");
-    emptyPath1.setLastModifiedTime(0L);
-    emptyPath2.setLastModifiedTime(1L);
-    emptyPath3.setLastModifiedTime(1L);
-    Path dirPath1 = dir1.getPath();
-    Path dirPath2 = dir2.getPath();
-    Path dirPath3 = dir3.getPath();
-    FileSystemUtils.createDirectoryAndParents(dirPath1);
-    FileSystemUtils.createDirectoryAndParents(dirPath2);
-    FileSystemUtils.createDirectoryAndParents(dirPath3);
-    dirPath1.setLastModifiedTime(0L);
-    dirPath2.setLastModifiedTime(1L);
-    dirPath3.setLastModifiedTime(1L);
-    EqualsTester equalsTester = new EqualsTester();
-    equalsTester
-        .addEqualityGroup(create(artifact1), create(artifact2), create(diffMtime))
-        .addEqualityGroup(create(empty1), create(empty2), create(empty3))
-        .addEqualityGroup(create(dir1))
-        .addEqualityGroup(create(dir2), create(dir3))
-        .testEquals();
-  }
-
-  @Test
   public void testActionTreeArtifactOutput() throws Throwable {
     Artifact artifact = createDerivedTreeArtifactWithAction("treeArtifact");
     TreeFileArtifact treeFileArtifact1 = createFakeTreeFileArtifact(artifact, "child1", "hello1");
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java
index 641c28e283..ccf282243f 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java
@@ -13,14 +13,39 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.skyframe.FileArtifactValue.create;
+import static org.junit.Assert.fail;
+
 import com.google.common.io.BaseEncoding;
 import com.google.common.testing.EqualsTester;
+import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
+import java.io.IOException;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 @RunWith(JUnit4.class)
 public class FileArtifactValueTest {
+  private final FileSystem fs = new InMemoryFileSystem();
+
+  private Path scratchFile(String name, long mtime, String content) throws IOException {
+    Path path = fs.getPath(name);
+    FileSystemUtils.createDirectoryAndParents(path.getParentDirectory());
+    FileSystemUtils.writeContentAsLatin1(path, content);
+    path.setLastModifiedTime(mtime);
+    return path;
+  }
+
+  private Path scratchDir(String name, long mtime) throws IOException {
+    Path path = fs.getPath(name);
+    FileSystemUtils.createDirectoryAndParents(path);
+    path.setLastModifiedTime(mtime);
+    return path;
+  }
 
   private static byte[] toBytes(String hex) {
     return BaseEncoding.base16().upperCase().decode(hex);
@@ -48,4 +73,93 @@ public class FileArtifactValueTest {
         .addEqualityGroup("a string")
         .testEquals();
   }
+
+  @Test
+  public void testEquality() throws Exception {
+    Path path1 = scratchFile("/dir/artifact1", 0L, "content");
+    Path path2 = scratchFile("/dir/artifact2", 0L, "content");
+    Path digestPath = scratchFile("/dir/diffDigest", 0L, "1234567");
+    Path mtimePath = scratchFile("/dir/diffMtime", 1L, "content");
+
+    Path empty1 = scratchFile("/dir/empty1", 0L, "");
+    Path empty2 = scratchFile("/dir/empty2", 1L, "");
+    Path empty3 = scratchFile("/dir/empty3", 1L, "");
+
+    Path dir1 = scratchDir("/dir1", 0L);
+    Path dir2 = scratchDir("/dir2", 1L);
+    Path dir3 = scratchDir("/dir3", 1L);
+
+    new EqualsTester()
+        .addEqualityGroup(create(path1), create(path2), create(mtimePath))
+        .addEqualityGroup(create(digestPath))
+        .addEqualityGroup(create(empty1), create(empty2), create(empty3))
+        .addEqualityGroup(create(dir1))
+        .addEqualityGroup(create(dir2), create(dir3))
+        .testEquals();
+  }
+
+  @Test
+  public void testNoMtimeIfNonemptyFile() throws Exception {
+    Path path = scratchFile("/root/non-empty", 1L, "abc");
+    FileArtifactValue value = create(path);
+    assertThat(value.getDigest()).isEqualTo(path.getDigest());
+    assertThat(value.getSize()).isEqualTo(3L);
+    try {
+      value.getModifiedTime();
+      fail("mtime for non-empty file should not be stored.");
+    } catch (UnsupportedOperationException e) {
+      // Expected.
+    }
+  }
+
+  @Test
+  public void testDirectory() throws Exception {
+    Path path = fs.getPath("/dir");
+    FileSystemUtils.createDirectoryAndParents(path);
+    path.setLastModifiedTime(1L);
+    FileArtifactValue value = create(path);
+    assertThat(value.getDigest()).isNull();
+    assertThat(value.getModifiedTime()).isEqualTo(1L);
+  }
+
+  // Empty files are the same as normal files -- mtime is not stored.
+  @Test
+  public void testEmptyFile() throws Exception {
+    Path path = scratchFile("/root/empty", 1L, "");
+    path.setLastModifiedTime(1L);
+    FileArtifactValue value = create(path);
+    assertThat(value.getDigest()).isEqualTo(path.getDigest());
+    assertThat(value.getSize()).isEqualTo(0L);
+    try {
+      value.getModifiedTime();
+      fail("mtime for non-empty file should not be stored.");
+    } catch (UnsupportedOperationException e) {
+      // Expected.
+    }
+  }
+
+  @Test
+  public void testIOException() throws Exception {
+    final IOException exception = new IOException("beep");
+    FileSystem fs = new InMemoryFileSystem() {
+      @Override
+      public byte[] getDigest(Path path, HashFunction hf) throws IOException {
+        throw exception;
+      }
+
+      @Override
+      protected byte[] getFastDigest(Path path, HashFunction hashFunction) throws IOException {
+        throw exception;
+      }
+    };
+    Path path = fs.getPath("/some/path");
+    FileSystemUtils.createDirectoryAndParents(path.getParentDirectory());
+    FileSystemUtils.writeContentAsLatin1(path, "content");
+    try {
+      create(path);
+      fail();
+    } catch (IOException e) {
+      assertThat(e).isSameAs(exception);
+    }
+  }
 }