Output Runner will only run each unique command+args once

[vpanta]

Prospective feature that would really help us, just putting it out there as a proposal.

Goes along with #421

[achew22]

This seems entirely reasonable to me! Could I have you add an e2e test to validate the behavior? Here is a preexisting output runner test that you can duplicate and extend to validate your new functionality.

Thanks so much for your contribution! This is a really good usability improvement

[vpanta]

This seems entirely reasonable to me! Could I have you add an e2e test to validate the behavior? Here is a preexisting output runner test that you can duplicate and extend to validate your new functionality.

+1, not the most straightforward, but crafted a test to write words to the sentinel file based on requested commands. So we can test that each word was unique.

[achew22]

+1, not the most straightforward, but crafted a test to write words to the sentinel file based on requested commands. So we can test that each word was unique.

Thanks for doing that. Out of curiosity, do these pass on your box locally?

[vpanta]

+1, not the most straightforward, but crafted a test to write words to the sentinel file based on requested commands. So we can test that each word was unique.

Thanks for doing that. Out of curiosity, do these pass on your box locally?

Shoot, it's still failing on buildkite. But yeah, they pass fine when I run bazel test //e2e/output_runner/.... I have another idea which shouldn't require a sentinel file in this manner, so let me give that a shot.

[vpanta]

Ok, maybe it'll work now. I'd actually been using --test_filter to just run my test locally, and it was passing. It seems the issue is that the tests aren't actually hermetic, and the main-dir doesn't get reset between them (an assumption I'd made), so when running as the full suite, the defs.bzl file actually keeps the content put into it by TestOutputRunner (print("not it")). This is likely why it was breaking with the old mechanism as well: it wasn't actually getting the signal to run any commands when executing as a suite!

While I'm sure some form of ensuring the hermeticity would be preferred, I for now just made TestOutputRunner and this new test both overwrite defs.bzl with what they expect before they act.

[achew22]

While I'm sure some form of ensuring the hermeticity would be preferred, I for now just made TestOutputRunner and this new test both overwrite defs.bzl with what they expect before they act.

I believe splitting the test data into two sets of targets would provide you this flexibility and make the tests more hermetic. WDYT about that as a solution?

[achew22]

Friendly ping on this

[vpanta]

Hah, sorry, crazy week (moving houses + work makes me forgetful)

I'll try and wrap this up this upcoming week!

[achew22]

No rush at all, just wanted to make sure I didn't scare you off 😁

[vpanta]

Ok, so looking at the way TestMain works, I'd have to move some of the tests to a separate package I believe. It sets up the tests and then runs them all, and at the moment the helpers for setting up the directory are not available outside of the bazel_testing package.

So did you mean for me to fully create a separate suite?

[achew22]

Ok, so looking at the way TestMain works, I'd have to move some of the tests to a separate package I believe. It sets up the tests and then runs them all, and at the moment the helpers for setting up the directory are not available outside of the bazel_testing package.

If you don't think this is possible, then please don't do it. I'd rather have the feature in. My expectation was that you could do something like this:

const mainFiles = `
-- single/defs.bzl --
def fix_deps():
  print("runacommand")
-- single/BUILD --
load("//single:defs.bzl", "fix_deps")
fix_deps()
sh_binary(
  name = "test",
  srcs = ["test.sh"],
)
sh_binary(
  name = "overwrite",
  srcs = ["overwrite.sh"],
)
-- single/test.sh --
printf "action"
-- single/overwrite.sh --
printf "overwrite"
-- multiple/defs.bzl --
def fix_deps():
  print("runacommand")
  print("runacommand")
  print("runacommand")
  print("runacommand")
-- multiple/BUILD --
load("//:defs.bzl", "fix_deps")
fix_deps()
sh_binary(
  name = "test",
  srcs = ["test.sh"],
)
sh_binary(
  name = "overwrite",
  srcs = ["overwrite.sh"],
)
-- multiple/test.sh --
printf "action"
-- multiple/overwrite.sh --
printf "overwrite"
`

Then you could target //single:test and //multiple:test in the tests. Would that work?

[vpanta]

facepalm

Yes! That works fine! Honestly for a minute when you said add targets I was thinking of the actual bazel e2e test call, not the inner generated rules.

And duh, just creating a second internal package should work I think.

[vpanta]

Ok, updated :D

[achew22]

Sorry to bring something like this up do late in the review process but I didn't realize till just now. Does this imply that a program who emits:

buildifier 'add deps test_dep1' //target
buildifier 'add deps test_dep2' //target

Would only have the first buildifier run? Wouldn't we want to dedupe based on both the command and arguments so that we don't have to do 2 builds in order to add 2 deps to a binary?

[vpanta]

This already dedupes based on command and arguments. This just looks unclear (in fact, when you raised this, I had to read back over it a few times to convince myself it's correct).

This is a buildifier call not buildozer, and the matcher for it does not use string parts in its call to buildifier below.

I've updated this with an extra string and some comments to make it clearer the diff between this line and the additional buildozer line I've added as well.

[achew22]

Ah! I think I see it now. buildifier != buildozer. I think this invocation of buildifier is a bit strange. Wouldn't buildifer be applied to a single file to format it? Why is it taking a cmd_ignore and a target?

[vpanta]

lol, copy-pasta? I was copying line 94 (as another bazel output to match on) but changing it slightly, so that it'd match the regex but not fully match the line.

I can do something else with it.

[achew22]

I mostly look at the test cases to see examples of how it would actually be used. My tiny monkey brain would be a little less taxed if the examples were a little more straight forward. Sorry!

[vpanta]

lol, np, I just updated this to match the buildozer and gazelle suggestions from documentation.