Repository navigation
fix(testsuite): resolve every gated protocol reference and build expressions from objects - #23
Merged
vistart merged 3 commits intoOct 8, 2026
Conversation
Core's expression classes now take the object they act on rather than a parsed string, so the fixtures construct them from objects and several need explicit constructors where the introspective guess cannot satisfy the shape. The introspective constructor read a parameter's annotation and computed its text as the alias' own __name__. If that text mentioned a relation word -- Table, View, Sequence and the rest -- it handed the parameter a catalogue object. List[str] was safe because the container branch answered first, but Sequence[X] was not: get_origin yields collections.abc.Sequence, the test for list/tuple/set misses, and what is left is the literal string "Sequence". So a parameter asking for a list of columns received a Sequence object and the class could not be built. _empty_container_for now answers for any parameterised alias before the relation-name test, matching the collections.abc ABCs rather than the concrete types an annotation happens to name, so Sequence[str], FrozenSet[str] and Mapping[str, int] are recognised as what they are. A bare class annotation returns None, so the relation test still sees the parameters that really do name an object. Measured by walking every class's __init__ in the core expression package and re-deriving what the old code decided: 58 parameters reached the relation test, 51 of them annotated with a single class, and 7 wrongly annotated with a parameterised alias -- columns, attributes, actions, parts, items, keys and passing, all Sequence[...]. Against MariaDB the seven classes went from none constructible to four. The remaining three are a different defect, keyword-only parameters hidden behind defaulted positionals, and stay unbuilt.
… tests run
`requires_protocol` resolves a string with `rsplit('.', 1)` before importing it. Two
forms in this suite cannot resolve, and both make the test SKIP rather than fail:
* the bare name `"JoinSupport"` and `"SchemaSupport"` -- no dot, so `rsplit` raises
ValueError and the resolver skips;
* the dotted path `...dialect.protocols.IndexSupport`, which stopped resolving when
the core's single `protocols.py` became a `protocols/` package and the class was
renamed.
The effect was invisible: the full-text-search compatibility test skipped 11 of 11
parametrisations and every join test skipped, on every backend, with no failure to
show for it. Verified after the fix -- full-text now runs 11 passed, and the join
bridge files run 209 passed, with nothing newly failing.
|
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.
Description
Core's expression / dialect separation landed as
rhosocial/python-activerecord#125 on
release/v1.0.0.dev30. This suite had to follow it, and two of the adaptations it needed failedsilently — in both cases the failure mode was a skip, not an error, so nothing in CI ever went
red and the coverage simply stopped existing.
Adds # (issue)
1. Protocol references that could not resolve, so the gated tests skipped
requires_protocoltakes a dotted string, splits on the last dot and imports the result. Two formsin this suite could not resolve:
"JoinSupport"and"SchemaSupport"— no dot at all, sorsplitraisesValueErrorand the resolver skips;...dialect.protocols.IndexSupport, which stopped resolving when core'ssingle
protocols.pybecame aprotocols/package and the class was renamed.Measured effect before the fix: the full-text-search compatibility test skipped 11 of 11
parametrisations, and every join test skipped, on every backend, with no failure to show for it.
Fixed at the call sites, not in the resolver: 44 protocol references — 41 inside
requires_protocol(...)across 15 files (bareJoinSupport×28, bareSchemaSupport×13) plus3 occurrences of
IndexSupportrenamed toFulltextIndexSupport, one of which is the docstringexample in
utils/common.py. The resolver itself is unchanged; it was doing exactly what itdocuments.
2. A container annotation that reads as a relation name, so the class could not be built
Core's expression classes now take the object they act on rather than a parsed string, so the
fixtures construct them from objects. Where the shape allows, the fixture reads a parameter's
annotation and computes its text as the alias' own
__name__; if that text mentions a relation word—
Table,View,Sequenceand the rest — it hands the parameter a catalogue object.List[str]was safe, because the container branch answered first.Sequence[X]was not:typing.get_originyieldscollections.abc.Sequence, the test forlist/tuple/setmisses it,and what is left is the literal string
"Sequence". A parameter asking for a list of columnstherefore received a
Sequenceobject and the class could not be built at all._empty_container_fornow answers for any parameterised alias before the relation-name test,matching the
collections.abcABCs rather than the concrete types an annotation happens to name —so
Sequence[str],FrozenSet[str]andMapping[str, int]are recognised as what they are. Abare class annotation still returns
None, so the relation test keeps seeing the parameters thatreally do name an object.
Two further corrections in the same helper:
TableConstraintandAddTableConstraintare constraint actions, not relations, and a substring test hands them a
Table.SchemaObjectis the base every catalogue object shares, so an annotation naming it wants anobject too.
INSERT's data source is an expression now, so it needs a real one — the generic placeholdercannot invent the rows a
VALUESlist is made of.Where the introspective guess still cannot satisfy the shape, 26 explicit per-class constructors
were added (
_create_sequence_expr,_alter_type_expr,_drop_domain_expr,_create_property_graph_expr, …).Measured, by walking every class's
__init__in the core expression package and re-derivingwhat the old code decided: 58 parameters reached the relation test, 51 of them annotated
with a single class, and 7 wrongly annotated with a parameterised alias — columns, attributes,
actions, parts, items, keys and passing, all
Sequence[...]. Against MariaDB those seven classeswent from none constructible to four. The remaining three are a different defect — keyword-only
parameters hidden behind defaulted positionals — and stay unbuilt.
Type of change
feat: A new featuredocs: Documentation only changes (if related to the new feature)perf: A code change that improves performance (if part of the feature)test: Adding missing tests or correcting existing tests (for the new feature)chore: Changes to the build process or auxiliary tools and libraries suchs as documentation generation (if related to the new feature)ci: Changes to our CI configuration files and scripts (if related to the new feature)build: Changes that affect the build system or external dependencies (if related to the new feature)revert: Reverts a previous commitBreaking Change
(If Yes, please describe the impact and migration path below):
N/A for the API surface: no function is removed and no public symbol is renamed. All 28
functions this PR adds to
utils/expression.pyare private (_-prefixed), and the diff removeszero functions. Downstream repositories import six symbols from
testsuite.utils—select_fixture(45 uses),requires_protocol(4),requires_json_operations(2),roundtrip_expression,skip_test_if_protocol_unsupportedandassert_params_equal— and all sixare still present with unchanged signatures.
requires_protocol's own behaviour is untouched; onlythe strings passed to it changed.
The behaviour change is in coverage, not in the interface: tests that used to skip now run. A
backend carrying a latent defect can therefore start failing where it previously passed. The nine
backends that were pointed at this branch while it was in development are the evidence that it does
not — see Testing.
Related Repositories/Packages
This is the eleventh and last repository in the coordinated
feature/expression-schema-object-refactorchange. All the others are merged:
release/v1.0.0.dev30release/v1.0.0.dev17release/v1.0.0.dev20release/v1.0.0.dev3release/v1.0.0.dev3release/v1.0.0.dev3release/v1.0.0.dev2release/v1.0.0.dev2release/v1.0.0.dev1release/v1.0.0.dev1Dependency. This branch requires core at
release/v1.0.0.dev30or later — includingf84320f, a follow-up fix on that line that binds theSubquerythatSQLValueExpression.in_()builds (it was constructed with
Noneas the dialect). Without it,feature/query/joins/test_join_subquery.py::test_join_filtered_by_in_subqueryfails wherever therelation renders through that node; it was observed failing on BigQuery's emulator, and passed on
all backends once the core fix was on the line.
Testing
Test Plan:
This repository has no CI pipeline — it is a shared test suite library installed into the other
projects' virtual environments, so it has no self-tests to run. Verification therefore has two
parts.
Locally, after the fix: the full-text-search compatibility test runs 11 passed (it skipped
11 of 11 before), and the join bridge files run 209 passed, with nothing newly failing.
Through the backends. While this branch was in development, all nine backends'
test.ymlpinnedthe testsuite at
feature/expression-schema-object-refactor, so this exact branch ran inside ninefull CI matrices against core
release/v1.0.0.dev30(includingf84320f):"All existing tests pass" is deliberately left unticked. No test failed an assertion anywhere,
but four jobs did not report a pass, and I am not going to count a timeout as a pass:
Python 3.14 / postgres 19beta4andRun benchmarks with Python 3.14 / postgres 18—both ran 6h0m42s / 6h0m43s and were cut off by the Actions job limit;
Python 3.11 / SQL Server 2019andPython 3.12 / SQL Server 2025— 6h0m41s /6h1m59s, same cause;
cancelled.These are timeouts, not assertion failures, and they are not attributable to this branch: the same
two jobs passed in an earlier
workflow_dispatchrun of the same workflow, and the pattern appearsin the backend's
pull_requestruns both before and after this suite's fragment commit — so itdoes not track this change. I could not retrieve their logs (the archive fetch returned truncated
output), so the hang itself is undiagnosed; it is worth a separate look at the service containers
for those images.
One further red is not a test result at all:
SonarCloud Code Analysisfails on mysql, mariadb,oracle, sqlserver and firebird because the dependency-repoint commit in those PRs touched the
pip install ... git+https://...lines in.github/workflows/test.yml, which SonarCloud'sgithubactions:S8544/S8541rules then score as new-code security findings(
new_security_rating3 against a threshold of 1). The same lines existed before under a differentref; making them new code is what surfaced them. Those PRs were merged with the ruleset's
OrganizationAdminbypass.Checklist
README.md, etc.).changelog.d/<issue_number>.<type>.md).Two boxes are unticked on purpose. No changelog fragment yet — the fragment is numbered for the
PR, so it can only be written once the PR exists; it is added in a follow-up commit. No new
warnings / SQL injection / performance: this change touches test fixtures and one test helper, not
the SQL-rendering path, and nothing here adds a query, a warning, or per-test work beyond
constructing the expressions the tests already needed. The
docsclaim is ticked only for thedocstring corrections in
utils/common.pyandutils/expression.py.Additional Notes
The common thread is that a skip is not a failure. Both defects turned "I could not check this"
into a green run, and in the first case eleven parametrisations of a compatibility test plus every
join test across every backend reported success while testing nothing.
requires_protocoltreats anunresolvable reference as "not supported" and skips; that is the right behaviour for a probe that
legitimately answers
False, and the wrong behaviour for a reference that cannot be imported atall. Nothing here changes that policy — but it is why the two are worth telling apart, and why a
green matrix is not by itself evidence that a suite is running.
Worth noting for whoever picks this up next: this branch's real function is as the last piece of
the eleven-repository change, and the fact that it is also the piece whose failures were silent is
not a coincidence. The two adapters — a resolver that skips, and a constructor that guesses — were
both written to be forgiving, and both were wrong in the same direction.