From 2a45909ba25827df2a4198a9ca3c0810e1c4e115 Mon Sep 17 00:00:00 2001 From: Matt Stark Date: Tue, 14 May 2024 08:28:59 +1000 Subject: [PATCH 1/3] fix: Set size to a default value as well as timeout. Currently, we are unable to run our `write_source_files` tests in our pre-upload checks, because we have `--test_size_filter=small`, and setting `size` will attempt to set it on both the run rule and the test rule, the former being invalid. --- docs/diff_test.md | 2 +- lib/private/diff_test.bzl | 8 +++----- lib/private/write_source_file.bzl | 1 + lib/testing.bzl | 5 ++--- 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/docs/diff_test.md b/docs/diff_test.md index de63d9d5b..a57bf1804 100644 --- a/docs/diff_test.md +++ b/docs/diff_test.md @@ -30,7 +30,7 @@ The test succeeds if the files' contents match. | name | The name of the test rule. | none | | file1 | Label of the file to compare to <code>file2</code>. | none | | file2 | Label of the file to compare to <code>file1</code>. | none | -| size | standard attribute for tests | None | +| size | standard attribute for tests. Defaults to "small" if unspecified. | None | | timeout | standard attribute for tests. Defaults to "short" if both timeout and size are unspecified. | None | | kwargs | The <a href="https://docs.bazel.build/versions/main/be/common-definitions.html#common-attributes-tests">common attributes for tests</a>. | none | diff --git a/lib/private/diff_test.bzl b/lib/private/diff_test.bzl index 68d0b951c..4ca5e2bd4 100644 --- a/lib/private/diff_test.bzl +++ b/lib/private/diff_test.bzl @@ -21,7 +21,6 @@ The rule uses a Bash command (diff) on Linux/macOS/non-Windows, and a cmd.exe command (fc.exe) on Windows (no Bash is required). """ -load("//lib:utils.bzl", "default_timeout") load(":directory_path.bzl", "DirectoryPathInfo") def _runfiles_path(f): @@ -116,16 +115,15 @@ def diff_test(name, file1, file2, size = None, timeout = None, **kwargs): name: The name of the test rule. file1: Label of the file to compare to file2. file2: Label of the file to compare to file1. - size: standard attribute for tests + size: standard attribute for tests. Defaults to "small" if unspecified. timeout: standard attribute for tests. Defaults to "short" if both timeout and size are unspecified. **kwargs: The common attributes for tests. """ - _diff_test( name = name, file1 = file1, file2 = file2, - size = size, - timeout = default_timeout(size, timeout), + size = size or "small", + timeout = timeout, **kwargs ) diff --git a/lib/private/write_source_file.bzl b/lib/private/write_source_file.bzl index d5ff41ecc..ed74e51e6 100644 --- a/lib/private/write_source_file.bzl +++ b/lib/private/write_source_file.bzl @@ -122,6 +122,7 @@ To create an update *only* this file, run: message = message, visibility = kwargs.get("visibility"), tags = kwargs.get("tags"), + size = "small", ) else: if suggested_update_target == None: diff --git a/lib/testing.bzl b/lib/testing.bzl index 1b6feb2e9..b2767b481 100644 --- a/lib/testing.bzl +++ b/lib/testing.bzl @@ -5,7 +5,6 @@ load("@bazel_skylib//rules:write_file.bzl", "write_file") load("//lib:diff_test.bzl", "diff_test") load("//lib:jq.bzl", "jq") load("//lib:params_file.bzl", "params_file") -load("//lib:utils.bzl", "default_timeout") def assert_contains(name, actual, expected, size = None, timeout = None, **kwargs): """Generates a test target which fails if the file doesn't contain the string. @@ -44,8 +43,8 @@ def assert_contains(name, actual, expected, size = None, timeout = None, **kwarg name = name, srcs = [test_sh], args = ["$(rootpath %s)" % expected_file, "$(rootpath %s)" % actual], - size = size, - timeout = default_timeout(size, timeout), + size = size or "small", + timeout = timeout, data = [actual, expected_file], **kwargs ) From 0daee15347c4ca4d417819fef33e9882d5a5ad87 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Fri, 19 Jul 2024 12:46:34 -0700 Subject: [PATCH 2/3] code review feedback --- docs/diff_test.md | 5 ++--- docs/testing.md | 5 ++--- lib/private/diff_test.bzl | 8 +++----- lib/testing.bzl | 8 +++----- 4 files changed, 10 insertions(+), 16 deletions(-) diff --git a/docs/diff_test.md b/docs/diff_test.md index a57bf1804..c4038fbd5 100644 --- a/docs/diff_test.md +++ b/docs/diff_test.md @@ -14,7 +14,7 @@ command (fc.exe) on Windows (no Bash is required). ## diff_test 
-diff_test(name, file1, file2, size, timeout, kwargs)
+diff_test(name, file1, file2, size, kwargs)
A test that compares two files. @@ -30,8 +30,7 @@ The test succeeds if the files' contents match. | name | The name of the test rule. | none | | file1 | Label of the file to compare to <code>file2</code>. | none | | file2 | Label of the file to compare to <code>file1</code>. | none | -| size | standard attribute for tests. Defaults to "small" if unspecified. | None | -| timeout | standard attribute for tests. Defaults to "short" if both timeout and size are unspecified. | None | +| size | standard attribute for tests | "small" | | kwargs | The <a href="https://docs.bazel.build/versions/main/be/common-definitions.html#common-attributes-tests">common attributes for tests</a>. | none | diff --git a/docs/testing.md b/docs/testing.md index e2c543682..74a31425b 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -29,7 +29,7 @@ Assert that an archive file contains at least the given file entries. ## assert_contains 
-assert_contains(name, actual, expected, size, timeout, kwargs)
+assert_contains(name, actual, expected, size, kwargs)
Generates a test target which fails if the file doesn't contain the string. @@ -45,8 +45,7 @@ Depends on bash, as it creates an sh_test target. | name | target to create | none | | actual | Label of a file | none | | expected | a string which should appear in the file | none | -| size | the size attribute of the test target | None | -| timeout | the timeout attribute of the test target | None | +| size | standard attribute for tests | "small" | | kwargs | additional named arguments for the resulting sh_test | none | diff --git a/lib/private/diff_test.bzl b/lib/private/diff_test.bzl index 4ca5e2bd4..e1ea14ee6 100644 --- a/lib/private/diff_test.bzl +++ b/lib/private/diff_test.bzl @@ -106,7 +106,7 @@ _diff_test = rule( implementation = _diff_test_impl, ) -def diff_test(name, file1, file2, size = None, timeout = None, **kwargs): +def diff_test(name, file1, file2, size = "small", **kwargs): """A test that compares two files. The test succeeds if the files' contents match. @@ -115,15 +115,13 @@ def diff_test(name, file1, file2, size = None, timeout = None, **kwargs): name: The name of the test rule. file1: Label of the file to compare to file2. file2: Label of the file to compare to file1. - size: standard attribute for tests. Defaults to "small" if unspecified. - timeout: standard attribute for tests. Defaults to "short" if both timeout and size are unspecified. + size: standard attribute for tests **kwargs: The common attributes for tests. """ _diff_test( name = name, file1 = file1, file2 = file2, - size = size or "small", - timeout = timeout, + size = size, **kwargs ) diff --git a/lib/testing.bzl b/lib/testing.bzl index b2767b481..d50821881 100644 --- a/lib/testing.bzl +++ b/lib/testing.bzl @@ -6,7 +6,7 @@ load("//lib:diff_test.bzl", "diff_test") load("//lib:jq.bzl", "jq") load("//lib:params_file.bzl", "params_file") -def assert_contains(name, actual, expected, size = None, timeout = None, **kwargs): +def assert_contains(name, actual, expected, size = "small", **kwargs): """Generates a test target which fails if the file doesn't contain the string. Depends on bash, as it creates an sh_test target. @@ -15,8 +15,7 @@ def assert_contains(name, actual, expected, size = None, timeout = None, **kwarg name: target to create actual: Label of a file expected: a string which should appear in the file - size: the size attribute of the test target - timeout: the timeout attribute of the test target + size: standard attribute for tests **kwargs: additional named arguments for the resulting sh_test """ @@ -43,8 +42,7 @@ def assert_contains(name, actual, expected, size = None, timeout = None, **kwarg name = name, srcs = [test_sh], args = ["$(rootpath %s)" % expected_file, "$(rootpath %s)" % actual], - size = size or "small", - timeout = timeout, + size = size, data = [actual, expected_file], **kwargs ) From d5641f87ea57d7b475a6dd2420857df6dd0c0a21 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Fri, 19 Jul 2024 12:49:06 -0700 Subject: [PATCH 3/3] chore: fix one more test that should use size for defaulting --- lib/tests/write_source_files/write_source_file_test.bzl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/tests/write_source_files/write_source_file_test.bzl b/lib/tests/write_source_files/write_source_file_test.bzl index 0bebe6c13..d24206701 100644 --- a/lib/tests/write_source_files/write_source_file_test.bzl +++ b/lib/tests/write_source_files/write_source_file_test.bzl @@ -172,7 +172,7 @@ _write_source_file_test = rule( test = True, ) -def write_source_file_test(name, in_file, out_file, check_that_out_file_exists = True): +def write_source_file_test(name, in_file, out_file, check_that_out_file_exists = True, size = "small"): """Stamp a write_source_files executable and a test to run against it""" _write_source_file( @@ -190,5 +190,5 @@ def write_source_file_test(name, in_file, out_file, check_that_out_file_exists = write_source_file_target = name + "_updater", in_file = in_file, out_file = out_file, - timeout = "short", + size = size, )