Skip to content

Commit

Permalink
Improve test further with debug
Browse files Browse the repository at this point in the history
  • Loading branch information
fmeum committed Jan 20, 2025
1 parent e3faec8 commit 89398ca
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -452,10 +452,10 @@ public <T extends ActionContext> T getContext(Class<T> type) {
}

/**
* Report a subcommand event to this Executor's Reporter and, if action
* logging is enabled, post it on its EventBus.
* Report a subcommand event to this Executor's Reporter and, if action logging is enabled, post
* it on its EventBus.
*/
public void maybeReportSubcommand(Spawn spawn) {
public void maybeReportSubcommand(Spawn spawn, String spawnRunner) {
ShowSubcommands showSubcommands = executor.reportsSubcommands();
if (!showSubcommands.shouldShowSubcommands) {
return;
Expand Down Expand Up @@ -499,12 +499,7 @@ public void maybeReportSubcommand(Spawn spawn) {
Event.of(
EventKind.SUBCOMMAND,
null,
"# "
+ reason
+ "\n"
+ message
+ "\n# Combined execution properties: "
+ spawn.getCombinedExecProperties()));
"# " + reason + "\n" + message + "\n# Runner: " + spawnRunner));
}

public ImmutableMap<String, String> getClientEnv() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public ImmutableList<SpawnResult> exec(
ActionExecutionContext actionExecutionContext,
@Nullable SandboxedSpawnStrategy.StopConcurrentSpawns stopConcurrentSpawns)
throws ExecException, InterruptedException {
actionExecutionContext.maybeReportSubcommand(spawn);
actionExecutionContext.maybeReportSubcommand(spawn, spawnRunner.getName());

final Duration timeout = Spawns.getTimeout(spawn);
SpawnExecutionContext context =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ private static InputStream spawnGrep(
outputs,
LOCAL_RESOURCES);

actionExecutionContext.maybeReportSubcommand(spawn);
actionExecutionContext.maybeReportSubcommand(spawn, "internal");

// Don't share the originalOutErr across spawnGrep calls. Doing so would not be thread-safe.
FileOutErr originalOutErr = actionExecutionContext.getFileOutErr();
Expand Down
37 changes: 16 additions & 21 deletions src/test/shell/bazel/remote/remote_execution_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3484,7 +3484,7 @@ platform(
name = "host",
constraint_values = [":has_foo"],
exec_properties = {
"no-remote-exec": "1",
"test.no-remote-exec": "1",
},
parents = ["@bazel_tools//tools:host_platform"],
visibility = ["//visibility:public"],
Expand All @@ -3504,46 +3504,43 @@ platform(
my_test(
name = "test",
# TODO: This uses a hack by setting test.no-remote-exec on the exec platform
# forced by this constraint for both the build and the test action. Instead,
# use exec_group_compatible_with = {"test": [":has_foo"]} once it is
# implemented.
exec_compatible_with = [":has_foo"],
)
my_test(
name = "test2",
exec_compatible_with = [":has_foo"],
target_compatible_with = [":has_foo"],
exec_properties = {
"no-remote-exec": "1",
"test.no-remote-exec": "1",
},
)
EOF

# A my_test target includes 2 actions: 1 build action (a) and 1 test action (b),
# with (b) running multiple spawns (test execution, test XML generation).
# This test currently demonstrates that:
# - (b) would always be executed on Bazel's target platform, set by "--platforms=" flag.
# - Regardless of 'no-remote-exec' set on (b)'s platform, (b) would still be executed remotely.
# The remote test action will be sent with `"no-remote-exec": "1"` in it's platform.
#
# TODO: Make this test's result consistent with 'test_platform_no_remote_exec'.
# Test action (b) should be executed locally instead of remotely in this setup.

# with (b) running two spawns (test execution, test XML generation).
# The genrule spawn runs remotely, both test spawns run locally.
bazel test \
--extra_execution_platforms=//a:remote,//a:host \
--platforms=//a:remote -s \
--spawn_strategy=remote,local \
--remote_executor=grpc://localhost:${worker_port} \
-s \
//a:test >& $TEST_log || fail "Failed to test //a:test"
expect_log "1 local, 1 remote"
expect_log "2 local, 1 remote"

# Note: While the test spawns are executed locally, they still select the
# remote platform as it is the first registered execution platform and there
# are no constraints to force a different one. This is not desired behavior,
# but it isn't covered by this test.
bazel test \
--extra_execution_platforms=//a:remote,//a:host \
--platforms=//a:host -s \
--spawn_strategy=remote,local \
--remote_executor=grpc://localhost:${worker_port} \
-s \
//a:test2 >& $TEST_log || fail "Failed to test //a:test2"
expect_log "1 local, 1 remote"
expect_log "2 local, 1 remote"

bazel clean

Expand All @@ -3552,18 +3549,16 @@ EOF
--platforms=//a:remote -s \
--spawn_strategy=remote,local \
--remote_executor=grpc://localhost:${worker_port} \
-s \
//a:test >& $TEST_log || fail "Failed to test //a:test"
expect_log "2 remote cache hit"
expect_log "3 remote cache hit"

bazel test \
--extra_execution_platforms=//a:remote,//a:host \
--platforms=//a:host -s \
--spawn_strategy=remote,local \
--remote_executor=grpc://localhost:${worker_port} \
-s \
//a:test2 >& $TEST_log || fail "Failed to test //a:test2"
expect_log "2 remote cache hit"
expect_log "3 remote cache hit"
}

function setup_inlined_outputs() {
Expand Down

0 comments on commit 89398ca

Please sign in to comment.