Add golden_file_test to public API

[alexeagle]

Our golden_file_test
https://github.com/bazelbuild/rules_nodejs/blob/master/internal/golden_file_test/golden_file_test.bzl
is pretty useful and I've pointed a bunch of users to it, as a way to check build outputs into their source tree (in cases where they really have to do that, it's a hacky workaround for dev workflows that can't rely on files in bazel-out)

It belongs in bazel-skylib bazelbuild/bazel-skylib#90 (comment) but it feels it will be hard to add there. We already have the public api copy_to_bin
https://github.com/bazelbuild/rules_nodejs/blob/master/docs/Built-ins.md#copy_to_bin
which belonged in bazel-skylib but isn't getting merged there bazelbuild/bazel-skylib#217

So we could probably commit to making it public

[alexeagle]

To bike-shed the name a little: a lot of people ask "how do I check in generated sources" or "how do I get Bazel to write to the input directory" or questions like this. They're not thinking in terms of writing a test, at least to begin with.

Also "golden" isn't a universal term. Jest calls similar files "snapshot" for example.

Maybe generated_file_test or output_to_srcdir_test or something would help people discover that this is the rule they want.

Any thoughts?

[DavidANeil]

I can confirm that I have seen "golden" show up dozens of time in this project and never really known what it meant, but your comments here have made me realize this is probably what someone would need to get something like Betterer working within Bazel.

Maybe a name that reflects this need for persistence by version control? nodejs_test_with_persistence? nodejs_test_with_srcdir_output?

Naming things is hard.

[longlho]

generated_file_test sounds great to me

[alexeagle]

The only thing I'm pretty sure of is that it should end with _test. Bazel says "Test rules (but not necessarily their targets) must have names that end in _test. Non-test rules must not have this suffix." so I think this is load-bearing in some way.

Let's have some votes with reasoning? Try to think of it from a number of perspectives including confused/novice users

1. generated_file_test
2. checked_in_generated_file_test
3. generated_files_up_to_date_test
4. regenerate_test
5. generated_files_test
6. checked_in_output_test
7. file_snapshot_test
8. compare_lkg_test (last-known good)

also maybe the custom verb "accept" should be "copy" or "update"

[joeljeske]

What about file_snapshot_test or golden_file_snapshot_test? Like you mentioned, in the javascript space jest snapshot testing is the most well known type of test that uses a generated file as a test expectation. So the word snapshot might connect the dots quicker for javascript folks.

To me, I don't know if something like output_to_sources_test really invokes the correct thinking of how the rule should be used. It might have the problem of confusing people to use this rule just to check-in generated outputs to be use in other build steps, which really isn't the goal if I understand it correctly.

[alexeagle]

Having snapshot in the name is a good idea, I added option 10 above.
You totally can use this rule to check in a file which is used as an input to downstream rules, we have that shape in rules_nodejs where we check in some bootstrappy files that are inputs to running node, in our case we must do this to avoid a cycle.

[joeljeske]

Interesting! How would a bazel build rule depend on a test rule? Meaning, how would bazel know that it must run the test to ensure that the generated file is up to date?

[alexeagle]

@joeljeske err no, the build rule depends on the generated file, which might be out-of-date. Only when the test runs do you find out. Your understanding is right.
Discussed in our sync meeting today, agree that we don't want to encourage users to do this, and also that the test is about the generated file, taking the checked-in one as Source-of-Truth. So it's more "assert that the generated file looks like mine"

In slack we have some votes for generated_file_test and checked_in_generated_file_test

[alexeagle]

okay I'm going with generated_file_test
and the custom verb "update" rather than "accept"

[tetromino]

It belongs in bazel-skylib bazelbuild/bazel-skylib#90 (comment) but it feels it will be hard to add there.

Belated comment: the problems with adding this to bazel-skylib are:

• we don't want a pure Starlark diff implementation (painfully slow reinvented wheel), and relying on the system's diff command is not ideal (do we just not support Windows?)
• we'd probably want to support common preprocessing on the golden and test file (e.g. sorting, ignoring leading/trailing whitespace).

I agree that it would be useful to have a generated_file_test rule outside node rules, but it should probably live in its own rule set and it would have to provide a diff-like command written in something fast and portable (e.g. Go or Rust).

[tetromino]

Belated comment 2: found that Bazel uses msys2's diff on Windows, which makes bazelbuild/bazel-skylib#90 feasible after all.