Skip to content

Verify every enum has a test asserting its contents #97

Description

@jnasbyupgrade

Part of #93.

Enum contents are one of the easiest things to get wrong across an ALTER EXTENSION UPDATE: the install script states the full label list in CREATE TYPE … AS ENUM (…), while an update script has to say ALTER TYPE … ADD VALUE — two different statements expressing the same change, one label at a time, and adding a label to one file without the other is easy to miss in review.

A text/parser-level check is the wrong mechanism for this. Making a static checker understand that a changed CREATE TYPE … AS ENUM list corresponds to a set of ALTER TYPE … ADD VALUE statements means teaching it that two unrelated statement forms mean the same thing — exactly the sort of special-casing that makes such a tool grow without bound. Enum label support was deliberately dropped from #92 for this reason.

The right mechanism is a test asserting each enum's contents. The suite already runs identical expected output against a fresh install, an updated database, and a pg_upgraded one, so an enum whose install and update scripts disagree fails on its own, in whichever mode diverges, naming the enum. No new comparison machinery, and it covers paths a static check never sees.

What this issue is

Relying on those tests only works if they actually exist for every enum, and nothing currently enforces that. So: a check that every enum the extension defines has a test asserting its contents.

The design constraint that makes this cheap: mandate something trivially greppable on both sides. Enum definitions are already trivial to extract from the install source —

$ grep -oE 'CREATE TYPE [a-z_.]+ AS ENUM' sql/cat_tools.sql.in | sed 's/CREATE TYPE //;s/ AS ENUM//'
cat_tools.constraint_type
cat_tools.procedure_type
cat_tools.relation_type
cat_tools.relation_relkind
cat_tools.routine_prokind
cat_tools.routine_type
cat_tools.routine_proargmode
cat_tools.routine_argument_mode
cat_tools.routine_provolatile
cat_tools.routine_volatility
cat_tools.routine_proparallel
cat_tools.routine_parallel_safety
cat_tools.object_type

— 13 of them. The other half is picking a convention in the test files that is equally greppable, so the check is a set difference between two greps rather than anything that has to parse SQL. That convention is the substance of this issue: it should be obvious to write, hard to write wrongly, and impossible to satisfy accidentally.

Current coverage is partial

test/helpers/enum_mapping.sql already does roundtrip testing for paired enums (a "char"-based one and its human-readable counterpart), preceded by a size check — so some enums are covered, via that helper. test/sql/enum.sql tests cat_tools' own enum_range/enum_range_srf functions rather than the label contents of the extension's enum types.

Worth establishing before designing the convention: which of the 13 currently have their contents asserted, and whether the existing helper's size check is strong enough to count (a size check catches an added label but not a renamed or reordered one).

Notes

  • A size check alone is weaker than an explicit label list — it catches additions but not substitutions.
  • Label order matters for enums (enumsortorder), and ALTER TYPE … ADD VALUE takes optional BEFORE/AFTER, so a fresh install and an updated one can agree on the label set while disagreeing on order. Decide whether the assertion covers order.
  • If an explicit comparison is ever wanted beyond per-enum tests, the better shape is comparing a freshly-created database against an updated one, not comparing script text.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    update-safetyALTER EXTENSION UPDATE correctness: fresh-vs-updated equivalence

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions