support java supplier type

[mollder]

Summary

https://github.com/naver/fixture-monkey/issues/678

Originally, I was going to provide Java Fuction types and Kotlin function types, but as I implemented the Supplier, I had some questions (in NodeSetLazyManipulator class), and I thought it would be better to talk about this first and then implement other types, so I posted the Supplier first.

The questions I have are as follows.

1 operate the set code below, the Manipulator does not return the Supplier, but returns the value inside it. to solve this problem i forcibly wrapped it with the Supplier once more in the NodeSetLazyManipulator.

set("supplier", (Supplier<String>) () -> "test")

Although my code solve issue, it is so hard to recognize and I think it is easy to cause bugs in long-term perspective, so I was wondering what you think about fixing the feature as below.

• What do you think about adding an setLazy method to the user?
• what do you think about removing behavior of returning the value Lazyly When delivering Supplier parameters to the set method?

2 in CombinableArbitrary, it is supposed to return the value in a Lazy way once again, and I tried to use FixedCombinableArbitrary to return the Supplier itself, but it was blocked by dependence. So in order to solve issue, I forced to wrap it up with a Supplier once more. i think it is not good

• What do you think about adding the fromLazy method from the Util methods of CombinableArbitrary and removing it from the from (Supplier value) method?

3 i haven't implemented InnerSpec yet, do you want to support InnerSpec? (When I tested it, I found that innerSpec did not work well yet with Supplier)

How Has This Been Tested?

Multiple tests have been added in the FixureMonkeyTest class.

Is the Document updated?

No

I think it's better to add documentation after adding other types, but I wonder what you think about this.

If you want, I'll add the documentation to the location you want.

[CLAassistant]

All committers have signed the CLA.

[seongahjo]

@mollder Hello, thank you for your contributing 😃

I'll answer your questions later comments. Before that, I want to know if your approach can handle this situation.

class Foo {
  String foo;
}

String actual = fixture.giveMeBuilder(new TypeReference<Supplier<Foo>>(){})
    .set("$.foo", "expected")
    .sample()
    .get()
     .foo;

then(actual).isEqualTo("expected")

[seongahjo]

1 What do you think about adding an setLazy method to the user? what do you think about removing behavior of returning the value Lazyly When delivering Supplier parameters to the set method?

I think you want to set Supplier itself. In order to do that, You must wrapping it by Values.just. Just like below.

set("supplier", Values.just((Supplier<String>)() -> "test")))

2 in CombinableArbitrary, it is supposed to return the value in a Lazy way once again, and I tried to use FixedCombinableArbitrary to return the Supplier itself, but it was blocked by dependence. So in order to solve issue, I forced to wrap it up with a Supplier once more. i think it is not good What do you think about adding the fromLazy method from the Util methods of CombinableArbitrary and removing it from the from (Supplier value) method?

I see, but it should be careful to add new method fromLazy and remove form(Supplier), because it leads to breaking change. I think it is better to support Values.just in CombinableArbitrary just like set.

3 i haven't implemented InnerSpec yet, do you want to support InnerSpec? (When I tested it, I found that innerSpec did not work well yet with Supplier)

Can you tell me more details about it?

[mollder]

@seongahjo

Thank you for your detailed answer 👍 👍

1 using Value.just

I think you want to set Supplier itself. In order to do that, You must wrapping it by Values.just. Just like below.

set("supplier", Values.just((Supplier)() -> "test")))

I see, but it should be careful to add new method fromLazy and remove form(Supplier), because it leads to breaking change. I think it is better to support Values.just in CombinableArbitrary just like set.

I didn't know about how to use Value.just, and I changed it because I thought it was better to use Value.just

6fdb1b0

2 Expression bug

class Foo {
  String foo;
}

String actual = fixture.giveMeBuilder(new TypeReference<Supplier<Foo>>(){})
    .set("$.foo", "expected")
    .sample()
    .get()
     .foo;

then(actual).isEqualTo("expected")

As a result of my debugging, this bug was occurring because rootNode couldn't get children when I was covering objects like Suppliers or Optional.

So I fixed the problem by changing the IdentityNodeResolver to point directly to the child node.

how do you think about this approach

af05fd2

3 inner spec

I think if using Supplier in innerSpec, it won't work, so I asked the question But I actually wrote a test using Value.just and it worked well, so it is just my misunderstood

37786db

[seongahjo]

@mollder

So I fixed the problem by changing the IdentityNodeResolver to point directly to the child node. how do you think about this approach

Actually, IdentityNodeResolver is a node resolver that is converted from $ (root). It should not know the properties of the child because it is not its responsibility. Our recommended approach is to use the combination of NodeResolvers without changing them.

For example, $.str is converted to a combination of IdentityNodeResolver and PropertyNameResolver.

I think the reason it does not work without changing IdentityNodeResolver is that the Supplier type is treated as a container. The elements of the container type are treated as ElementProperty which has no property name. In order to referencing ElementProperty, you should use the expression [0].

So, I think it is time to create a new property for types like Optional, Supplier etc whose index is not meaningful.

What do you think?

[mollder]

@seongahjo

sorry to late reply

Are you thinking of creating a new Expression like (0) like the code below?

I thought there should be a new expression to create a CompositeNodeResolver in the toNodeResolver method of the ArbitraryExpression class.

And it becomes easier when adding a Function type or a BiFunction type

class Foo {
  String foo;
}

String actual = fixture.giveMeBuilder(new TypeReference<Supplier<Foo>>(){})
    .set("$(0).foo", "expected")
    .sample()
    .get()
     .foo;

[mollder]

If you have thought about not to make a new Expression, please comment

[seongahjo]

@mollder

If you have thought about not to make a new Expression, please comment

No, I think it should be used the expression as below.

String actual = fixture.giveMeBuilder(new TypeReference<Supplier<Foo>>(){})
    .set("$.foo", "expected")
    .sample()
    .get()
     .foo;

It should work without modifying IdentityNodeResolver

I think it could be done by using ContainerPropertyGenerator

[mollder]

ok I'll try it in that direction

Thank you for commenting every time

[mollder]

@seongahjo

I think the ObjectNode consists of three stages, including children and grandchildren, is scalable structure when SupplierIntrospector generates values, so I left that structure intact

However, if the ObjectNode consists of three stages, it is impossible to set the expression with the existing NodeResolver, so I added a WrappedNodeResolver and modified the test breaks accordingly.

What do you think of this direction?

[seongahjo]

@mollder

However, if the ObjectNode consists of three stages, it is impossible to set the expression with the existing NodeResolver, so I added a WrappedNodeResolver and modified the test breaks accordingly.

I think it is better to modify IdentityNodeResolver instead of making a new WrappedNodeResolver.

The IdentityNodeResolver should resolve the last child of a nested Supplier type.

For example, consider the generation of `Supplier<Supplier>'.

ObjectTree would look like this.

Supplier<Supplier<String>> -> Supplier<String> ->  String

IdentityNodeResolver should resolve the node of String type.

SingleElementProperty is a property that indicates IdentityNodeResolver should resolve the child node recursively.

[mollder]

@seongahjo 69353ab

As i understood, I have modified the code Is this the direction you're talking about?

[seongahjo]

@mollder Yes, that is exactly what I am thinking of.

Only one small change is needed.

[mollder]

conflict resolved

and thank for your comment 👍 i can't do it without your comment