(Rebase) Add snapshot_images_diff_path to apple_test (facebook#2528)

* Add snapshot_images_diff_path to apple_test * Fix some issues found at code review Co-authored-by: Lucas Marçal <lucas.fernandes@easynvest.com.br>
ifood · Lcsmarcal · Oct 20, 2020 · Jun 25, 2020 · Jun 29, 2020 · Jun 29, 2020
commit ffa062a699a248d49b8239fe013484b45559eba5
diff --git a/src/com/facebook/buck/apple/AppleTest.java b/src/com/facebook/buck/apple/AppleTest.java
@@ -104,6 +104,7 @@ public class AppleTest extends AbstractBuildRuleWithDeclaredAndExtraDeps
   private final Path testLogsPath;
 
   private final Optional<Either<SourcePath, String>> snapshotReferenceImagesPath;
+  private final Optional<Either<SourcePath, String>> snapshotImagesDiffPath;
 
   private Optional<Long> testRuleTimeoutMs;
 
@@ -216,6 +217,7 @@ public ImmutableList<TestCaseSummary> getTestCaseSummaries() {
       Optional<Long> testRuleTimeoutMs,
       boolean isUiTest,
       Optional<Either<SourcePath, String>> snapshotReferenceImagesPath,
+      Optional<Either<SourcePath, String>> snapshotImagesDiffPath,
       Optional<ImmutableMap<String, String>> testSpecificEnvironmentVariables,
       boolean useIdb,
       Path idbPath) {
@@ -244,6 +246,7 @@ public ImmutableList<TestCaseSummary> getTestCaseSummaries() {
     this.testLogLevel = testLogLevel;
     this.isUiTest = isUiTest;
     this.snapshotReferenceImagesPath = snapshotReferenceImagesPath;
+    this.snapshotImagesDiffPath = snapshotImagesDiffPath;
     this.testSpecificEnvironmentVariables = testSpecificEnvironmentVariables;
     this.useIdb = useIdb;
     this.idbPath = idbPath;
@@ -345,23 +348,8 @@ public Pair<ImmutableList<Step>, ExternalTestRunnerTestSpec> getTestCommand(
         destinationSpecifierArg = defaultDestinationSpecifier;
       }
 
-      Optional<String> snapshotReferenceImagesPath = Optional.empty();
-      if (this.snapshotReferenceImagesPath.isPresent()) {
-        if (this.snapshotReferenceImagesPath.get().isLeft()) {
-          snapshotReferenceImagesPath =
-              Optional.of(
-                  buildContext
-                      .getSourcePathResolver()
-                      .getAbsolutePath(this.snapshotReferenceImagesPath.get().getLeft())
-                      .toString());
-        } else if (this.snapshotReferenceImagesPath.get().isRight()) {
-          snapshotReferenceImagesPath =
-              Optional.of(
-                  getProjectFilesystem()
-                      .getPathForRelativePath(this.snapshotReferenceImagesPath.get().getRight())
-                      .toString());
-        }
-      }
+      Optional<String> snapshotReferenceImagesPath = getAbsoluteSnapshotTestingArgumentPath(this.snapshotReferenceImagesPath, buildContext);
+      Optional<String> snapshotImagesDiffPath = getAbsoluteSnapshotTestingArgumentPath(this.snapshotImagesDiffPath, buildContext);
 
       XctoolRunTestsStep xctoolStep =
           new XctoolRunTestsStep(
@@ -384,7 +372,8 @@ public Pair<ImmutableList<Step>, ExternalTestRunnerTestSpec> getTestCommand(
               Optional.of(testLogLevelEnvironmentVariable),
               Optional.of(testLogLevel),
               testRuleTimeoutMs,
-              snapshotReferenceImagesPath);
+              snapshotReferenceImagesPath,
+              snapshotImagesDiffPath);
 
       if (useIdb) {
         idbStdoutReader = Optional.of(new AppleTestIdbStdoutReader(testReportingCallback));
@@ -456,6 +445,25 @@ public Pair<ImmutableList<Step>, ExternalTestRunnerTestSpec> getTestCommand(
     return new Pair<>(steps.build(), externalSpec.build());
   }
 
+  private Optional<String> getAbsoluteSnapshotTestingArgumentPath(
+    Optional<Either<SourcePath, String>> snapshotTestArgument, BuildContext buildContext) {
+    if (snapshotTestArgument.isPresent()) {
+      if (snapshotTestArgument.get().isLeft()) {
+        return Optional.of(
+          buildContext
+            .getSourcePathResolver()
+            .getAbsolutePath(snapshotTestArgument.get().getLeft())
+            .toString());
+      } else if (snapshotTestArgument.get().isRight()) {
+        return Optional.of(
+          getProjectFilesystem()
+            .getPathForRelativePath(snapshotTestArgument.get().getRight())
+            .toString());
+      }
+    }
+    return Optional.empty();
+  }
+
   static Optional<Path> extractBundlePathForBundle(
       Optional<AppleBundle> bundle, SourcePathResolverAdapter sourcePathResolverAdapter) {
     if (!bundle.isPresent()) {

diff --git a/src/com/facebook/buck/apple/AppleTestDescription.java b/src/com/facebook/buck/apple/AppleTestDescription.java
@@ -421,6 +421,7 @@ public BuildRule createBuildRule(
               AppleDeveloperDirectoryForTestsProvider.class),
           args.getIsUiTest(),
           args.getSnapshotReferenceImagesPath(),
+          args.getSnapshotImagesDiffPath(),
           appleConfig.useIdb(),
           appleConfig.getIdbPath());
     }
@@ -458,6 +459,7 @@ public BuildRule createBuildRule(
                     .getDefaultTestRuleTimeoutMs()),
         args.getIsUiTest(),
         args.getSnapshotReferenceImagesPath(),
+        args.getSnapshotImagesDiffPath(),
         args.getEnv(),
         appleConfig.useIdb(),
         appleConfig.getIdbPath());
@@ -952,6 +954,9 @@ default boolean getIsUiTest() {
     // for use with FBSnapshotTestcase, injects the path as FB_REFERENCE_IMAGE_DIR
     Optional<Either<SourcePath, String>> getSnapshotReferenceImagesPath();
 
+    // for use with FBSnapshotTestcase, injects the path as IMAGE_DIFF_DIR
+    Optional<Either<SourcePath, String>> getSnapshotImagesDiffPath();
+
     // Bundle related fields.
     ImmutableMap<String, String> getDestinationSpecifier();
 

diff --git a/src/com/facebook/buck/apple/AppleTestX.java b/src/com/facebook/buck/apple/AppleTestX.java
@@ -104,6 +104,7 @@ public class AppleTestX extends AbstractBuildRuleWithDeclaredAndExtraDeps
       AppleDeveloperDirectoryForTestsProvider appleDeveloperDirectoryForTestsProvider,
       boolean isUiTest,
       Optional<Either<SourcePath, String>> snapshotReferenceImagesPath,
+      Optional<Either<SourcePath, String>> snapshotImagesDiffPath,
       boolean useIdb,
       Path idbPath) {
     super(buildTarget, projectFilesystem, params);
@@ -121,6 +122,7 @@ public class AppleTestX extends AbstractBuildRuleWithDeclaredAndExtraDeps
             appleDeveloperDirectoryForTestsProvider,
             isUiTest,
             snapshotReferenceImagesPath,
+            snapshotImagesDiffPath,
             useIdb,
             idbPath);
 
@@ -248,6 +250,7 @@ private static class AppleConfigs implements AddsToRuleKey {
     public static final String DEFAULT_DESTINATION = "default_destination";
     public static final String DEVELOPER_DIRECTORY_FOR_TESTS = "developer_directory_for_tests";
     public static final String SNAPSHOT_REFERENCE_IMG_PATH = "snapshot_reference_img_path";
+    public static final String SNAPSHOT_IMAGES_DIFF_PATH = "snapshot_images_diff_path";
 
     private static final String UI_TEST_TARGET_APP = "ui_test_target_app";
     private static final String TEST_HOST_APP = "test_host_app";
@@ -270,6 +273,7 @@ private static class AppleConfigs implements AddsToRuleKey {
 
     @AddToRuleKey private final boolean isUiTest;
     private final Optional<Either<SourcePath, String>> snapshotReferenceImagesPath;
+    private final Optional<Either<SourcePath, String>> snapshotImagesDiffPath;
     private final boolean useIdb;
     private final Path idbPath;
 
@@ -285,6 +289,7 @@ private AppleConfigs(
         AppleDeveloperDirectoryForTestsProvider appleDeveloperDirectoryForTestsProvider,
         boolean isUiTest,
         Optional<Either<SourcePath, String>> snapshotReferenceImagesPath,
+        Optional<Either<SourcePath, String>> snapshotImagesDiffPath,
         boolean useIdb,
         Path idbPath) {
       this.testHostApp = testHostApp;
@@ -298,6 +303,7 @@ private AppleConfigs(
       this.appleDeveloperDirectoryForTestsProvider = appleDeveloperDirectoryForTestsProvider;
       this.isUiTest = isUiTest;
       this.snapshotReferenceImagesPath = snapshotReferenceImagesPath;
+      this.snapshotImagesDiffPath = snapshotImagesDiffPath;
       this.useIdb = useIdb;
       this.idbPath = idbPath;
     }
@@ -341,6 +347,19 @@ private String asJson(
                     })
                 .orElse(""));
 
+        generator.writeStringField(
+          SNAPSHOT_IMAGES_DIFF_PATH,
+          snapshotImagesDiffPath
+            .map(
+              pathOrStr -> {
+                if (pathOrStr.isLeft()) {
+                  return sourcePathResolver.getAbsolutePath(pathOrStr.getLeft()).toString();
+                }
+
+                return filesystem.getPathForRelativePath(pathOrStr.getRight()).toString();
+              })
+            .orElse(""));
+
         generator.writeObjectField(
             UI_TEST_TARGET_APP,
             AppleTest.extractBundlePathForBundle(uiTestTargetApp, sourcePathResolver)

diff --git a/src/com/facebook/buck/apple/XctoolRunTestsStep.java b/src/com/facebook/buck/apple/XctoolRunTestsStep.java
@@ -74,6 +74,7 @@ class XctoolRunTestsStep implements Step {
       Executors.newSingleThreadScheduledExecutor();
   private static final String XCTOOL_ENV_VARIABLE_PREFIX = "XCTOOL_TEST_ENV_";
   private static final String FB_REFERENCE_IMAGE_DIR = "FB_REFERENCE_IMAGE_DIR";
+  private static final String IMAGE_DIFF_DIR = "IMAGE_DIFF_DIR";
 
   private final ProjectFilesystem filesystem;
 
@@ -96,6 +97,7 @@ public interface StdoutReadingCallback {
   private final Optional<String> logLevel;
   private final Optional<Long> timeoutInMs;
   private final Optional<String> snapshotReferenceImagesPath;
+  private final Optional<String> snapshotImagesDiffPath;
   private final Map<Path, Map<Path, Path>> appTestPathsToTestHostAppPathsToTestTargetAppPaths;
   private final boolean isUsingXCodeBuildTool;
 
@@ -172,7 +174,8 @@ public XctoolRunTestsStep(
       Optional<String> logLevelEnvironmentVariable,
       Optional<String> logLevel,
       Optional<Long> timeoutInMs,
-      Optional<String> snapshotReferenceImagesPath) {
+      Optional<String> snapshotReferenceImagesPath,
+      Optional<String> snapshotImagesDiffPath) {
     Preconditions.checkArgument(
         !(logicTestBundlePaths.isEmpty()
             && appTestBundleToHostAppPaths.isEmpty()
@@ -207,6 +210,7 @@ public XctoolRunTestsStep(
     this.logLevel = logLevel;
     this.timeoutInMs = timeoutInMs;
     this.snapshotReferenceImagesPath = snapshotReferenceImagesPath;
+    this.snapshotImagesDiffPath = snapshotImagesDiffPath;
     // Super hacky, but xcodebuildtool is an alternative wrapper
     // around xcodebuild and forwarding the -f arguments only makes
     // sense in that context.
@@ -240,6 +244,11 @@ public ImmutableMap<String, String> getEnv(ExecutionContext context) {
           XCTOOL_ENV_VARIABLE_PREFIX + FB_REFERENCE_IMAGE_DIR, snapshotReferenceImagesPath.get());
     }
 
+    if (snapshotImagesDiffPath.isPresent()) {
+      environment.put(
+        XCTOOL_ENV_VARIABLE_PREFIX + IMAGE_DIFF_DIR, snapshotImagesDiffPath.get());
+    }
+
     environment.putAll(this.environmentOverrides);
     return ImmutableMap.copyOf(environment);
   }

diff --git a/test/com/facebook/buck/apple/XctoolRunTestsStepTest.java b/test/com/facebook/buck/apple/XctoolRunTestsStepTest.java
@@ -76,6 +76,7 @@ public void xctoolCommandWithOnlyLogicTests() throws Exception {
             Optional.empty(),
             Optional.empty(),
             Optional.empty(),
+            Optional.empty(),
             Optional.empty());
     ProcessExecutorParams xctoolParams =
         ProcessExecutorParams.builder()
@@ -128,6 +129,7 @@ public void xctoolCommandWhichFailsPrintsStderrToConsole() throws Exception {
             Optional.empty(),
             Optional.empty(),
             Optional.empty(),
+            Optional.empty(),
             Optional.empty());
     ProcessExecutorParams xctoolParams =
         ProcessExecutorParams.builder()
@@ -185,6 +187,7 @@ public void xctoolCommandWithOnlyAppTests() throws Exception {
             Optional.empty(),
             Optional.empty(),
             Optional.empty(),
+            Optional.empty(),
             Optional.empty());
 
     ProcessExecutorParams xctoolParams =
@@ -242,6 +245,7 @@ public void xctoolCommandWithOnlyUITests() throws Exception {
             Optional.empty(),
             Optional.empty(),
             Optional.empty(),
+            Optional.empty(),
             Optional.empty());
 
     ProcessExecutorParams xctoolParams =
@@ -297,6 +301,7 @@ public void xctoolCommandWithAppAndLogicTests() throws Exception {
             Optional.empty(),
             Optional.empty(),
             Optional.empty(),
+            Optional.empty(),
             Optional.empty());
 
     ProcessExecutorParams xctoolParams =
@@ -354,6 +359,7 @@ public void xctoolCommandWhichReturnsExitCode1DoesNotFailStep() throws Exception
             Optional.empty(),
             Optional.empty(),
             Optional.empty(),
+            Optional.empty(),
             Optional.empty());
 
     ProcessExecutorParams xctoolParams =
@@ -409,6 +415,7 @@ public void xctoolCommandWhichReturnsExitCode400FailsStep() throws Exception {
             Optional.empty(),
             Optional.empty(),
             Optional.empty(),
+            Optional.empty(),
             Optional.empty());
 
     ProcessExecutorParams xctoolParams =
@@ -463,6 +470,7 @@ public void xctoolCommandWithTestSelectorFiltersTests() throws Exception {
             Optional.empty(),
             Optional.empty(),
             Optional.empty(),
+            Optional.empty(),
             Optional.empty());
 
     ProcessExecutorParams xctoolListOnlyParams =
@@ -554,6 +562,7 @@ public void xctoolCommandWithTestSelectorFailsIfListTestsOnlyFails() throws Exce
             Optional.empty(),
             Optional.empty(),
             Optional.empty(),
+            Optional.empty(),
             Optional.empty());
 
     ProcessExecutorParams xctoolListOnlyParams =
@@ -620,6 +629,7 @@ public void xctoolCommandWithTestSelectorMatchingNothingDoesNotFail() throws Exc
             Optional.empty(),
             Optional.empty(),
             Optional.empty(),
+            Optional.empty(),
             Optional.empty());
     ProcessExecutorParams xctoolListOnlyParams =
         ProcessExecutorParams.builder()
@@ -682,7 +692,8 @@ public void testDirectoryAndLevelPassedInEnvironment() throws Exception {
             Optional.of("TEST_LOG_LEVEL"),
             Optional.of("verbose"),
             Optional.empty(),
-            Optional.of("/path/to/snapshotimages"));
+            Optional.of("/path/to/snapshotimages"),
+            Optional.of("/path/to/snapshotdiffimages"));
     ProcessExecutorParams xctoolParams =
         ProcessExecutorParams.builder()
             .setCommand(
@@ -698,12 +709,14 @@ public void testDirectoryAndLevelPassedInEnvironment() throws Exception {
             // This is the important part of this test: only if the process is executed
             // with a matching environment will the test pass.
             .setEnvironment(
-                ImmutableMap.of(
-                    "DEVELOPER_DIR", "/path/to/developer/dir",
-                    "XCTOOL_TEST_ENV_TEST_LOG_PATH", "/path/to/test-logs",
-                    "XCTOOL_TEST_ENV_TEST_LOG_LEVEL", "verbose",
-                    "XCTOOL_TEST_ENV_FB_REFERENCE_IMAGE_DIR", "/path/to/snapshotimages",
-                    "LLVM_PROFILE_FILE", "/tmp/some.profraw"))
+                ImmutableMap.<String, String>builder()
+                  .put("DEVELOPER_DIR", "/path/to/developer/dir")
+                  .put("XCTOOL_TEST_ENV_TEST_LOG_PATH", "/path/to/test-logs")
+                  .put("XCTOOL_TEST_ENV_TEST_LOG_LEVEL", "verbose")
+                  .put("XCTOOL_TEST_ENV_FB_REFERENCE_IMAGE_DIR", "/path/to/snapshotimages")
+                  .put("XCTOOL_TEST_ENV_IMAGE_DIFF_DIR", "/path/to/snapshotdiffimages")
+                  .put("LLVM_PROFILE_FILE", "/tmp/some.profraw")
+                .build())
             .setDirectory(projectFilesystem.getRootPath().getPath())
             .setRedirectOutput(ProcessBuilder.Redirect.PIPE)
             .build();