test: Add regression test for issue #2172 (ValueRangeCache equals vs identity)#2173
Conversation
…equals vs identity) This test verifies that ValueRangeCache uses .equals() for containment checks, not object identity (==). This is critical for JPA entities that implement equals() by ID field. The bug manifested when: - Using filtering value range providers - With JPA entities that have equals() by ID - Entities persisted in one transaction and loaded in another (different instances) Before PR TimefoldAI#2111, ValueRangeCache.Builder.FOR_USER_VALUES used IdentityHashSet (==) which rejected equal-but-not-identical values, causing IllegalStateException. Related: TimefoldAI#2172, TimefoldAI#2111
|
Hi there. As you can guess this has been AI-supported. I've stumbled on the bug reported, using the released version. It throws a proper exception. We have put together a fix and a corresponding unit test to show the problem in action. We've noticed that the fix is already in place, despite being tracked more generically as a refactor. All good, we will wait for the next release. But we still see value in adding the original unit test we had prepared contextually to the fix, to avoid eventual future regressions. HTH |
|
Hello, and thanks for the PR - as you noticed, the fix is already available. As to the test - this is too low-level to be tested by running the entire solver. Also, adding 200 lines of code for this simple thing seems like an overkill. Furthermore, all sufficient test coverage is already present, as you can verify in the commit. Therefore I will be closing this PR as not necessary. |
Summary
Add regression test for issue #2172 - ValueRangeCache should use
.equals()not==for JPA entities.Background
Issue #2172 reported that
ValueRangeCache.Builder.FOR_USER_VALUESusedIdentityHashSet(==) instead ofHashSet(.equals()) for containment checks. This causedIllegalStateExceptionwhen:equals()by IDFix Status
This was already fixed in PR #2111 ("refactor: value range cleanup"), but the fix wasn't explicitly tracked as a bug fix for this scenario. The commit message says:
This PR
Adds a regression test that would have caught this bug in 1.31.0. The test:
.equals()by ID (different instances, same ID)IllegalStateExceptionBenefits
Related