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

Generate nulls for parameters annotated as Nullable #210

Merged
merged 15 commits into from
Nov 27, 2018

Conversation

berrueta
Copy link
Contributor

This allows to write properties that take nullable parameters. This is particularly useful in Kotlin, where explicitly nullable types are very common (they serve similar purposes to the Optional type in Java 8+).

Rather than adding support for all the annotations that can indicate nullability (for example, these are the ones that the Kotlin compiler understands: https://github.com/JetBrains/kotlin/blob/master/core/descriptors.jvm/src/org/jetbrains/kotlin/load/java/JvmAnnotationNames.kt#L21 ), I've kept it to a minimum, but enough to have interoperability with Kotlin.

In the absence of annotations, null will not be generated, therefore this change is backwards compatible and will not break existing properties in Java or Kotlin.

@pholser
Copy link
Owner

pholser commented Nov 12, 2018

@berrueta Thanks for this! I'll review and get back with you soon!

@pholser
Copy link
Owner

pholser commented Nov 12, 2018

@berrueta A couple of questions for you:

  1. It looks as though the change makes it so that, when deciding what set of generators could produce values for a given parameter, GeneratorRepository will include the NullGenerator in the set of matching generators if the parameter is marked with one of the recognized @Nullable annotations. I wonder if, instead, NullGenerator could be more like a decorator on the set of matching generators. When asked to generate(), the NullGenerator could produce null with some (configurable) probability p, and defer to the matching set with probability 1 - p. Do you think such a design might prove even more useful for test writers?

  2. What should happen in the event of @Nullable being discovered on a primitive parameter? Do we lean on compilers not to allow this?

  3. Whatever design we land on, would you be willing to add some discussion to the Markdown docs? Feel free to add yourself as a contributor in the root POM also.

Cheers!

@berrueta
Copy link
Contributor Author

Hi @pholser ,

Thank you for your feedback. I intentionally aimed for a minimalistic change in this first iteration so I could receive some feedback. I like your suggestion to turn NullGenerator into a decorator. That would allow not only to configure the probability of nulls, but also to extend the nullability to other types like arrays and enums. I'm going to give it a try and I'll update the PR.

Do you have any suggestion on how to expose the configurable probability p? Perhaps a new annotation?

I'll confirm the behaviour in the case of primitive types, and add some documentation.

@pholser
Copy link
Owner

pholser commented Nov 14, 2018

@berrueta For configuring the probability of null generation on a parameter, I'm thinking either 1) a configuration annotation, 2) an attribute on the @Property annotation, 3) a separate annotation, maybe something like com.pholser.junit.quickcheck.NullsAllows(probability = ...); along with what jqwik does, for the Gen interface add something like its injectNulls.

@berrueta
Copy link
Contributor Author

Hi @pholser ,

I followed your suggestion and transformed NullGenerator into a decorator called NullableGenerator. I also added a check to make sure nulls are not generated for primitive types.

I'm not done yet. I still have to make the probability p configurable, add the documentation, etc., but I would appreciate if you could give me some feedback on the new design.

Thank you.

@berrueta
Copy link
Contributor Author

I've pushed a commit introducing a new NullAllows annotation (tentative name). I have a couple of design questions, please let me know what you think:

  • By design, the new annotation is not sufficient by itself to indicate that the parameter is nullable. I didn't want to introduce yet another nullable annotation. Instead, I see it as a companion to the conventional nullable annotations like javax.annotation.Nullable.

  • Also by design, the new annotation is not a GeneratorConfiguration. I feel like the semantics are slightly different from the other generator configuration annotations. For example, it is not expected that the delegate generator (e.g., IntegerGenerator) will understand this annotation.

Any feedback would be welcome.

Copy link
Owner

@pholser pholser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Diego, thanks for this! I have a few suggestions, spread throughout the changeset.

Once we agree on the code, please do add as many tests as you can stand, and a section in the Markdown docs indicating that you can signal to the generators how often to inject null.