refactor(tests): generalise scalar matrix harness off integer-inherent consts#258
Conversation
…t consts Make the SQLx scalar-matrix harness type-agnostic ahead of the first non-integer scalar, without adding one. Three integer assumptions are lifted: - `ScalarType::FIXTURE_VALUES` (a `const`) becomes `fn fixture_values()`, so a scalar whose values can't be const-constructed can return a borrow of a lazily-built `Vec` instead. Integer impls still hand back their `eql_scalars::<T>_VALUES` const. - New `min_pivot()` / `max_pivot()` trait methods replace the matrix's direct `<T>::MIN` / `<T>::MAX` pivot references, so a scalar without an inherent `::MIN`/`::MAX` const can supply an explicit sentinel. - The ORDER BY arms build their `WHERE` clause from `to_sql_literal(zero)` instead of a hardcoded `> 0`, so a non-integer plaintext column typechecks. Behaviour-preserving for the existing `int4` / `int2` types: the integer `min_pivot`/`max_pivot` resolve to `Self::MIN`/`Self::MAX`, `to_sql_literal(0)` renders `0`, and the generated test names are unchanged. The int4 cargo-expand snapshot is regenerated to track the method-based bodies.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/eql-tests-macros/src/lib.rs`:
- Around line 268-273: The test currently only checks for the substrings
"MIN"/"MAX" which may appear in doc comments; update the assertions to verify
the actual emitted pivot function bodies for min_pivot and max_pivot instead of
loose substrings. Specifically, in the test that inspects out, replace the
out.contains("MIN") and out.contains("MAX") checks with assertions that match a
more precise snippet from the generated functions (e.g., the signature and the
body that returns the inherent bounds for min_pivot and max_pivot) so you
validate the functions min_pivot and max_pivot actually return the expected
bounds; keep the existing checks for "fn fixture_values" and the presence of the
pivot function names.
In `@tests/sqlx/src/scalar_domains.rs`:
- Around line 37-46: The contract for fixture_values() is ambiguous about
ordering but callers (e.g., ordered_numeric_matrix! driven code) depend on a
stable, insertion-consistent ordering and perform positional indexing; update
the trait/docs and implementations so fixture_values() guarantees a
deterministic order matching the fixture insertion order (or explicitly sorted
by a defined key) — modify the fixture_values() implementations for non-integer
scalars to build and return a statically stable slice (e.g., by producing a Vec
once in a deterministic order and leaking or storing it as &'static [Self]) and
update the trait comment for fixture_values() to state that callers may rely on
its ordering (or alternatively have callers sort before positional access if
ordering cannot be guaranteed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39aa6b97-4a18-4a3e-8315-f083e656290e
📒 Files selected for processing (5)
crates/eql-tests-macros/src/lib.rstests/sqlx/snapshots/int4_expanded.rstests/sqlx/src/matrix.rstests/sqlx/src/scalar_domains.rstests/sqlx/tests/encrypted_domain/family/mutations.rs
- scalar_domains.rs: document fixture_values() stable-order contract (callers compare element-wise and index positionally without sorting) - eql-tests-macros: assert emitted min_pivot/max_pivot bodies instead of loose MIN/MAX substrings that also match the doc comment 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixes Applied SuccessfullyFixed 2 file(s) based on 2 CodeRabbit feedback item(s). Files modified:
Commit: The latest autofix changes are on the |
Summary
PR 1 of 2 (stacked). Makes the SQLx scalar-matrix harness type-agnostic ahead of the first non-integer scalar, without adding one — so the date type (#256, stacked on this branch) becomes a clean catalog-row-plus-wiring change. Behaviour-preserving for the existing
int4/int2types.What changed
Three integer assumptions in the harness are lifted:
ScalarType::FIXTURE_VALUES(aconst) →fn fixture_values(). A method, so a scalar whose values can't be const-constructed (e.g. a chrono type) can return a borrow of a lazily-builtVec. Integer impls still hand back theireql_scalars::<T>_VALUESconst.min_pivot()/max_pivot()trait methods replace the matrix's direct<T>::MIN/<T>::MAXpivot references, so a scalar without an inherent::MIN/::MAXconst can supply an explicit sentinel.ORDER BYarms build theirWHEREclause fromto_sql_literal(zero)instead of a hardcoded> 0, so a non-integer plaintext column typechecks.The macro emits the new method shape for integer impls; the int4 cargo-expand snapshot is regenerated to track the method-based bodies.
Why behaviour-preserving
For
int4/int2:min_pivot/max_pivotresolve toSelf::MIN/Self::MAX,to_sql_literal(0)renders0(soWHERE plaintext > 0is byte-identical), and the generated test names are unchanged — thematrix_tests.txtinventory snapshot does not move.Verification
cargo test -p eql-tests-macros— greencargo check -p eql_tests --tests— clean (the const→fn rename compiles across all ~20 call sites)cargo fmt --check,clippy -D warnings— cleanFull PG matrix runs in CI. PR 2 (
eql_v3.date) stacks on this branch.🤖 Generated with Claude Code
Summary by CodeRabbit