bazelbuild / bazel-skylib

Common useful functions and rules for Bazel
https://bazel.build/
Apache License 2.0
388 stars 178 forks source link

Proposal for golden output testing rule #90

Open achew22 opened 5 years ago

achew22 commented 5 years ago

In #86 it appears the first rule is being added to skylib. Would there be any interest in adding a new rule that does golden comparison? To pick a name let's call it golden_test. I propose it is usable in 3 ways:

Literal:

golden_test(
    name = "content",
    src = ":myfile.txt", # Note that this could be the output of a genrule or other target
    content = "demonstration content for a test",
)

Regexp

golden_test(
    name = "content",
    src = ":myfile.txt", # Note that this could be the output of a genrule or other target
    regexp = "demonstration content for a .*",
)

Golden file

golden_test(
    name = "content",
    src = ":myfile.txt", # Note that this could be the output of a genrule or other target
    golden_file = ":myfile.golden.txt",
)

Would there be any interest in adding a rule like this? I believe that almost every rules_x has implemented something like this and consolidating on a single implementation seems like a not terrible idea.

CC: @c-parsons

c-parsons commented 5 years ago

Golden file verification does indeed does seem like useful functionality we may want to put into bazel-skylib. For one, Stardoc tests re-implement golden file verification from scratch, and I assume there are a number of places similar is done.

Would you mind writing a brief proposal and circulating to bazel-dev@googlegroups.com ? I'd like some user feedback before we commit to an API :) We should try to motivate the exact API with some desired usage.

We should also try to ensure the rule is platform-agnostic (and thus works for no-Bash Windows, much like the work done in #86)

achew22 commented 5 years ago

@c-parsons, thanks for your response.

Golden file verification does indeed does seem like useful functionality we may want to put into bazel-skylib. For one, Stardoc tests re-implement golden file verification from scratch, and I assume there are a number of places similar is done.

I know that rules_typescript, rules_closure and a few other rules have also implemented something like this, not to mention the couple that I've implemented in my own personal projects. We are well past the rule of three.

Would you mind writing a brief proposal and circulating to bazel-dev@googlegroups.com ? I'd like some user feedback before we commit to an API :) We should try to motivate the exact API with some desired usage.

What would you think of a PR as the proposal. It's nice to have a design doc but this shouldn't be extremely complex to implement and we could have a specific discussion about the exact API.

Would you mind writing a brief proposal and circulating to bazel-dev@googlegroups.com ? I'd like some user feedback before we commit to an API :) We should try to motivate the exact API with some desired usage.

I would be very happy to do that. Do you have any tips/suggestions/pitfalls that I need to look out for? I assume that Windows compatibility is the hard one to come by, but don't have a box that runs it. Any suggestions for that?

c-parsons commented 5 years ago

What would you think of a PR as the proposal. It's nice to have a design doc but this shouldn't be extremely complex to implement and we could have a specific discussion about the exact API.

That's fine -- just send a brief introduction email and link to the PR to bazel-dev to give that forum some notice there's something to look at!

I would be very happy to do that. Do you have any tips/suggestions/pitfalls that I need to look out for? I assume that Windows compatibility is the hard one to come by, but don't have a box that runs it. Any suggestions for that?

Unfortunately not my area of expertise :\ cc @laszlocsomor

laszlocsomor commented 5 years ago

FYI there's already such a rule in @bazel_tools: https://github.com/bazelbuild/bazel/blob/25d2a3cd2b9701bc5c4b3d481ec0b05e8d134703/tools/build_rules/test_rules.bzl#L306

This rule doesn't work on Windows at the moment -- I tried fixing it but had to roll back: https://github.com/bazelbuild/bazel/issues/6122

IMO this rule should live in skylib, not in bazel_tools. Moving it out might break existing users though. We can discuss possible approaches.

In any case I'm happy to answer question about rule development pifalls on Windows. (Which I'll hopefully get around to document once, see https://github.com/bazelbuild/bazel/issues/3889.)

@achew22 if you work for Google, talk to @philwo about requesting access to Bazel team's testing Windows VM on GCP (which isn't fancy in any way, just a vanilla Windows image with Bazel and some compilers + tools already installed).

laurentlb commented 5 years ago

+1 for moving file_test from @bazel_tools to skylib. We can revisit its design at the same time, if we need.

achew22 commented 5 years ago

Is it okay for the core Bazel to depend on starlib? Should I move it over and add a dependency on starlib? How should I handle places where that rule is used in the core repo?

laszlocsomor commented 5 years ago

We check in a copy of everything Bazel depends on, for better or worse. So you would copy Skylib to Bazel's //third_party directory, add a local_repository rule to Bazel's WORKSPACE, and use it like so.

alexeagle commented 3 years ago

I think this was fixed by #136 @tetromino want to close it? (I'd be happy to sign up as a maintainer for bazel-skylib if the load is currently too high)

alexeagle commented 1 year ago

Ping @tetromino ☝🏻