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

Support multiple targets under test #67

Closed
rickeylev opened this issue Aug 17, 2023 · 3 comments · Fixed by #78
Closed

Support multiple targets under test #67

rickeylev opened this issue Aug 17, 2023 · 3 comments · Fixed by #78
Labels
enhancement New feature or request

Comments

@rickeylev
Copy link
Contributor

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:

  • Add a targets arg. It is mutually exclusive with the target arg.
  • It is either a list of target names or a dict of name -> <target under test kwargs>.
  • When list is provided, each target under test is given the same settings as if a single target was provided (e.g. the same aspect, config settings, expect failure, etc settings)
  • When a dict is provided, each target under test is given the settings as specified by their associated key. This allows each target under test to have custom config settings, aspects, etc.

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.

@rickeylev rickeylev added the enhancement New feature or request label Aug 17, 2023
@rickeylev
Copy link
Contributor Author

cc @murali42

@rickeylev
Copy link
Contributor Author

rickeylev commented Dec 3, 2023

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

  • Add a 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.
  • Add a 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.
  • Treat 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.

copybara-service bot pushed a commit that referenced this issue Dec 6, 2023
This allows customizing what attributes are considered a target under test,
i.e. that they have aspects, config settings, etc applied. This allows having
multiple targets be tested, which is useful for comparing targets in different
configurations, multiple targets with a custom configuration, or some combination
of that.

When this is used, the implementation function is passed a struct of the
targets under test, keyed by their attribute name, instead of the singular
target under test.

Fixes #67

PiperOrigin-RevId: 588220718
copybara-service bot pushed a commit that referenced this issue Dec 6, 2023
This allows customizing what attributes are considered a target under test,
i.e. that they have aspects, config settings, etc applied. This allows having
multiple targets be tested, which is useful for comparing targets in different
configurations, multiple targets with a custom configuration, or some combination
of that.

When this is used, the implementation function is passed a struct of the
targets under test, keyed by their attribute name, instead of the singular
target under test.

Fixes #67

PiperOrigin-RevId: 588220718
@rickeylev
Copy link
Contributor Author

After some discussion, going to revise it a bit:

  • Add targets arg. It's a map of attribute -> target string.
  • The attributes in targets are automatically created. This eliminates boilerplate for a common case (multiple TUT with same config).
  • The attributes can be customized using attrs as described.

copybara-service bot pushed a commit that referenced this issue Dec 11, 2023
This allows customizing what attributes are considered a target under test,
i.e. that they have aspects, config settings, etc applied. This allows having
multiple targets be tested, which is useful for comparing targets in different
configurations, multiple targets with a custom configuration, or some combination
of that.

When this is used, the implementation function is passed a struct of the
targets under test, keyed by their attribute name, instead of the singular
target under test.

Fixes #67

PiperOrigin-RevId: 588220718
copybara-service bot pushed a commit that referenced this issue Dec 11, 2023
This allows customizing what attributes are considered a target under test,
i.e. that they have aspects, config settings, etc applied. This allows having
multiple targets be tested, which is useful for comparing targets in different
configurations, multiple targets with a custom configuration, or some combination
of that.

When this is used, the implementation function is passed a struct of the
targets under test, keyed by their attribute name, instead of the singular
target under test.

Fixes #67

PiperOrigin-RevId: 588220718
copybara-service bot pushed a commit that referenced this issue Dec 11, 2023
This allows customizing what attributes are considered a target under test,
i.e. that they have aspects, config settings, etc applied. This allows having
multiple targets be tested, which is useful for comparing targets in different
configurations, multiple targets with a custom configuration, or some combination
of that.

When this is used, the implementation function is passed a struct of the
targets under test, keyed by their attribute name, instead of the singular
target under test.

Fixes #67

PiperOrigin-RevId: 588220718
copybara-service bot pushed a commit that referenced this issue Dec 11, 2023
This allows customizing what attributes are considered a target under test,
i.e. that they have aspects, config settings, etc applied. This allows having
multiple targets be tested, which is useful for comparing targets in different
configurations, multiple targets with a custom configuration, or some combination
of that.

When this is used, the implementation function is passed a struct of the
targets under test, keyed by their attribute name, instead of the singular
target under test.

Fixes #67

PiperOrigin-RevId: 588220718
copybara-service bot pushed a commit that referenced this issue Dec 11, 2023
This allows customizing what attributes are considered a target under test,
i.e. that they have aspects, config settings, etc applied. This allows having
multiple targets be tested, which is useful for comparing targets in different
configurations, multiple targets with a custom configuration, or some combination
of that.

When this is used, the implementation function is passed a struct of the
targets under test, keyed by their attribute name, instead of the singular
target under test.

Fixes #67

PiperOrigin-RevId: 588220718
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant