Skip to content

QA: track pre-existing Aqua/ExplicitImports findings marked @test_broken in test/qa #1496

Description

@ChrisRackauckas-Claude

The new test/qa group (added in the run_qa v1.6 conversion PR) runs Aqua, JET, and the full ExplicitImports check suite. Most checks pass, but the following pre-existing findings are currently marked @test_broken in test/qa/qa.jl (via aqua_broken / ei_broken) so the QA lane goes green while tracking the work to fix them. This issue tracks resolving them and removing the broken markers.

Aqua (aqua_broken)

  • ambiguities — Aqua reports 55 method ambiguities across the Catalyst API surface (run with Aqua.test_ambiguities(Catalyst; broken = false) to list them).
  • unbound_args — the GridLattice{N,T} = Union{Array{Bool,N}, JumpProcesses.CartesianGridRej{N,T}} alias leaves the type parameter T unbound on the Array{Bool,N} branch, so every method dispatching on GridLattice (grid_dims, get_neighbours, DiscreteSpaceReactionSystem, etc. in src/spatial_reaction_systems/discrete_space_reaction_systems.jl) is flagged.
  • undefined_exportsVariable appears in names(Catalyst) (re-exported via @reexport using ModelingToolkitBase) but is no longer defined upstream, so Aqua flags it as an undefined export. (The other former offender, the CartesianGridReJ typo, is fixed in the conversion PR.)

ExplicitImports (ei_broken)

Catalyst pervasively imports and qualified-accesses internal (not public/exported) names of its upstream dependencies. These are not bugs in Catalyst per se — the names become public as the upstream packages adopt public declarations — but the checks fail today:

  • all_qualified_accesses_via_owners — ~15 names accessed via a re-exporter rather than their owner (e.g. LaTeXString via Latexify, NullParameters via DiffEqBase, unwrap/scalarize/Operator via Symbolics instead of SymbolicUtils, getname/setp/variable_index via ModelingToolkitBase instead of SymbolicIndexingInterface).
  • all_qualified_accesses_are_public and all_explicit_imports_are_public — ~200 imports/accesses of internal names from ModelingToolkitBase, Symbolics, SymbolicUtils, SciMLBase, DiffEqBase, JumpProcesses, Latexify, Graphs, MacroTools, RuntimeGeneratedFunctions and Base.

These two checks can be narrowed to per-name ignore lists (documenting each name's source package) as upstream packages release public declarations; once a source package marks its names public, the corresponding ignore entries (and eventually the broken marker) can be dropped.

The three EI checks that already pass hard (no_implicit_imports, no_stale_explicit_imports, all_explicit_imports_via_owners) are run as normal @tests.

JET (jet_broken)

Update (QA conversion PR #1497): the JET typo-mode check (report_package(Catalyst; mode = :typo)) is now run via run_qa. It passes hard on the lts lane (JET 0.9 reports nothing) but on the Julia 1 lane (JET 0.11 on Julia 1.12) it reports 5 pre-existing latent findings. All reproduce byte-identical on master — they are not introduced by the conversion; Catalyst simply had no JET check before, and JET 0.9 (lts) does not surface them. jet_broken is therefore gated on VERSION >= v"1.12-" so the lts lane keeps the hard check (avoiding an Unexpected Pass) while the Julia 1 lane marks them @test_broken.

The 5 names, to be fixed and then un-broken:

  • metadata_entries (src/dsl.jl:436) — error("Some reaction metadata fields where repeated: $(metadata_entries)") interpolates an undefined name; the in-scope value is metadata_i.
  • reaction (src/chemistry_functionality.jl:400) — error("The space of possible balanced versions of the reaction ($reaction) ...") interpolates an undefined name; the function argument is rx.
  • clipboard ×2 (src/latexify_recipes.jl:65, :161) — Latexify.COPY_TO_CLIPBOARD && clipboard(latexstr); clipboard (from InteractiveUtils) is not imported into Catalyst, so this would UndefVarError if COPY_TO_CLIPBOARD were set. Fix requires importing clipboard.
  • V (src/reactionsystem.jl:78) — the unparameterised NetworkProperties() @kwdef keyword constructor references the type parameter V in its Set{V}()/Dict{V,Int}() defaults. That bare constructor is never called (only NetworkProperties{Int, SymbolicT}() is), so it is a latent dead method. Fix by removing/parameterising the bare constructor.

Once these are fixed, report_package returns zero reports and the @test_broken auto-flags as an Unexpected Pass; drop jet_broken (and the VERSION gate) then.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions