Fix NPE and inverted numeric guidance - #24
Closed
mprimi wants to merge 7 commits into
Closed
Conversation
Only a small surface of the SDK was tested, and those tests were superficial, not verifying actual state. This change covers the SDK surface more thoroughly by verifying the object passed down to the FFI layer. This is done by adding a seam (via reflection) that captures said objects.
Maximize for `sometimes{Less,Greater}Than*` was not consistent with Rust and Go implementations. Bring it in line.
Address the following: - *LessThan and *GreaterThan assertions would throw NPE if the user passed a `null` detail object - Guidance data would modify user's details object in-place, which could lead to undesireable and unexpected side-effects - A new ObjectMapper would be created in each assertion This change switches to a single static object mapper. The user's details object is copied rather than being modified in place Null details are handled without raising NPE.
- Shallow-copy details when is non-null - Avoid creating an empty node when details is null
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note for reviewers: this PR consists of 4 atomic commits, and may be easier to review them individually, in order.
nulldetails. Others fail, I think correctly so.Summary
Fixes two bugs in the public
Assertnumeric/comparison guidance helpers, adds test coverage for theAssertpublic API, and stops tracking generated FFI files.The bugs were found by adding tests for the
Assertpublic interface and cross-checked against the reference Go and Rust SDKs.Bug 1 — numeric guidance
maximizewas inverted forsometimes*helpersThe four
sometimes*numeric comparison helpers emitted the samemaximizedirection as theiralways*counterparts. Per the reference SDKs, for a given operator thesometimesvariant must use the opposite direction — so the platform was being steered the wrong way for these assertions.alwaysGreaterThan/…OrEqualToalwaysLessThan/…OrEqualTosometimesGreaterThan/…OrEqualTosometimesLessThan/…OrEqualToReferences:
assert/rich_assert.go(guidanceFnMinimize→maximize=false,guidanceFnMaximize→maximize=true) andassert/numeric_guidance.golib/src/assert/macros.rsandlib/src/assert/guidance.rsBug 2 — comparison helpers mutated the caller's
detailsand threw onnullThe numeric and boolean-map helpers merged guidance into details via
details.setAll(guidanceData), which (a) mutated the caller-suppliedObjectNodein place (clobbering same-named keys, leaking across reused instances) and (b) threwNullPointerExceptionon anulldetails argument — unlike the plainalways/sometimes/etc. methods, which toleratenull.Fix: a single
mergeGuidance(details, guidanceData)helper that merges into a null-safedeepCopy(), leaving the caller's object untouched (at the cost of one additional copy).Emitted-details content is unchanged (guidance keys still win on collision).
Also hoisted the per-call
new ObjectMapper()allocations to onestatic final MAPPER.Tests
Adds coverage for the
Assertpublic API (previously smoke-tested only). A test-onlyCaptureSupportreflectively installs an in-memory output handler (output otherwise goes to aNoOpHandlerin unit tests) and resets the static dedup trackers between tests — no production code was changed to enable testing.Covers: the assert-type/display-type/condition/hit/must_hit matrix, the first-pass/first-fail dedup contract, numeric & boolean guidance (direction, strictly-better gating, NaN carve-out),
rawAssert/rawGuidancepassthrough,detailsnon-mutation, null handling, and concurrent dedup.Misc.
Adds generated FFI files (
ffi/swig, generatedFfiWrapperJNI.java, generated resources) to.gitignore.