Fix unit test attr forwarding #927
Conversation
rules/test.bzl
Outdated
| test_attrs = {k: v for (k, v) in kwargs.items() if k not in _APPLE_BUNDLE_ATTRS} | ||
| test_attrs = {k: v for (k, v) in kwargs.items() if k in _IOS_TEST_KWARGS} |
There was a problem hiding this comment.
This has been broken for a while it seems but afaik the right logic here is to grab the attrs for the unit test rule.. not what it was originally doing. Without this change features is dropped because it appars in _APPLE_BUNDLE_ATTRS (because every rule supports the features attr).
There was a problem hiding this comment.
My guess is this might have been to support custom rules with attributes unknown to this macro?
There was a problem hiding this comment.
Updated this to maintain the original logic with the addition of including implicit rule attrs as well (like features)
The `test.bzl` macros were incorrectly forwarding some of the attrs like `features`. This was leading to silent errors where things passed via `features` were dropped (such as main thread checking). This PR fixes the macro by properly forwarding ALL supported unit test attrs to the unit test rule.
luispadron
force-pushed
the
luis/fix-attr-forwarding-for-test-rules
branch
from
5c042d8
to
37659c1
Compare
| @@ -45,6 +43,22 @@ _APPLE_BUNDLE_ATTRS = { | |||
| ] | |||
| } | |||
|
|
|||
| # See: https://bazel.build/reference/be/common-definitions#common-attributes | |||
There was a problem hiding this comment.
so if bazel's newer version contains new common attribute, we need to update this list right?
There was a problem hiding this comment.
Ideally yeah, however most of these attrs are unused
| @@ -104,7 +118,7 @@ def _make_test(name, test_rule, **kwargs): | |||
| Helper to create an individual test | |||
| """ | |||
| runner = kwargs.pop("runner", None) or _DEFAULT_APPLE_TEST_RUNNER | |||
| test_attrs = {k: v for (k, v) in kwargs.items() if k not in _APPLE_BUNDLE_ATTRS} | |||
| test_attrs = {k: v for (k, v) in kwargs.items() if (k not in _APPLE_BUNDLE_ATTRS) or (k in _ALWAYS_AVAILABLE_ATTRS)} | |||
There was a problem hiding this comment.
when downstream project point to this commit, probably worth printing out what is dropped before VS after, to see if anything other than features are dropped also
There was a problem hiding this comment.
Do you mean adding a print here?
There was a problem hiding this comment.
yeah but do it temp. during the testing
The
test.bzlmacros were incorrectly forwarding some of the attrs likefeatures. This was leading to silent errors where things passed viafeatureswere dropped (such as main thread checking). This PR fixes the macro by properly forwarding ALL supported unit test attrs to the unit test rule.