Skip to content

Fix NPE and inverted numeric guidance - #24

Closed
mprimi wants to merge 7 commits into
mainfrom
mprimi/fix-npe-and-more
Closed

Fix NPE and inverted numeric guidance#24
mprimi wants to merge 7 commits into
mainfrom
mprimi/fix-npe-and-more

Conversation

@mprimi

@mprimi mprimi commented Jul 21, 2026

Copy link
Copy Markdown

Note for reviewers: this PR consists of 4 atomic commits, and may be easier to review them individually, in order.

  1. Gitignore FFI generated files
  2. Add tests, some document existing behavior, like NPE on null details. Others fail, I think correctly so.
  3. Fix the numeric guidance, turns corresponding tests green
  4. Fix NPE and do a little refactoring around it, all tests are green

Summary

Fixes two bugs in the public Assert numeric/comparison guidance helpers, adds test coverage for the Assert public API, and stops tracking generated FFI files.

The bugs were found by adding tests for the Assert public interface and cross-checked against the reference Go and Rust SDKs.

Bug 1 — numeric guidance maximize was inverted for sometimes* helpers

The four sometimes* numeric comparison helpers emitted the same maximize direction as their always* counterparts. Per the reference SDKs, for a given operator the sometimes variant must use the opposite direction — so the platform was being steered the wrong way for these assertions.

Method Before After Go SDK Rust SDK
alwaysGreaterThan / …OrEqualTo false false false false
alwaysLessThan / …OrEqualTo true true true true
sometimesGreaterThan / …OrEqualTo false true true true
sometimesLessThan / …OrEqualTo true false false false

References:

Bug 2 — comparison helpers mutated the caller's details and threw on null

The numeric and boolean-map helpers merged guidance into details via details.setAll(guidanceData), which (a) mutated the caller-supplied ObjectNode in place (clobbering same-named keys, leaking across reused instances) and (b) threw NullPointerException on a null details argument — unlike the plain always/sometimes/etc. methods, which tolerate null.

Fix: a single mergeGuidance(details, guidanceData) helper that merges into a null-safe deepCopy(), 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 one static final MAPPER.

Tests

Adds coverage for the Assert public API (previously smoke-tested only). A test-only CaptureSupport reflectively installs an in-memory output handler (output otherwise goes to a NoOpHandler in 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/rawGuidance passthrough, details non-mutation, null handling, and concurrent dedup.

Misc.

Adds generated FFI files (ffi/swig, generated FfiWrapperJNI.java, generated resources) to .gitignore.

mprimi added 7 commits July 21, 2026 13:40
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
@mprimi mprimi closed this Jul 30, 2026
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.

1 participant