ConstraintVerifier rewards and penalties inverted when using impact()

[matthias-roussel-soyhuce]

Describe the bug I have a constraint that uses impact() to either penalize or reward the score based on the planning entities. Since the ConstraintVerifier does not have a specific method to test these constraints, I tried to use rewardsWith instead (in a test cases that expects a reward), as recommended in this StackOverflow answer back on Optaplanner.

However that results in a broken expectation (see below), but works when using penalizesBy() instead.

Expected behavior I expected the test assertion using rewardsWith() to pass, and the assertion using penalizesBy() to fail.

Actual behavior The test assertion using rewardsWith() fails with the following exception:

java.lang.AssertionError: Broken expectation.
        Constraint: rules/preference is respected (reward)
   Expected reward: 20 (class java.lang.Integer)
     Actual impact: 20 (class java.lang.Long)

[...]

and the assertion using penalizesBy() passes.

However, a multi constraint assertion with scores() passes with a score of the expected sign, i.e. negative for a penalty and positive for a reward.

To Reproduce Create a constraint with impact() instead of reward() and a positive match weight, and test it with a ConstraintVerifier using the rewardsWith() assertion with the expected weight.

Environment

Timefold Solver Version or Git ref: 1.4.0

Output of java -version: 17.0.6-amzn

Output of uname -a or ver: Linux <computer_name> 5.15.0-89-generic #99~20.04.1-Ubuntu SMP Thu Nov 2 15:16:47 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Additional information

The same problem was described and qualified as a bug in the aforementioned StackOverflow thread (on Optaplanner 8.25.0.Final).

[triceo]

@matthias-roussel-soyhuce Thank you for reporting!

You are absolutely correct that this is a known bug. Unfortunately, we are stuck with it - fixing this would break people's tests, and therefore we can not do it on 1.x. But for 2.x, whenever that is (no plans exists), this is a must-fix.

[ge0ffrey]

Given that this does seem to impact (pun intended) multiple users, shall we rediscuss the deprecation approach after the holidays? Proposal A) Replace impact() with foo() which works correctly. Deprecate impact().