Skip to content

Add IntegerCombinableArbitrary for easy Integer customization#1192

Merged
seongahjo merged 4 commits intonaver:mainfrom
Denia-park:feat/add-IntegerCombinableArbitrary-for-easy-Integer-customization
Jun 10, 2025
Merged

Add IntegerCombinableArbitrary for easy Integer customization#1192
seongahjo merged 4 commits intonaver:mainfrom
Denia-park:feat/add-IntegerCombinableArbitrary-for-easy-Integer-customization

Conversation

@Denia-park
Copy link
Contributor

@Denia-park Denia-park commented May 31, 2025

Summary

Add IntegerCombinableArbitrary to allow easy customization of randomly generated Integer values, similar to the existing StringCombinableArbitrary. (#1188)

(Optional): Description

providing below APIs to customize the randomly generated Integer

.positive() - only positive numbers
.negative() - only negative numbers
.withRange(min, max) - numbers within range
.even() - only even numbers
.odd() - only odd numbers

How Has This Been Tested?

  • integerCombinableArbitraryIsJqwik
  • integerCombinableArbitraryInjectNull
  • integerCombinableArbitraryFilter
  • integerCombinableArbitraryPositive
  • integerCombinableArbitraryNegative
  • integerCombinableArbitraryWithRange
  • integerCombinableArbitraryWithRangeAndFilter
  • integerCombinableArbitraryEven
  • integerCombinableArbitraryOdd
  • integerCombinableArbitraryLastOperationWinsWithPositiveAndNegative
  • integerCombinableArbitraryLastOperationWinsWithEvenAndOdd
  • integerCombinableArbitraryLastOperationWinsWithNegativeAndRange

Is the Document updated?

No. Once all code reviews are complete, would it be OK if I update and push the documentation just before merging this PR?
If that is not acceptable, I will update the documentation right away. Thank you for your understanding.

@Denia-park
Copy link
Contributor Author

@seongahjo

I have updated the code according to your review comments. Please review it again.

@Denia-park Denia-park requested a review from seongahjo June 2, 2025 15:13
Copy link
Contributor

@seongahjo seongahjo Jun 4, 2025

Choose a reason for hiding this comment

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

It should return Object, you do not have to override rawValue()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seongahjo

When I don’t override the rawValue() method in the IntegerCombinableArbitrary interface,
could you please clarify how the following classes are expected to implement rawValue() and what they should return?

  • IntegerCombinableArbitraryDelegator
  • JqwikIntegerCombinableArbitrary
  • KotestIntegerCombinableArbitrary

I’m not entirely sure what value rawValue() is supposed to provide, so I’m having trouble understanding how to implement it correctly.
Could you also explain if there’s something I’m missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Denia-park

I was mistaken, it should override as well. All the implementations should return an Integer.

The method rawValue() returns the actual, uncustomised instance of the type, ensuring compatibility with deserialization.

Jackson, for example, is the right case.

As each CombinableArbitrary is responsible for generating an arbitrary value, each one should deserialize the JSON.

In that case, We cannot support the jackson specialized annotations, @JsonTypeInfo, @JsonSubTypes.

The JacksonCombinableArbitrary class and the tests using JacksonSpecs would be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seongahjo

i update my code.

- change rawValue return type
@Seol-JY
Copy link
Contributor

Seol-JY commented Jun 5, 2025

I hope I'm not missing something, but I'd like to discuss what appears to be some inconsistencies in the current design. There might be design principles I'm not aware of, so please help me understand the intended behavior.

1. Should "Last Wins" apply to compatible conditions?

The current implementation uses "Last Wins" for all specialized methods, which works well for mutually exclusive conditions but seems questionable for compatible ones:

Mutually exclusive - Last Wins makes sense ✅

.positive().negative()  // Impossible combination → negative wins (reasonable)
.even().odd()          // Impossible combination → odd wins (reasonable)

Compatible conditions - Last Wins seems problematic ❌

.positive().odd()           // Possible combination (1,3,5,7...)
                           // Current: Only odd (includes negative odds like -1,-3...)
                           // Expected: Positive odd numbers only?

.even().withRange(10, 20)   // Possible combination (10,12,14,16,18,20)
                           // Current: Only range 10-20 (includes odds like 11,13...)
                           // Expected: Even numbers in range?

2. Inconsistent behavior with filters

There are multiple inconsistencies in how filters interact with specialized methods:

2.1. Filter gets overwritten

.filter(i -> i > 1000).positive()  // filter condition lost
// Result: All positive numbers (can include numbers ≤ 1000)

2.2. Order-dependent behavior

// Same logical intention, different results based on order
.positive().filter(i -> i < 100)   // Both applied: positive numbers < 100
.filter(i -> i > 0).positive()     // Only positive: filter condition lost

2.3. Filter-to-filter behavior

.filter(a).filter(b)  // Current: Cumulative (a AND b)
// Question: If "Last Wins" is the policy, shouldn't this be just filter(b)?

This creates unpredictable behavior where the same logical conditions produce different results based on method call order.

Questions for Discussion

  1. Design Intent: Is the "Last Wins" policy intended to apply even to compatible conditions like .positive().odd()?

  2. Consistency: Should there be a unified policy across all method combinations? Currently, we have different behaviors for different combinations.

  3. User Expectations: Would users expect .positive().odd() to produce positive odd numbers, or just odd numbers?

  4. Filter Semantics: Should filters follow the same "Last Wins" policy as specialized methods, or are they intended to always be cumulative?

I may be overthinking this, but the current mixed approach seems to create opportunities for confusion.
Thank you for your time reviewing this! 🙏

@Denia-park
Copy link
Contributor Author

Denia-park commented Jun 6, 2025

Hello @Seol-JY

Thank you very much for your detailed review on PR.

Below, I answer each question you raised in the comments.

1. “Should Last Wins apply to compatible conditions?”

@Override
public IntegerCombinableArbitrary positive() {
    return new JqwikIntegerCombinableArbitrary(
        Arbitraries.integers()..greaterOrEqual(1)
    );
}

All specialized methods — positive(), negative(), withRange(), even(), odd() — always start from a new Arbitraries.integers() stream. (code link)

Therefore:

withRange(-10, 10).positive()   // Result: (0‥10)
positive().withRange(-10, 10)   // Result: (-10‥10)  ← Last call takes precedence

In other words, the rule “the most recently called method overrides all previous conditions (Last Wins)” naturally holds.

Reason for adding the test case
Because each specialized method of IntegerCombinableArbitrary always creates a new stream, i added this test to clearly demonstrate that the operators invoked later take precedence.

2. “Inconsistent behavior with filters”

Call Order Works? Explanation
.positive().filter(i -> i < 100) ✅ Yes filter is applied last, layering an extra constraint on top of the existing positive().
.filter(i -> i > 0).positive() ❌ No positive() returns a new instance → the earlier filter condition is lost.

As shown above, filter accumulates constraints on the existing instance, whereas the specialized methods (positive(), negative() ...) return a new instance, causing the behavior to vary depending on call order.

3. "Questions for Discussion"

@seongahjo
i believe this part requires further discussion.

How would you like it to be structured?

Personally, I think they should be usable in combination.
(For example: .positive().odd() // Possible combination (1,3,5,7...))


I will apply any feedback promptly. Thank you!

@YongGoose
Copy link
Contributor

Compatible conditions - Last Wins seems problematic ❌

.positive().odd()           // Possible combination (1,3,5,7...)
                           // Current: Only odd (includes negative odds like -1,-3...)
                           // Expected: Positive odd numbers only?

.even().withRange(10, 20)   // Possible combination (10,12,14,16,18,20)
                           // Current: Only range 10-20 (includes odds like 11,13...)
                           // Expected: Even numbers in range?

That makes sense.

If we want to apply all selected operations instead of relying on a last wins strategy, it might be possible to store the integerArbitrary instances in order within JqwikIntegerCombinableArbitrary.

However, I suspect there could be conflicts in cases where conflicting operations like .positive() and .negative() are applied together.

This is just my assumption, as I haven’t deeply looked into the internal implementation yet.

@Denia-park
Copy link
Contributor Author

@YongGoose

If we want to apply all selected operations instead of relying on a last wins strategy, it might be possible to store the integerArbitrary instances in order within JqwikIntegerCombinableArbitrary.

However, I suspect there could be conflicts in cases where conflicting operations like .positive() and .negative() are applied together.

You’re absolutely right.

At first, I tried to implement them so they would all work together,
but I got confused by directly opposing methods such as odd()even() or positive()negative().

So for now, I’ve decided to implement each method as a standalone feature. 😂

Copy link
Contributor

@seongahjo seongahjo left a comment

Choose a reason for hiding this comment

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

Well done! Thank you for your contribution.

@seongahjo seongahjo merged commit 3c39f67 into naver:main Jun 10, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments