test(query): extract ResultByteEstimator and unit test every type arm#115
Merged
Conversation
The byte estimator was a private static method on QueryExecutor reached
only via integration tests that exercise three of its seventeen switch
arms — every other arm (Guid, decimal, DateTime, DateTimeOffset,
TimeSpan, byte[], char, ushort, uint, ulong, float, double, bool, the
unknown-type fallback) had no coverage and accounted for ~1pp of the
recent codecov drop.
Move the method into a new internal static class
ResultByteEstimator (SRP, one reason to change, and reachable from unit
tests). QueryExecutor now calls
ResultByteEstimator.EstimateBytes(stored) — identical behavior. Add
InternalsVisibleTo("Equibles.AgentQL.UnitTests") on the EFCore csproj
so future PRs can unit-test the rest of the internal surface
(ReadOnlyStatementValidator, the per-provider enforcers) without
making them public.
Unit tests cover all primitive arms with their fixed widths, ASCII and
multi-byte UTF-8 strings, empty and populated byte arrays, the
DateTime / DateTimeOffset / TimeSpan trio, and both fallback shapes:
unknown type stringified via ToString and the rare ToString-returns-
null case (custom type) that must not throw.
Integration MaxBytes tests still pass — no behavior change.
This was referenced May 27, 2026
daniel3303
added a commit
that referenced
this pull request
May 27, 2026
#117) SQL Server has no in-band session-level read-only enforcement (see PR #108), so this enforcer is intentionally a no-op that only surfaces a Limitation message. With no integration test fixture for SQL Server in the repo, every line of the enforcer was unreachable — that accounted for ~2.4pp of the recent codecov drop. Unit tests pin the no-op contract: Limitation names SQL Server and recommends a concrete remediation (db_datareader, ApplicationIntent, or SELECT-only), Apply / Reset send no commands on the connection (verified via NSubstitute DidNotReceive on CreateCommand), IsReadOnlyViolation returns false for every shape of exception including a DbException subclass that mimics the PostgreSQL read-only SqlState. Instance is a singleton. No source changes — the enforcer is reached via the InternalsVisibleTo added in PR #115.
daniel3303
added a commit
that referenced
this pull request
May 27, 2026
MySQL has real session-level read-only enforcement (PR #109), but the repo has no MySQL Testcontainer so every line of the enforcer was unreachable — that accounted for ~1.8pp of the recent codecov drop. Two halves of the contract get pinned: Apply / Reset send the expected SET SESSION TRANSACTION READ {ONLY,WRITE} commands. A minimal RecordingDbConnection / RecordingDb- Command pair captures every command text that the enforcer's Execute helper sends — no real provider needed. IsReadOnlyViolation recognises MySQL's ER_CANT_EXECUTE_IN_READ_ONLY_TRANSACTION via reflection on the runtime exception's Number property. The fake mimics MySqlException by exposing a public int Number, so the reflection path executes exactly as in production. Tests cover: Number == 1792 → true, Number in {1, 1062, 2006} → false, DbException without a Number property → false (the property lookup returns null and the predicate falls through), non- DbException → false. No source changes — the enforcer is reached via the InternalsVisibleTo added in PR #115.
daniel3303
added a commit
that referenced
this pull request
May 27, 2026
…119) Oracle's SET TRANSACTION READ ONLY is per-transaction and an embedded COMMIT escapes it (see PR #110), so the enforcer is intentionally a no-op that only exposes a Limitation message — same shape as the SQL Server enforcer. With no Oracle Testcontainer in the repo every line of the enforcer was unreachable, accounting for ~1.6pp of the recent codecov drop. Pins the same contract as the SQL Server tests with one Oracle-specific twist: the Limitation message must explain why per-transaction read- only is the wrong layer of defense (the SQL Server message has no equivalent caveat to assert). 9 cases: Limitation non-null and mentions Oracle, explains the per-transaction gap (contains "transaction" / "COMMIT" / "session- level"), recommends a concrete remediation (SELECT, READ ONLY, or ALTER DATABASE), Apply and Reset send no commands (NSubstitute DidNotReceive on CreateCommand), IsReadOnlyViolation returns false for every shape of exception, Instance is a singleton. Closes the 5-PR coverage-recovery series (#115, #116, #117, #118, this PR). Combined recovery target ~7pp. No source changes — the enforcer is reached via the InternalsVisibleTo added in PR #115.
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.
Summary
Recovers ~1 pp of the recent coverage drop by exercising every branch of the byte estimator that drives
MaxBytes. The method was previously aprivate staticonQueryExecutorreached only via integration tests that hit three of its seventeen switch arms.Extracts the method to a new internal class
ResultByteEstimator(SRP — one reason to change, reachable from unit tests).QueryExecutornow callsResultByteEstimator.EstimateBytes(stored). Identical behavior — the integrationMaxBytestests still pass.Code changes
src/Equibles.AgentQL.EntityFrameworkCore/Query/ResultByteEstimator.cs(new) — moved verbatim fromQueryExecutor.EstimateValueBytes.src/Equibles.AgentQL.EntityFrameworkCore/Query/QueryExecutor.cs— call-site swap + removal of the inline method.src/Equibles.AgentQL.EntityFrameworkCore/Equibles.AgentQL.EntityFrameworkCore.csproj—InternalsVisibleTo("Equibles.AgentQL.UnitTests")so the unit-test project can reach internals. Lets future PRs coverReadOnlyStatementValidatorand the per-provider enforcers without making thempublic.tests/Equibles.AgentQL.UnitTests/Query/ResultByteEstimatorTests.cs(new) — 25 cases: every primitive arm at its fixed width, ASCII + multi-byte UTF-8 strings, empty + populated byte arrays,DateTime/DateTimeOffset/TimeSpan, the unknown-type stringified fallback (Uri), and the rare ToString-returns-null custom type that must not throw.25 / 25 unit tests pass. 3 / 3 integration
MaxBytestests still pass.First in a 5-PR series
Quick-win unit tests to recover the ~9 pp coverage drop from the per-provider enforcement work (#106-#114). Subsequent PRs cover:
ReadOnlyStatementValidator— parser edge cases not hit by integration testsSqlServerReadOnlySessionEnforcer— limitation message + no-op Apply/ResetMySqlReadOnlySessionEnforcer— reflection-basedIsReadOnlyViolationOracleReadOnlySessionEnforcer— same shape as SQL Server