Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Nov 13, 2020

Conversation

dws
Copy link
Contributor

@dws dws commented Sep 24, 2020

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 dws requested review from aiuto, c-parsons, jin and laurentlb as code owners Sep 24, 2020
@aiuto aiuto removed their request for review Sep 24, 2020
@dws
Copy link
Contributor Author

@dws dws commented Sep 24, 2020

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

@dws
Copy link
Contributor Author

@dws dws commented Sep 24, 2020

#278 should address the CI issues.

@dws dws force-pushed the dws-unittest-partial-98 branch from 3cf001b to 333c80d Compare Sep 25, 2020
@dws
Copy link
Contributor Author

@dws dws commented Sep 25, 2020

CI is now green!

@dws
Copy link
Contributor Author

@dws dws commented Sep 25, 2020

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
Copy link
Contributor Author

@dws dws commented Sep 25, 2020

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 dws force-pushed the dws-unittest-partial-98 branch from 333c80d to c4f81ca Compare Sep 25, 2020
@dws
Copy link
Contributor Author

@dws dws commented Nov 12, 2020

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

lib/types.bzl Outdated
Returns:
True if v was created by partial.make(), False otherwise.
"""
return type(v) == _a_struct_type \
Copy link
Contributor

@alandonovan alandonovan Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dws dws Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@alandonovan alandonovan Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dws dws Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor

@alandonovan alandonovan Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dws dws Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@dws dws Nov 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dws dws Nov 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lib/types.bzl Outdated
@@ -149,4 +163,5 @@ types = struct(
is_function = _is_function,
is_depset = _is_depset,
is_set = _is_set,
is_partial = _is_partial,
Copy link
Contributor

@alandonovan alandonovan Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dws dws Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@alandonovan alandonovan Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dws dws left a comment

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.

lib/types.bzl Outdated
Returns:
True if v was created by partial.make(), False otherwise.
"""
return type(v) == _a_struct_type \
Copy link
Contributor Author

@dws dws Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

lib/types.bzl Outdated
@@ -149,4 +163,5 @@ types = struct(
is_function = _is_function,
is_depset = _is_depset,
is_set = _is_set,
is_partial = _is_partial,
Copy link
Contributor Author

@dws dws Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dws dws force-pushed the dws-unittest-partial-98 branch 3 times, most recently from 062c9bf to cd7a3e1 Compare Nov 12, 2020
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
@dws dws force-pushed the dws-unittest-partial-98 branch from cd7a3e1 to 8909370 Compare Nov 12, 2020
#
# 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 \
Copy link
Contributor

@alandonovan alandonovan Nov 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dws dws Nov 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alandonovan alandonovan merged commit ed7f03c into bazelbuild:master Nov 13, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants