Enable unittest.suite to accept partial calls of test rules #276

dws · 2020-09-24T20:54:05Z

This permits using unittest.suite with test rules that have nondefault
attributes, while retaining compatibility with current usage.

For instance, this permits setting a timeout on each test in a
unittest.suite. Previously, all tests in a unittest.suite would
have the default timeout, with no good way to alter this. This
made it hard to eliminate all the warnings produced from using the
--test_verbose_timeout_warnings bazel option.

While timeouts were the motivation, the solution here is not specific
to timeouts. It will permit arbitrary additional arguments to the test
rules in a unittest.suite.

Fixes #98

dws · 2020-09-24T21:18:49Z

Looks like the buildifier issues in CI are pre-existing problems: https://buildkite.com/bazel/bazel-skylib/builds/1240#annotation-buildifier

dws · 2020-09-24T23:07:02Z

#278 should address the CI issues.

dws · 2020-09-25T21:19:19Z

CI is now green!

dws · 2020-09-25T21:27:54Z

Note: I did not make any changes under docs. These files appear to be generated. In order to run docs/regenerate_docs.sh with bazel 3.1.0 I needed to remove the option --experimental_remap_main_repo from that script. Then, when I ran docs/regenerate_docs.sh, it did not recreate the docs as they currently exist in that directory. So I'm not sure what the proper process is for updating the files in that directory.

dws · 2020-09-25T21:35:23Z

It occurs to me that the new types.is_partial() predicate itself could be considered a reasonable unit of change for a PR.
If you'd prefer to have a separate PR for that, please just let me know.

dws · 2020-11-12T17:50:57Z

@c-parsons @jin @meisterT Any thoughts on this?

alandonovan · 2020-11-12T18:21:37Z

lib/types.bzl

+    Returns:
+      True if v was created by partial.make(), False otherwise.
+    """
+    return type(v) == _a_struct_type \


This is only a heuristic. I wonder if making the partial struct an instance of a distinct provider might allow a more robust check.

FWIW, partial is hack to work around the fact that nested def statements don't currently work, but I would like to fix that, at which point we can get start to rid of partial.

I was following the precedent and model of _is_set() just above, since it, too, pertains to a skylib-specific type.
Note that _is_partial() is a macro and gets used by other macros in skylib, in particular, in _suite() in lib/unittest.bzl. My understanding is that providers are available only within rules, so they would not be available for use by macros.

Is there an example that you could point me to that would illustrate the alternative implementation you're suggesting?

I was following the precedent and model of _is_set() just above, since it, too, pertains to a skylib-specific type.

Yes, your inclination to follow the local style is good; it's the local style that is the problem. The is_set function and the underlying set abstraction really shouldn't exist: it's wasteful to create a struct to wrap a hash table: it adds two additional indirections and several more allocations.

The whole types.bzl file should ideally cease to exist. Any time a function's doc string is longer than its body, and it holds no special knowledge or decisions, it's a bad abstraction. Every one of these functions could be implemented just as well by the caller and they would safe themselves a dependency. (This module is Starlark's answer to JavaScript's "leftpad".)

Regarding my providers suggestion, you can construct a distinct provider and use its instances for partial values, but apparently we provide no way to query the provider of a struct value, which is what you'd need for the is_partial function, so it won't work. Please ignore it.

I suggest you move the is_partial function into the partial module.

Will do. Would you like the name to remain is_partial? That way the usage sites would look like partial.is_partial(), which strikes me as a bit redundant. Another possibility that occurs to me is to call it made, since it checks whether something came from make. That way the usage sites would look like partial.made(), to check if something came from partial.make().

partial.is_instance(x)? made seems a little too cryptic.

Sounds good, I'll use is_instance().

Now, is it okay to make partial.is_instance() use the predicates defined in types.bzl? Or should I code it so that partial.bzl does not do any additional load()s?

I will err on the side of being self-contained. If you'd prefer to keep things DRY at the cost of making partial.bzl depend on types.bzl, please just let me know.

Okay, I believe I've addressed all your comments so far.

alandonovan · 2020-11-12T18:22:25Z

lib/types.bzl

@@ -149,4 +163,5 @@ types = struct(
    is_function = _is_function,
    is_depset = _is_depset,
    is_set = _is_set,
+    is_partial = _is_partial,


Can we move this function to the partial file so that types doesn't depend on it?

I'm not sure what you mean by "so that types doesn't depend on it". In my current changes, is_partial() in types.bzl does not depend on partial.bzl, just as is_set() does not depend on any of the skylib set implementations. Both of these predicates do duplicate knowledge of how their respective data structures are implemented elsewhere.

Exactly: the knowledge belongs in sets or partial, not in types. Even though there may be no physical dependency, there is a logical one, and it is unnecessary.

dws

My goal here is to enable constructing a unittest.suite with rules that have nondefault attributes. Since skylib itself provides for partial instantiation of rules, it seemed reasonable to try to solve the problem using tools that skylib already provides. But, if there's a different approach that you'd prefer to see, I'll be happy to explore it.

dws · 2020-11-12T20:22:18Z

lib/types.bzl

+    Returns:
+      True if v was created by partial.make(), False otherwise.
+    """
+    return type(v) == _a_struct_type \


