Cannot register auto generators
Closed this issue · 10 comments
I'm trying to use junit-quickcheck to help me build a module which ensures equals() is implemented correctly over a codebase.
The way it works is (as a simple overview) reflecting over the classpath to find everything which implements equals(), generating instances of them with differing sets of parameters, and ensuring that if objects are constructed with different parameters, equals() returns false. I want this to be a drop-in module which doesn't require configuring generators in order to work.
To do this, I'm using junit-pholser to reflectively build generators using the constructor. This works beautifully as long as the constructor params already have generators.
Ideally, I'd like to also handle types which take custom types as parameters by using reflectively build generators for them. However, the Ctor and Fields generators can't be registered with GeneratorRepository.
Looking into the code, there's a practical reason why not: generators are regularly copied, by invoking their no-arg constructor. Ctor and Fields, being more flexible, take constructor params and therefore can't be reflectively copied in this manner.
This can be worked around by providing a new copying mechanism: I added copyOf() to Generator (defaulting to getClass().newInstance(), and overridden in Ctor and Fields to return a newly constructed instance with the original constructor arguments) and everything still appears to work fine (the only tests which failed were the ones which asserted I can't do that). See https://github.com/writeoncereadmany/junit-quickcheck for my changes, and https://github.com/writeoncereadmany/testeverything as the motivating example.
That then allows me to construct instances of types with an arbitrary depth of custom types in their field-hierarchy, without having to manually implement any generators. Practical reasons are one thing, though, and I've been very focused on solving this one problem outside the quickcheck context.
There may be good philosophical reasons not to permit registering of autogenerators. For example, to ensure that generators are built to provide a realistic, coherent distribution of values for the type in question (something which, for this context, I don't really care about). However, it would be nice (and powerful!) to at least allow opting in to registration of auto-generators.
@writeoncereadmany Thanks for this! I'll have a look, and perhaps we can merge the copyOf functionality in.
@writeoncereadmany Your changes look really good. I have one suggestion, which I added as a comment on the commit at your fork's master's HEAD. Feel free to submit a pull request; I can get this in reasonably quickly.
Thanks again!
Re: registration of auto-generators: What if we provided something along the lines of AutoCtor and AutoFields, that behaved just like Ctor and Fields, but answer canRegisterAsType(Class) with true?
Well that all depends on why Ctor and Fields answer canRegisterAsType(Class) as false.
If it's because they can't be instantiated reflectively, that's what copyOf addresses (and we need some sort of mechanism like this so we can create new instances which copy the args).
If it's because there's a design reason behind it, well, that depends on what the reason is. But if that's the case, why does that reason not apply in the AutoCtor and AutoFields case, which are identical to Ctor and Fields apart from this property?
I haven't delved super-deep - is there a concern Ctor and Fields might get auto-registered unintentionally?
@writeoncereadmany Ah, I see. testeverything is programmatically constructing a GeneratorRepository and registering generators for the types it discovers. Missed that earlier.
When a generator is registered, the repo arranges for it to potentially be called upon to generate values of either the concrete type (e.g. HashMap) and any of its supertypes (Object, Map, Serializable, ...). I added canRegisterAsType(Class) as way to say "let's not let this generator get called upon for certain supertypes". For example, I decided to have PropertiesGenerator not participate in generating values for property parameters of types Object, Hashtable, Map, or Dictionary, as I didn't want the (potentially) expensive generation of Properties objects to bog down property verification for properties with parameters of those types. In practice, I don't know whether this matters much; it certainly made junit-quickcheck's test suite faster.
Ctor and Fields were sort of intended for use by the library only (and in @From clauses), so I didn't want them registered. Now that there are other uses for these, I'm certainly open to reconsidering this decision.
Let me work with this a bit and see how it looks. Thanks!
Hmm, yeah, that makes a lot of sense.
The initial thought is maybe we don't want to register Ctor and Fields against its supertypes, but it's not as simple as that.
I've been developing testeverything's recursive generator a bunch more today, trying to get it to the point where I can run it against a real codebase, and am running against some interesting problems (eg: generics, interfaces, wildcards).
So for example if I need a generator for an interface, I'll need generators for implementing classes, but it does make sense to register them against the interface.
What I posted on my fork wasn't ever supposed to be taken as mergeable, rather: this is something I'm dabbling with, is this something you're open to and is there anything I should be aware of?
This is still very early days for me - I'm only a couple of days into spiking it and I don't know what shape it should end up in. Now I know you're open to this use-case, and have a bit more context, I need to get a firmer understanding of my problem's requirements.
@writeoncereadmany Absolutely! I'm excited to see people exploring uses of the library that I hadn't imagined.
The more I think about it, the more I believe I need not have restricted Ctor and Fields in the way I did. The "internal"/core generators, with the exception of the special ZilchGenerator, are excluded from the service loader manifest for junit-quickcheck-core-*.jar, so they don't get auto-registered anyway. And, if you want to register a Ctor or Fields as a generator for things of type X or any of its supertypes, you should be free to do so.
@writeoncereadmany Have a look at this pull req, at your convenience -- pretty straightforward change, hopefully removes some impediments to what you're looking to accomplish. Appreciate the feedback!
@writeoncereadmany Let me know as soon as you can verify this -- after that, we can consider the issue resolved. Thanks!
Looks good to me! Closing.