dev: test using datadriven

[irfansharif]

This commit groups a few changes to speed up dev and make it easier to
test.

1. Update dev bench to run benchmarks using bazel test directly
   instead of first grepping for benchmarks, querying test targets, and
   only then invoking test binaries using bazel run. The latter
   approach was both slower and more difficult to test. dev bench now
   looks identical to the dev test implementation.

2. Simplify dev test; now that protobuf targets are directly
   buildable, we don't need any manual querying of build targets and
   filtering for only go_test ones -- we can more easy bazel test
   whatever was top-level package was specified. This reduces how much
   I/O needs to happen with the host environment which both speeds
   things up and makes it easier to test.
   a. It also allows us to partially revert #74808 while still retaining
   informative error messages about missing targets (bazel's ones
   out-of-the-box are pretty good).

3. Finally, introduce TestDataDriven and TestRecorderDriven.
   a. TestDataDriven makes use of datadriven to capture all operations
   executed by individual dev invocations. The testcases are defined
   under testdata/datadriven/*. DataDriven divvies up these files as
   subtests, so individual "files" are runnable through:

      dev test pkg/cmd/dev -f TestDataDrivenDriven/<fname> [--rewrite]
      OR  go test ./pkg/cmd/dev -run TestDataDrivenDriven/<fname> [-rewrite]
   

   b. TestRecorderDriven makes use of datadriven/recorder to record (if
   --rewrite is specified) or play back (if --rewrite is omitted) all
   operations executed by individual dev invocations. The testcases
   are defined under testdata/recorderdriven/*; each test file
   corresponds to a captured recording found in
   testdata/recorderdriven/*.rec.

      dev test pkg/cmd/dev -f TestRecorderDriven/<fname>
      OR  go test ./pkg/cmd/dev -run TestRecorderDriven/<fname> [-rewrite]
   

   Recordings are used to mock out "system" behavior. When --rewrite
   is specified, attempts to shell out to bazel or perform other OS
   operations (like creating, removing, symlinking filepaths) are
   intercepted and system responses are recorded for future playback.

It's worth comparing TestDataDriven and TestRecorderDriven.
a. TestDataDriven is well suited for exercising flows that don't depend
on reading external state in order to function (simply translating a
dev test <target> to its corresponding bazel invocation for e.g.)
All operations are run in "dry-run" mode when --rewrite is specified;
all exec/os commands return successfully with no error + an empty
response, and just the commands are recorded as test data.
b. TestRecorderDriven in contrast works better for flows that do depend
on external state during execution (like reading the set of targets
from a file for e.g., or hoisting files from a sandbox by searching
through the file system directly). With these, when --rewrite is
specified, we actually do shell out and record the data for future
playback.

We previously (#68514) ripped out the equivalent of TestRecorderDriven
because it was difficult to use (large recordings, execution errors
failing the test, etc.) -- issues this PR tries to address. There are
some real limitations here though that may still make this approach
untenable: When --rewrite is used for TestRecorderDriven, because it
shells out, will:

• takes time proportional to the actual dev invocations;
• makes it difficult to run under bazel (through without --rewrite it
  works just fine);

The few remaining tests for this recorder stuff are probably examples of
dev doing too much, when it should instead push the logic down into
bazel rules. As we continue doing so, we should re-evaluate whether this
harness provides much value.

Release note: None

[cockroach-teamcity]

This change is 

[dt]

Brief skim so far: I'm a big fan of the first two changes mentioned in the commit message. Would it be difficult to separate them at the commit level from the third?

Like you, I'm skeptical that TestRecorderDriven is needed. Like you say, it almost certainly is pointing to the fact dev is doing too much work, if what it does starts to depend on reading state and reacting to it, not just a pure function of it arguments into arguments to a bazel command, and the logic it uses to do it becomes so complex we aren't comfortable leaving it uncovered by tests. You note that we've previously found a test like TestRecorderDriven added ongoing maintenance burden, so I'd argue we're better off putting that time to gettin that stuff out of dev entirely instead (as your first two changes here are doing)?

[irfansharif]

Would it be difficult to separate them at the commit level from the third?

Not difficult at all, will do it after more consensus on this PR's direction.

not just a pure function of it arguments into arguments to a bazel command, and the logic it uses to do it becomes so complex we aren't comfortable leaving it uncovered by tests.

Yes, but that said, I think we'll always have some limited from where things aren't a pure function and/or need to interact with external state (though it should scrutinized whenever adding new instances of this); dev {doctor,cache} for e.g. As for cases where we're doing more than dev should "above" the build system, I still think some test coverage is better than what we have today (no coverage and with hand-maintained mocks to make CI happy). If you look at the test cases, most have been converted over to use the "pure" form, and it wasn't much work to keep around the impure form.

---

I encourage reviewers to pull down the branch, edit the testdata files and see how it feels. It's all very snappy now -- TestDataDriven with and without --rewrite will run in <1s. TestRecorderDriven too in <10s under --rewrite, <1s without.

[rickystewart]

I checked out the code and played around with it. I don't understand everything here 100%, but the changes all seem sensible to me and I agree this should be more maintainable.

Can you bump the DEV_VERSION please?

[rickystewart]

Can you bump the DEV_VERSION please?

FYI I'm bumping it in #76245, so just keep that in mind so there's no conflict.

[irfansharif]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)