Unit testing: ConstraintVerifier must support updating all shadow variables too

[ge0ffrey]

When using shadow variables, and writing constraint tests, it's a huge pain to set all shadow variables correctly in the items given to given().

Allow keeping all shadow variables null and let the ConstraintVerifier do that for you.

This especially create a lot of DRY code: the VariableListener methods requires a ScoreDirector, so all that code needs to be rewritten without the ScoreDirector calls (think before/after etc), which is a maintenance problem.

Proposal A)

Figure it out alongside shadow streams and how to unit test those

Proposal B)

.given(...)
.triggerAllShadowVariables()
.penalizesBy(...)

[triceo]

This is not currently possible.
Variable listeners (which update shadow variables) require a working solution as their input. With .given(facts), no such solution exists and therefore these listeners can not be triggered.

[greyhairredbear]

The current state surely requires some extra work for tests - we've worked around this by extracting the Timefold-independent logic so it can be easily reused. IMO, that's not too bad, but having something like this still would be nice.

Without knowing the internals here, would it be possible to have an overload of given that takes a planning solution as input?

[triceo]

@greyhairredbear That is currently the only option I can see.

[Christopher-Chianelli]

FWIW, there is already an overload of given that takes the planning solution: givenSolution.

[triceo]

@Christopher-Chianelli True, but that is used for a different purpose. That is used to test an entire solution, not individual constraints.

[greyhairredbear]

@Christopher-Chianelli Thanks for pointing that out! Now you've mentioned it, I feel like I've come across this in the past :). So triggerAllShadowVariables() would only work for the result of that

@triceo Out of curiosity: What is the difference in purpose here? Couldn't one test a single constraint using that?

Again, without knowing the internals and just throwing an idea out there: Could it make sense to try and create a test instance of a planning solution and fill that with the provided facts when triggerAllShadowVariables() is called or would that be too complicated to be sensible with all the potential code users could write?

[Christopher-Chianelli]

@Christopher-Chianelli True, but that is used for a different purpose. That is used to test an entire solution, not individual constraints.

Actually both now! SingleConstraintVerification and MultiConstraintVerification both have givenSolution; to test all constraints, use verifyThat() (no args), to test a single constraint use verifyThat(MethodReference). (And if SingleConstraintVerification.givenSolution() test all constraints despite being a single constraint verification, I argue that a bug).

[triceo]

I have to backtrack, @Christopher-Chianelli is correct - my memory did me wrong.
Both the single-constraint and multi-constraint tools actually support the givenSolution(...) method, so for this one, we could potentially trigger shadow variables.

But I wonder... if you have to fill out an entire solution just to have the shadow vars triggered automatically, isn't that just replacing one boilerplate (filling in shadows) with another (creating an entire solution)?

[greyhairredbear]

But I wonder... if you have to fill out an entire solution just to have the shadow vars triggered automatically, isn't that just replacing one boilerplate (filling in shadows) with another (creating an entire solution)?

I guess to create a solution you only need to know your own API of how to create a solution and no Timefold-specific implementation details like setting shadow variables correctly. Also, I guess impossible to forget calling a constructor for an object you need while it's quite possible to forget to update your shadow variables.

Mentioning this, it also occurs to me that it'd be possible for a user to forget calling triggerAllShadowVariables in the call chain. So I suppose, from my personal perspective as a user, the most ergonomic usage of givenSolution would be that the shadow variables are updated during givenSolution. But I also don't know if that could cause issues with implementation or break existing code using that.