Fix 2 bugs (NdefRecord URI, OpenAiClient error swallowing) + add 250 unit tests across 16 previously-0%-coverage classes#5233
Merged
Conversation
… add NFC/grpc unit tests NdefRecord.getUriPayload() returned null for any TNF_ABSOLUTE_URI record with an empty payload, because the leading `payload.length < 1` guard short-circuited before the absolute-URI branch ran. An absolute-URI record carries its URI in the type field and commonly has no payload, so this was always broken for that record type. Move the TNF_ABSOLUTE_URI branch ahead of the payload-length guard (the guard is only needed by the well-known URI form, whose prefix code lives in payload[0]). Also adds unit-test coverage for four previously-uncovered pure-logic classes (all were at 0% line coverage): - com.codename1.nfc.NdefRecord (25 tests) - com.codename1.nfc.NdefMessage (17 tests) - com.codename1.io.grpc.ProtoWriter (17 tests) - com.codename1.io.grpc.ProtoReader (24 tests) Tests drive the real public APIs only -- no mocks, reflection, or internal-state exposure -- and exercise round-trips, canonical protobuf byte sequences, defensive-copy guarantees, proto3 default omission, and malformed-input handling. Includes a regression test for the URI fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 128 screenshots: 128 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Contributor
Cloudflare Preview
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Broadens coverage beyond pure-logic classes: OidcClient was 1050 missed instructions at 0% coverage because its main paths (discovery, the token-endpoint POST, revocation) are network-bound. These are now driven against the real mock platform -- UITestBase for an initialized Display and TestCodenameOneImplementation.addNetworkMockResponse for HTTP -- with no Mockito and no reflection. 21 tests covering: - discover(): success, trailing-slash tolerance, empty-document failure, null-issuer guard - refresh()/token-endpoint POST: token exchange + persistence, refresh-token fallback, OAuth error-field surfacing, guards - revoke(): null token, no-endpoint, and success responses - refreshIfExpired()/loadStoredTokens() with an in-memory TokenStore - configuration guards and chainable setters Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
shai-almog
changed the title
Collaborator
Author
|
Compared 128 screenshots: 128 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 128 screenshots: 128 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
… UI) Extends the coverage push to a broad set of previously-untested classes, 132 new tests total, all driving the real public APIs against the in-tree mock platform (UITestBase + TestCodenameOneImplementation). No Mockito, no reflection, no visibility widening. Pure logic: - com.codename1.io.JSONWriter (14) serializer, escaping, builders - com.codename1.binding.Binders (14) registry, bind, update regions - com.codename1.gpu.GltfLoader (11) glTF/GLB parse, normals, errors Network / HTTP (mocked via addNetworkMockResponse): - com.codename1.io.grpc.GrpcWeb (20) framing, decode, unary call - com.codename1.ai.OpenAiClient (12) chat/embeddings parse, config - com.codename1.io.graphql.GraphQLSubscription (7) guards, payload, endpoint Social / UI: - com.codename1.social.Auth0Connect (15) passkey challenge/register flows - com.codename1.social.AppleSignIn (18) native packed-result parse, web - com.codename1.components.OtpField (21) value, auto-advance, paste Paths requiring a live browser/WebSocket/native sheet are driven up to the deterministic boundary (guards, builders, request/response parsing, error mapping) and the unreachable remainder noted, rather than faked brittlely. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
shai-almog
changed the title
…ting tests
Two fixes surfaced by the coverage work, each with tests that fail without
the fix and pass with it.
OpenAiClient.chat/embed: because configureRequest sets
readResponseForErrors=true and the client suppresses handleErrorResponseCode,
the framework routes HTTP 4xx/5xx through postResponse() (never
handleException). The old postResponse parsed the error body with the lenient
non-streaming parser, turning an {"error":...} envelope into an empty,
successful ChatResponse -- so the rich OpenAiSseDecoder.mapErrorStatic
status->ErrorType classification never fired. postResponse now inspects the
status code (and a 200 carrying an {"error":...} body) and surfaces the typed
LlmException via mapErrorStatic. New tests assert AUTH/RATE_LIMIT/
INVALID_REQUEST/SERVER/MODEL_OVERLOADED/CONTEXT_LENGTH mapping plus the
200-with-error-envelope and embeddings error paths.
Base64.allocByteMaybeSimd: called Simd.get() ->
Display.getInstance().getSimd() unguarded, raising a raw NullPointerException
when Base64 is used before Display is initialized (early boot, background
threads, tests) for any output >= 16 bytes. Now wrapped in try/catch(Throwable)
with a plain-array fallback, mirroring VertexBuffer.allocAligned. New
Base64Test (plain, no Display) covers encode/decode/url-safe round-trips over
the >=16-byte SIMD-allocation path; reverting the guard makes 6 of its 7
tests NPE.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
shai-almog
changed the title
Pre-init usage of Base64 (before Display is initialized) is not a supported scenario, so the allocByteMaybeSimd try/catch guard was solving a non-problem (a test-harness artifact, not a real-world code path). Reverts that source change -- Base64.java is now byte-identical to master. Base64Test is retained for the genuine coverage value but now extends UITestBase so Display is initialized exactly as in real usage; the pre-init framing and reverting-the-guard commentary are removed. Still covers encode/encodeNoNewline/encodeUrlSafe/decode round-trips across all length-mod-3 cases and the >= 16-byte SIMD-allocation path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
shai-almog
changed the title
Collaborator
Author
|
Compared 124 screenshots: 124 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
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
1 participant
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.
Summary
Two correctness fixes plus 250 unit tests covering 16 classes that were at 0% line coverage, spanning pure-logic, network/HTTP, and UI. Everything drives the real public APIs against the in-tree mock platform (
UITestBase+TestCodenameOneImplementation). No Mockito, no reflection, no visibility widening.Bug fixes (each with a test that fails without the fix)
NdefRecord.getUriPayload()returnednullforTNF_ABSOLUTE_URIrecords with an empty payload (the URI lives in the type field; thepayload.length < 1guard short-circuited first). Moved the absolute-URI branch ahead of the guard.OpenAiClient.chat/embedswallowed HTTP errors. WithreadResponseForErrors=trueandhandleErrorResponseCodesuppressed, the framework routes 4xx/5xx throughpostResponse(), which parsed the{"error":...}envelope as an empty successfulChatResponse— so the typedmapErrorStaticclassification never fired.postResponsenow inspects the status code (and a 200 carrying an error envelope) and surfaces the typedLlmException. Tests assert AUTH / RATE_LIMIT / INVALID_REQUEST / SERVER / MODEL_OVERLOADED / CONTEXT_LENGTH mapping + the 200-with-error and embeddings paths.New coverage
nfc.NdefRecordnfc.NdefMessageio.grpc.ProtoWriterio.grpc.ProtoReaderio.grpc.GrpcWebio.JSONWriterbinding.Bindersgpu.GltfLoaderutil.Base64io.oidc.OidcClientai.OpenAiClientio.graphql.GraphQLSubscriptionsocial.Auth0Connectsocial.AppleSignIncomponents.OtpFieldPaths that genuinely require a live browser, WebSocket, or native sheet are driven up to the deterministic boundary (guards, builders, request/response parsing, error mapping) with the unreachable remainder noted, rather than faked brittlely. Source changes are limited to the two fixes above.
Testing
All 250 tests pass on JBR-17:
🤖 Generated with Claude Code