Closed rickeylev closed 9 months ago
cc @murali42
After running into this situation again, I think the originally proposed API is a bit too niche (it only allows controlling the config settings). With the addition of the analysis_test.attr_values
arg, it's possible to make something much more powerful with some minor tweaks.
Revised proposal
In the example below, we have two targets under test, subject1 and subject2. Each has different config settings and aspects applied. This is done using a dict with some special keys.
The dicts tell how to build the attribute. Within the analysis_test function, we mixin the extra aspects and cfg transition into those values to construct the attr.<whatever>
call. This is a small extension of how the attrs
arg is processed, so relatively non-invasive.
analysis_test(
attr_values = {"subject1": ":s1", "subject2": ":s2"}
attrs = {
"subject1": {"@attr": attr.label, "@config_settings": ..., "aspects": ...}
"subject1": {"@attr": attr.label, "@config_settings": ..., "aspects": ...}
}
)
I'm not a fan of magic keys and raw dicts, but the two things we require are (1) the attr.XXX
function to call to create the attribute, and (2) a mutable dict of kwargs to pass to that call (it has to be mutable so we can append to the aspects
key and set the cfg
key).
I suppose a builder-esq type of API would be possible, analysis_test_builder().add_attr(...)
, but ehhh, that seems like a lot of work, and seems like a much more complicated API to understand (the apis for Subjects are harder to figure out because of their fluency; adding that barrier just to define a test seems like too much).
Another alternative would be to allow passing function for the attribute and push all the attribute construction onto the caller. e.g. attrs = {"foo": lambda spec: attr.label(<use spec>)}
. The spec
arg would be a struct with stuff to create the equivalent transition, aspects, etc. This would be pretty powerful, but maybe too powerful. Also a bit awkward; the contained logic wouldn't nice fit in a lambda and would involve boilerplate logic to merge kwargs, config_settings, aspect lists, build the transition, etc. That seems like a lot of work when the two main use cases are changing config settings and asepects. It's feasible to later extend the api to accept a function, so we're not backed into a corner here.
Discarded ideas
targets_attrs
arg to carry the raw dicts. This better separates the attribute definitions for targets under tests, but is a bit awkward to split the singular set of attributes into two args.targets
arg, which is a dict of attribute name -> spec
, where "spec" tells the target to use, the config settings to apply, and the attribute definition. This just seems like a very complex object when written out, and reuse of the pieces seems a bit awkward.analysis_test
**kwargs
as attribute names that are targets under test. This is basically just moving the analysis_test(attr_values = {k:v})
part "up" to be analysis_test(k=v)
. However, this idea was originally considered when attr_values
was introduced; it was discarded because using **kwargs
for that makes it difficult to grow the analysis_test
function signature safely.After some discussion, going to revise it a bit:
targets
arg. It's a map of attribute -> target string.targets
are automatically created. This eliminates boilerplate for a common case (multiple TUT with same config).attrs
as described.
On more than one occasion, I or others have needed to have multiple targets under test. This is usually done to verify that a particular configuration change doesn't change behavior from a base line, or to verify that the rule-under-test ignores some configuration change. By having a "baseline" with fixed config settings, it makes it possible to reliably check the expected vs actual state.
This is somewhat possible today, but is rather verbose. If config settings are involved, it's even more verbose, and if different config settings are involved for the targets under test, then even more.
Proposed API:
targets
arg. It is mutually exclusive with thetarget
arg.name -> <target under test kwargs>
.Hm, well, I'm not sure I 100% like this proposed api, but it works. I'm just not a fan of currying through arbitrary lists of kwargs through things.