Add a test execgroup to Starlark test rules, if not manually defined. #14019
Conversation
8d0137a to
c287c4b
Compare
|
Unfortunately, the one specific shard that had a legitimate failure before, failed again on some technical issue – just when I thought I fixed the problem... is there a way to rerun that specific shard? |
|
I hit the "retry" button for the failing shard, we'll see what happens. |
| @@ -402,6 +403,9 @@ public StarlarkCallable rule( | |||
| } | |||
| builder.addExecGroups(execGroupDict); | |||
| } | |||
| if (test && (execGroupDict == null || !execGroupDict.containsKey(TEST_RUNNER_EXEC_GROUP))) { | |||
There was a problem hiding this comment.
The actual execGroups data in RuleClass.Builder is a map: https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/packages/RuleClass.java;drc=8965d25af90ba975cd207c84fa461b499d8d51c9;l=820
Would it be easier to expose a Set<String> getExecGroupNames() or boolean hasExecGroup(String name) method on RuleClass.Builder?
This works but it's not elegant and requires keeping track of the existing groups. It's fine for Starlark, but native rules can have inheritance which would make this trickier.
There was a problem hiding this comment.
Done. I was wondering about the same thing – my reasoning against this was that I didn't see getters on any other builder classes, and I wasn't sure what you'd think of it.
| grep "platform_key" out.txt || fail "Did not find the platform key" | ||
| grep "test_value" out.txt || fail "Did not find the test-action value" | ||
|
|
||
| bazel test --extra_execution_platforms="${pkg}:my_platform" ${pkg}:b --execution_log_json_file out.txt || fail "Test failed" |
There was a problem hiding this comment.
Can you split this into two separate tests? One for the default test exec group, one for the manual? I think that will make any future errors easier to diagnose.
c287c4b to
2c95c3e
Compare
|
|
||
| starlark_test = rule( | ||
| implementation = _impl, | ||
| test = True, |
There was a problem hiding this comment.
Add a comment here that the "test" exec group exists by default in "test" rules.
There was a problem hiding this comment.
Done (I thought it was enough that the test name said it).
| implementation = _impl, | ||
| test = True, | ||
| exec_groups = { | ||
| "test": exec_group(), |
There was a problem hiding this comment.
Add a comment here that this overrides the default "test" exec group.
Can we add a property or something here and verify it's set when the target is built?
There was a problem hiding this comment.
Done. Initially I used run_shell instead of write and tested that we aren't seeing the value for the test execgroup on bazel build, but unfortunately this seemed not to build on Windows, and write actions don't show up in the execution log. So I'm not sure how to do this.
There was a problem hiding this comment.
Hmm, you're right, I don't see an easy way to do that. This is better than what we have now, so that's good.
| implementation = _impl, | ||
| test = True, | ||
| exec_groups = { | ||
| "test": exec_group(), |
There was a problem hiding this comment.
Hmm, you're right, I don't see an easy way to do that. This is better than what we have now, so that's good.
Fixes #13986.