flyingmutant / rapid

Rapid is a modern Go property-based testing library
https://pkg.go.dev/pgregory.net/rapid
Mozilla Public License 2.0
579 stars 25 forks source link

Support Make customization #64

Open matthchr opened 7 months ago

matthchr commented 7 months ago

I'd like to use rapid.Make to generate structs. It already does this great for simple structures, but in my case the structure I'd like to generate is quite complex:

  1. Deeply nested.
  2. Some fields are recursive.
  3. Some fields are built out of types that have private fields

These facts mean that by default Make doesn't work for me.

One possible solution is that I use Custom instead and build my struct that way. I'm very new to rapid but as far as I can tell that requires me explicitly build each section of this structure. This is possible but a lot of work.

Instead, it would be nice if I could customize Make by overriding the default generators, but only for certain types or certain fields. gopter arbitraries has a similar capability.

Naïvely, something like Make[V any](overrides ...*Generator[any]) *Generator[V] might work, where in my test I could first set up the overrides I know that I need, and then call Make and supply them, and it would prefer the overrides but fall back to the defaults. Obviously if there's a desire to avoid changing the signature of Make, a variant could be used instead.

The disadvantage of this approach is that it allows type overrides but doesn't achieve field-specific overrides. To achieve a field specific override you'd need to override the type the field resided on and use Custom to generate the whole struct. I suppose a MakeVariant (ignore the name) could take (overrides []*Generator[any], fieldOverrides []*FieldGenerator[any]) where

type FieldGenerator[V any] struct {
    typ reflect.Type // Type of the containing struct
    field string
    gen *Generator[V]
}

I'm not sure that the field-specific override really fits the interfaces that you have defined well, but at least the type-specific one feels like it would slot in quite nicely.

Thoughts? Is there some way to do this already that I just haven't noticed?

flyingmutant commented 7 months ago

When I designed Make, I mostly though about it as a helper to avoid writing trivial Custom generators for POD types. Possible extension I considered (but did not implement) was to allow types to implement something like

type Drawable interface {
    Draw(t *rapid.T)
}

This seems simpler to understand and use than allowing type-specific overrides specific to each Make() call. Field-specific overrides look like too much magic to my taste, relatively complex API for niche use case that does not solve the problem completely (e.g. private fields are still impossible to generate).

I hope that this interface-based design can be good enough extensibility story, and for anything more complex Custom is the way to go (which, of course, can internally make calls to Make to generate most of the fields).

matthchr commented 7 months ago

This seems simpler to understand and use than allowing type-specific overrides specific to each Make() call.

It's probably slightly simpler to understand and probably moderately simpler to implement (doing type-detection based on generics well would prefer https://github.com/golang/go/issues/54393 but it can be done hackily without that, I think). It does limit things to only a single override per-type per test package, because interface impls are package scoped, versus an overrides ...*Generator[any] approach which can have different overrides for different tests - which seems nice.

Field-specific overrides look like too much magic to my taste, relatively complex API for niche use case that does not solve the problem completely (e.g. private fields are still impossible to generate).

Tend to agree here, field-specific overrides maybe don't make a lot of sense with the interfaces defined in rapid.

I may not have been clear here though: the overrides wasn't to solve the problem of "let me set private fields" and instead is trying to let me specify the minimal set of generators rather than do them all by hand. With at least type-specific overrides to the reflection run by Make the types containing private fields can have a Drawable interface or override specified for them, where users could produce one using the public factory method or equivalent without needing to use Custom. In my case, the type with private fields in question is time.Time and I can write a generator that produces valid time.Times without attempting to set any private fields, but the current interface offered by rapid requires me to specify the entire rest of my object in Custom in order to do that. I know I can call Make inside Custom but if you imagine an object whose JSON representation is something like:

{
    "outer": {
        "someString": "abc",
        "someInt": 5,
        "someNested1": {
            "someString": "abc",
            "someInt": 5,
            ... // 10 other fields at this level
            "someMoreNested": {
                "time": "..." // Need to override this as it can't be generated with reflection
            }
        },
        ... // 20 other fields at this level, or more
    },
}

The amount of work/maintenance to just supply a construction mechanism for time's via some override, versus use Custom for everything is quite significant and doesn't age well; it would be easy to add new fields to the structure but forget to include them in the Custom. Leaning on the reflection of Make with a minimal set of overrides defined would probably reduce initial work and maintenance burden significantly.

How open are you to either overrides []*Generator[any] or Drawable interface support? I don't think actually implementing either of those options is particularly hard. I've got a local (mostly untested) copy of overrides []*Generator[any] I was playing around with.

flyingmutant commented 7 months ago

I am open to interface-based extensibility here because it does not change or complicate the meaning of Make ("just give me a random thing"), while making it work for more things and making it possible to customize behaviour based on the type in an idiomatic Go fashion.

As for per-call overrides, I feel that while it has more flexibility, and may help with your use-case more, it will enourage people to expect Make to be some kind of micro-DSL for random data generation. It will be harder to draw the line where the extensibility and flexibility should stop, once we start to follow that path.

By the way, how much of your problem will be solved if rapid would have built-in time.Time generators?

matthchr commented 7 months ago

By the way, how much of your problem will be solved if rapid would have built-in time.Time generators?

There are two things causing Make to panic from my large struct: One is time.Time, the other is a recursive structure. So it's ~half of the issue causing Make to panic would be resolved with automatic time.Time support. The other half I think can only be resolved with some form of override. Then I can override the recursive type to avoid generating infinitely recursive structures. In practice there's a max depth that's interesting to generate and that max is pretty small (1-2).

As for per-call overrides, I feel that while it has more flexibility, and may help with your use-case more, it will enourage people to expect Make to be some kind of micro-DSL for random data generation.

I'll be honest: this is somewhat what I was trying to use it for, as an alternative to gopter arbitraries. Gopter doesn't seem to have been updated much recently and arbitraries is really slow in comparison to Make (I'm not 100% sure why). If you're against Make being expanded in this way (which is fine), maybe what I need to do is write my own DSL for random data generation and then use it in the context of Custom to accomplish what I want (which is some sort of reflection-based, but configurable, random data generation + property testing).

I am open to interface-based extensibility...

k, let me play around with my requirements and I may send a PR. I'll also consider sending a PR for built-in time.Time handling if you're open to that as well.

flyingmutant commented 7 months ago

Make should work for recursive types, if it does not — can you open a separate issue with a reproducer for that?

As for time.Time, that would require durations (trivial) and locations as well. Probably a good idea to research what Hypothesis is doing, as well as jqwik and others.

flyingmutant commented 3 weeks ago

@matthchr take a look at #72, is it suitable for your use-cases?

matthchr commented 3 weeks ago

yes, I believe that it would. I have a similar change that I was prototyping here but got sidetracked and never finished it.

chrisseto commented 3 weeks ago

Apologies for not search through the issues before opening the pull request. @matthchr if you have any requests/suggestions/feedback on the linked PR I'm all ears.

jharveyb commented 3 weeks ago

The change in #72 greatly simplified using rapid for my use case, which is testing gRPC parsing code. The gRPC generated types have private fields that are just interfaces, which caused Make() to fail. But now I can set default empty structs for those fields, and also override Make() to generate gRPC messages that have other gRPC messages as members.

As an example:

https://github.com/lightninglabs/taproot-assets/commit/8b579dab660d8a48163f6a0b85a762af11af6cf2

Default empty constructors for private fields (inherited by all the custom MakeConfig objects defined later):

https://github.com/lightninglabs/taproot-assets/blob/8b579dab660d8a48163f6a0b85a762af11af6cf2/rpcserver_test.go#L31

Setting generators for gRPC message members:

https://github.com/lightninglabs/taproot-assets/blob/8b579dab660d8a48163f6a0b85a762af11af6cf2/rpcserver_test.go#L98

So this makes rapid a proper alternative to other tools like https://github.com/trailofbits/protofuzz and https://github.com/google/libprotobuf-mutator/ .

matthchr commented 3 weeks ago

Apologies for not search through the issues before opening the pull request. @matthchr if you have any requests/suggestions/feedback on the linked PR I'm all ears.

No apologies needed! I never got around to submitting a PR. I took at look at your PR and it seems good to me, I don't have much feedback other than it would be useful and I hope it's accepted/merged as I'll likely make use of it.