diff_test: add rule and tests #136

laszlocsomor · 2019-03-25T15:27:13Z

This PR adds a new rule: diff_test().

The test rule compares the contents of two files.
The 'diff_test.expect_same' attribute tells
whether the files are expected to be the same or
not.

The test succeeds if the files are the same and
expected to be so, or if the files are different
and expected to be so. Otherwise the test fails.

This rules requires no Bash on Windows, so it
works without Bash, without /usr/bin/diff, and
with the Windows-native test wrapper.

See bazelbuild/bazel#5508
See bazelbuild/bazel#4319

laszlocsomor · 2019-03-25T15:27:42Z

Generated Stardoc:

diff_test

diff_test(name, file1, file2, expect_same, kwargs)

A test that compares the contents of two files.

The test succeeds when the files are expected to be the same (with regard to
file contents) and are in fact the same, or when the files are expected to
be different and are in fact so.

Parameters

`name`	required. The name of the test rule.
`file1`	required. Label of the file to compare to `file2`.
`file2`	required. Label of the file to compare to `file1`.
`expect_same`	optional. default is `True` Whether the files are expected to be the same or not. The test passes if this is True and the files are the same, or if this is False and the files are not the same.
`kwargs`	optional. The common attributes for tests.

laszlocsomor · 2019-03-26T10:53:50Z

All green now, ping @laurentlb

laurentlb · 2019-03-26T11:11:46Z

rules/private/diff_test_private.bzl

+"""
+
+def _impl(ctx):
+    if ctx.attr.is_windows:


It may be more readable to have two functions, one for Windows, one for Linux (to avoid a long if block).

laurentlb · 2019-03-26T11:20:17Z

rules/private/diff_test_private.bzl

+            was_different = "'#!/bin/sh\nexit 0'"
+
+        # Run the shell command to generate 'test_script'.
+        ctx.actions.run_shell(


The code is a bit surprising. You run the diff as part of the build, and you generate a dummy file (exit 0 / exit 1) that will be executed during the test.

Instead, you could write the shell script with actions.write_file. Then the diff is executed as part of the test.
(the same pattern as in https://github.com/bazelbuild/examples/blob/master/rules/test_rule/line_length.bzl)

Yes, but inputs must become runfiles. Finding them is hard enough in a Bash script, it's even harder in a Batch file.

laurentlb · 2019-03-26T11:20:46Z

rules/private/diff_test_private.bzl

+        # Contents of 'test_script' depending on diff result and expectation.
+        if ctx.attr.expect_same:
+            was_same = "'#!/bin/sh\nexit 0'"
+            was_different = "\"#!/bin/sh\necho 'Expected \'$1\' and \'$2\' to be the same' >&2\nexit 1\""


it can be useful for the user to print the diff

Yes, with Bash that's easy (diff ... | head) but on Windows fc.exe is extremely chatty and there's no head-like utility.

laurentlb · 2019-03-26T11:21:43Z

rules/private/diff_test_private.bzl

+    attrs = {
+        "file1": attr.label(mandatory = True, allow_single_file = True),
+        "file2": attr.label(mandatory = True, allow_single_file = True),
+        "expect_same": attr.bool(default = True),


Do you actually need the expect_same attribute?
I don't see a good use-case for that. I'd suggest to keep the rule even simpler and remove it.

laszlocsomor · 2019-03-27T13:13:36Z

All green, PTAL.

Updated documentation:

diff_test

diff_test(name, file1, file2, kwargs)

A test that compares the contents of two files.

The rule compares the files at build time. If the files match, the rule
writes a dummy platform-specific script (.bat file on Windows, .sh on other
platforms) that just exits with 0, and which is the actual test script. If
the files do not match, the rule fails to build.

(The comparison is done at build time, not test time, because it makes the
rule easier to implement.)

Parameters

`name`	required. The name of the test rule.
`file1`	required. Label of the file to compare to `file2`.
`file2`	required. Label of the file to compare to `file1`. be the same or not. The test passes if this is True and the files are the same, or if this is False and the files are not the same.
`kwargs`	optional. The common attributes for tests.

laurentlb · 2019-03-27T15:21:56Z

Yes, but inputs must become runfiles. Finding them is hard enough in a Bash script, it's even harder in a Batch file.

It feels like it defeats the purpose of tests. Do we have a tracking bug? Or can you start a discussion on the mailing-list?

If our recommendation is to generate files that just do exit 0, I'd say something is wrong.

rules/diff_test.bzl

rules/private/diff_test_private.bzl

laszlocsomor · 2019-03-27T15:24:56Z

It feels like it defeats the purpose of tests.

Actually, that makes sense. Let me think it over and ping this thread later.

This new test rule compares two files and passes if the files match. On Linux/macOS/non-Windows, the test compares files using 'diff'. On Windows, the test compares files using 'fc.exe'. This utility is available on all Windows versions I tried (Windows 2008 Server, Windows 2016 Datacenter Core). See bazelbuild/bazel#5508 See bazelbuild/bazel#4319

laszlocsomor · 2019-04-09T14:21:15Z

PTAL.

Stardoc below.

diff_test

diff_test(name, file1, file2, kwargs)

A test that compares two files.

The test succeeds if the files' contents match.

Parameters

`name`	required. The name of the test rule.
`file1`	required. Label of the file to compare to `file2`.
`file2`	required. Label of the file to compare to `file1`.
`kwargs`	optional. The common attributes for tests.

laszlocsomor · 2019-04-10T09:29:04Z

Hold on, I discovered a bug with --nolegacy_external_runfiles. Let me fix it and ping again.

laszlocsomor · 2019-04-10T11:52:36Z

PTAL -- all fixed, ready for review.

laszlocsomor · 2019-04-11T06:26:38Z

ping

c-parsons

Looking pretty good. Just one concern.

Sorry for the review delay.

c-parsons · 2019-04-11T17:00:43Z

rules/diff_test.bzl

+        test_bin = ctx.actions.declare_file(ctx.label.name + "-test.bat")
+        ctx.actions.write(
+            output = test_bin,
+            content = r"""@echo off


I find it a little awkward to have this much content written as a string literal in Starlark. Could we move it to a bat file and use that as an action input?

I tried, but couldn't: I created templates under //rules/private and added a label attribute to _diff_test that select()s from them:

diff_script_template = select({ "@bazel_tools//src/conditions:host_windows": "//rules/private:diff_test_tmpl.bat", "//conditions:default": "//rules/private:diff_test_tmpl.sh", }),

Unfortunately the rule can't find the templates when the rule itself is in an external repo.

If you don't mind, I'd leave the ugly strings here; that approach works.

I edited a test to pull the rule from an external repo.

I don't understand the issue you're running into ...

It's not necessary to select on the diff script -- you can depend on the scripts for both windows and non-windows, and only actually use the script you need based on the value of is_windows.

Depending on a script "tool" dependency is a pretty normal pattern for rules. If there's a serious problem with adopting this pattern from this repository, we should investigate.

That said, I know this review has been going slowly, so I'm fine with proceeding as-is and following up later. It should be an easy cleanup.

c-parsons · 2019-04-12T17:33:43Z

rules/diff_test.bzl

+        test_bin = ctx.actions.declare_file(ctx.label.name + "-test.bat")
+        ctx.actions.write(
+            output = test_bin,
+            content = r"""@echo off


I don't understand the issue you're running into ...

It's not necessary to select on the diff script -- you can depend on the scripts for both windows and non-windows, and only actually use the script you need based on the value of is_windows.

Depending on a script "tool" dependency is a pretty normal pattern for rules. If there's a serious problem with adopting this pattern from this repository, we should investigate.

That said, I know this review has been going slowly, so I'm fine with proceeding as-is and following up later. It should be an easy cleanup.

laszlocsomor · 2019-04-15T06:57:33Z

Thank you @c-parsons !

laszlocsomor · 2019-04-16T09:06:49Z

Well, this is embarrassing -- I found a simpler solution to the problem I meant to solve with diff_test.
I'm sorry for wasting your time @c-parsons. Still, I'd rather admit this now than carry the rule for no reason. Are you OK with me reverting this PR?

c-parsons · 2019-04-16T12:40:29Z

Is the solution generalized to make diff_test entirely obsolete? (Care to elaborate?)
A rollback would be fine.

laszlocsomor · 2019-04-16T12:47:30Z

No, it's specialized: bazelbuild/buildtools#610
The solution fixes that one test macro's genrule.

diff_test is generic, but I had no purpose for it other than fixing //api_proto:api.gen.pb.go_checkshtest in https://github.com/bazelbuild/buildtools.

Using diff_test in Buildtools would require either to depend on Skylib as an external repo, or to copy diff_test.bzl into Buildtools.

Buildtools already depends transitively on an older Skylib version, updating the dependencies might break the affected projects. And copying diff_test into Buildtools repo risks diverging from the version in Skylib.

All in all, fixing Buildtools without diff_test is easier, but now I have no users for diff_test.

It's a nice-to-have rule, but we shouldn't maintain it until we actually need it. If I delete it now, we can revive it if we need so.

WDYT?

c-parsons · 2019-04-16T14:30:59Z

diff_test fits a pattern which is used in many places... For one, I was hoping to use it for Stardoc's tests instead of a home-grown diff test shell script

Hopefully that would be sufficient to justify the maintenance of diff_test ?

As a side note, I would encourage you to upgrade other repositories to the newest version of skylib :)

laszlocsomor · 2019-04-16T14:36:37Z

Yes, that's enough reason to keep the test. Yay!

laszlocsomor requested review from c-parsons and laurentlb as code owners March 25, 2019 15:27

googlebot added the cla: yes label Mar 25, 2019

laurentlb reviewed Mar 26, 2019

View reviewed changes

laszlocsomor force-pushed the diff-test branch from 67122d7 to a59a039 Compare March 27, 2019 13:01

laurentlb reviewed Mar 27, 2019

View reviewed changes

rules/diff_test.bzl Outdated Show resolved Hide resolved

rules/private/diff_test_private.bzl Outdated Show resolved Hide resolved

laszlocsomor force-pushed the diff-test branch from 8b2faef to 9bfa958 Compare April 9, 2019 13:50

laszlocsomor added 2 commits April 9, 2019 15:55

Fix Buildifier warning

fa16487

cmd.exe: delayed variable expansion

f35124f

laszlocsomor added 2 commits April 10, 2019 12:55

Fix runfile-lookup w/ --nolegacy_external_runfiles

d04a3df

Linux, diff-test: fix manifest-only case

c876787

laszlocsomor requested a review from laurentlb April 10, 2019 11:52

c-parsons reviewed Apr 11, 2019

View reviewed changes

Test pulling diff_test.bzl from a repo

b17dc2d

c-parsons approved these changes Apr 12, 2019

View reviewed changes

c-parsons merged commit be3b1fc into bazelbuild:master Apr 12, 2019

laszlocsomor deleted the diff-test branch April 15, 2019 06:57

alexeagle mentioned this pull request May 25, 2021

Proposal for golden output testing rule #90

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

diff_test: add rule and tests #136

diff_test: add rule and tests #136

laszlocsomor commented Mar 25, 2019

laszlocsomor commented Mar 25, 2019

laszlocsomor commented Mar 26, 2019

laurentlb Mar 26, 2019

laszlocsomor Mar 27, 2019

laurentlb Mar 26, 2019

laszlocsomor Mar 26, 2019

laurentlb Mar 26, 2019

laszlocsomor Mar 27, 2019

laurentlb Mar 26, 2019

laszlocsomor Mar 26, 2019

laszlocsomor commented Mar 27, 2019

laurentlb commented Mar 27, 2019

laszlocsomor commented Mar 27, 2019

laszlocsomor commented Apr 9, 2019

laszlocsomor commented Apr 10, 2019

laszlocsomor commented Apr 10, 2019

laszlocsomor commented Apr 11, 2019

c-parsons left a comment

c-parsons Apr 11, 2019

laszlocsomor Apr 12, 2019

c-parsons Apr 12, 2019

c-parsons Apr 12, 2019

laszlocsomor commented Apr 15, 2019

laszlocsomor commented Apr 16, 2019 •

edited

Loading

c-parsons commented Apr 16, 2019

laszlocsomor commented Apr 16, 2019 •

edited

Loading

c-parsons commented Apr 16, 2019

laszlocsomor commented Apr 16, 2019

diff_test: add rule and tests #136

diff_test: add rule and tests #136

Conversation

laszlocsomor commented Mar 25, 2019

laszlocsomor commented Mar 25, 2019

diff_test

Parameters

laszlocsomor commented Mar 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laszlocsomor commented Mar 27, 2019

diff_test

Parameters

laurentlb commented Mar 27, 2019

laszlocsomor commented Mar 27, 2019

laszlocsomor commented Apr 9, 2019

diff_test

Parameters

laszlocsomor commented Apr 10, 2019

laszlocsomor commented Apr 10, 2019

laszlocsomor commented Apr 11, 2019

c-parsons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laszlocsomor commented Apr 15, 2019

laszlocsomor commented Apr 16, 2019 • edited Loading

c-parsons commented Apr 16, 2019

laszlocsomor commented Apr 16, 2019 • edited Loading

c-parsons commented Apr 16, 2019

laszlocsomor commented Apr 16, 2019

laszlocsomor commented Apr 16, 2019 •

edited

Loading

laszlocsomor commented Apr 16, 2019 •

edited

Loading