add tests

bazelbuild · Oct 12, 2023 · 2a846c7 · 2a846c7
1 parent 351be6c
commit 2a846c7
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 21 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformUtils.java
@@ -83,24 +83,22 @@ public static Platform getPlatformProto(
       return null;
     }
 
-    Map<String, String> properties;
+    Map<String, String> properties = new HashMap<>();
     if (!spawn.getCombinedExecProperties().isEmpty()) {
       // Apply default exec properties if the execution platform does not already set
       // exec_properties
       if (spawn.getExecutionPlatform() == null
           || spawn.getExecutionPlatform().execProperties().isEmpty()) {
-        properties = new HashMap<>();
         properties.putAll(defaultExecProperties);
         properties.putAll(spawn.getCombinedExecProperties());
       } else {
         properties = spawn.getCombinedExecProperties();
       }
     } else if (spawn.getExecutionPlatform() != null) {
       properties = new HashMap<>();
-      Platform.Builder platformBuilder = Platform.newBuilder();
-
       String remoteExecutionProperties = spawn.getExecutionPlatform().remoteExecutionProperties();
       if (!remoteExecutionProperties.isEmpty()) {
+        Platform.Builder platformBuilder = Platform.newBuilder();
         // Try and get the platform info from the execution properties.
         try {
           TextFormat.getParser()
@@ -117,10 +115,12 @@ public static Platform getPlatformProto(
         for (Property property : platformBuilder.getPropertiesList()) {
           properties.put(property.getName(), property.getValue());
         }
-      } else {
+      } else if (!spawn.getExecutionPlatform().execProperties().isEmpty()) {
         properties.putAll(spawn.getExecutionPlatform().execProperties());
       }
-    } else {
+    }
+
+    if (properties.isEmpty()) {
       properties = defaultExecProperties;
     }
 

diff --git a/src/test/shell/bazel/remote/remote_execution_test.sh b/src/test/shell/bazel/remote/remote_execution_test.sh
@@ -1336,19 +1336,11 @@ EOF
   [[ ! -f bazel-bin/test.runfiles/MANIFEST ]] || fail "expected output manifest to exist"
 }
 
-# FIXME remove this comment
-#
-# - I run this test with: bazel test //src/test/shell/bazel/remote:remote_execution_test --test_filter=test_platform_default_properties_invalidation
-# - I do not understand how to set the platform yet
 function test_platform_default_properties_invalidation() {
   # Test that when changing values of --remote_default_platform_properties all actions are
-  # invalidated.
+  # invalidated if no platform is used.
   mkdir -p test
   cat > test/BUILD << 'EOF'
-platform(
-    name = "platform_without_any_exec_properties",
-)
-
 genrule(
     name = "test",
     srcs = [],
@@ -1358,26 +1350,22 @@ genrule(
 EOF
 
   bazel build \
-    --extra_execution_platforms=//test:platform_without_any_exec_properties \
     --remote_executor=grpc://localhost:${worker_port} \
     --remote_default_exec_properties="build=1234" \
     //test:test >& $TEST_log || fail "Failed to build //test:test"
 
   expect_log "2 processes: 1 internal, 1 remote"
 
   bazel build \
-    --extra_execution_platforms=//test:platform_without_any_exec_properties \
     --remote_executor=grpc://localhost:${worker_port} \
     --remote_default_exec_properties="build=88888" \
     //test:test >& $TEST_log || fail "Failed to build //test:test"
 
   # Changing --remote_default_platform_properties value should invalidate SkyFrames in-memory
   # caching and make it re-run the action.
   expect_log "2 processes: 1 internal, 1 remote"
-  # FIXME the above assert fails
 
   bazel build \
-    --extra_execution_platforms=//test:platform_without_any_exec_properties \
     --remote_executor=grpc://localhost:${worker_port} \
     --remote_default_exec_properties="build=88888" \
     //test:test >& $TEST_log || fail "Failed to build //test:test"
@@ -1389,7 +1377,6 @@ EOF
   bazel shutdown
 
   bazel build \
-    --extra_execution_platforms=//test:platform_without_any_exec_properties \
     --remote_executor=grpc://localhost:${worker_port} \
     --remote_default_exec_properties="build=88888" \
     //test:test >& $TEST_log || fail "Failed to build //test:test"
@@ -1399,7 +1386,6 @@ EOF
   expect_log "1 process: 1 internal"
 
   bazel build\
-    --extra_execution_platforms=//test:platform_without_any_exec_properties \
     --remote_executor=grpc://localhost:${worker_port} \
     --remote_default_exec_properties="build=88888" \
     --remote_default_platform_properties='properties:{name:"build" value:"1234"}' \
@@ -1411,6 +1397,81 @@ EOF
   expect_log "Setting both --remote_default_platform_properties and --remote_default_exec_properties is not allowed"
 }
 
+function test_platform_default_properties_invalidation_with_platform_exec_properties() {
+  # Test that when changing values of --remote_default_platform_properties all actions are
+  # invalidated.
+  mkdir -p test
+  cat > test/BUILD << 'EOF'
+platform(
+    name = "platform_without_any_exec_properties",
+    exec_properties = {
+        "foo": "bar",
+    },
+)
+
+genrule(
+    name = "test",
+    srcs = [],
+    outs = ["output.txt"],
+    cmd = "echo \"foo\" > \"$@\""
+)
+EOF
+
+  bazel build \
+    --extra_execution_platforms=//test:platform_without_any_exec_properties \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_default_exec_properties="build=1234" \
+    //test:test >& $TEST_log || fail "Failed to build //test:test"
+
+  expect_log "2 processes: 1 internal, 1 remote"
+
+  bazel build \
+    --extra_execution_platforms=//test:platform_without_any_exec_properties \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_default_exec_properties="build=88888" \
+    //test:test >& $TEST_log || fail "Failed to build //test:test"
+
+  # Changing --remote_default_platform_properties value does not invalidate SkyFrames
+  # given its is super-seeded by the platform exec_properties.
+  expect_log "1 process: 1 internal."
+}
+
+function test_platform_default_properties_invalidation_with_platform_remote_execution_properties() {
+  # Test that when changing values of --remote_default_platform_properties all actions are
+  # invalidated.
+  mkdir -p test
+  cat > test/BUILD << 'EOF'
+platform(
+    name = "platform_without_any_exec_properties",
+    remote_execution_properties = """properties: {name: "foo" value: "baz"}""",
+)
+
+genrule(
+    name = "test",
+    srcs = [],
+    outs = ["output.txt"],
+    cmd = "echo \"foo\" > \"$@\""
+)
+EOF
+
+  bazel build \
+    --extra_execution_platforms=//test:platform_without_any_exec_properties \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_default_exec_properties="build=1234" \
+    //test:test >& $TEST_log || fail "Failed to build //test:test"
+
+  expect_log "2 processes: 1 internal, 1 remote"
+
+  bazel build \
+    --extra_execution_platforms=//test:platform_without_any_exec_properties \
+    --remote_executor=grpc://localhost:${worker_port} \
+    --remote_default_exec_properties="build=88888" \
+    //test:test >& $TEST_log || fail "Failed to build //test:test"
+
+  # Changing --remote_default_platform_properties value does not invalidate SkyFrames
+  # given its is super-seeded by the platform remote_execution_properties.
+  expect_log "2 processes: 1 remote cache hit, 1 internal"
+}
 
 function test_combined_disk_remote_exec_with_flag_combinations() {
   rm -f ${TEST_TMPDIR}/test_expected