I was following the precedent and model of _is_set() just above, since it, too, pertains to a skylib-specific type.
Note that _is_partial() is a macro and gets used by other macros in skylib, in particular, in _suite() in lib/unittest.bzl. My understanding is that providers are available only within rules, so they would not be available for use by macros.

Is there an example that you could point me to that would illustrate the alternative implementation you're suggesting?

dws · 2020-11-12T20:28:10Z

lib/types.bzl

@@ -149,4 +163,5 @@ types = struct(
    is_function = _is_function,
    is_depset = _is_depset,
    is_set = _is_set,
+    is_partial = _is_partial,


I'm not sure what you mean by "so that types doesn't depend on it". In my current changes, is_partial() in types.bzl does not depend on partial.bzl, just as is_set() does not depend on any of the skylib set implementations. Both of these predicates do duplicate knowledge of how their respective data structures are implemented elsewhere.

This permits using `unittest.suite` with test rules that have nondefault attributes, while retaining compatibility with current usage. For instance, this permits setting a `timeout` on each test in a `unittest.suite`. Previously, all tests in a `unittest.suite` would have the default timeout, with no good way to alter this. This made it hard to eliminate all the warnings produced from using the `--test_verbose_timeout_warnings` bazel option. While timeouts were the motivation, the solution here is not specific to timeouts. It will permit arbitrary additional arguments to the test rules in a `unittest.suite`. Fixes bazelbuild#98

alandonovan · 2020-11-13T02:03:45Z

lib/partial.bzl

+    #
+    # Since this check is heuristic anyway, we simply check for the
+    # presence of a "function" attribute without checking its type.
+    return type(v) == _a_struct_type \


It's frustrating that Starlark doesn't offer better features for encapsulation. Like I mentioned, providers act as distinct constructors but don't have an 'instanceof' operation. Another approach would be to add a field whose value is a distinguished value, and check that it matches in is_instance, but there's no way to make the field private to stop external clients from accessing it.

Fortunately in this instance, lambdas should render the whole module unnecessary.

I will look forward to better mechanisms in the future. In the meantime, thank you for helping to get this landed!

dws requested review from aiuto, c-parsons, jin and laurentlb as code owners September 24, 2020 20:54

googlebot added the cla: yes label Sep 24, 2020

dws mentioned this pull request Sep 24, 2020

How does unittest support attributes on the tests? #98

Closed

aiuto removed their request for review September 24, 2020 20:58

dws force-pushed the dws-unittest-partial-98 branch from 3cf001b to 333c80d Compare September 25, 2020 21:15

dws force-pushed the dws-unittest-partial-98 branch from 333c80d to c4f81ca Compare September 25, 2020 22:01

alandonovan reviewed Nov 12, 2020

View reviewed changes

dws commented Nov 12, 2020

View reviewed changes

dws force-pushed the dws-unittest-partial-98 branch 3 times, most recently from 062c9bf to cd7a3e1 Compare November 12, 2020 22:09

dws force-pushed the dws-unittest-partial-98 branch from cd7a3e1 to 8909370 Compare November 12, 2020 22:14

dws added 2 commits November 12, 2020 19:06

Respond to review feedback.

bdcd227

Document a breaking change in bazel that this code needs to be aware of.

884ce4e

alandonovan approved these changes Nov 13, 2020

View reviewed changes

alandonovan merged commit ed7f03c into bazelbuild:master Nov 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable unittest.suite to accept partial calls of test rules #276

Enable unittest.suite to accept partial calls of test rules #276

dws commented Sep 24, 2020

dws commented Sep 24, 2020

dws commented Sep 24, 2020

dws commented Sep 25, 2020

dws commented Sep 25, 2020 •

edited

dws commented Sep 25, 2020

dws commented Nov 12, 2020

alandonovan Nov 12, 2020

dws Nov 12, 2020

alandonovan Nov 12, 2020

dws Nov 12, 2020

alandonovan Nov 12, 2020

dws Nov 12, 2020

dws Nov 13, 2020

dws Nov 13, 2020

alandonovan Nov 12, 2020

dws Nov 12, 2020

alandonovan Nov 12, 2020

dws left a comment

dws Nov 12, 2020

dws Nov 12, 2020

alandonovan Nov 13, 2020

dws Nov 13, 2020

Enable unittest.suite to accept partial calls of test rules #276

Enable unittest.suite to accept partial calls of test rules #276

Conversation

dws commented Sep 24, 2020

dws commented Sep 24, 2020

dws commented Sep 24, 2020

dws commented Sep 25, 2020

dws commented Sep 25, 2020 • edited

dws commented Sep 25, 2020

dws commented Nov 12, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dws 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

dws commented Sep 25, 2020 •

edited