fix: Use execProperties when remoteExecutionProperties is empty

[stevebarrau]

When defining a platform (for RBE); remote_execution_properties is marked as deprecated, thus we do not set it. If it is not set getPlatformProto does not read the execProperties instead, before defaulting to defaultExecProperties.

[katre]

This change looks reasonable, but it also changed the behavior in the test //src/test/shell/bazel/remote:remote_execution_test.

Can you please update the tests to make sure they're still covering the same behavior that they were (not just changing the message checks to match the current). You'll probably need to explicitly clear out the exec properties (if that's what's causing the change).

Also can you add tests for these scenarios:

• exec properties is set and remote execution properties is not
• exec properties is set and remote execution properties is also set (exec properties should be ignored here)

[stevebarrau]

I am not sure I understand how to modify the exec properties from within those tests @katre ; do you have an example I could follow? I am planning on having tests to check the precedence order as follow:

1. remote exec properties
2. exec proporties
3. default exec properties (this is the current test_platform_default_properties_invalidation IIUC)

[katre]

So, for the existing test failures in remote_execution_test:

• test_remote_exec_properties: I'm going to guess that your change means the default remote exec properties (from the flag) are not being applied, but that's just a guess. Regardless, the test clearly expects that both builds will succeed, but the second will not be a cache hit, because of the changed properties.
• test_platform_default_properties_invalidation: Since this test also uses --remote_default_exec_properties, I'm assuming it's the same underlying problem as the above.

As far as new tests, here are some samples to crib from:

• src/test/shell/bazel:bazel_docker_sandboxing_test | test_platform_container: This creates a platform with remote_execution_properties and checks that it is used and --remote_default_exec_properties is ignored, so that's a good setup (but using the remote worker from :remote_execution_test
  • This also shows the general pattern of "write a platform target in a BUILD file, then use it with --extra_execution_platforms"
• //src/test/shell/bazel:bazel_java_test | test_platform_container shows using exec_properties.

That will hopefully get you started, let me know if you have further questions.

[katre]

Thanks so much! The new tests are very useful. I have a few small comments but once you resolve those I'll be happy to get this merged. I really appreciate you working through the review with me and adding the tests.

[stevebarrau]

@katre thanks for all the reviews. I pushed many small commits for the review comment for ease of review. I am not familiar with the merge process in this project but the intent here to squash all those into one commit on merge.

Thanks for the pointers into other examples; they really helped in putting those tests together. Other things that helped were:

• the ability to run bazel under a debugger (with the --host_jvm_debug flag)
• test filtering to runes specific test. Kudos to the authors of the bash test framework.

I used IDEA JVM remote debugger in combination with the following command to run a single test without sharding; pretty good debug experience!

bazel test \
    --test_output=streamed \
    --test_filter=test_platform_default_properties_invalidation \
    //src/test/shell/bazel/remote:remote_execution_test

[katre]

This looks good! It will be imported to our external repository for a further review and testing (which our team will handle), and then once submitted there be re-exported to GitHub as a single squashed commit.